Skip to content

Commit 41daf94

Browse files
rishipalcopybara-github
authored andcommitted
Fix crash in IsolatePolyfills pass with optional chaining
The IsolatePolyfills pass (run during stage 3 finalization) loses the optional chaining operator when rewriting the polyfill usages and produces code that crashes on the ASTValidator. For example, it rewrites the usage of "includes" below: ``` a.b()?.includes(); ``` into ``` var $jscomp$polyfillTmp = a.b(); $jscomp$lookupPolyfilledValue($jscomp$polyfillTmp,"includes").call($jscomp$polyfillTmp); ``` which loses the optional chain and crashes the ASTValidator. ``` java.lang.IllegalStateException: Start of optional chain node OPTCHAIN_CALL is not marked as the start.. ``` This CL changes the rewriting to produce: ``` var $jscomp$polyfillTmp = a.b(); $jscomp$lookupPolyfilledValue($jscomp$polyfillTmp,"includes", isOptChainAccess)?.call($jscomp$polyfillTmp); ``` This also fixes the ASTValidator crash by preserving the `START_OF_OPT_CHAIN` property on the lhs of `?.` to indicate the start of the optional chain. PiperOrigin-RevId: 511875065
1 parent d3f8dbd commit 41daf94

File tree

5 files changed

+181
-73
lines changed

5 files changed

+181
-73
lines changed

src/com/google/javascript/jscomp/IsolatePolyfills.java

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -219,18 +219,24 @@ private void rewritePolyfill(PolyfillUsage polyfillUsage) {
219219

220220
if (isGlobalClass) {
221221
// e.g. Symbol, Map, window.Map, or goog.global.Map
222+
// Optional chaining is forbidden on global classes, hence producing `getelem` for property
223+
// access would suffice.
222224
polyfillAccess.replaceWith(
223225
IR.getelem(jscompPolyfillsObject.cloneTree(), IR.string(name)) // $jscomp.polyfills['Map']
224226
.srcrefTree(polyfillAccess));
225-
} else if (parent.isCall() && polyfillAccess.isFirstChildOf(parent)) {
226-
// e.g. getStr().includes('x')
227+
} else if ((parent.isCall() || parent.isOptChainCall())
228+
&& polyfillAccess.isFirstChildOf(parent)) {
229+
// e.g. `getStr().includes('x')` or `getStr()?.includes('x')`
227230
rewritePolyfillInCall(polyfillAccess);
228231
} else {
229232
// e.g. [].includes.call(myIter, 0)
230233
Node methodName = IR.string(polyfillAccess.getString()).srcref(polyfillAccess);
231234
Node receiver = polyfillAccess.removeFirstChild();
235+
// The `$jscomp$lookupPolyfilledValue` can handle both normal prop access as well as optional
236+
// by checking whether the lhs (receiver) is null or undefined first.
232237
polyfillAccess.replaceWith(
233-
createPolyfillMethodLookup(receiver, methodName).srcrefTree(polyfillAccess));
238+
createPolyfillMethodLookup(receiver, methodName, NodeUtil.isOptChainNode(polyfillAccess))
239+
.srcrefTree(polyfillAccess));
234240
}
235241

236242
compiler.reportChangeToEnclosingScope(parent);
@@ -257,6 +263,7 @@ private void rewritePolyfill(PolyfillUsage polyfillUsage) {
257263
private void rewritePolyfillInCall(Node callee) {
258264
final Node methodName = IR.string(callee.getString()).srcref(callee);
259265
final Node receiver = callee.removeFirstChild();
266+
final boolean isCalleeOptChain = NodeUtil.isOptChainNode(callee);
260267

261268
boolean requiresTemp = compiler.getAstAnalyzer().mayEffectMutableState(receiver);
262269

@@ -270,15 +277,22 @@ private void rewritePolyfillInCall(Node callee) {
270277
polyfilledMethod =
271278
IR.comma(
272279
IR.assign(thisNode.cloneTree(), receiver),
273-
createPolyfillMethodLookup(thisNode.cloneTree(), methodName));
280+
createPolyfillMethodLookup(thisNode.cloneTree(), methodName, isCalleeOptChain));
274281
} else {
275282
thisNode = receiver;
276-
polyfilledMethod = createPolyfillMethodLookup(receiver.cloneTree(), methodName);
283+
polyfilledMethod =
284+
createPolyfillMethodLookup(receiver.cloneTree(), methodName, isCalleeOptChain);
277285
}
278286

279287
// Fix the `this` type by using .call:
280-
// lookupMethod(receiver, 'includes').call(receiver, arg)
281-
Node receiverDotCall = IR.getprop(polyfilledMethod, "call").srcrefTree(callee);
288+
// lookupMethod(receiver, 'includes', isOptChainNode).call(receiver, arg)
289+
Node receiverDotCall;
290+
if (NodeUtil.isOptChainNode(callee)) {
291+
// If optional chaining exists at this point, we can have it in the output
292+
receiverDotCall = IR.startOptChainGetprop(polyfilledMethod, "call").srcrefTree(callee);
293+
} else {
294+
receiverDotCall = IR.getprop(polyfilledMethod, "call").srcrefTree(callee);
295+
}
282296
callee.replaceWith(receiverDotCall);
283297
thisNode.insertAfter(receiverDotCall);
284298
}
@@ -296,10 +310,19 @@ private Node createTempName(Node srcref) {
296310
return IR.name(POLYFILL_TEMP).srcref(srcref);
297311
}
298312

299-
/** Returns a call <code>$jscomp$lookupPolyfilledValue(receiver, 'methodName')</code> */
300-
private Node createPolyfillMethodLookup(Node receiver, Node methodName) {
313+
/**
314+
* Returns a call <code>$jscomp$lookupPolyfilledValue(receiver, 'methodName', isOptChainNode)
315+
* </code>
316+
*/
317+
private Node createPolyfillMethodLookup(Node receiver, Node methodName, boolean isOptChainNode) {
301318
usedPolyfillMethodLookup = true;
302-
Node call = IR.call(jscompLookupMethod.cloneTree(), receiver, methodName);
319+
Node call;
320+
if (isOptChainNode) {
321+
call = IR.call(jscompLookupMethod.cloneTree(), receiver, methodName, IR.trueNode());
322+
} else {
323+
// 3rd param of `jscompLookupMethod` is optional
324+
call = IR.call(jscompLookupMethod.cloneTree(), receiver, methodName);
325+
}
303326
call.putBooleanProp(Node.FREE_CALL, true);
304327
return call;
305328
}

0 commit comments

Comments
 (0)