From 63dd0dc51d9c2d14d5c47c71f3260b45a6280389 Mon Sep 17 00:00:00 2001 From: lharker Date: Tue, 5 Dec 2017 10:20:56 -0800 Subject: [PATCH] Partially inline local constructor aliases in AggressiveInlineAliases, since constructor properties are always collapsed. Still only inlines usages of an alias that are guaranteed to refer to the aliased constructor, so unsafe constructor aliasing is still a problem and can break code. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177973158 --- .../jscomp/AggressiveInlineAliases.java | 159 ++++++++++++-- .../jscomp/AggressiveInlineAliasesTest.java | 201 ++++++++++++++++++ .../InlineAndCollapsePropertiesTest.java | 28 +++ .../javascript/jscomp/IntegrationTest.java | 7 +- .../runtime_tests/async_function_test.js | 5 +- .../runtime_tests/simulated_j2cl_test.js | 4 - 6 files changed, 374 insertions(+), 30 deletions(-) diff --git a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java index 506ea06bf5c..409355269ec 100644 --- a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java +++ b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java @@ -140,6 +140,11 @@ private JSModule getRefModule(Reference ref) { * variable is inlineable. If (a) and (b) and (d) are true, then inline the alias if possible (if * it is assigned exactly once unconditionally). * + *

For (a), (b), and (c) are true and the alias is of a constructor, we may also partially + * inline the alias - i.e. replace some references with the constructor but not all - since + * constructor properties are always collapsed, so we want to be more aggressive about removing + * aliases. + * * @see InlineVariables */ private void inlineAliases(GlobalNamespace namespace) { @@ -160,7 +165,7 @@ private void inlineAliases(GlobalNamespace namespace) { List refs = new ArrayList<>(name.getRefs()); for (Ref ref : refs) { Scope hoistScope = ref.scope.getClosestHoistScope(); - if (ref.type == Type.ALIASING_GET && (hoistScope.isLocal() || !mayBeGlobalAlias(ref))) { + if (ref.type == Type.ALIASING_GET && !mayBeGlobalAlias(ref)) { // {@code name} meets condition (c). Try to inline it. // TODO(johnlenz): consider picking up new aliases at the end // of the pass instead of immediately like we do for global @@ -268,14 +273,26 @@ private boolean rewriteAllSubclassInheritedAccesses( * @return If the alias is possibly defined in the global scope. */ private boolean mayBeGlobalAlias(Ref alias) { + // Note: alias.scope is the closest scope in which the aliasing assignment occurred. + // So for "if (true) { var alias = aliasedVar; }", the alias.scope would be the IF block scope. if (alias.scope.isGlobal()) { return true; } + // If the scope in which the alias is assigned is not global, look up the LHS of the assignment. Node aliasParent = alias.node.getParent(); - if (aliasParent.isName()) { - if (aliasParent.getParent().isLet() || aliasParent.getParent().isConst()) { - return false; - } + if (!aliasParent.isAssign() && !aliasParent.isName()) { + // Only handle variable assignments and initializing declarations. + return true; + } + Node aliasLhsNode = aliasParent.isName() ? aliasParent : aliasParent.getFirstChild(); + if (!aliasLhsNode.isName()) { + // Only handle assignments to simple names, not qualified names or GETPROPs. + return true; + } + String aliasVarName = aliasLhsNode.getString(); + Var aliasVar = alias.scope.getVar(aliasVarName); + if (aliasVar != null) { + return aliasVar.scope.isGlobal(); } return true; } @@ -285,14 +302,17 @@ private boolean inlineAliasIfPossible(Name name, Ref alias, GlobalNamespace name // variable's declaration. If the alias's parent is a NAME, // then the NAME must be the child of a VAR, LET, or CONST node, and we must // be in a VAR, LET, or CONST assignment. + // Otherwise if the parent is an assign, we are in a "a = alias" case. Node aliasParent = alias.node.getParent(); - if (aliasParent.isName()) { - // Ensure that the local variable is well defined and never reassigned. - Node declarationType = aliasParent.getParent(); - Scope scope = declarationType.isVar() ? alias.scope.getClosestHoistScope() : alias.scope; - String aliasVarName = aliasParent.getString(); - Var aliasVar = scope.getVar(aliasVarName); - + if (aliasParent.isName() || aliasParent.isAssign()) { + Node aliasLhsNode = aliasParent.isName() ? aliasParent : aliasParent.getFirstChild(); + String aliasVarName = aliasLhsNode.getString(); + + Var aliasVar = alias.scope.getVar(aliasVarName); + checkState(aliasVar != null, "Expected variable to be defined in scope", aliasVarName); + Node aliasDeclarationParent = aliasVar.getParentNode(); + Scope scope = + aliasDeclarationParent.isVar() ? alias.scope.getClosestHoistScope() : alias.scope; ReferenceCollectingCallback collector = new ReferenceCollectingCallback( compiler, @@ -304,6 +324,8 @@ private boolean inlineAliasIfPossible(Name name, Ref alias, GlobalNamespace name ReferenceCollection aliasRefs = collector.getReferences(aliasVar); Set newNodes = new LinkedHashSet<>(); + // TODO(lharker): Is "aliasRefs.firstReferenceIsAssigningDeclaration()" necessary + // to inline safely? if (aliasRefs.isWellDefined() && aliasRefs.firstReferenceIsAssigningDeclaration()) { if (!aliasRefs.isAssignedOnceInLifetime()) { // Static properties of constructors are always collapsed. @@ -331,11 +353,7 @@ private boolean inlineAliasIfPossible(Name name, Ref alias, GlobalNamespace name int size = aliasRefs.references.size(); for (int i = 1; i < size; i++) { Reference aliasRef = aliasRefs.references.get(i); - - Node newNode = alias.node.cloneTree(); - aliasRef.getParent().replaceChild(aliasRef.getNode(), newNode); - compiler.reportChangeToEnclosingScope(newNode); - newNodes.add(new AstChange(getRefModule(aliasRef), aliasRef.getScope(), newNode)); + newNodes.add(replaceAliasReference(alias, aliasRef)); } // just set the original alias to null. @@ -348,11 +366,118 @@ private boolean inlineAliasIfPossible(Name name, Ref alias, GlobalNamespace name namespace.scanNewNodes(newNodes); return true; } + + if (name.isConstructor()) { + // TODO(lharker): the main reason this was added is because method decomposition inside + // generators introduces some constructor aliases that weren't getting inlined. + // If we find another (safer) way to avoid aliasing in method decomposition, consider + // removing this. + return partiallyInlineAlias(alias, namespace, aliasRefs, aliasLhsNode); + } + } + + return false; + } + + /** + * Inlines some references to an alias with its value. This handles cases where the alias is not + * declared at initialization. It does nothing if the alias is reassigned after being initialized, + * unless the reassignment occurs because of an enclosing function or a loop. + * + * @param alias An alias of some variable, which may not be well-defined. + * @param namespace The GlobalNamespace, which will be updated with all new nodes created. + * @param aliasRefs All references to the alias in its scope. + * @param aliasLhsNode The lhs name of the alias when it is first initialized. + * @return Whether any references to the alias were inlined. + */ + private boolean partiallyInlineAlias( + Ref alias, GlobalNamespace namespace, ReferenceCollection aliasRefs, Node aliasLhsNode) { + BasicBlock aliasBlock = null; + // This initial iteration through all the alias references does two things: + // a) Find the control flow block in which the alias is assigned. + // b) See if the alias var is assigned to in multiple places, and return if that's the case. + // NOTE: we still may inline if the alias is assigned in a loop or inner function and that + // assignment statement is potentially executed multiple times. + // This is more aggressive than what "inlineAliasIfPossible" does. + for (Reference aliasRef : aliasRefs) { + Node aliasRefNode = aliasRef.getNode(); + if (aliasRefNode == aliasLhsNode) { + aliasBlock = aliasRef.getBasicBlock(); + continue; + } else if (aliasRef.isLvalue()) { + // Don't replace any references if the alias is reassigned + return false; + } + } + + Set newNodes = new LinkedHashSet<>(); + boolean alreadySeenInitialAlias = false; + boolean foundNonReplaceableAlias = false; + // Do a second iteration through all the alias references, and replace any inlinable references. + for (Reference aliasRef : aliasRefs) { + Node aliasRefNode = aliasRef.getNode(); + if (aliasRefNode == aliasLhsNode) { + alreadySeenInitialAlias = true; + continue; + } else if (aliasRef.isDeclaration()) { + // Ignore any alias declarations, e.g. "var alias;", since there's nothing to inline. + continue; + } + + BasicBlock refBlock = aliasRef.getBasicBlock(); + if ((refBlock != aliasBlock && aliasBlock.provablyExecutesBefore(refBlock)) + || (refBlock == aliasBlock && alreadySeenInitialAlias)) { + // We replace the alias only if the alias and reference are in the same BasicBlock, + // the aliasing assignment takes place before the reference, and the alias is + // never reassigned. + codeChanged = true; + newNodes.add(replaceAliasReference(alias, aliasRef)); + } else { + foundNonReplaceableAlias = true; + } + } + + // We removed all references to the alias, so remove the original aliasing assignment. + if (!foundNonReplaceableAlias && canReplaceAliasAssignment(alias, aliasLhsNode)) { + // alias.node is the RHS of the assignment or initializing declaration. + Node aliasParent = alias.node.getParent(); + // `aliasName = aliasedVar;` => `aliasName = null;` + aliasParent.replaceChild(alias.node, IR.nullNode()); + compiler.reportChangeToEnclosingScope(aliasParent); } + if (codeChanged) { + // Inlining the variable may have introduced new references + // to descendants of {@code name}. So those need to be collected now. + namespace.scanNewNodes(newNodes); + return true; + } return false; } + private boolean canReplaceAliasAssignment(Ref alias, Node aliasLhsNode) { + if (alias.getTwin() != null) { + return false; + } + if (NodeUtil.isNameDeclaration(aliasLhsNode)) { + return true; + } + Node assign = aliasLhsNode.getParent(); + return !NodeUtil.isExpressionResultUsed(assign); + } + + /** + * @param alias A GlobalNamespace.Ref of the variable being aliased + * @param aliasRef One particular usage of an alias that we want to replace with the aliased var. + * @return an AstChange representing the new node(s) added to the AST * + */ + private AstChange replaceAliasReference(Ref alias, Reference aliasRef) { + Node newNode = alias.node.cloneTree(); + aliasRef.getParent().replaceChild(aliasRef.getNode(), newNode); + compiler.reportChangeToEnclosingScope(newNode); + return new AstChange(getRefModule(aliasRef), aliasRef.getScope(), newNode); + } + /** * Attempt to inline an global alias of a global name. This requires that the name is well * defined: assigned unconditionally, assigned exactly once. It is assumed that, the name for diff --git a/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java b/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java index 5529b2d171d..996a04a87d6 100644 --- a/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java +++ b/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java @@ -248,6 +248,207 @@ public void testAliasCreatedForObjectDepth1_1() { "var a = { b: 0 };" + "var c = null;" + "a.b = 1;" + "a.b == a.b;" + "use(a);"); } + public void testLocalNonCtorAliasCreatedAfterVarDeclaration1() { + // We only inline non-constructor local aliases if they are assigned upon declaration. + // TODO(lharker): We should be able to inline these. InlineVariables does, and it also + // uses ReferenceCollectingCallback to track references. + testSame( + lines( + "var Main = {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " tmp = Main;", + " tmp.doSomething(5);", + "}")); + } + + public void testLocalCtorAliasCreatedAfterVarDeclaration1() { + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " tmp = Main;", + " tmp.doSomething(5);", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " tmp = null;", + " Main.doSomething(5);", + "}")); + } + + public void testLocalCtorAliasCreatedAfterVarDeclaration2() { + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " tmp = Main;", + " tmp.doSomething(5);", + " if (true) {", + " tmp.doSomething(6);", + " }", + " var tmp;", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " tmp = null;", + " Main.doSomething(5);", + " if (true) {", + " Main.doSomething(6);", + " }", + " var tmp;", + "}")); + } + + public void testLocalCtorAliasCreatedAfterVarDeclaration3() { + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " if (tmp = Main) {", + " tmp.doSomething(6);", + " }", + " var tmp;", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " if (tmp = Main) {", // Don't set "tmp = null" because that would change control flow. + " Main.doSomething(6);", + " }", + " var tmp;", + "}")); + } + + public void testLocalCtorAliasCreatedAfterVarDeclaration4() { + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var a = tmp = Main;", + " tmp.doSomething(6);", + " a.doSomething(7);", + " var tmp;", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var a = tmp = Main;", // Don't set "tmp = null" because that would mess up a's value. + " Main.doSomething(6);", + " a.doSomething(7);", // Main doesn't get inlined here, which makes collapsing unsafe. + " var tmp;", + "}")); + } + + public void testLocalCtorAliasAssignedInLoop1() { + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " for (let i = 0; i < n(); i++) {", + " tmp = Main;", + " tmp.doSomething(5);", + " use(tmp);", + " }", + " use(tmp);", + " use(tmp.doSomething);", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " for (let i = 0; i < n(); i++) {", + " tmp = Main;", + " Main.doSomething(5);", + " use(Main);", + " }", + " use(tmp);", + " use(tmp.doSomething);", // This line may break if Main$doSomething is collapsed. + "}")); + } + + + public void testLocalCtorAliasAssignedInLoop2() { + // Test when the alias is assigned in a loop after being used. + testSame( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " for (let i = 0; i < n(); i++) {", + " use(tmp);", // Don't inline tmp here since it changes between loop iterations. + " tmp = Main;", + " }", + " use(tmp);", + " use(tmp.doSomething);", + "}")); + } + + public void testLocalCtorAliasAssignedInSwitchCase() { + // This mimics how the async generator polyfill behaves. + test( + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " function g() {", + " for (;;) {", + " switch(state) {", + " case 0:", + " tmp1 = Main;", + " tmp2 = tmp1.doSomething;", + " state = 1;", + " return;", + " case 1:", + " return tmp2.call(tmp1, 3);", + " }", + " }", + " }", + " var tmp1, tmp2, state = 0;", + " g();", + " return g();", + "}"), + lines( + "/** @constructor @struct */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " function g() {", + " for (;;) {", + " switch(state) {", + " case 0:", + " tmp1 = Main;", + " tmp2 = Main.doSomething;", + " state = 1;", + " return;", + " case 1:", + " return tmp2.call(tmp1, 3);", + " }", + " }", + " }", + " var tmp1, tmp2, state = 0;", + " g();", + " return g();", + "}")); + } + + public void testCodeGeneratedByGoogModule() { test( "var $jscomp = {};" diff --git a/test/com/google/javascript/jscomp/InlineAndCollapsePropertiesTest.java b/test/com/google/javascript/jscomp/InlineAndCollapsePropertiesTest.java index 8f0c091c7b2..68191fa7379 100644 --- a/test/com/google/javascript/jscomp/InlineAndCollapsePropertiesTest.java +++ b/test/com/google/javascript/jscomp/InlineAndCollapsePropertiesTest.java @@ -379,6 +379,34 @@ public void testAddPropertyToChildOfUncollapsibleCtorInLocalScope() { + "(function() {a$b$y = 0;})(); a$b$y;"); } + public void testPartialLocalCtorAlias() { + test( + lines( + "/** @constructor */ var Main = function() {};", + "Main.doSomething = function(i) {}", + "function f() {", + " var tmp;", + " if (g()) {", + " use(tmp.doSomething);", + " tmp = Main;", + " tmp.doSomething(5);", + " }", + " use(tmp.doSomething);", + "}"), + lines( + "/** @constructor */ var Main = function() {};", + "var Main$doSomething = function(i) {}", + "function f() {", + " var tmp;", + " if (g()) {", + " use(tmp.doSomething);", + " tmp = Main;", + " Main$doSomething(5);", + " }", + " use(tmp.doSomething);", // This line will work incorrectly if g() is true. + "}")); + } + public void testFunctionAlias2() { test("var a = {}; a.b = {}; a.b.c = function(){}; a.b.d = a.b.c;use(a.b.d)", "var a$b$c = function(){}; var a$b$d = null;use(a$b$c);"); diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index cb342b46093..25ef7b0fda5 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -4902,9 +4902,6 @@ public void testTranspilingEs2016ToEs2015() { } public void testMethodDestructuringInTranspiledAsyncFunction() { - // TODO(b/69456597) Fix this test. - // Currently the compiler unsafely collapses A.doSomething to A$doSomething, causing - // JSCompiler_temp_const$jscomp$0.doSomething to be undefined and breaking at runtime. CompilerOptions options = createCompilerOptions(); options.setCollapseProperties(true); options.setOptimizeCalls(false); @@ -4942,8 +4939,8 @@ public void testMethodDestructuringInTranspiledAsyncFunction() { " $jscomp$generator$throw$arg) {", " for (; 1;) switch ($jscomp$generator$state) {", " case 0:", - " JSCompiler_temp_const$jscomp$0 = A;", // This alias of A should be inlined. - " JSCompiler_temp_const = JSCompiler_temp_const$jscomp$0.doSomething;", + " JSCompiler_temp_const$jscomp$0 = A;", + " JSCompiler_temp_const = A$doSomething;", " $jscomp$generator$state = 1;", " return {value: 3, done: false};", " case 1:", diff --git a/test/com/google/javascript/jscomp/runtime_tests/async_function_test.js b/test/com/google/javascript/jscomp/runtime_tests/async_function_test.js index a0dd0983d04..e4412342ecc 100644 --- a/test/com/google/javascript/jscomp/runtime_tests/async_function_test.js +++ b/test/com/google/javascript/jscomp/runtime_tests/async_function_test.js @@ -218,12 +218,9 @@ testSuite({ * Confirms that method decomposition aliasing a constructor is * handled correctly. * - * TODO(b/69456597): Fix this. (Currently fails with - * "Cannot read property 'call' of undefined". - * * @return {!Promise} */ - disabledTestMethodCallDecomposingInAsyncFunction() { + testMethodCallDecomposingInAsyncFunction() { async function f() { return C.doSomething(await 5); } diff --git a/test/com/google/javascript/jscomp/runtime_tests/simulated_j2cl_test.js b/test/com/google/javascript/jscomp/runtime_tests/simulated_j2cl_test.js index 1c285970062..7e89170cf9e 100644 --- a/test/com/google/javascript/jscomp/runtime_tests/simulated_j2cl_test.js +++ b/test/com/google/javascript/jscomp/runtime_tests/simulated_j2cl_test.js @@ -22,11 +22,9 @@ goog.module('jscomp.test.static_async'); const testSuite = goog.require('goog.testing.testSuite'); -// TODO(b/69456597): Remove @nocollapse on Main's static methods. class Main { /** * @return {IThenable} - * @nocollapse */ static async ten() { return 10; @@ -34,7 +32,6 @@ class Main { /** * @return {IThenable} - * @nocollapse */ static async main() { return Main.same(await Main.ten()); @@ -43,7 +40,6 @@ class Main { /** * @param {number} i * @return {number} - * @nocollapse */ static same(i) { return i;