Skip to content

Commit

Permalink
CrossModuleCodeMotion: support object literal methods and computed pr…
Browse files Browse the repository at this point in the history 
…operties

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=176561267
  • Loading branch information
@brad4d @lauraharker
brad4d authored and lauraharker committed Nov 22, 2017
1 parent b913b88 commit f5fb684
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 15 deletions.
45 changes: 38 additions & 7 deletions src/com/google/javascript/jscomp/CrossModuleReferenceCollector.java
View file
Expand Up @@ -317,23 +317,54 @@ private boolean canMoveValue(Scope scope, Node valueNode) {
// c) a function, or // c) a function, or
// d) an array/object literal of movable values. // d) an array/object literal of movable values.
// e) a function stub generated by CrossModuleMethodMotion. // e) a function stub generated by CrossModuleMethodMotion.
if (valueNode == null if (valueNode == null || NodeUtil.isLiteralValue(valueNode, true) || valueNode.isFunction()) {
|| NodeUtil.isLiteralValue(valueNode, true) return true;
|| valueNode.isFunction() } else if (valueNode.isClass()) {
|| valueNode.isClass()) { Node classMembers = valueNode.getLastChild();
for (Node member = classMembers.getFirstChild(); member != null; member = member.getNext()) {
if (member.isComputedProp()) {
Node keyExpr = member.getFirstChild();
Node method = member.getLastChild();
checkState(method.isFunction(), method);
if (!canMoveValue(scope, keyExpr)) {
return false;
}
} else {
checkState(member.isMemberFunctionDef() || NodeUtil.isGetOrSetKey(member), member);
}
}
return true; return true;
} else if (valueNode.isCall()) { } else if (valueNode.isCall()) {
Node functionName = checkNotNull(valueNode.getFirstChild()); Node functionName = checkNotNull(valueNode.getFirstChild());
return functionName.isName() return functionName.isName()
&& (functionName.getString().equals(CrossModuleMethodMotion.STUB_METHOD_NAME)); && (functionName.getString().equals(CrossModuleMethodMotion.STUB_METHOD_NAME));
} else if (valueNode.isArrayLit() || valueNode.isObjectLit()) { } else if (valueNode.isArrayLit()) {
boolean isObjectLit = valueNode.isObjectLit(); // Movable if all of the array values are movable.
for (Node child = valueNode.getFirstChild(); child != null; child = child.getNext()) { for (Node child = valueNode.getFirstChild(); child != null; child = child.getNext()) {
if (!canMoveValue(scope, isObjectLit ? child.getFirstChild() : child)) { if (!canMoveValue(scope, child)) {
return false; return false;
} }
} }


return true;
} else if (valueNode.isObjectLit()) {
// Movable if all of the keys and values are movable.
for (Node child = valueNode.getFirstChild(); child != null; child = child.getNext()) {
if (child.isMemberFunctionDef() || NodeUtil.isGetOrSetKey(child)) {
continue;
} else if (child.isComputedProp()) {
if (!canMoveValue(scope, child.getFirstChild())
|| !canMoveValue(scope, child.getLastChild())) {
return false;
}
} else {
checkState(child.isStringKey());
if (!canMoveValue(scope, child.getOnlyChild())) {
return false;
}
}
}

return true; return true;
} else if (valueNode.isName()) { } else if (valueNode.isName()) {
// If the value is guaranteed to never be changed after // If the value is guaranteed to never be changed after
Expand Down
44 changes: 44 additions & 0 deletions test/com/google/javascript/jscomp/CrossModuleCodeMotionTest.java
View file
Expand Up @@ -1572,5 +1572,49 @@ public void testSpreadCountsAsAReference() {
test( test(
createModuleChain("let a = [];", "function f(...args) {} f(...a);", "a;"), createModuleChain("let a = [];", "function f(...args) {} f(...a);", "a;"),
new String[] {"", "let a = []; function f(...args) {} f(...a);", "a;"}); new String[] {"", "let a = []; function f(...args) {} f(...a);", "a;"});
}

public void testObjectLiteralMethods() {
// Object literal methods, getters, and setters are movable and references within them are
// handled correctly.
test(
createModuleChain(
"const a = 1;",
"const o = { foo() {return a;}, get x() {}, set x(v) {a = v;} };",
"o;",
"a;"),
new String[] {
"",
"",
"const a = 1; const o = { foo() {return a;}, get x() {}, set x(v) {a = v;} }; o;",
"a;"
});
}

public void testComputedProperties() {
// Computed properties are movable if the key expression is a literal.
test(
createModuleChain("let a = { ['something']: 1};", "a;"),
new String[] {"", "let a = { ['something']: 1}; a;"});

// Computed properties are movable if the key is a well defined variable
test(createModuleChain(
"const x = 1; let a = { [x]: 1};", "a;"),
new String[] {"", "const x = 1; let a = { [x]: 1}; a;"});
test(createModuleChain(
"const x = 1; let a = class { [x]() {} };", "a;"),
new String[] {"", "const x = 1; let a = class { [x]() {} }; a;"});

// Computed properties are not movable if the key is an unknown variable
testSame(createModuleChain("let a = { [x]: 1};", "a;"));
testSame(createModuleChain("let a = class { [x]() {} };", "a;"));

// Computed properties are not movable if the key is a
testSame(createModuleChain("let a = { [x]: 1};", "a;"));

// references in computed properties are honored
test(
createModuleChain("let a = 1;", "let b = { [a + 1]: 2 };", "a;"),
new String[] {"", "let a = 1; let b = { [a + 1]: 2 };", "a;"});
} }
} }
51 changes: 43 additions & 8 deletions test/com/google/javascript/jscomp/CrossModuleReferenceCollectorTest.java
View file
Expand Up @@ -33,6 +33,7 @@ public final class CrossModuleReferenceCollectorTest extends CompilerTestCase {
@Override @Override
protected void setUp() throws Exception { protected void setUp() throws Exception {
super.setUp(); super.setUp();
enableNormalize();
setLanguage(ECMASCRIPT_NEXT, ECMASCRIPT_NEXT); setLanguage(ECMASCRIPT_NEXT, ECMASCRIPT_NEXT);
} }


Expand Down Expand Up @@ -301,8 +302,7 @@ public void testFunctionCallsAreNotMovableExceptForMethodStubs() {
} }


