Skip to content

Commit

Permalink
Add new InlineFunctions tests, showing that the pass can generate inc…
Browse files Browse the repository at this point in the history 
…orrect code in some (hopefully rare) cases. Switch the FeatureSet back to ES5 for now until that's resolved.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=177245201
  • Loading branch information
@brad4d
tbreisacher authored and brad4d committed Nov 29, 2017
1 parent 4ba3d69 commit 628ee1b
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 70 deletions.
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -2751,7 +2751,8 @@ protected CompilerPass create(AbstractCompiler compiler) {

@Override
public FeatureSet featureSet() {
return ES8_MODULES;
// TODO(b/69850796): Switch this back to ES8_MODULES.
return ES5;
}
};

Expand Down
22 changes: 10 additions & 12 deletions src/com/google/javascript/jscomp/FunctionArgumentInjector.java
View file
Expand Up @@ -104,6 +104,7 @@ private static Node inject(
} else if (node.isFunction()) {
// Once we enter another scope the "this" value changes, don't try
// to replace it within an inner scope.
// TODO(tbreisacher): Make sure this works correctly for arrow functions.
replaceThis = false;
}

Expand All @@ -122,7 +123,6 @@ private static Node inject(
static LinkedHashMap<String, Node> getFunctionCallParameterMap(
Node fnNode, Node callNode, Supplier<String> safeNameIdSupplier) {
// Create an argName -> expression map
// NOTE: A linked map is created here to provide ordering.
LinkedHashMap<String, Node> argMap = new LinkedHashMap<>();

// CALL NODE: [ NAME, ARG1, ARG2, ... ]
Expand All @@ -132,7 +132,7 @@ static LinkedHashMap<String, Node> getFunctionCallParameterMap(
cArg = cArg.getNext();
} else {
// 'apply' isn't supported yet.
checkState(!NodeUtil.isFunctionObjectApply(callNode));
checkState(!NodeUtil.isFunctionObjectApply(callNode), callNode);
argMap.put(THIS_MARKER, NodeUtil.newUndefinedNode(callNode));
}

Expand Down Expand Up @@ -180,6 +180,7 @@ static LinkedHashMap<String, Node> getFunctionCallParameterMap(
} else if (fnParam.isDefaultValue()) {
argMap.put(fnParam.getFirstChild().getString(), cArg);
} else {
checkState(fnParam.isName(), fnParam);
argMap.put(fnParam.getString(), cArg);
}
cArg = cArg.getNext();
Expand Down Expand Up @@ -272,8 +273,7 @@ static Set<String> findModifiedParameters(Node fnNode) {
* function.
*/
private static Set<String> findModifiedParameters(
Node n, Node parent, Set<String> names, Set<String> unsafe,
boolean inInnerFunction) {
Node n, Node parent, Set<String> names, Set<String> unsafe, boolean inInnerFunction) {
checkArgument(unsafe != null);
if (n.isName()) {
if (names.contains(n.getString()) && (inInnerFunction || canNameValueChange(n, parent))) {
Expand Down Expand Up @@ -301,7 +301,7 @@ private static Set<String> findModifiedParameters(
* after assignment, where in as "o = x", "o" is now "x").
*
* This also looks for the redefinition of a name.
* function (x){var x;}
* function (x) {var x;}
*
* @param n The NAME node in question.
* @param parent The parent of the node.
Expand Down Expand Up @@ -330,7 +330,7 @@ static void maybeAddTempsForCallArguments(
return;
}

checkArgument(fnNode.isFunction());
checkArgument(fnNode.isFunction(), fnNode);
Node block = fnNode.getLastChild();

int argCount = argMap.size();
Expand Down Expand Up @@ -447,8 +447,8 @@ static boolean mayHaveConditionalCode(Node n) {
}

/**
* Boot strap a traversal to look for parameters referenced
* after a non-local side-effect.
* Bootstrap a traversal to look for parameters referenced after a non-local side-effect.
*
* NOTE: This assumes no-inner functions.
* @param parameters The set of parameter names.
* @param root The function code block.
Expand Down Expand Up @@ -491,8 +491,7 @@ private static Set<String> findParametersReferencedAfterSideEffect(
* parameters are recorded and the decision to keep or throw away those
* references is deferred until exiting the loop structure.
*/
private static class ReferencedAfterSideEffect
implements Visitor, Predicate<Node> {
private static class ReferencedAfterSideEffect implements Visitor, Predicate<Node> {
private final Set<String> parameters;
private final Set<String> locals;
private boolean sideEffectSeen = false;
Expand All @@ -517,8 +516,7 @@ public boolean apply(Node node) {

// If we have found all the parameters, don't bother looking
// at the children.
return !(sideEffectSeen
&& parameters.size() == parametersReferenced.size());
return !(sideEffectSeen && parameters.size() == parametersReferenced.size());
}

boolean inLoop() {
Expand Down
93 changes: 39 additions & 54 deletions test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java
View file
Expand Up @@ -13,12 +13,15 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.javascript.jscomp;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
import static com.google.javascript.jscomp.CompilerOptions.LanguageMode.ECMASCRIPT_2017;
import static com.google.javascript.jscomp.FunctionArgumentInjector.findModifiedParameters;
import static com.google.javascript.jscomp.FunctionArgumentInjector.getFunctionCallParameterMap;
import static com.google.javascript.jscomp.FunctionArgumentInjector.inject;
import static com.google.javascript.jscomp.FunctionArgumentInjector.maybeAddTempsForCallArguments;
import static com.google.javascript.jscomp.NodeUtil.getFunctionBody;
import static com.google.javascript.jscomp.testing.NodeSubject.assertNode;

Expand All @@ -33,19 +36,18 @@
import junit.framework.TestCase;

/**
* Inline function tests.
* Tests for the static methods in {@link FunctionArgumentInjector}.
*
* @author johnlenz@google.com (John Lenz)
*/
public final class FunctionArgumentInjectorTest extends TestCase {

// TODO(johnlenz): Add unit tests for "getFunctionCallParameterMap"

private static final ImmutableSet<String> EMPTY_STRING_SET = ImmutableSet.of();

public void testInject0() {
Compiler compiler = getCompiler();
Node result =
FunctionArgumentInjector.inject(
inject(
compiler,
getFunctionBody(parseFunction("function f(x) { alert(x); }")),
null,
Expand All @@ -56,7 +58,7 @@ public void testInject0() {
public void testInject1() {
Compiler compiler = getCompiler();
Node result =
FunctionArgumentInjector.inject(
inject(
compiler,
getFunctionBody(parseFunction("function f() { alert(this); }")),
null,
Expand All @@ -67,83 +69,68 @@ public void testInject1() {
// TODO(johnlenz): Add more unit tests for "inject"

public void testFindModifiedParameters0() {
assertThat(
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a){ return a; }"))).isEmpty();
assertThat(findModifiedParameters(parseFunction("function f(a){ return a; }"))).isEmpty();
}

public void testFindModifiedParameters1() {
assertThat(
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a){ return a==0; }"))).isEmpty();
assertThat(findModifiedParameters(parseFunction("function f(a){ return a==0; }"))).isEmpty();
}

public void testFindModifiedParameters2() {
assertThat(
FunctionArgumentInjector.findModifiedParameters(parseFunction("function f(a){ b=a }")))
.isEmpty();
assertThat(findModifiedParameters(parseFunction("function f(a){ b=a }"))).isEmpty();
}

public void testFindModifiedParameters3() {
assertEquals(ImmutableSet.of("a"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a){ a=0 }")));
assertThat(findModifiedParameters(parseFunction("function f(a){ a=0 }"))).containsExactly("a");
}

public void testFindModifiedParameters4() {
assertEquals(ImmutableSet.of("a", "b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a=0;b=0 }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ a=0;b=0 }")))
.containsExactly("a", "b");
}

public void testFindModifiedParameters5() {
assertEquals(ImmutableSet.of("b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a; if (a) b=0 }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; if (a) b=0 }")))
.containsExactly("b");
}

public void testFindModifiedParameters6() {
assertEquals(ImmutableSet.of("a", "b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ function f(){ a;b; } }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ function f(){ a;b; } }")))
.containsExactly("a", "b");
}

public void testFindModifiedParameters7() {
assertEquals(ImmutableSet.of("b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a; function f(){ b; } }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; function f(){ b; } }")))
.containsExactly("b");
}

public void testFindModifiedParameters8() {
assertEquals(
ImmutableSet.of("b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a; function f(){ function g() { b; } } }")));
assertThat(
findModifiedParameters(
parseFunction("function f(a,b){ a; function f(){ function g() { b; } } }")))
.containsExactly("b");
}

public void testFindModifiedParameters9() {
assertEquals(ImmutableSet.of("a", "b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ (function(){ a;b; }) }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ (function(){ a;b; }) }")))
.containsExactly("a", "b");
}

public void testFindModifiedParameters10() {
assertEquals(ImmutableSet.of("b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a; (function (){ b; }) }")));
assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; (function (){ b; }) }")))
.containsExactly("b");
}

public void testFindModifiedParameters11() {
assertEquals(
ImmutableSet.of("b"),
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a,b){ a; (function(){ (function () { b; }) }) }")));
assertThat(
findModifiedParameters(
parseFunction("function f(a,b){ a; (function(){ (function () { b; }) }) }")))
.containsExactly("b");
}

public void testFindModifiedParameters12() {
assertThat(
FunctionArgumentInjector.findModifiedParameters(
parseFunction("function f(a=5){ return a;} f(1);"))).isEmpty();
assertThat(findModifiedParameters(parseFunction("function f(a=5){ return a;} f(1);")))
.isEmpty();
}

public void testMaybeAddTempsForCallArguments1() {
Expand Down Expand Up @@ -540,24 +527,22 @@ private void assertArgMapHasKeys(String code, String fnName, Set<String> expecte
Node call = findCall(n, fnName);
assertNotNull(call);
LinkedHashMap<String, Node> actualMap =
FunctionArgumentInjector.getFunctionCallParameterMap(fn, call, getNameSupplier());
getFunctionCallParameterMap(fn, call, getNameSupplier());
assertThat(actualMap.keySet()).isEqualTo(expectedKeys);
}

private void testNeededTemps(String code, String fnName, Set<String> expectedTemps) {
private void testNeededTemps(String code, String fnName, ImmutableSet<String> expectedTemps) {
Node n = parse(code);
Node fn = findFunction(n, fnName);
assertNotNull(fn);
Node call = findCall(n, fnName);
assertNotNull(call);
Map<String, Node> args =
FunctionArgumentInjector.getFunctionCallParameterMap(fn, call, getNameSupplier());
Map<String, Node> args = getFunctionCallParameterMap(fn, call, getNameSupplier());

Set<String> actualTemps = new HashSet<>();
FunctionArgumentInjector.maybeAddTempsForCallArguments(
fn, args, actualTemps, new ClosureCodingConvention());
maybeAddTempsForCallArguments(fn, args, actualTemps, new ClosureCodingConvention());

assertEquals(expectedTemps, actualTemps);
assertThat(actualTemps).isEqualTo(expectedTemps);
}


Expand Down
53 changes: 53 additions & 0 deletions test/com/google/javascript/jscomp/InlineFunctionsTest.java
View file
Expand Up @@ -2697,6 +2697,14 @@ public void testFunctionProperty() {
"a.bar;"));
}

public void testFunctionRestParam() {
test(
lines(
"var f = function(...args) { return args[0]; }",
"f(8);"),
"[8][0]");
}

public void testArrowFunctionRestParam() {
test(
lines(
Expand Down Expand Up @@ -3219,6 +3227,51 @@ public void testDefaultParam() {
"{ var b$jscomp$inline_1=[7,8];3+b$jscomp$inline_1[1] }");
}

public void testDefaultParam_argIsUndefined() {
test(
lines(
"function foo(a, b = 1) {",
" return a + b;",
"}",
"foo(1, undefined);"),
// TODO(tbreisacher): This is incorrect!
// If the caller passes undefined, 'b' gets its default value, 1.
"1+undefined");
}

public void testDefaultParam_argIsUnknown() {
testSame(
lines(
"function foo(a, b = 1) {",
" return a + b;",
"}",
"foo(1, x);"),
// TODO(tbreisacher): This is incorrect!
// If x happens to be undefined, 'b' gets its default value, 1.
"1 + x");
}

// This test currently produces an invalid tree (with a "void 0" on the left side of an assign).
// TODO(b/69850796): Fix this and enable the test.
public void disabled_testDefaultParam_withAssign() {
test(
lines(
"function foo(x = undefined) {",
" if (!x) {",
" x = 2;",
" }",
" return x;",
"}",
"foo(4);"),
lines(
"{",
" var x$jscomp$inline_0 = 4;",
" if (!x$jscomp$inline_0)",
" x$jscomp$inline_0 = 2;",
" x$jscomp$inline_0;",
"}"));
}

//TODO(b/64614552): Get the following tests to pass
public void disabled_testNestedDefaultParam() {
test(
Expand Down
9 changes: 6 additions & 3 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -4788,7 +4788,8 @@ public void testInlineRestParam() {
+ "b[a-0]=arguments[a];return b[0]}(8))");
}

public void testInlineRestParamNonTranspiling() {
// TODO(b/69850796): Re-enable when InlineFunctions' FeatureSet is set back to ES6+.
public void disabled_testInlineRestParamNonTranspiling() {
CompilerOptions options = createCompilerOptions();
options.setLanguageIn(LanguageMode.ECMASCRIPT_2017);
options.setLanguageOut(LanguageMode.ECMASCRIPT_2017);
Expand Down Expand Up @@ -4820,7 +4821,8 @@ public void testDefaultParameters() {
"var a={a:9}; a=void 0===a?{a:5}:a;alert(3+a.a)");
}

public void testDefaultParametersNonTranspiling() {
// TODO(b/69850796): Re-enable when InlineFunctions' FeatureSet is set back to ES6+.
public void disabled_testDefaultParametersNonTranspiling() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
options.setLanguageIn(LanguageMode.ECMASCRIPT_2017);
Expand Down Expand Up @@ -4858,7 +4860,8 @@ public void testRestObjectPatternParameters() {
"}(1,1,1,1,1))"));
}

public void testRestObjectPatternParametersNonTranspiling() {
// TODO(b/69850796): Re-enable when InlineFunctions' FeatureSet is set back to ES6+.
public void disabled_testRestObjectPatternParametersNonTranspiling() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
options.setLanguageIn(LanguageMode.ECMASCRIPT_2017);
Expand Down

0 comments on commit 628ee1b

Please sign in to comment.