Skip to content

Commit

Permalink
Add explicit checks for a goog.provide()d name being defined multiple…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Jul 14, 2016
1 parent 42037f6 commit 97f760d
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 137 deletions.
3 changes: 3 additions & 0 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
View file
Expand Up @@ -346,6 +346,8 @@ public DiagnosticGroup forName(String name) {
DiagnosticGroups.registerGroup("const", DiagnosticGroups.registerGroup("const",
CheckAccessControls.CONST_PROPERTY_DELETED, CheckAccessControls.CONST_PROPERTY_DELETED,
CheckAccessControls.CONST_PROPERTY_REASSIGNED_VALUE, CheckAccessControls.CONST_PROPERTY_REASSIGNED_VALUE,
ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE,
ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR,
ConstCheck.CONST_REASSIGNED_VALUE_ERROR, ConstCheck.CONST_REASSIGNED_VALUE_ERROR,
NewTypeInference.CONST_REASSIGNED, NewTypeInference.CONST_REASSIGNED,
NewTypeInference.CONST_PROPERTY_REASSIGNED); NewTypeInference.CONST_PROPERTY_REASSIGNED);
Expand All @@ -363,6 +365,7 @@ public DiagnosticGroup forName(String name) {


public static final DiagnosticGroup DUPLICATE_VARS = public static final DiagnosticGroup DUPLICATE_VARS =
DiagnosticGroups.registerGroup("duplicate", DiagnosticGroups.registerGroup("duplicate",
ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR,
VarCheck.VAR_MULTIPLY_DECLARED_ERROR, VarCheck.VAR_MULTIPLY_DECLARED_ERROR,
TypeValidator.DUP_VAR_DECLARATION, TypeValidator.DUP_VAR_DECLARATION,
TypeValidator.DUP_VAR_DECLARATION_TYPE_MISMATCH, TypeValidator.DUP_VAR_DECLARATION_TYPE_MISMATCH,
Expand Down
55 changes: 37 additions & 18 deletions src/com/google/javascript/jscomp/ProcessClosurePrimitives.java
View file
Expand Up @@ -76,6 +76,16 @@ class ProcessClosurePrimitives extends AbstractPostOrderCallback
"JSC_DUPLICATE_NAMESPACE_ERROR", "JSC_DUPLICATE_NAMESPACE_ERROR",
"namespace \"{0}\" cannot be provided twice"); "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( static final DiagnosticType WEAK_NAMESPACE_TYPE = DiagnosticType.warning(
"JSC_WEAK_NAMESPACE_TYPE", "JSC_WEAK_NAMESPACE_TYPE",
"Provided symbol declared with type Object. This is rarely useful. " "Provided symbol declared with type Object. This is rarely useful. "
Expand Down Expand Up @@ -503,23 +513,27 @@ private void handleTypedefDefinition(
/** /**
* Handles a candidate definition for a goog.provided name. * Handles a candidate definition for a goog.provided name.
*/ */
private void handleCandidateProvideDefinition( private void handleCandidateProvideDefinition(NodeTraversal t, Node n, Node parent) {
NodeTraversal t, Node n, Node parent) { String name = null;
if (t.inGlobalHoistScope()) { if (n.isName() && NodeUtil.isNameDeclaration(parent)) {
String name = null; name = n.getString();
if (n.isName() && NodeUtil.isNameDeclaration(parent)) { } else if (n.isAssign() && parent.isExprResult()) {
name = n.getString(); name = n.getFirstChild().getQualifiedName();
} else if (n.isAssign() && parent.isExprResult()) { }
name = n.getFirstChild().getQualifiedName();
}


if (name != null) { if (name != null) {
if (parent.getBooleanProp(Node.IS_NAMESPACE)) { if (parent.getBooleanProp(Node.IS_NAMESPACE)) {
processProvideFromPreviousPass(t, name, parent); processProvideFromPreviousPass(t, name, parent);
} else { } else {
ProvidedName pn = providedNames.get(name); ProvidedName pn = providedNames.get(name);
if (pn != null) { if (pn != null) {
if (t.inGlobalHoistScope()) {
pn.addDefinition(parent, t.getModule()); 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));
}
} }
} }
} }
Expand Down Expand Up @@ -1277,10 +1291,15 @@ void addDefinition(Node node, JSModule module) {
|| node.isFunction() || node.isFunction()
|| NodeUtil.isNameDeclaration(node)); || NodeUtil.isNameDeclaration(node));
Preconditions.checkArgument(explicitNode != node); Preconditions.checkArgument(explicitNode != node);
if ((candidateDefinition == null) || !node.isExprResult()) { if (candidateDefinition != null) {
candidateDefinition = node; Node errorNode = node.isExprResult() ? node.getFirstChild() : node;
updateMinimumModule(module); compiler.report(
JSError.make(
errorNode, DUPLICATE_DEFINITION_ERROR, namespace, candidateDefinition.toString()));
return;
} }
candidateDefinition = node;
updateMinimumModule(module);
} }


private void updateMinimumModule(JSModule newModule) { private void updateMinimumModule(JSModule newModule) {
Expand Down
14 changes: 3 additions & 11 deletions test/com/google/javascript/jscomp/CommonJSIntegrationTest.java
View file
Expand Up @@ -108,17 +108,9 @@ public void testMultipleExportAssignments1() {
"var Foobar = require('./i0');", "var Foobar = require('./i0');",
"var show = new Foobar();", "var show = new Foobar();",
"show.foobar();")}, "show.foobar();")},
new String[] { // TODO(tbreisacher): Add a clearer error for this. The user didn't type goog.provide
LINE_JOINER.join( // at all so mentioning goog.provide in the error is probably confusing.
"function Hello$$module$i0() {}", ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR);
"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() { public void testMultipleExportAssignments2() {
Expand Down
35 changes: 20 additions & 15 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -1265,7 +1265,7 @@ public void testClosurePassPreservesJsDoc() {
"var COMPILED=true;var goog={};goog.exportSymbol=function(){};var Foo={a:3}"); "var COMPILED=true;var goog={};goog.exportSymbol=function(){};var Foo={a:3}");
} }


