From 97f760d01d162bb8eebf0d6c47dc4abfb9fe6169 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Wed, 13 Jul 2016 17:16:03 -0700 Subject: [PATCH] Add explicit checks for a goog.provide()d name being defined multiple times, or being defined inside a function instead of at the top level. We already check for these in ConstCheck but that check will stop working when we move ConstCheck from optimizations to checks. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=127378544 --- .../javascript/jscomp/DiagnosticGroups.java | 3 + .../jscomp/ProcessClosurePrimitives.java | 55 +++++--- .../jscomp/CommonJSIntegrationTest.java | 14 +- .../javascript/jscomp/IntegrationTest.java | 35 +++-- .../jscomp/ProcessClosurePrimitivesTest.java | 132 ++++++------------ 5 files changed, 102 insertions(+), 137 deletions(-) diff --git a/src/com/google/javascript/jscomp/DiagnosticGroups.java b/src/com/google/javascript/jscomp/DiagnosticGroups.java index 28e81a1bbf9..d609499a5e5 100644 --- a/src/com/google/javascript/jscomp/DiagnosticGroups.java +++ b/src/com/google/javascript/jscomp/DiagnosticGroups.java @@ -346,6 +346,8 @@ public DiagnosticGroup forName(String name) { DiagnosticGroups.registerGroup("const", CheckAccessControls.CONST_PROPERTY_DELETED, CheckAccessControls.CONST_PROPERTY_REASSIGNED_VALUE, + ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE, + ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR, ConstCheck.CONST_REASSIGNED_VALUE_ERROR, NewTypeInference.CONST_REASSIGNED, NewTypeInference.CONST_PROPERTY_REASSIGNED); @@ -363,6 +365,7 @@ public DiagnosticGroup forName(String name) { public static final DiagnosticGroup DUPLICATE_VARS = DiagnosticGroups.registerGroup("duplicate", + ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR, VarCheck.VAR_MULTIPLY_DECLARED_ERROR, TypeValidator.DUP_VAR_DECLARATION, TypeValidator.DUP_VAR_DECLARATION_TYPE_MISMATCH, diff --git a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java index 82c2fcca9d0..0f2127aaccd 100644 --- a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java +++ b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java @@ -76,6 +76,16 @@ class ProcessClosurePrimitives extends AbstractPostOrderCallback "JSC_DUPLICATE_NAMESPACE_ERROR", "namespace \"{0}\" cannot be provided twice"); + static final DiagnosticType DUPLICATE_DEFINITION_ERROR = + DiagnosticType.error( + "JSC_DUPLICATE_DEFINITION_ERROR", + "Found multiple definitions of goog.provided name {0}."); + + static final DiagnosticType DEFINITION_NOT_IN_GLOBAL_SCOPE = + DiagnosticType.error( + "JSC_DEFINITION_NOT_IN_GLOBAL_SCOPE", + "Definition of goog.provided name {0} must be in the global hoist scope"); + static final DiagnosticType WEAK_NAMESPACE_TYPE = DiagnosticType.warning( "JSC_WEAK_NAMESPACE_TYPE", "Provided symbol declared with type Object. This is rarely useful. " @@ -503,23 +513,27 @@ private void handleTypedefDefinition( /** * Handles a candidate definition for a goog.provided name. */ - private void handleCandidateProvideDefinition( - NodeTraversal t, Node n, Node parent) { - if (t.inGlobalHoistScope()) { - String name = null; - if (n.isName() && NodeUtil.isNameDeclaration(parent)) { - name = n.getString(); - } else if (n.isAssign() && parent.isExprResult()) { - name = n.getFirstChild().getQualifiedName(); - } + private void handleCandidateProvideDefinition(NodeTraversal t, Node n, Node parent) { + String name = null; + if (n.isName() && NodeUtil.isNameDeclaration(parent)) { + name = n.getString(); + } else if (n.isAssign() && parent.isExprResult()) { + name = n.getFirstChild().getQualifiedName(); + } - if (name != null) { - if (parent.getBooleanProp(Node.IS_NAMESPACE)) { - processProvideFromPreviousPass(t, name, parent); - } else { - ProvidedName pn = providedNames.get(name); - if (pn != null) { + if (name != null) { + if (parent.getBooleanProp(Node.IS_NAMESPACE)) { + processProvideFromPreviousPass(t, name, parent); + } else { + ProvidedName pn = providedNames.get(name); + if (pn != null) { + if (t.inGlobalHoistScope()) { pn.addDefinition(parent, t.getModule()); + } else { + Var var = t.getScope().getVar(name); + if (var == null || var.isGlobal()) { + compiler.report(JSError.make(n, DEFINITION_NOT_IN_GLOBAL_SCOPE, name)); + } } } } @@ -1277,10 +1291,15 @@ void addDefinition(Node node, JSModule module) { || node.isFunction() || NodeUtil.isNameDeclaration(node)); Preconditions.checkArgument(explicitNode != node); - if ((candidateDefinition == null) || !node.isExprResult()) { - candidateDefinition = node; - updateMinimumModule(module); + if (candidateDefinition != null) { + Node errorNode = node.isExprResult() ? node.getFirstChild() : node; + compiler.report( + JSError.make( + errorNode, DUPLICATE_DEFINITION_ERROR, namespace, candidateDefinition.toString())); + return; } + candidateDefinition = node; + updateMinimumModule(module); } private void updateMinimumModule(JSModule newModule) { diff --git a/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java b/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java index e03cb852f64..77a44868755 100644 --- a/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java +++ b/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java @@ -108,17 +108,9 @@ public void testMultipleExportAssignments1() { "var Foobar = require('./i0');", "var show = new Foobar();", "show.foobar();")}, - new String[] { - LINE_JOINER.join( - "function Hello$$module$i0() {}", - "var module$i0 = Hello$$module$i0;", - "function Bar$$module$i0(){} ", - "Bar$$module$i0.prototype.foobar = function() { alert('foobar') };", - "module$i0 = Bar$$module$i0;"), - LINE_JOINER.join( - "var Foobar = module$i0;", - "var show = new Foobar();", - "show.foobar();")}); + // TODO(tbreisacher): Add a clearer error for this. The user didn't type goog.provide + // at all so mentioning goog.provide in the error is probably confusing. + ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR); } public void testMultipleExportAssignments2() { diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 21e22141504..b66d4499ea4 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -1265,7 +1265,7 @@ public void testClosurePassPreservesJsDoc() { "var COMPILED=true;var goog={};goog.exportSymbol=function(){};var Foo={a:3}"); } - public void testProvidedNamespaceIsConst() { + public void testProvidedNamespaceCannotBeReassigned1() { CompilerOptions options = createCompilerOptions(); options.setClosurePass(true); options.setInlineConstantVars(true); @@ -1276,27 +1276,38 @@ public void testProvidedNamespaceIsConst() { "var goog = {};", "goog.provide('foo');", "function f() { foo = {};}"), - LINE_JOINER.join( - "var foo = {};", - "function f() { foo = {}; }"), - ConstCheck.CONST_REASSIGNED_VALUE_ERROR); + ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE); } - public void testProvidedNamespaceIsConst2() { + public void testProvidedNamespaceCannotBeReassigned2() { CompilerOptions options = createCompilerOptions(); options.setClosurePass(true); options.setInlineConstantVars(true); options.setCollapseProperties(true); + options.setWarningLevel(DiagnosticGroups.ACCESS_CONTROLS, CheckLevel.ERROR); test( options, LINE_JOINER.join( "var goog = {};", "goog.provide('foo.bar');", + "foo.bar = function() {};", "function f() { foo.bar = {};}"), + ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE); + } + + public void testProvidedNamespaceCannotBeReassigned3() { + CompilerOptions options = createCompilerOptions(); + options.setClosurePass(true); + options.setInlineConstantVars(true); + options.setCollapseProperties(true); + options.setWarningLevel(DiagnosticGroups.ACCESS_CONTROLS, CheckLevel.ERROR); + test( + options, LINE_JOINER.join( - "var foo$bar = {};", - "function f() { foo$bar = {}; }"), - ConstCheck.CONST_REASSIGNED_VALUE_ERROR); + "var goog = {};", + "goog.provide('foo.bar');", + "function f() { foo.bar = {};}"), + ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE); } public void testProvidedNamespaceIsConst3() { @@ -1338,12 +1349,6 @@ public void testProvidedNamespaceIsConst5() { "var foo = {}; foo = {}; foo.Bar = {};"); } - public void testProcessDefinesAlwaysOn() { - test(createCompilerOptions(), - "/** @define {boolean} */ var HI = true; HI = false;", - "var HI = false;false;"); - } - public void testProcessDefinesAdditionalReplacements() { CompilerOptions options = createCompilerOptions(); options.setDefineToBooleanLiteral("HI", false); diff --git a/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java b/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java index 4f42afe1aab..f4784c538be 100644 --- a/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java +++ b/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java @@ -19,6 +19,8 @@ import static com.google.javascript.jscomp.ProcessClosurePrimitives.BASE_CLASS_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.CLASS_NAMESPACE_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.CLOSURE_DEFINES_ERROR; +import static com.google.javascript.jscomp.ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE; +import static com.google.javascript.jscomp.ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.DUPLICATE_NAMESPACE_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.EXPECTED_OBJECTLIT_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.FUNCTION_NAMESPACE_ERROR; @@ -235,15 +237,9 @@ public void testRemovalOfProvidedObjLit() { "foo.bar={};", "foo.bar.moo={E:1,S:2};")); - test( + testError( "goog.provide('foo.bar.moo'); foo.bar.moo={E:1}; foo.bar.moo={E:2};", - LINE_JOINER.join( - "/** @const */", - "var foo={};", - "/** @const */", - "foo.bar={};", - "foo.bar.moo={E:1};", - "foo.bar.moo={E:2};")); + DUPLICATE_DEFINITION_ERROR); testEs6("goog.provide('foo'); var foo = class {}", "var foo = class {}"); } @@ -256,35 +252,42 @@ public void testProvidedDeclaredClassError() { testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR); } - public void testRemovalMultipleAssignment1() { - test("goog.provide('foo'); foo = 0; foo = 1", - "var foo = 0; foo = 1;"); + public void testMultipleAssignment1() { + testError( + "goog.provide('foo'); foo = 0; foo = 1", + DUPLICATE_DEFINITION_ERROR); } - public void testRemovalMultipleAssignment2() { - test("goog.provide('foo'); var foo = 0; foo = 1", - "var foo = 0; foo = 1;"); - testEs6("goog.provide('foo'); let foo = 0; let foo = 1", - "let foo = 0; let foo = 1;"); + public void testMultipleAssignment2() { + testError( + "goog.provide('foo'); var foo = 0; foo = 1", + DUPLICATE_DEFINITION_ERROR); + + testErrorEs6( + "goog.provide('foo'); let foo = 0; let foo = 1", + DUPLICATE_DEFINITION_ERROR); } - public void testRemovalMultipleAssignment3() { - test("goog.provide('foo'); foo = 0; var foo = 1", - "foo = 0; var foo = 1;"); - testEs6("goog.provide('foo'); foo = 0; let foo = 1", - "foo = 0; let foo = 1;"); + public void testMultipleAssignment3() { + testError( + "goog.provide('foo'); foo = 0; var foo = 1", + DUPLICATE_DEFINITION_ERROR); + + testErrorEs6( + "goog.provide('foo'); foo = 0; let foo = 1", + DUPLICATE_DEFINITION_ERROR); } - public void testRemovalMultipleAssignment4() { - test( + public void testMultipleAssignment4() { + testError( "goog.provide('foo.bar'); foo.bar = 0; foo.bar = 1", - "/** @const */ var foo = {}; foo.bar = 0; foo.bar = 1"); + DUPLICATE_DEFINITION_ERROR); } public void testNoRemovalFunction1() { - test( + testError( "goog.provide('foo'); function f(){foo = 0}", - "/** @const */ var foo = {}; function f(){foo = 0}"); + DEFINITION_NOT_IN_GLOBAL_SCOPE); } public void testNoRemovalFunction2() { @@ -299,65 +302,26 @@ public void testNoRemovalFunction3() { "/** @const */ var foo = {}; function f(foo = 0){}"); } - public void testRemovalMultipleAssignmentInIf1() { - test("goog.provide('foo'); if (true) { var foo = 0 } else { foo = 1 }", - "if (true) { var foo = 0 } else { foo = 1 }"); - } - - public void testRemovalMultipleAssignmentInIf2() { - test("goog.provide('foo'); if (true) { foo = 0 } else { var foo = 1 }", - "if (true) { foo = 0 } else { var foo = 1 }"); - } - - public void testRemovalMultipleAssignmentInIf3() { - test("goog.provide('foo'); if (true) { foo = 0 } else { foo = 1 }", - "if (true) { var foo = 0 } else { foo = 1 }"); - } - - public void testRemovalMultipleAssignmentInIf4() { - test( - "goog.provide('foo.bar'); if (true) { foo.bar = 0 } else { foo.bar = 1 }", - LINE_JOINER.join( - "/** @const */ var foo = {};", - "if (true) {", - " foo.bar = 0;", - "} else {", - " foo.bar = 1;", - "}")); - } - public void testMultipleDeclarationError1() { - String rest = "if (true) { foo.bar = 0 } else { foo.bar = 1 }"; - test("goog.provide('foo.bar');" + "var foo = {};" + rest, - "var foo = {};" + "var foo = {};" + rest); + testError( + "goog.provide('foo.bar'); var foo = {}; if (true) { foo.bar = 0 } else { foo.bar = 1 }", + DUPLICATE_DEFINITION_ERROR); } public void testMultipleDeclarationError2() { - test( + testError( LINE_JOINER.join( "goog.provide('foo.bar');", "if (true) { var foo = {}; foo.bar = 0 } else { foo.bar = 1 }"), - LINE_JOINER.join( - "var foo = {};", - "if (true) {", - " var foo = {}; foo.bar = 0", - "} else {", - " foo.bar = 1", - "}")); + DUPLICATE_DEFINITION_ERROR); } public void testMultipleDeclarationError3() { - test( + testError( LINE_JOINER.join( "goog.provide('foo.bar');", "if (true) { foo.bar = 0 } else { var foo = {}; foo.bar = 1 }"), - LINE_JOINER.join( - "var foo = {};", - "if (true) {", - " foo.bar = 0", - "} else {", - " var foo = {}; foo.bar = 1", - "}")); + DUPLICATE_DEFINITION_ERROR); } public void testProvideAfterDeclarationError() { @@ -751,25 +715,8 @@ public void testProvideOrder3b() { "a.b.c;")); } - public void testProvideOrder4a() { - test( - LINE_JOINER.join( - "goog.provide('goog.a');", - "goog.provide('goog.a.b');", - "if (x) {", - " goog.a.b = 1;", - "} else {", - " goog.a.b = 2;", - "}"), - LINE_JOINER.join( - "/** @const */", "goog.a={};", "if(x)", " goog.a.b=1;", "else", " goog.a.b=2;")); - } - - public void testProvideOrder4b() { - additionalEndCode = ""; - addAdditionalNamespace = false; - // This tests a cleanly provided name, below a namespace. - test( + public void testProvideOrder4() { + testError( LINE_JOINER.join( "goog.provide('goog.a');", "goog.provide('goog.a.b');", @@ -778,8 +725,7 @@ public void testProvideOrder4b() { "} else {", " goog.a.b = 2;", "}"), - LINE_JOINER.join( - "/** @const */", "goog.a={};", "if(x)", " goog.a.b=1;", "else", " goog.a.b=2;")); + DUPLICATE_DEFINITION_ERROR); } public void testInvalidProvide() {