public void testUnknownNameValueIsImmovable() { public void testUnknownNameValueIsImmovable() {
testSame("var a = unknownName;"); assertStatementIsImmovable("var a = unknownName;");
assertThat(testedCollector.getTopLevelStatements().get(0).isMovableDeclaration()).isFalse();
} }


public void testWellDefinedNameValueIsMovable() { public void testWellDefinedNameValueIsMovable() {
Expand Down Expand Up @@ -331,8 +331,7 @@ public void testArrayLiteralOfMovablesIsMovable() {
} }


public void testArrayLiteralWithImmovableIsImmovable() { public void testArrayLiteralWithImmovableIsImmovable() {
testSame("var a = [unknownValue];"); assertStatementIsImmovable("var a = [unknownValue];");
assertThat(testedCollector.getTopLevelStatements().get(0).isMovableDeclaration()).isFalse();
} }


public void testEmptyObjectLiteralIsMovable() { public void testEmptyObjectLiteralIsMovable() {
Expand All @@ -343,12 +342,49 @@ public void testEmptyObjectLiteralIsMovable() {
public void testObjectLiteralOfMovablesIsMovable() { public void testObjectLiteralOfMovablesIsMovable() {
testSame(lines( testSame(lines(
"var wellDefinedName = 1;", "var wellDefinedName = 1;",
"var o = { f: function(){}, one: 1, n: wellDefinedName, o: {}};")); "var o = {",
" f: function(){},",
" one: 1,",
" n: wellDefinedName,",
" o: {},",
" 'quoted': 1,",
" 123: 2,",
// computed
" ['computed string']: 1,",
" [234]: 1,",
// method shorthand
" method() {},",
" 'quoted method'() {},",
" ['computed method']() {},",
" [345]() {},",
// variable shorthand
" wellDefinedName,",
// getters
" get x() {},",
" get 'a'() {},",
" get ['a']() {},",
" get 456() {},",
" get [567]() {},",
// setters
" set x(x) {},",
" set 'a'(v) {},",
" set ['a'](v) {},",
" set 678(v) {},",
" set [678](v) {},",
"};"));
assertThat(testedCollector.getTopLevelStatements().get(1).isMovableDeclaration()).isTrue(); assertThat(testedCollector.getTopLevelStatements().get(1).isMovableDeclaration()).isTrue();
} }


public void testObjectLiteralWithImmovableIsImmovable() { public void testObjectLiteralWithImmovableIsImmovable() {
testSame("var o = { v: unknownValue };"); assertStatementIsImmovable("var o = { v: unknownValue };");
assertStatementIsImmovable("var o = { [unknownValue]: 1 };");
assertStatementIsImmovable("var o = { [unknownValue]() {} };");
assertStatementIsImmovable("var o = { get [unknownValue]() {} };");
assertStatementIsImmovable("var o = { set [unknownValue](x) {} };");
}

private void assertStatementIsImmovable(String statement) {
testSame(statement);
assertThat(testedCollector.getTopLevelStatements().get(0).isMovableDeclaration()).isFalse(); assertThat(testedCollector.getTopLevelStatements().get(0).isMovableDeclaration()).isFalse();
} }


Expand All @@ -360,8 +396,7 @@ public void testTemplateLiteralIsMovableIfSubstitutionsAreMovable() {
} }


public void testTemplateLiteralIsImmovableIfSubstitutionsAreImmovable() { public void testTemplateLiteralIsImmovableIfSubstitutionsAreImmovable() {
testSame("var t = `${unknownValue}`"); assertStatementIsImmovable("var t = `${unknownValue}`");
assertThat(testedCollector.getTopLevelStatements().get(0).isMovableDeclaration()).isFalse();
} }


// try to find cases to copy from CrossModuleCodeMotion // try to find cases to copy from CrossModuleCodeMotion
Expand Down

0 comments on commit f5fb684

Please sign in to comment.