diff --git a/src/com/google/javascript/jscomp/DiagnosticGroups.java b/src/com/google/javascript/jscomp/DiagnosticGroups.java index d609499a5e5..28e81a1bbf9 100644 --- a/src/com/google/javascript/jscomp/DiagnosticGroups.java +++ b/src/com/google/javascript/jscomp/DiagnosticGroups.java @@ -346,8 +346,6 @@ 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); @@ -365,7 +363,6 @@ 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 0f2127aaccd..82c2fcca9d0 100644 --- a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java +++ b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java @@ -76,16 +76,6 @@ 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. " @@ -513,27 +503,23 @@ private void handleTypedefDefinition( /** * Handles a candidate definition for a goog.provided name. */ - 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(); - } + 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(); + } - 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()) { + if (name != null) { + if (parent.getBooleanProp(Node.IS_NAMESPACE)) { + processProvideFromPreviousPass(t, name, parent); + } else { + ProvidedName pn = providedNames.get(name); + if (pn != null) { 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)); - } } } } @@ -1291,15 +1277,10 @@ void addDefinition(Node node, JSModule module) { || node.isFunction() || NodeUtil.isNameDeclaration(node)); Preconditions.checkArgument(explicitNode != node); - if (candidateDefinition != null) { - Node errorNode = node.isExprResult() ? node.getFirstChild() : node; - compiler.report( - JSError.make( - errorNode, DUPLICATE_DEFINITION_ERROR, namespace, candidateDefinition.toString())); - return; + if ((candidateDefinition == null) || !node.isExprResult()) { + candidateDefinition = node; + updateMinimumModule(module); } - 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 77a44868755..e03cb852f64 100644 --- a/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java +++ b/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java @@ -108,9 +108,17 @@ public void testMultipleExportAssignments1() { "var Foobar = require('./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); + 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();")}); } public void testMultipleExportAssignments2() { diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index b66d4499ea4..21e22141504 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 testProvidedNamespaceCannotBeReassigned1() { + public void testProvidedNamespaceIsConst() { CompilerOptions options = createCompilerOptions(); options.setClosurePass(true); options.setInlineConstantVars(true); @@ -1276,38 +1276,27 @@ public void testProvidedNamespaceCannotBeReassigned1() { "var goog = {};", "goog.provide('foo');", "function f() { foo = {};}"), - ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE); - } - - 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); + "var foo = {};", + "function f() { foo = {}; }"), + ConstCheck.CONST_REASSIGNED_VALUE_ERROR); } - public void testProvidedNamespaceCannotBeReassigned3() { + public void testProvidedNamespaceIsConst2() { 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');", "function f() { foo.bar = {};}"), - ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE); + LINE_JOINER.join( + "var foo$bar = {};", + "function f() { foo$bar = {}; }"), + ConstCheck.CONST_REASSIGNED_VALUE_ERROR); } public void testProvidedNamespaceIsConst3() { @@ -1349,6 +1338,12 @@ 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 f4784c538be..4f42afe1aab 100644 --- a/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java +++ b/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java @@ -19,8 +19,6 @@ 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; @@ -237,9 +235,15 @@ public void testRemovalOfProvidedObjLit() { "foo.bar={};", "foo.bar.moo={E:1,S:2};")); - testError( + test( "goog.provide('foo.bar.moo'); foo.bar.moo={E:1}; foo.bar.moo={E:2};", - DUPLICATE_DEFINITION_ERROR); + LINE_JOINER.join( + "/** @const */", + "var foo={};", + "/** @const */", + "foo.bar={};", + "foo.bar.moo={E:1};", + "foo.bar.moo={E:2};")); testEs6("goog.provide('foo'); var foo = class {}", "var foo = class {}"); } @@ -252,42 +256,35 @@ public void testProvidedDeclaredClassError() { testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR); } - public void testMultipleAssignment1() { - testError( - "goog.provide('foo'); foo = 0; foo = 1", - DUPLICATE_DEFINITION_ERROR); + public void testRemovalMultipleAssignment1() { + test("goog.provide('foo'); foo = 0; foo = 1", + "var foo = 0; 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 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 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 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 testMultipleAssignment4() { - testError( + public void testRemovalMultipleAssignment4() { + test( "goog.provide('foo.bar'); foo.bar = 0; foo.bar = 1", - DUPLICATE_DEFINITION_ERROR); + "/** @const */ var foo = {}; foo.bar = 0; foo.bar = 1"); } public void testNoRemovalFunction1() { - testError( + test( "goog.provide('foo'); function f(){foo = 0}", - DEFINITION_NOT_IN_GLOBAL_SCOPE); + "/** @const */ var foo = {}; function f(){foo = 0}"); } public void testNoRemovalFunction2() { @@ -302,26 +299,65 @@ 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() { - testError( - "goog.provide('foo.bar'); var foo = {}; if (true) { foo.bar = 0 } else { foo.bar = 1 }", - DUPLICATE_DEFINITION_ERROR); + String rest = "if (true) { foo.bar = 0 } else { foo.bar = 1 }"; + test("goog.provide('foo.bar');" + "var foo = {};" + rest, + "var foo = {};" + "var foo = {};" + rest); } public void testMultipleDeclarationError2() { - testError( + test( LINE_JOINER.join( "goog.provide('foo.bar');", "if (true) { var foo = {}; foo.bar = 0 } else { foo.bar = 1 }"), - DUPLICATE_DEFINITION_ERROR); + LINE_JOINER.join( + "var foo = {};", + "if (true) {", + " var foo = {}; foo.bar = 0", + "} else {", + " foo.bar = 1", + "}")); } public void testMultipleDeclarationError3() { - testError( + test( LINE_JOINER.join( "goog.provide('foo.bar');", "if (true) { foo.bar = 0 } else { var foo = {}; foo.bar = 1 }"), - DUPLICATE_DEFINITION_ERROR); + LINE_JOINER.join( + "var foo = {};", + "if (true) {", + " foo.bar = 0", + "} else {", + " var foo = {}; foo.bar = 1", + "}")); } public void testProvideAfterDeclarationError() { @@ -715,8 +751,8 @@ public void testProvideOrder3b() { "a.b.c;")); } - public void testProvideOrder4() { - testError( + public void testProvideOrder4a() { + test( LINE_JOINER.join( "goog.provide('goog.a');", "goog.provide('goog.a.b');", @@ -725,7 +761,25 @@ public void testProvideOrder4() { "} else {", " goog.a.b = 2;", "}"), - DUPLICATE_DEFINITION_ERROR); + 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( + 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 testInvalidProvide() {