From e12413fe20f83520ed1942d60ef1e7d3958c9231 Mon Sep 17 00:00:00 2001 From: lharker Date: Tue, 30 Apr 2019 14:11:15 -0700 Subject: [PATCH] Make PolymerClassRewriter insert new nodes in MODULE_BODY nodes, not SCRIPTs ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=246018077 --- .../jscomp/ModuleImportResolver.java | 2 +- .../jscomp/PolymerClassRewriter.java | 45 +++++-- .../jscomp/PolymerClassRewriterTest.java | 112 +++++++++++++++++- 3 files changed, 140 insertions(+), 19 deletions(-) diff --git a/src/com/google/javascript/jscomp/ModuleImportResolver.java b/src/com/google/javascript/jscomp/ModuleImportResolver.java index dcecfcdf63e..b567121f579 100644 --- a/src/com/google/javascript/jscomp/ModuleImportResolver.java +++ b/src/com/google/javascript/jscomp/ModuleImportResolver.java @@ -41,7 +41,7 @@ final class ModuleImportResolver { } /** Returns whether this is a CALL node for goog.require(Type) or goog.forwardDeclare */ - boolean isGoogModuleDependencyCall(Node value) { + static boolean isGoogModuleDependencyCall(Node value) { if (value == null || !value.isCall()) { return false; } diff --git a/src/com/google/javascript/jscomp/PolymerClassRewriter.java b/src/com/google/javascript/jscomp/PolymerClassRewriter.java index 42d25a79b6f..9b83f1f4327 100644 --- a/src/com/google/javascript/jscomp/PolymerClassRewriter.java +++ b/src/com/google/javascript/jscomp/PolymerClassRewriter.java @@ -73,9 +73,10 @@ final class PolymerClassRewriter { * @param exprRoot The root expression of the call to Polymer({}). * @param cls The extracted {@link PolymerClassDefinition} for the Polymer element created by this * call. + * @param isInGlobalOrModuleScope whether this call is either directly in a script or module body */ void rewritePolymerCall( - Node exprRoot, final PolymerClassDefinition cls, boolean isInGlobalScope) { + Node exprRoot, final PolymerClassDefinition cls, boolean isInGlobalOrModuleScope) { Node objLit = checkNotNull(cls.descriptor); // Add {@code @lends} to the object literal. @@ -135,17 +136,22 @@ void rewritePolymerCall( Node statements = block.removeChildren(); Node parent = exprRoot.getParent(); - // If the call to Polymer() is not in the global scope and the assignment target - // is not namespaced (which likely means it's exported to the global scope), put the type - // declaration into the global scope at the start of the current script. - // - // This avoids unknown type warnings which are a result of the compiler's poor understanding of - // types declared inside IIFEs or any non-global scope. We should revisit this decision as - // the typechecker's support for non-global types improves. - if (!isInGlobalScope && !cls.target.isGetProp()) { - Node scriptNode = NodeUtil.getEnclosingScript(parent); - scriptNode.addChildrenToFront(statements); - compiler.reportChangeToChangeScope(scriptNode); + // Put the type declaration in to either the enclosing module scope, if in a module, or the + // enclosing script node. Compiler support for local scopes like IIFEs is sometimes lacking but + // module scopes are well-supported. If this is not in a module or the global scope it is likely + // exported. + if (!isInGlobalOrModuleScope && !cls.target.isGetProp()) { + Node scriptOrModuleNode = + NodeUtil.getEnclosingNode(parent, node -> node.isScript() || node.isModuleBody()); + if (scriptOrModuleNode.isModuleBody() + && scriptOrModuleNode.getParent().getBooleanProp(Node.GOOG_MODULE)) { + // The goog.module('ns'); call must remain the first statement in the module. + Node insertionPoint = getInsertionPointForGoogModule(scriptOrModuleNode); + scriptOrModuleNode.addChildrenAfter(statements, insertionPoint); + } else { + scriptOrModuleNode.addChildrenToFront(statements); + compiler.reportChangeToChangeScope(NodeUtil.getEnclosingScript(scriptOrModuleNode)); + } } else { Node beforeRoot = exprRoot.getPrevious(); if (beforeRoot == null) { @@ -988,4 +994,19 @@ private static boolean isParamLiteral(String param) { } return false; } + + private static Node getInsertionPointForGoogModule(Node moduleBody) { + checkArgument(moduleBody.isModuleBody(), moduleBody); + Node insertionPoint = moduleBody.getFirstChild(); // goog.module('ns'); + Node next = insertionPoint.getNext(); + while ((NodeUtil.isNameDeclaration(next) + && next.hasOneChild() + && ModuleImportResolver.isGoogModuleDependencyCall(next.getFirstFirstChild())) + || (NodeUtil.isExprCall(next) + && ModuleImportResolver.isGoogModuleDependencyCall(next.getOnlyChild()))) { + insertionPoint = next; + next = next.getNext(); + } + return insertionPoint; + } } diff --git a/test/com/google/javascript/jscomp/PolymerClassRewriterTest.java b/test/com/google/javascript/jscomp/PolymerClassRewriterTest.java index b626d05d836..e7784b9a4c3 100644 --- a/test/com/google/javascript/jscomp/PolymerClassRewriterTest.java +++ b/test/com/google/javascript/jscomp/PolymerClassRewriterTest.java @@ -64,6 +64,7 @@ public final class PolymerClassRewriterTest extends CompilerTypeTestCase { private Node rootNode; private GlobalNamespace globalNamespace; private Node polymerCall; + private boolean inGlobalScope; @Override @Before @@ -71,6 +72,7 @@ public void setUp() throws Exception { super.setUp(); polymerCall = null; rootNode = null; + inGlobalScope = true; } // TODO(jlklein): Add tests for non-global definitions, interface externs, read-only setters, etc. @@ -98,6 +100,103 @@ public void testVarTarget() { "};")); } + @Test + public void testVarTarget_inGoogModule() { + inGlobalScope = false; + test( + lines( + "goog.module('mod');", // + "var X = Polymer({", + " is: 'x-element',", + "});"), + lines( + "goog.module('mod');", + "/** @constructor @extends {PolymerElement} @implements {PolymerXInterface} */", + "var X = function() {};", + "X = Polymer(/** @lends {X.prototype} */ {", + " is: 'x-element',", + "});")); + + testSame( + lines( + "goog.module('mod');", + "var X = class extends Polymer.Element {", + " static get is() { return 'x-element'; }", + " static get properties { return { }; }", + "};")); + } + + @Test + public void testVarTarget_inIifeInGoogModule() { + inGlobalScope = false; + test( + lines( + "goog.module('mod');", // + "(function() {", + " var X = Polymer({", + " is: 'x-element',", + " });", + "})();"), + lines( + "goog.module('mod');", + "/** @constructor @extends {PolymerElement} @implements {PolymerXInterface} */", + "var X = function() {};", + "(function() {", + " X = Polymer(/** @lends {X.prototype} */ {", + " is: 'x-element',", + " });", + "})();")); + } + + @Test + public void testVarTarget_inGoogModuleWithRequires() { + inGlobalScope = false; + test( + lines( + "goog.module('mod');", // + "const Component = goog.require('goog.Component');", + "goog.forwardDeclare('something.else');", + "const someLocal = (function() { return 0; })();", + "var X = Polymer({", + " is: 'x-element',", + "});"), + lines( + "goog.module('mod');", + "const Component = goog.require('goog.Component');", + "goog.forwardDeclare('something.else');", + "/** @constructor @extends {PolymerElement} @implements {PolymerXInterface} */", + "var X = function() {};", + "const someLocal = (function() { return 0; })();", + "X = Polymer(/** @lends {X.prototype} */ {", + " is: 'x-element',", + "});")); + } + + @Test + public void testVarTarget_inEsModule() { + test( + lines( + "var X = Polymer({", // + " is: 'x-element',", + "});", + "export {X};"), + lines( + "/** @constructor @extends {PolymerElement} @implements {PolymerXInterface} */", + "var X = function() {};", + "X = Polymer(/** @lends {X.prototype} */ {", + " is: 'x-element',", + "});", + "export {X};")); + + testSame( + lines( + "var X = class extends Polymer.Element {", + " static get is() { return 'x-element'; }", + " static get properties { return { }; }", + "};", + "export {X};")); + } + @Test public void testDefaultTypeNameTarget() { test( @@ -144,6 +243,7 @@ protected CompilerOptions getDefaultOptions() { options.setWarningLevel( DiagnosticGroups.INVALID_CASTS, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING); + options.setWarningLevel(DiagnosticGroups.MODULE_LOAD, CheckLevel.OFF); options.setCodingConvention(getCodingConvention()); options.setPreserveTypeAnnotations(true); options.setPrettyPrint(true); @@ -153,21 +253,21 @@ protected CompilerOptions getDefaultOptions() { private void test(String originalCode, String expectedResult) { parseAndRewrite(originalCode, 1); Node expectedNode = compiler.parseSyntheticCode(expectedResult); - assertNode(expectedNode).isEqualTo(rootNode); + assertNode(rootNode).isEqualTo(expectedNode); parseAndRewrite(originalCode, 2); expectedNode = compiler.parseSyntheticCode(expectedResult); - assertNode(expectedNode).isEqualTo(rootNode); + assertNode(rootNode).isEqualTo(expectedNode); } private void testSame(String originalCode) { parseAndRewrite(originalCode, 1); Node expectedNode = compiler.parseSyntheticCode(originalCode); - assertNode(expectedNode).isEqualTo(rootNode); + assertNode(rootNode).isEqualTo(expectedNode); parseAndRewrite(originalCode, 2); expectedNode = compiler.parseSyntheticCode(originalCode); - assertNode(expectedNode).isEqualTo(rootNode); + assertNode(rootNode).isEqualTo(expectedNode); } private void parseAndRewrite(String code, int version) { @@ -203,9 +303,9 @@ public void visit(Node node) { Node parent = polymerCall.getParent(); Node grandparent = parent.getParent(); if (NodeUtil.isNameDeclaration(grandparent) || parent.isAssign()) { - rewriter.rewritePolymerCall(grandparent, classDef, true); + rewriter.rewritePolymerCall(grandparent, classDef, inGlobalScope); } else { - rewriter.rewritePolymerCall(parent, classDef, true); + rewriter.rewritePolymerCall(parent, classDef, inGlobalScope); } }