public void testProvidedNamespaceIsConst() { public void testProvidedNamespaceCannotBeReassigned1() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
options.setClosurePass(true); options.setClosurePass(true);
options.setInlineConstantVars(true); options.setInlineConstantVars(true);
Expand All @@ -1276,27 +1276,38 @@ public void testProvidedNamespaceIsConst() {
"var goog = {};", "var goog = {};",
"goog.provide('foo');", "goog.provide('foo');",
"function f() { foo = {};}"), "function f() { foo = {};}"),
LINE_JOINER.join( ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE);
"var foo = {};",
"function f() { foo = {}; }"),
ConstCheck.CONST_REASSIGNED_VALUE_ERROR);
} }


public void testProvidedNamespaceIsConst2() { public void testProvidedNamespaceCannotBeReassigned2() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
options.setClosurePass(true); options.setClosurePass(true);
options.setInlineConstantVars(true); options.setInlineConstantVars(true);
options.setCollapseProperties(true); options.setCollapseProperties(true);
options.setWarningLevel(DiagnosticGroups.ACCESS_CONTROLS, CheckLevel.ERROR);
test( test(
options, options,
LINE_JOINER.join( LINE_JOINER.join(
"var goog = {};", "var goog = {};",
"goog.provide('foo.bar');", "goog.provide('foo.bar');",
"foo.bar = function() {};",
"function f() { foo.bar = {};}"), "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( LINE_JOINER.join(
"var foo$bar = {};", "var goog = {};",
"function f() { foo$bar = {}; }"), "goog.provide('foo.bar');",
ConstCheck.CONST_REASSIGNED_VALUE_ERROR); "function f() { foo.bar = {};}"),
ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE);
} }


public void testProvidedNamespaceIsConst3() { public void testProvidedNamespaceIsConst3() {
Expand Down Expand Up @@ -1338,12 +1349,6 @@ public void testProvidedNamespaceIsConst5() {
"var foo = {}; foo = {}; foo.Bar = {};"); "var foo = {}; foo = {}; foo.Bar = {};");
} }


public void testProcessDefinesAlwaysOn() {
test(createCompilerOptions(),
"/** @define {boolean} */ var HI = true; HI = false;",
"var HI = false;false;");
}

public void testProcessDefinesAdditionalReplacements() { public void testProcessDefinesAdditionalReplacements() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
options.setDefineToBooleanLiteral("HI", false); options.setDefineToBooleanLiteral("HI", false);
Expand Down
132 changes: 39 additions & 93 deletions test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java
View file
Expand Up @@ -19,6 +19,8 @@
import static com.google.javascript.jscomp.ProcessClosurePrimitives.BASE_CLASS_ERROR; 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.CLASS_NAMESPACE_ERROR;
import static com.google.javascript.jscomp.ProcessClosurePrimitives.CLOSURE_DEFINES_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.DUPLICATE_NAMESPACE_ERROR;
import static com.google.javascript.jscomp.ProcessClosurePrimitives.EXPECTED_OBJECTLIT_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.EXPECTED_OBJECTLIT_ERROR;
import static com.google.javascript.jscomp.ProcessClosurePrimitives.FUNCTION_NAMESPACE_ERROR; import static com.google.javascript.jscomp.ProcessClosurePrimitives.FUNCTION_NAMESPACE_ERROR;
Expand Down Expand Up @@ -235,15 +237,9 @@ public void testRemovalOfProvidedObjLit() {
"foo.bar={};", "foo.bar={};",
"foo.bar.moo={E:1,S:2};")); "foo.bar.moo={E:1,S:2};"));


test( testError(
"goog.provide('foo.bar.moo'); foo.bar.moo={E:1}; foo.bar.moo={E:2};", "goog.provide('foo.bar.moo'); foo.bar.moo={E:1}; foo.bar.moo={E:2};",
LINE_JOINER.join( DUPLICATE_DEFINITION_ERROR);
"/** @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 {}"); testEs6("goog.provide('foo'); var foo = class {}", "var foo = class {}");
} }
Expand All @@ -256,35 +252,42 @@ public void testProvidedDeclaredClassError() {
testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR); testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR);
} }


