Skip to content

Commit ef7743a

Browse files
lauraharkercopybara-github
lauraharker
authored and
copybara-github
committed
Fix FunctionToBlockMutator to propagate constant-ness to new parameter aliases
It now tracks the original function parameter Node in a 'ParamArgPair' & clones that node, to preserve any constant annotations on the original. Contains some minor cleanups to the parameter injection logic, but shouldn't change any behavior. PiperOrigin-RevId: 830957031
1 parent 2ba8fbc commit ef7743a

File tree

7 files changed

+218
-81
lines changed

7 files changed

+218
-81
lines changed

‎src/com/google/javascript/jscomp/FunctionArgumentInjector.java‎

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ class FunctionArgumentInjector {
4040
// A string to use to represent "this". Anything that is not a valid
4141
// identifier can be used, so we use "this".
4242
static final String THIS_MARKER = "this";
43+
static final Node THIS_MARKER_NODE = IR.name(THIS_MARKER);
4344

4445
static final String REST_MARKER = "rest param";
4546

@@ -113,21 +114,22 @@ private Node inject(
113114
}
114115

115116
/** Get a mapping for function parameter names to call arguments. */
116-
ImmutableMap<String, Node> getFunctionCallParameterMap(
117+
ImmutableMap<String, ParamArgPair> getFunctionCallParameterMap(
117118
final Node fnNode, Node callNode, Supplier<String> safeNameIdSupplier) {
118119
checkNotNull(fnNode);
119120
// Create an parameterName -> expression map
120-
ImmutableMap.Builder<String, Node> argMap = ImmutableMap.builder();
121+
ImmutableMap.Builder<String, ParamArgPair> argMap = ImmutableMap.builder();
121122

122123
// CALL NODE: [ NAME, ARG1, ARG2, ... ]
123124
Node cArg = callNode.getSecondChild();
124125
if (cArg != null && NodeUtil.isFunctionObjectCall(callNode)) {
125-
argMap.put(THIS_MARKER, cArg);
126+
argMap.put(THIS_MARKER, new ParamArgPair(THIS_MARKER_NODE, cArg));
126127
cArg = cArg.getNext();
127128
} else {
128129
// 'apply' isn't supported yet.
129130
checkState(!NodeUtil.isFunctionObjectApply(callNode), callNode);
130-
argMap.put(THIS_MARKER, NodeUtil.newUndefinedNode(callNode));
131+
argMap.put(
132+
THIS_MARKER, new ParamArgPair(THIS_MARKER_NODE, NodeUtil.newUndefinedNode(callNode)));
131133
}
132134

133135
for (Node fnParam = NodeUtil.getFunctionParameters(fnNode).getFirstChild();
@@ -142,23 +144,29 @@ ImmutableMap<String, Node> getFunctionCallParameterMap(
142144
array.addChildToBack(cArg.cloneTree());
143145
cArg = cArg.getNext();
144146
}
145-
argMap.put(fnParam.getOnlyChild().getString(), array);
146-
return argMap.buildOrThrow();
147+
return argMap
148+
.put(
149+
fnParam.getOnlyChild().getString(),
150+
new ParamArgPair(fnParam.getOnlyChild(), array))
151+
.buildOrThrow();
147152
} else {
148153
checkState(fnParam.isName(), fnParam);
149-
argMap.put(fnParam.getString(), cArg);
154+
argMap.put(fnParam.getString(), new ParamArgPair(fnParam, cArg));
150155
}
151156
cArg = cArg.getNext();
152-
} else { // cArg != null
157+
} else { // cArg == null
153158
if (fnParam.isRest()) {
154159
checkState(fnParam.getOnlyChild().isName(), fnParam);
155160
// No arguments for REST parameters
156161
Node array = IR.arraylit().srcref(fnParam);
157-
argMap.put(fnParam.getOnlyChild().getString(), array);
162+
argMap.put(
163+
fnParam.getOnlyChild().getString(), new ParamArgPair(fnParam.getOnlyChild(), array));
158164
} else {
159165
checkState(fnParam.isName(), fnParam);
160166
Node srcLocation = callNode;
161-
argMap.put(fnParam.getString(), NodeUtil.newUndefinedNode(srcLocation));
167+
argMap.put(
168+
fnParam.getString(),
169+
new ParamArgPair(fnParam, NodeUtil.newUndefinedNode(srcLocation)));
162170
}
163171
}
164172
}
@@ -167,7 +175,7 @@ ImmutableMap<String, Node> getFunctionCallParameterMap(
167175
// called function.
168176
while (cArg != null) {
169177
String uniquePlaceholder = getUniqueAnonymousParameterName(safeNameIdSupplier);
170-
argMap.put(uniquePlaceholder, cArg);
178+
argMap.put(uniquePlaceholder, new ParamArgPair(IR.name(uniquePlaceholder), cArg));
171179
cArg = cArg.getNext();
172180
}
173181

@@ -270,7 +278,7 @@ private static boolean canNameValueChange(Node n) {
270278
ImmutableSet<String> gatherCallArgumentsNeedingTemps(
271279
AbstractCompiler compiler,
272280
Node fnNode,
273-
ImmutableMap<String, Node> argMap,
281+
ImmutableMap<String, ParamArgPair> argMap,
274282
ImmutableSet<String> modifiedParameters,
275283
CodingConvention convention) {
276284
checkArgument(fnNode.isFunction(), fnNode);
@@ -295,7 +303,8 @@ ImmutableSet<String> gatherCallArgumentsNeedingTemps(
295303
(!block.hasChildren()
296304
|| (block.hasOneChild() && !bodyMayHaveConditionalCode(block.getLastChild())));
297305
boolean hasMinimalParameters =
298-
NodeUtil.isUndefined(argMap.get(THIS_MARKER)) && argCount <= 2; // this + one parameter
306+
NodeUtil.isUndefined(argMap.get(THIS_MARKER).arg())
307+
&& argCount <= 2; // this + one parameter
299308

300309
// Get the list of parameters that may need temporaries due to side-effects.
301310
SetContainer parametersThatMayNeedTemps =
@@ -306,9 +315,9 @@ ImmutableSet<String> gatherCallArgumentsNeedingTemps(
306315
parametersThatMayNeedTemps.parametersWithNamesReferencedBeforeParameter();
307316

308317
// Check for arguments that are evaluated more than once.
309-
for (Map.Entry<String, Node> entry : argMap.entrySet()) {
318+
for (Map.Entry<String, ParamArgPair> entry : argMap.entrySet()) {
310319
String parameterName = entry.getKey();
311-
Node cArg = entry.getValue();
320+
Node cArg = entry.getValue().arg();
312321
if (namesNeedingTemps.contains(parameterName)) {
313322
requiresTempsUpToThisParameterName = parameterName;
314323
continue;
@@ -378,9 +387,10 @@ ImmutableSet<String> gatherCallArgumentsNeedingTemps(
378387

379388
if (!requiresTempsUpToThisParameterName.isEmpty()) {
380389
// mark all names upto requiresTempsUptoParameterName as namesNeedingTemps
381-
for (Map.Entry<String, Node> entry : argMap.entrySet()) {
390+
for (Map.Entry<String, ParamArgPair> entry : argMap.entrySet()) {
382391
String parameterName = entry.getKey();
383-
if (parameterName.equals(THIS_MARKER) && NodeUtil.isUndefined(argMap.get(THIS_MARKER))) {
392+
if (parameterName.equals(THIS_MARKER)
393+
&& NodeUtil.isUndefined(argMap.get(THIS_MARKER).arg())) {
384394
/* When there is no explicit this arg passed into the call, the argMap contains an entry
385395
* <"this", undefined Node>. See the `getFunctionCallParameterMap` method.
386396
*
@@ -654,4 +664,20 @@ private static ImmutableSet<String> getFunctionParameterSet(Node fnNode) {
654664
}
655665
return builder.build();
656666
}
667+
668+
/**
669+
* Stores a pair of function formal parameter + argument value at a specific call site.
670+
*
671+
* @param paramNode the original formal parameter name, such as {@code x} in {@code function
672+
* foo(x) { ... }} or, when injecting a value for {@code this}, {@link
673+
* FunctionArgumentInjector#THIS_MARKER_NODE}.
674+
* @param arg the argument passed as `paramNode` at some call site, such as {@code 500} in {@code
675+
* f(500);}.
676+
*/
677+
record ParamArgPair(Node paramNode, Node arg) {
678+
ParamArgPair {
679+
checkNotNull(paramNode);
680+
checkNotNull(arg);
681+
}
682+
}
657683
}

‎src/com/google/javascript/jscomp/FunctionInjector.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,15 @@
2525
import com.google.common.collect.ImmutableMap;
2626
import com.google.common.collect.ImmutableSet;
2727
import com.google.errorprone.annotations.CanIgnoreReturnValue;
28+
import com.google.javascript.jscomp.FunctionArgumentInjector.ParamArgPair;
2829
import com.google.javascript.rhino.Node;
2930
import com.google.javascript.rhino.Token;
3031
import com.google.javascript.rhino.jstype.JSType;
3132
import java.util.ArrayList;
3233
import java.util.Collection;
3334
import java.util.LinkedHashMap;
35+
import java.util.Map;
36+
import java.util.Map.Entry;
3437
import java.util.Set;
3538
import java.util.function.Supplier;
3639
import org.jspecify.annotations.Nullable;
@@ -365,10 +368,14 @@ private Node inlineReturnValue(Reference ref, Node fnNode) {
365368
// shadowed and an expression can't introduce new names, there is
366369
// no need to check for conflicts.
367370

368-
// Create an argName -> expression map, checking for side effects.
369-
ImmutableMap<String, Node> argMap =
371+
// Create an paramName -> argument value map, checking for side effects.
372+
ImmutableMap<String, ParamArgPair> paramToArgMap =
370373
functionArgumentInjector.getFunctionCallParameterMap(
371374
fnNode, callNode, this.safeNameIdSupplier);
375+
Map<String, Node> paramReplacements = new LinkedHashMap<>();
376+
for (Entry<String, ParamArgPair> entry : paramToArgMap.entrySet()) {
377+
paramReplacements.put(entry.getKey(), entry.getValue().arg());
378+
}
372379

373380
Node newExpression;
374381
if (!block.hasChildren()) {
@@ -380,7 +387,8 @@ private Node inlineReturnValue(Reference ref, Node fnNode) {
380387

381388
// Clone the return node first.
382389
Node safeReturnNode = returnNode.cloneTree();
383-
Node inlineResult = functionArgumentInjector.inject(null, safeReturnNode, null, argMap);
390+
Node inlineResult =
391+
functionArgumentInjector.inject(null, safeReturnNode, null, paramReplacements);
384392
checkArgument(safeReturnNode == inlineResult);
385393
newExpression = safeReturnNode.removeFirstChild();
386394
NodeUtil.markNewScopesChanged(newExpression, compiler);
@@ -812,7 +820,7 @@ private boolean callMeetsBlockInliningRequirements(
812820
// If the caller contains functions or evals, verify we aren't adding any
813821
// additional VAR declarations because aliasing is needed.
814822
if (forbidTemps) {
815-
ImmutableMap<String, Node> args =
823+
ImmutableMap<String, ParamArgPair> args =
816824
functionArgumentInjector.getFunctionCallParameterMap(
817825
calleeFn, callRef.callNode, this.safeNameIdSupplier);
818826
boolean hasArgs = !args.isEmpty();
@@ -871,7 +879,7 @@ private CanInlineResult canInlineReferenceDirectly(
871879
}
872880
}
873881

874-
ImmutableMap<String, Node> args =
882+
ImmutableMap<String, ParamArgPair> args =
875883
functionArgumentInjector.getFunctionCallParameterMap(
876884
fnNode, callNode, this.throwawayNameSupplier);
877885
boolean hasArgs = !args.isEmpty();

‎src/com/google/javascript/jscomp/FunctionToBlockMutator.java‎

Lines changed: 59 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.common.collect.ImmutableList;
2525
import com.google.common.collect.ImmutableMap;
2626
import com.google.common.collect.ImmutableSet;
27+
import com.google.javascript.jscomp.FunctionArgumentInjector.ParamArgPair;
2728
import com.google.javascript.jscomp.MakeDeclaredNamesUnique.InlineRenamer;
2829
import com.google.javascript.rhino.IR;
2930
import com.google.javascript.rhino.Node;
@@ -149,7 +150,7 @@ private Node mutateInternal(
149150
// TODO(johnlenz): Mark NAME nodes constant for parameters that are not modified.
150151
ImmutableSet<String> modifiedParameters =
151152
functionArgumentInjector.findModifiedParameters(newFnNode);
152-
ImmutableMap<String, Node> args =
153+
ImmutableMap<String, ParamArgPair> args =
153154
functionArgumentInjector.getFunctionCallParameterMap(
154155
newFnNode, callNode, this.safeNameIdSupplier);
155156
boolean hasArgs = !args.isEmpty();
@@ -317,62 +318,78 @@ private String getUniqueThisName() {
317318
* @return The node or its replacement.
318319
*/
319320
private Node aliasAndInlineArguments(
320-
Node fnTemplateRoot, ImmutableMap<String, Node> argMap, Set<String> namesToAlias) {
321+
Node fnTemplateRoot,
322+
ImmutableMap<String, ParamArgPair> paramToArgMap,
323+
Set<String> namesToAlias) {
321324

322325
if (namesToAlias == null || namesToAlias.isEmpty()) {
323-
// There are no names to alias, just inline the arguments directly.
324-
Node result = functionArgumentInjector.inject(compiler, fnTemplateRoot, null, argMap);
326+
// There are no names to alias. Just inline the arguments directly.
327+
Map<String, Node> replacements = new LinkedHashMap<>();
328+
for (Entry<String, ParamArgPair> entry : paramToArgMap.entrySet()) {
329+
replacements.put(entry.getKey(), entry.getValue().arg());
330+
}
331+
Node result = functionArgumentInjector.inject(compiler, fnTemplateRoot, null, replacements);
325332
checkState(result == fnTemplateRoot);
326333
return result;
327334
} else {
328-
// Create local alias of names that can not be safely
329-
// used directly.
330-
331-
// An arg map that will be updated to contain the
332-
// safe aliases.
333-
Map<String, Node> newArgMap = new LinkedHashMap<>(argMap);
334-
335-
// Declare the alias in the same order as they
336-
// are declared.
337-
List<Node> newVars = new ArrayList<>();
338-
// NOTE: argMap is a linked map so we get the parameters in the
339-
// order that they were declared.
340-
for (Entry<String, Node> entry : argMap.entrySet()) {
335+
// Create local alias of names that can not be safely used directly.
336+
337+
// A map from function parameter to the value it should be replaced with post-inlining.
338+
// This is a subset of paramToArg: we exclude parameters for which we create
339+
// an explicit alias.
340+
Map<String, Node> paramReplacements = new LinkedHashMap<>();
341+
342+
// Declare the aliases in the same order as the arguments are defined.
343+
List<Node> newAliasesToAdd = new ArrayList<>();
344+
// NOTE: paramToArgMap is a linked map so we get the parameters in the order that they were
345+
// declared.
346+
for (Entry<String, ParamArgPair> entry : paramToArgMap.entrySet()) {
341347
String name = entry.getKey();
342-
if (namesToAlias.contains(name)) {
343-
if (name.equals(THIS_MARKER)) {
344-
boolean referencesThis = NodeUtil.referencesEnclosingReceiver(fnTemplateRoot);
345-
// Update "this", this is only necessary if "this" is referenced
346-
// and the value of "this" is not Token.THIS, or the value of "this"
347-
// has side effects.
348-
349-
Node value = entry.getValue();
350-
if (!value.isThis()
351-
&& (referencesThis || compiler.getAstAnalyzer().mayHaveSideEffects(value))) {
352-
String newName = getUniqueThisName();
353-
Node newValue = entry.getValue().cloneTree();
354-
Node newNode = NodeUtil.newVarNode(newName, newValue).srcrefTreeIfMissing(newValue);
355-
newVars.add(0, newNode);
356-
// Remove the parameter from the list to replace.
357-
newArgMap.put(THIS_MARKER, IR.name(newName).srcrefTree(newValue));
358-
}
359-
} else {
360-
Node newValue = entry.getValue().cloneTree();
361-
Node newNode = NodeUtil.newVarNode(name, newValue).srcrefTreeIfMissing(newValue);
362-
newVars.add(0, newNode);
363-
// Remove the parameter from the list to replace.
364-
newArgMap.remove(name);
348+
ParamArgPair arg = entry.getValue();
349+
if (!namesToAlias.contains(name)) {
350+
// Replace references to the parameter with the argument directly.
351+
paramReplacements.put(name, arg.arg());
352+
continue;
353+
}
354+
if (name.equals(THIS_MARKER)) {
355+
boolean referencesThis = NodeUtil.referencesEnclosingReceiver(fnTemplateRoot);
356+
// Update "this". We only need to create an alias if "this" is referenced
357+
// and the value of "this" is not Token.THIS, or the value of "this" has side effects.
358+
Node originalValue = arg.arg();
359+
Node replacement = originalValue;
360+
if (!originalValue.isThis()
361+
&& (referencesThis || compiler.getAstAnalyzer().mayHaveSideEffects(originalValue))) {
362+
String newName = getUniqueThisName();
363+
Node newValue = originalValue.cloneTree();
364+
Node newNode = NodeUtil.newVarNode(newName, newValue).srcrefTreeIfMissing(newValue);
365+
newAliasesToAdd.add(0, newNode);
366+
replacement = IR.name(newName).srcrefTree(newValue);
365367
}
368+
paramReplacements.put(THIS_MARKER, replacement);
369+
} else {
370+
// Add an alias for a given parameter/argument. For example, if inlining this function:
371+
//
372+
// function f(x) { use(x, x); } f(computeValue());
373+
//
374+
// This code defines
375+
// var x = computeValue();
376+
// (as use(computeValue(), computeValue()) would compute the value twice).
377+
Node newName = arg.paramNode().cloneNode();
378+
Node newValue = arg.arg().cloneTree();
379+
newName.srcref(newValue); // source information should point to the argument.
380+
Node newNode = IR.var(newName, newValue).srcrefTreeIfMissing(newValue);
381+
newAliasesToAdd.add(0, newNode);
366382
}
367383
}
368384

369385
// Inline the arguments.
370-
Node result = functionArgumentInjector.inject(compiler, fnTemplateRoot, null, newArgMap);
386+
Node result =
387+
functionArgumentInjector.inject(compiler, fnTemplateRoot, null, paramReplacements);
371388
checkState(result == fnTemplateRoot);
372389

373390
// Now that the names have been replaced, add the new aliases for
374391
// the old names.
375-
for (Node n : newVars) {
392+
for (Node n : newAliasesToAdd) {
376393
fnTemplateRoot.addChildToFront(n);
377394
}
378395

‎test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.common.base.Supplier;
2525
import com.google.common.collect.ImmutableMap;
2626
import com.google.common.collect.ImmutableSet;
27+
import com.google.javascript.jscomp.FunctionArgumentInjector.ParamArgPair;
2728
import com.google.javascript.rhino.Node;
2829
import java.util.Map.Entry;
2930
import org.junit.Before;
@@ -694,7 +695,7 @@ private void assertArgMapHasKeys(String code, String fnName, ImmutableSet<String
694695
assertThat(fn).isNotNull();
695696
Node call = findCall(n, fnName);
696697
assertThat(call).isNotNull();
697-
ImmutableMap<String, Node> actualMap = getAndValidateFunctionCallParameterMap(fn, call);
698+
ImmutableMap<String, ParamArgPair> actualMap = getAndValidateFunctionCallParameterMap(fn, call);
698699
assertThat(actualMap.keySet()).isEqualTo(expectedKeys);
699700
}
700701

@@ -704,7 +705,7 @@ private void testNeededTemps(String code, String fnName, ImmutableSet<String> ex
704705
assertThat(fn).isNotNull();
705706
Node call = findCall(n, fnName);
706707
assertThat(call).isNotNull();
707-
ImmutableMap<String, Node> args = getAndValidateFunctionCallParameterMap(fn, call);
708+
ImmutableMap<String, ParamArgPair> args = getAndValidateFunctionCallParameterMap(fn, call);
708709

709710
ImmutableSet<String> actualTemps =
710711
functionArgumentInjector.gatherCallArgumentsNeedingTemps(
@@ -713,13 +714,14 @@ private void testNeededTemps(String code, String fnName, ImmutableSet<String> ex
713714
assertThat(actualTemps).isEqualTo(expectedTemps);
714715
}
715716

716-
private ImmutableMap<String, Node> getAndValidateFunctionCallParameterMap(Node fn, Node call) {
717-
final ImmutableMap<String, Node> map =
717+
private ImmutableMap<String, ParamArgPair> getAndValidateFunctionCallParameterMap(
718+
Node fn, Node call) {
719+
final ImmutableMap<String, ParamArgPair> map =
718720
functionArgumentInjector.getFunctionCallParameterMap(fn, call, getNameSupplier());
719721
// Verify that all nodes in the map have source info, so they are valid to add to the AST
720-
for (Entry<String, Node> nameToNodeEntry : map.entrySet()) {
722+
for (Entry<String, ParamArgPair> nameToNodeEntry : map.entrySet()) {
721723
final String name = nameToNodeEntry.getKey();
722-
final Node node = nameToNodeEntry.getValue();
724+
final Node node = nameToNodeEntry.getValue().arg();
723725

724726
new SourceInfoCheck(compiler).setCheckSubTree(node);
725727
assertWithMessage("errors for name: %s", name).that(compiler.getErrors()).isEmpty();

0 commit comments

Comments
 (0)