From 931352f95a87823e17d12adbb2aa8ba6d7bb2ba6 Mon Sep 17 00:00:00 2001 From: bangert Date: Mon, 26 Jun 2017 15:41:38 -0700 Subject: [PATCH] [RefasterJS] Add support for correctly handling goog.require inside a goog.module. - Attempts to detect whether a file should use const foo = goog.require or just goog.require. - If using short names, rewrite the template to use aliases as appropriate. - Fix sorting so the goog.require statements added by SuggestedFix are handled better. The sorting still isn't ideal (but there's the sort imports script + g4 fix). - Use originalName in place of name nodes when replacing (otherwise de-sugared code will be emitted). - To accomplish all this, modify the compiler to preserve goog.module sugar when running refasterjs. At least for the given refactorings, replacements on the sugared + de-sugared source code apply cleanly to the original source. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=160205920 --- .../jscomp/ClosureRewriteModule.java | 29 +++-- .../javascript/jscomp/CompilerOptions.java | 25 +++- .../jscomp/ProcessClosurePrimitives.java | 7 +- .../javascript/jscomp/TemplateAstMatcher.java | 4 +- .../javascript/refactoring/Matchers.java | 6 +- .../refactoring/RefactoringDriver.java | 9 +- .../refactoring/RefasterJsScanner.java | 36 ++++-- .../javascript/refactoring/SuggestedFix.java | 86 +++++++++++++- .../javascript/jscomp/CodePrinterTest.java | 1 + .../refactoring/ErrorToFixMapperTest.java | 4 +- .../javascript/refactoring/MatchersTest.java | 11 ++ .../refactoring/RefasterJsScannerTest.java | 110 +++++++++++++++++- .../refactoring/SuggestedFixTest.java | 79 +++++++++++++ .../NavigationalXssSinksRefactoringTest.java | 11 +- .../examples/testdata/goog_base.js | 3 + .../refactoring/examples/testdata/goog_foo.js | 22 ++++ .../navigational_xss_sinks_test_module_in.js | 33 ++++++ .../navigational_xss_sinks_test_module_out.js | 34 ++++++ 18 files changed, 466 insertions(+), 44 deletions(-) create mode 100644 test/com/google/javascript/refactoring/examples/testdata/goog_foo.js create mode 100644 test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_in.js create mode 100644 test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_out.js diff --git a/src/com/google/javascript/jscomp/ClosureRewriteModule.java b/src/com/google/javascript/jscomp/ClosureRewriteModule.java index c97c6973dee..74e2771e686 100644 --- a/src/com/google/javascript/jscomp/ClosureRewriteModule.java +++ b/src/com/google/javascript/jscomp/ClosureRewriteModule.java @@ -198,6 +198,7 @@ final class ClosureRewriteModule implements HotSwapCompilerPass { private final AbstractCompiler compiler; private final PreprocessorSymbolTable preprocessorSymbolTable; + private final boolean preserveSugar; /** * Indicates where new nodes should be added in relation to some other node. @@ -646,6 +647,7 @@ void removeRoot(Node toRemove) { this.compiler = compiler; this.preprocessorSymbolTable = preprocessorSymbolTable; this.rewriteState = moduleRewriteState != null ? moduleRewriteState : new GlobalRewriteState(); + this.preserveSugar = compiler.getOptions().shouldPreserveGoogModule(); } private class UnwrapGoogLoadModule extends NodeTraversal.AbstractPreOrderCallback { @@ -993,7 +995,7 @@ private void updateGoogModule(Node call) { exportTheEmptyBinaryNamespaceAt(NodeUtil.getEnclosingStatement(call), AddAt.AFTER); } - if (!currentScript.declareLegacyNamespace) { + if (!currentScript.declareLegacyNamespace && !preserveSugar) { // Otherwise it's a regular module and the goog.module() line can be removed. compiler.reportChangeToEnclosingScope(call); NodeUtil.getEnclosingStatement(call).detach(); @@ -1056,9 +1058,11 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { if (currentScript.isModule || targetIsNonLegacyGoogModule) { if (isDestructuring) { - // Delete the goog.require() because we're going to inline its alias later. - compiler.reportChangeToEnclosingScope(statementNode); - statementNode.detach(); + if (!preserveSugar) { + // Delete the goog.require() because we're going to inline its alias later. + compiler.reportChangeToEnclosingScope(statementNode); + statementNode.detach(); + } } else if (targetIsNonLegacyGoogModule) { if (!isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { // Rewrite @@ -1069,11 +1073,15 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { call.replaceWith(binaryNamespaceName); compiler.reportChangeToEnclosingScope(binaryNamespaceName); } else if (importHasAlias || !rewriteState.isLegacyModule(legacyNamespace)) { - // Delete the goog.require() because we're going to inline its alias later. - compiler.reportChangeToEnclosingScope(statementNode); - statementNode.detach(); + if (!preserveSugar) { + // Delete the goog.require() because we're going to inline its alias later. + compiler.reportChangeToEnclosingScope(statementNode); + statementNode.detach(); + } } } else { + // TODO(bangert): make this compatible with preserveSugar. const B = goog.require('b') runs + // into problems because the type checker cannot handle const. // Rewrite // "var B = goog.require('B');" to // "goog.require('B');" @@ -1083,7 +1091,7 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { statementNode.replaceWith(IR.exprResult(call)); compiler.reportChangeToEnclosingScope(call); } - if (targetIsNonLegacyGoogModule) { + if (targetIsNonLegacyGoogModule && !preserveSugar) { // Add goog.require() and namespace name to preprocessor table because they're removed // by current pass. If target is not a module then goog.require() is retained for // ProcessClosurePrimitives pass and symbols will be added there instead. @@ -1472,7 +1480,6 @@ private void maybeSplitMultiVar(Node rhsNode) { Node nameNode = rhsNode.getParent(); nameNode.detach(); rhsNode.detach(); - statementNode.getParent().addChildBefore(IR.var(nameNode, rhsNode), statementNode); } @@ -1529,7 +1536,9 @@ private void reportUnrecognizedRequires() { legacyNamespace)); // Remove the require node so this problem isn't reported all over again in // ProcessClosurePrimitives. - NodeUtil.getEnclosingStatement(requireNode).detach(); + if (!preserveSugar) { + NodeUtil.getEnclosingStatement(requireNode).detach(); + } continue; } diff --git a/src/com/google/javascript/jscomp/CompilerOptions.java b/src/com/google/javascript/jscomp/CompilerOptions.java index 491e56e80d6..143e935aaa0 100644 --- a/src/com/google/javascript/jscomp/CompilerOptions.java +++ b/src/com/google/javascript/jscomp/CompilerOptions.java @@ -755,7 +755,7 @@ public void setReplaceMessagesWithChromeI18n( public boolean closurePass; /** Do not strip goog.provide()/goog.require() calls from the code. */ - private boolean preserveGoogProvidesAndRequires; + private boolean preserveClosurePrimitives; /** Processes AngularJS-specific annotations */ boolean angularPass; @@ -1263,7 +1263,7 @@ public CompilerOptions() { locale = null; markAsCompiled = false; closurePass = false; - preserveGoogProvidesAndRequires = false; + preserveClosurePrimitives = false; angularPass = false; polymerVersion = null; dartPass = false; @@ -2426,12 +2426,27 @@ public void setClosurePass(boolean closurePass) { this.closurePass = closurePass; } + /** Preserve closure primitives. + * + * For now, this only preserves goog.provide(), goog.require() and goog.module() calls. + */ + public void setPreserveClosurePrimitives(boolean preserveClosurePrimitives) { + this.preserveClosurePrimitives = preserveClosurePrimitives; + } + + // TODO(bangert): Delete this alias once it has been deprecated for 3 months. + /** Preserve goog.provide(), goog.require() and goog.module() calls. */ + @Deprecated public void setPreserveGoogProvidesAndRequires(boolean preserveGoogProvidesAndRequires) { - this.preserveGoogProvidesAndRequires = preserveGoogProvidesAndRequires; + setPreserveClosurePrimitives(preserveGoogProvidesAndRequires); } public boolean shouldPreservesGoogProvidesAndRequires() { - return this.preserveGoogProvidesAndRequires || this.shouldGenerateTypedExterns(); + return this.preserveClosurePrimitives || this.shouldGenerateTypedExterns(); + } + + public boolean shouldPreserveGoogModule() { + return this.preserveClosurePrimitives; } public void setPreserveTypeAnnotations(boolean preserveTypeAnnotations) { @@ -2874,7 +2889,7 @@ public String toString() { .add("preferSingleQuotes", preferSingleQuotes) .add("preferStableNames", preferStableNames) .add("preserveDetailedSourceInfo", preservesDetailedSourceInfo()) - .add("preserveGoogProvidesAndRequires", preserveGoogProvidesAndRequires) + .add("preserveGoogProvidesAndRequires", preserveClosurePrimitives) .add("preserveTypeAnnotations", preserveTypeAnnotations) .add("prettyPrint", prettyPrint) .add("preventLibraryInjection", preventLibraryInjection) diff --git a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java index e21329aec54..c947ec5834c 100644 --- a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java +++ b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java @@ -375,8 +375,11 @@ public void visit(NodeTraversal t, Node n, Node parent) { private boolean validPrimitiveCall(NodeTraversal t, Node n) { if (!n.getParent().isExprResult() || !t.inGlobalHoistScope()) { - compiler.report(t.makeError(n, INVALID_CLOSURE_CALL_ERROR)); - return false; + // Ignore invalid primitives if we didn't strip module sugar. + if (!compiler.getOptions().shouldPreserveGoogModule()) { + compiler.report(t.makeError(n, INVALID_CLOSURE_CALL_ERROR)); + return false; + } } return true; } diff --git a/src/com/google/javascript/jscomp/TemplateAstMatcher.java b/src/com/google/javascript/jscomp/TemplateAstMatcher.java index 3a7455fb6b1..00b002f19b8 100644 --- a/src/com/google/javascript/jscomp/TemplateAstMatcher.java +++ b/src/com/google/javascript/jscomp/TemplateAstMatcher.java @@ -440,7 +440,9 @@ private boolean matchesNode(Node template, Node ast) { // subsequent usages of the same named node are equivalent. return ast.getString().equals(this.localVarMatches.get(paramIndex)); } else { - this.localVarMatches.set(paramIndex, ast.getString()); + String originalName = ast.getOriginalName(); + String name = (originalName != null) ? originalName : ast.getString(); + this.localVarMatches.set(paramIndex, name); } } else if (isTemplateParameterStringLiteralNode(template)) { int paramIndex = (int) (template.getDouble()); diff --git a/src/com/google/javascript/refactoring/Matchers.java b/src/com/google/javascript/refactoring/Matchers.java index d8ba521cc86..702774a769d 100644 --- a/src/com/google/javascript/refactoring/Matchers.java +++ b/src/com/google/javascript/refactoring/Matchers.java @@ -221,12 +221,16 @@ public boolean matches(Node node, NodeMetadata metadata) { }; } + public static Matcher googModule() { + return functionCall("goog.module"); + } + public static Matcher googRequire() { return functionCall("goog.require"); } public static Matcher googModuleOrProvide() { - return anyOf(functionCall("goog.module"), functionCall("goog.provide")); + return anyOf(googModule(), functionCall("goog.provide")); } /** diff --git a/src/com/google/javascript/refactoring/RefactoringDriver.java b/src/com/google/javascript/refactoring/RefactoringDriver.java index 403dc9e1be9..cff51322cf3 100644 --- a/src/com/google/javascript/refactoring/RefactoringDriver.java +++ b/src/com/google/javascript/refactoring/RefactoringDriver.java @@ -106,10 +106,13 @@ public static CompilerOptions getCompilerOptions() { options.setCheckSuspiciousCode(true); options.setCheckSymbols(true); options.setCheckTypes(true); - options.setClosurePass(true); + options.setBrokenClosureRequiresLevel(CheckLevel.OFF); + // TODO(bangert): Remove this -- we want to rewrite code before closure syntax is removed. + // Unfortunately, setClosurePass is required, or code doesn't type check. + options.setClosurePass(true); options.setGenerateExports(true); - options.setPreserveGoogProvidesAndRequires(true); - + options.setPreserveClosurePrimitives(true); + options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING); diff --git a/src/com/google/javascript/refactoring/RefasterJsScanner.java b/src/com/google/javascript/refactoring/RefasterJsScanner.java index 1fbfa5fee24..b426cb92b03 100644 --- a/src/com/google/javascript/refactoring/RefasterJsScanner.java +++ b/src/com/google/javascript/refactoring/RefasterJsScanner.java @@ -29,6 +29,7 @@ import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.SourceFile; import com.google.javascript.jscomp.TypeMatchingStrategy; +import com.google.javascript.rhino.IR; import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.TypeIRegistry; @@ -140,10 +141,18 @@ public List processMatch(Match match) { .isEquivalentTo(matchedTemplate.afterTemplate.getLastChild())) { return ImmutableList.of(); } + + HashMap shortNames = new HashMap<>(); + for (String require : matchedTemplate.getGoogRequiresToAdd()) { + fix.addGoogRequire(match, require); + shortNames.put(require, fix.getRequireName(match, require)); + } + Node newNode = transformNode( matchedTemplate.afterTemplate.getLastChild(), - matchedTemplate.matcher.getTemplateNodeToMatchMap()); + matchedTemplate.matcher.getTemplateNodeToMatchMap(), + shortNames); Node nodeToReplace = match.getNode(); fix.attachMatchedNodeInfo(nodeToReplace, match.getMetadata().getCompiler()); fix.replace(nodeToReplace, newNode, match.getMetadata().getCompiler()); @@ -161,10 +170,7 @@ public List processMatch(Match match) { fix.delete(n); n = n.getNext(); } - // Add/remove any goog.requires - for (String require : matchedTemplate.getGoogRequiresToAdd()) { - fix.addGoogRequire(match, require); - } + for (String require : matchedTemplate.getGoogRequiresToRemove()) { fix.removeGoogRequire(match, require); } @@ -172,10 +178,13 @@ public List processMatch(Match match) { } /** - * Transforms the template node to a replacement node by mapping the template names to - * the ones that were matched against in the JsSourceMatcher. + * Transforms the template node to a replacement node by mapping the template names to the ones + * that were matched against in the JsSourceMatcher. */ - private Node transformNode(Node templateNode, Map templateNodeToMatchMap) { + private Node transformNode( + Node templateNode, + Map templateNodeToMatchMap, + Map shortNames) { Node clone = templateNode.cloneNode(); if (templateNode.isName()) { String name = templateNode.getString(); @@ -201,8 +210,17 @@ private Node transformNode(Node templateNode, Map templateNodeToMa clone.putBooleanProp(Node.FREE_CALL, false); } } + if (templateNode.isQualifiedName()) { + String name = templateNode.getQualifiedName(); + if (shortNames.containsKey(name)) { + String shortName = shortNames.get(name); + if (!shortName.equals(name)) { + return IR.name(shortNames.get(name)); + } + } + } for (Node child : templateNode.children()) { - clone.addChildToBack(transformNode(child, templateNodeToMatchMap)); + clone.addChildToBack(transformNode(child, templateNodeToMatchMap, shortNames)); } return clone; } diff --git a/src/com/google/javascript/refactoring/SuggestedFix.java b/src/com/google/javascript/refactoring/SuggestedFix.java index e740d30c7c9..6917910c698 100644 --- a/src/com/google/javascript/refactoring/SuggestedFix.java +++ b/src/com/google/javascript/refactoring/SuggestedFix.java @@ -27,6 +27,8 @@ import com.google.javascript.jscomp.AbstractCompiler; import com.google.javascript.jscomp.CodePrinter; import com.google.javascript.jscomp.CompilerOptions; +import com.google.javascript.jscomp.NodeTraversal; +import com.google.javascript.jscomp.NodeTraversal.AbstractPreOrderCallback; import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.parsing.JsDocInfoParser; import com.google.javascript.rhino.IR; @@ -99,6 +101,7 @@ public SetMultimap getReplacements() { return sb.toString(); } + // TODO(bangert): Find a non-conflicting name. static String getShortNameForRequire(String namespace) { int lastDot = namespace.lastIndexOf('.'); if (lastDot == -1) { @@ -201,7 +204,8 @@ public Builder insertBefore(Node nodeToInsertBefore, String content) { private Builder insertBefore(Node nodeToInsertBefore, String content, String sortKey) { int startPosition = nodeToInsertBefore.getSourceOffset(); JSDocInfo jsDoc = NodeUtil.getBestJSDocInfo(nodeToInsertBefore); - if (jsDoc != null) { + // ClosureRewriteModule adds jsDOC everywhere. + if (jsDoc != null && jsDoc.getOriginalCommentString() != null) { startPosition = jsDoc.getOriginalCommentPosition(); } Preconditions.checkNotNull(nodeToInsertBefore.getSourceFileName(), @@ -603,7 +607,7 @@ public Builder addLhsToGoogRequire(Match m, String namespace) { insertBefore(existingNode, "const " + shortName + " = "); return this; } - + /** * Adds a goog.require for the given namespace to the file if it does not * already exist. @@ -630,8 +634,8 @@ public Builder addGoogRequire(Match m, String namespace) { IR.string(namespace)); String shortName = getShortNameForRequire(namespace); - - if (script.isModuleBody()) { + boolean useConstRequire = usesConstGoogRequires(metadata, script); + if (useConstRequire) { googRequireNode = IR.constNode(IR.name(shortName), googRequireNode); } else { googRequireNode = IR.exprResult(googRequireNode); @@ -642,6 +646,9 @@ public Builder addGoogRequire(Match m, String namespace) { Node nodeToInsertBefore = null; Node child = script.getFirstChild(); while (child != null) { + if (Matchers.googModule().matches(child, metadata)) { + lastModuleOrProvideNode = child; + } if (NodeUtil.isExprCall(child)) { // TODO(mknichel): Replace this logic with a function argument // Matcher when it exists. @@ -659,7 +666,13 @@ public Builder addGoogRequire(Match m, String namespace) { } else if (NodeUtil.isNameDeclaration(child) && child.getFirstFirstChild() != null && Matchers.googRequire().matches(child.getFirstFirstChild(), metadata)) { - if (shortName.compareTo(child.getFirstChild().getString()) < 0) { + lastGoogRequireNode = child.getFirstFirstChild(); + String requireName = child.getFirstChild().getString(); + String originalName = child.getFirstChild().getOriginalName(); + if (originalName != null) { + requireName = originalName; + } + if (shortName.compareTo(requireName) < 0) { nodeToInsertBefore = child; break; } @@ -700,6 +713,39 @@ public Builder addGoogRequire(Match m, String namespace) { nodeToInsertBefore, googRequireNode, m.getMetadata().getCompiler(), namespace); } + /** + * If the namespace has a short name, return it. Otherwise return the full name. + * + *

Assumes {@link addGoogRequire} was already called. + */ + public String getRequireName(Match m, String namespace) { + Node existingNode = findGoogRequireNode(m.getNode(), m.getMetadata(), namespace); + if (existingNode != null && (existingNode.isConst() || existingNode.isVar())) { + Node lhsAssign = existingNode.getFirstChild(); + String originalName = lhsAssign.getOriginalName(); + if (originalName != null) { + return originalName; // The import was renamed inside a module. + } + return lhsAssign.getQualifiedName(); + } + Node script = NodeUtil.getEnclosingScript(m.getNode()); + + if (script != null && usesConstGoogRequires(m.getMetadata(), script)) { + return getShortNameForRequire(namespace); + } + return namespace; + } + + /** True if the file uses {@code const foo = goog.require('namespace.foo');} */ + private boolean usesConstGoogRequires(final NodeMetadata metadata, Node script) { + if (script.isModuleBody()) { + return true; + } + HasConstRequireOrModuleCallback callback = new HasConstRequireOrModuleCallback(metadata); + NodeTraversal.traverseEs6(metadata.getCompiler(), script, callback); + return callback.getUsesConstRequires(); + } + /** * Removes a goog.require for the given namespace to the file if it * already exists. @@ -826,4 +872,34 @@ public boolean isInClosurizedFile() { return isInClosurizedFile; } } + + /** Traverse an AST and find {@code goog.module} or {@code const X = goog.require('...');}. */ + private static class HasConstRequireOrModuleCallback extends AbstractPreOrderCallback { + private boolean usesConstRequires; + final NodeMetadata metadata; + + public HasConstRequireOrModuleCallback(NodeMetadata metadata) { + this.usesConstRequires = false; + this.metadata = metadata; + } + + boolean getUsesConstRequires() { + return usesConstRequires; + } + + @Override + public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) { + if (Matchers.googModule().matches(n, metadata) || isConstRequire(n, metadata)) { + usesConstRequires = true; + return false; + } + return true; + } + + private static boolean isConstRequire(Node node, NodeMetadata metadata) { + return node.isConst() + && node.getFirstFirstChild() != null + && Matchers.googRequire().matches(node.getFirstFirstChild(), metadata); + } + } } diff --git a/test/com/google/javascript/jscomp/CodePrinterTest.java b/test/com/google/javascript/jscomp/CodePrinterTest.java index 087d19ed8cd..c1ad10524d7 100644 --- a/test/com/google/javascript/jscomp/CodePrinterTest.java +++ b/test/com/google/javascript/jscomp/CodePrinterTest.java @@ -2670,6 +2670,7 @@ public void testEs6GoogModule() { + "}\n" + "exports.fn = fn;\n"; String expectedCode = "" + + "goog.module('foo.bar');\n" + "void 0;\n" + "var module$exports$foo$bar = {};\n" + "const STR = '3';\n" diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index a84017423e5..54d737f6b9f 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -651,7 +651,7 @@ public void testMissingRequire_unsorted1() { @Test public void testMissingRequire_unsorted2() { // Both the fix for requires being unsorted, and the fix for the missing require, are applied. - // However, the end result is still out of order. + // The end result is ordered. assertChanges( LINE_JOINER.join( "goog.module('module');", @@ -664,10 +664,10 @@ public void testMissingRequire_unsorted2() { "alert(new DomHelper());"), LINE_JOINER.join( "goog.module('module');", - "const Xray = goog.require('goog.rays.Xray');", "", "const Anteater = goog.require('goog.Anteater');", "const DomHelper = goog.require('goog.dom.DomHelper');", + "const Xray = goog.require('goog.rays.Xray');", "", "alert(new Anteater());", "alert(new Xray());", diff --git a/test/com/google/javascript/refactoring/MatchersTest.java b/test/com/google/javascript/refactoring/MatchersTest.java index 8dcc8d11847..972620c4a11 100644 --- a/test/com/google/javascript/refactoring/MatchersTest.java +++ b/test/com/google/javascript/refactoring/MatchersTest.java @@ -208,6 +208,17 @@ public void testFunctionCall_prototype() { assertFalse(Matchers.functionCall("Foo.prototype.baz").matches(fnCall, metadata)); } + @Test + public void testGoogModule() { + String input = "goog.module('testcase');"; + Compiler compiler = getCompiler(input); + Node root = compileToScriptRoot(compiler); + Node fnCall = root.getFirstFirstChild(); + NodeMetadata metadata = new NodeMetadata(compiler); + assertTrue(Matchers.googModule().matches(fnCall, metadata)); + assertTrue(Matchers.googModuleOrProvide().matches(fnCall, metadata)); + } + @Test public void testEnum() { String input = "/** @enum {string} */ var foo = {BAR: 'baz'};"; diff --git a/test/com/google/javascript/refactoring/RefasterJsScannerTest.java b/test/com/google/javascript/refactoring/RefasterJsScannerTest.java index b3e75cd124a..b1685e1b9f4 100644 --- a/test/com/google/javascript/refactoring/RefasterJsScannerTest.java +++ b/test/com/google/javascript/refactoring/RefasterJsScannerTest.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.javascript.jscomp.Compiler; @@ -32,7 +33,6 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; - /** * Unit tests for {RefasterJsScanner}. * @@ -727,6 +727,95 @@ public void test_unknownTemplateTypesNonNullable() throws Exception { assertChanges(externs, originalCode, null, template); } + @Test + public void test_importConstGoogRequire() throws Exception { + String externs = ""; + String originalCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "", + "function f() { var loc = 'str'; }"); + String expectedCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "const foo = goog.require('goog.foo');", + "", + "function f() { var loc = foo.f(); }"); + String template = + Joiner.on('\n').join( + "/**", + "* +require {goog.foo}", + "*/", + "function before_foo() {", + " var a = 'str';", + "};", + "function after_foo() {", + " var a = goog.foo.f();", + "}"); + assertChanges(externs, originalCode, expectedCode, template); + } + + @Test + public void test_importConstGoogRequireMultipleImports() throws Exception { + String externs = ""; + String originalCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "const alpha = goog.require('goog.alpha');", + "const omega = goog.require('goog.omega');", + "", + "function f() { var loc = 'str'; }"); + String expectedCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "const alpha = goog.require('goog.alpha');", + "const foo = goog.require('goog.foo');", + "const omega = goog.require('goog.omega');", + "", + "function f() { var loc = foo.f(); }"); + String template = + Joiner.on('\n').join( + "/**", + "* +require {goog.foo}", + "*/", + "function before_foo() {", + " var a = 'str';", + "};", + "function after_foo() {", + " var a = goog.foo.f();", + "}"); + assertChanges(externs, originalCode, expectedCode, template); + } + + @Test + public void test_importConstGoogRequireAlreadyNamed() throws Exception { + String externs = ""; + String originalCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "const bar = goog.require('goog.foo');", + "", + "var loc = 'str';"); + String expectedCode = + Joiner.on('\n').join( + "goog.module('testcase');", + "const bar = goog.require('goog.foo');", + "", + "var loc = bar.f();"); + String template = + Joiner.on('\n').join( + "/**", + "* +require {goog.foo}", + "*/", + "function before_foo() {", + " var a = 'str';", + "};", + "function after_foo() {", + " var a = goog.foo.f();", + "}"); + assertChanges(externs, originalCode, expectedCode, template); + } + private static Compiler createCompiler() { return new Compiler(); } @@ -753,10 +842,21 @@ private static void assertChanges( RefasterJsScanner scanner = new RefasterJsScanner(); scanner.loadRefasterJsTemplateFromCode(refasterJsTemplate); - RefactoringDriver driver = new RefactoringDriver.Builder(scanner) - .addExternsFromCode("function Symbol() {};" + externs) - .addInputsFromCode(originalCode) - .build(); + RefactoringDriver driver = + new RefactoringDriver.Builder(scanner) + .addExternsFromCode("function Symbol() {};" + externs) + .addInputsFromCode(originalCode) + .addInputsFromCode( + Joiner.on('\n').join( + "goog.module('goog.foo');", + "/** Trivial function. \n", + " * @return {string}", + " */", + "exports.f = function () { ", + " return 'str';", + "};"), + "foo.js") + .build(); List fixes = driver.drive(); String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( fixes, ImmutableMap.of("input", originalCode)).get("input"); diff --git a/test/com/google/javascript/refactoring/SuggestedFixTest.java b/test/com/google/javascript/refactoring/SuggestedFixTest.java index d48c80577de..2e289c19b51 100644 --- a/test/com/google/javascript/refactoring/SuggestedFixTest.java +++ b/test/com/google/javascript/refactoring/SuggestedFixTest.java @@ -786,6 +786,85 @@ public void testAddGoogRequire_alreadyExists() { assertThat(replacementMap).isEmpty(); } + @Test + public void testAddRequireModule() { + String input = + Joiner.on('\n').join( + "goog.module('js.Foo');", + "", + "/** @private */", + "function foo_() {", + "}"); + String expected = + Joiner.on('\n').join( + "goog.module('js.Foo');", + "const safe = goog.require('goog.safe');", + "", + "/** @private */", + "function foo_() {", + "}"); + Compiler compiler = getCompiler(input); + Node root = compileToScriptRoot(compiler); + Match match = new Match(root.getFirstChild(), new NodeMetadata(compiler)); + SuggestedFix fix = new SuggestedFix.Builder().addGoogRequire(match, "goog.safe").build(); + assertChanges(fix, "", input, expected); + } + + @Test + public void testAddRequireConst() { + String input = + Joiner.on('\n').join( + "const bar = goog.require('goog.bar');", + "", + "/** @private */", + "function foo_() {};"); + String expected = + Joiner.on('\n').join( + "const bar = goog.require('goog.bar');", + "const safe = goog.require('goog.safe');", + "", + "/** @private */", + "function foo_() {};"); + Compiler compiler = getCompiler(input); + Node root = compileToScriptRoot(compiler); + Match match = new Match(root.getFirstChild(), new NodeMetadata(compiler)); + SuggestedFix fix = new SuggestedFix.Builder().addGoogRequire(match, "goog.safe").build(); + assertChanges(fix, "", input, expected); + } + + @Test + public void testAddRequireModuleUnchanged() { + String input = + Joiner.on('\n').join( + "goog.module('js.Foo');", + "const safe = goog.require('goog.safe');", + "", + "/** @private */", + "function foo_() {};"); + Compiler compiler = getCompiler(input); + Node root = compileToScriptRoot(compiler); + Match match = new Match(root.getFirstChild(), new NodeMetadata(compiler)); + SuggestedFix fix = new SuggestedFix.Builder().addGoogRequire(match, "goog.safe").build(); + assertThat(fix.getReplacements()).isEmpty(); + } + + @Test + public void testAddRequireModuleDifferentNameUnchanged() { + String input = + Joiner.on('\n').join( + "goog.module('js.Foo');", + "const googSafe = goog.require('goog.safe');", + "", + "/** @private */", + "function foo_() {};"); + Compiler compiler = getCompiler(input); + Node root = compileToScriptRoot(compiler); + Match match = new Match(root.getFirstChild(), new NodeMetadata(compiler)); + SuggestedFix.Builder fixBuilder = new SuggestedFix.Builder().addGoogRequire(match, "goog.safe"); + assertThat(fixBuilder.getRequireName(match, "goog.safe")).isEqualTo("googSafe"); + assertThat(fixBuilder.build().getReplacements()).isEmpty(); + } + private static void assertAddGoogRequire(String before, String after, String namespace) { Compiler compiler = getCompiler(before + after); Node root = compileToScriptRoot(compiler); diff --git a/test/com/google/javascript/refactoring/examples/NavigationalXssSinksRefactoringTest.java b/test/com/google/javascript/refactoring/examples/NavigationalXssSinksRefactoringTest.java index 608b841a2d0..7de58dc1b11 100644 --- a/test/com/google/javascript/refactoring/examples/NavigationalXssSinksRefactoringTest.java +++ b/test/com/google/javascript/refactoring/examples/NavigationalXssSinksRefactoringTest.java @@ -18,7 +18,6 @@ import static com.google.javascript.refactoring.testing.RefasterJsTestUtils.assertFileRefactoring; import com.google.common.collect.ImmutableList; - import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -46,4 +45,14 @@ public void test_refactorings() throws Exception { ImmutableList.of("goog_base.js"), "navigational_xss_sinks_test_out.js"); } + + @Test + public void testModuleRefactoring() throws Exception { + assertFileRefactoring( + NAVIGATIONAL_XSS_SINKS_TEMPLATE, + TESTDATA_DIR, + "navigational_xss_sinks_test_module_in.js", + ImmutableList.of("goog_base.js", "goog_foo.js"), + "navigational_xss_sinks_test_module_out.js"); + } } diff --git a/test/com/google/javascript/refactoring/examples/testdata/goog_base.js b/test/com/google/javascript/refactoring/examples/testdata/goog_base.js index 76cff422a94..df313374f3e 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/goog_base.js +++ b/test/com/google/javascript/refactoring/examples/testdata/goog_base.js @@ -24,3 +24,6 @@ goog.provide = function(name) {}; /** @param {string} name */ goog.require = function(name) {}; + +/** @param {string} name */ +goog.module = function(name) {}; diff --git a/test/com/google/javascript/refactoring/examples/testdata/goog_foo.js b/test/com/google/javascript/refactoring/examples/testdata/goog_foo.js new file mode 100644 index 00000000000..2e75a157ba1 --- /dev/null +++ b/test/com/google/javascript/refactoring/examples/testdata/goog_foo.js @@ -0,0 +1,22 @@ +/* + * Copyright 2016 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** @fileoverview Mock namespace . */ + +goog.module('goog.foo'); + +/** Does nothing. */ +exports.bar = function() {}; diff --git a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_in.js b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_in.js new file mode 100644 index 00000000000..409b12e820a --- /dev/null +++ b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_in.js @@ -0,0 +1,33 @@ +/* + * Copyright 2016 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @fileoverview Test cases for the navigational_xss_sinks.js RefasterJs + * template. + */ + +goog.module('refactoring_testcase'); +const foo = goog.require('goog.foo'); + +/** + * @param {!Location} target The target. + * @param {string} val The value. + */ +exports.nonnull_location_href = function(target, val) { + foo.bar(); + // Should match before_setLocationHref. + target.href = val; +}; diff --git a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_out.js b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_out.js new file mode 100644 index 00000000000..b8d6543b762 --- /dev/null +++ b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_module_out.js @@ -0,0 +1,34 @@ +/* + * Copyright 2016 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @fileoverview Test cases for the navigational_xss_sinks.js RefasterJs + * template. + */ + +goog.module('refactoring_testcase'); +const foo = goog.require('goog.foo'); +const safe = goog.require('goog.dom.safe'); + +/** + * @param {!Location} target The target. + * @param {string} val The value. + */ +exports.nonnull_location_href = function(target, val) { + foo.bar(); + // Should match before_setLocationHref. + safe.setLocationHref(target, val); +};