public void testRemovalMultipleAssignment1() { public void testMultipleAssignment1() {
test("goog.provide('foo'); foo = 0; foo = 1", testError(
"var foo = 0; foo = 1;"); "goog.provide('foo'); foo = 0; foo = 1",
DUPLICATE_DEFINITION_ERROR);
} }


public void testRemovalMultipleAssignment2() { public void testMultipleAssignment2() {
test("goog.provide('foo'); var foo = 0; foo = 1", testError(
"var foo = 0; foo = 1;"); "goog.provide('foo'); var foo = 0; foo = 1",
testEs6("goog.provide('foo'); let foo = 0; let foo = 1", DUPLICATE_DEFINITION_ERROR);
"let foo = 0; let foo = 1;");
testErrorEs6(
"goog.provide('foo'); let foo = 0; let foo = 1",
DUPLICATE_DEFINITION_ERROR);
} }


public void testRemovalMultipleAssignment3() { public void testMultipleAssignment3() {
test("goog.provide('foo'); foo = 0; var foo = 1", testError(
"foo = 0; var foo = 1;"); "goog.provide('foo'); foo = 0; var foo = 1",
testEs6("goog.provide('foo'); foo = 0; let foo = 1", DUPLICATE_DEFINITION_ERROR);
"foo = 0; let foo = 1;");
testErrorEs6(
"goog.provide('foo'); foo = 0; let foo = 1",
DUPLICATE_DEFINITION_ERROR);
} }


public void testRemovalMultipleAssignment4() { public void testMultipleAssignment4() {
test( testError(
"goog.provide('foo.bar'); foo.bar = 0; foo.bar = 1", "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() { public void testNoRemovalFunction1() {
test( testError(
"goog.provide('foo'); function f(){foo = 0}", "goog.provide('foo'); function f(){foo = 0}",
"/** @const */ var foo = {}; function f(){foo = 0}"); DEFINITION_NOT_IN_GLOBAL_SCOPE);
} }


public void testNoRemovalFunction2() { public void testNoRemovalFunction2() {
Expand All @@ -299,65 +302,26 @@ public void testNoRemovalFunction3() {
"/** @const */ var foo = {}; function f(foo = 0){}"); "/** @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() { public void testMultipleDeclarationError1() {
String rest = "if (true) { foo.bar = 0 } else { foo.bar = 1 }"; testError(
test("goog.provide('foo.bar');" + "var foo = {};" + rest, "goog.provide('foo.bar'); var foo = {}; if (true) { foo.bar = 0 } else { foo.bar = 1 }",
"var foo = {};" + "var foo = {};" + rest); DUPLICATE_DEFINITION_ERROR);
} }


public void testMultipleDeclarationError2() { public void testMultipleDeclarationError2() {
test( testError(
LINE_JOINER.join( LINE_JOINER.join(
"goog.provide('foo.bar');", "goog.provide('foo.bar');",
"if (true) { var foo = {}; foo.bar = 0 } else { foo.bar = 1 }"), "if (true) { var foo = {}; foo.bar = 0 } else { foo.bar = 1 }"),
LINE_JOINER.join( DUPLICATE_DEFINITION_ERROR);
"var foo = {};",
"if (true) {",
" var foo = {}; foo.bar = 0",
"} else {",
" foo.bar = 1",
"}"));
} }


public void testMultipleDeclarationError3() { public void testMultipleDeclarationError3() {
test( testError(
LINE_JOINER.join( LINE_JOINER.join(
"goog.provide('foo.bar');", "goog.provide('foo.bar');",
"if (true) { foo.bar = 0 } else { var foo = {}; foo.bar = 1 }"), "if (true) { foo.bar = 0 } else { var foo = {}; foo.bar = 1 }"),
LINE_JOINER.join( DUPLICATE_DEFINITION_ERROR);
"var foo = {};",
"if (true) {",
" foo.bar = 0",
"} else {",
" var foo = {}; foo.bar = 1",
"}"));
} }


public void testProvideAfterDeclarationError() { public void testProvideAfterDeclarationError() {
Expand Down Expand Up @@ -751,25 +715,8 @@ public void testProvideOrder3b() {
"a.b.c;")); "a.b.c;"));
} }


public void testProvideOrder4a() { public void testProvideOrder4() {
test( testError(
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(
LINE_JOINER.join( LINE_JOINER.join(
"goog.provide('goog.a');", "goog.provide('goog.a');",
"goog.provide('goog.a.b');", "goog.provide('goog.a.b');",
Expand All @@ -778,8 +725,7 @@ public void testProvideOrder4b() {
"} else {", "} else {",
" goog.a.b = 2;", " goog.a.b = 2;",
"}"), "}"),
LINE_JOINER.join( DUPLICATE_DEFINITION_ERROR);
"/** @const */", "goog.a={};", "if(x)", " goog.a.b=1;", "else", " goog.a.b=2;"));
} }


public void testInvalidProvide() { public void testInvalidProvide() {
Expand Down

0 comments on commit 97f760d

Please sign in to comment.