Skip to content

Commit

Permalink
Automated g4 rollback of changelist 127378544.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Produces too many errors which are not worth fixing

*** Original change description ***

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=127582799
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Jul 18, 2016
1 parent af9cf94 commit 8b154ff
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 102 deletions.
3 changes: 0 additions & 3 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
View file
Expand Up @@ -346,8 +346,6 @@ 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 @@ -365,7 +363,6 @@ 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: 18 additions & 37 deletions src/com/google/javascript/jscomp/ProcessClosurePrimitives.java
View file
Expand Up @@ -76,16 +76,6 @@ 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 @@ -513,27 +503,23 @@ private void handleTypedefDefinition(
/** /**
* Handles a candidate definition for a goog.provided name. * Handles a candidate definition for a goog.provided name.
*/ */
private void handleCandidateProvideDefinition(NodeTraversal t, Node n, Node parent) { private void handleCandidateProvideDefinition(
String name = null; NodeTraversal t, Node n, Node parent) {
if (n.isName() && NodeUtil.isNameDeclaration(parent)) { if (t.inGlobalHoistScope()) {
name = n.getString(); String name = null;
} else if (n.isAssign() && parent.isExprResult()) { if (n.isName() && NodeUtil.isNameDeclaration(parent)) {
name = n.getFirstChild().getQualifiedName(); name = n.getString();
} } 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 @@ -1291,15 +1277,10 @@ 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) { if ((candidateDefinition == null) || !node.isExprResult()) {
Node errorNode = node.isExprResult() ? node.getFirstChild() : node; candidateDefinition = node;
compiler.report( updateMinimumModule(module);
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: 11 additions & 3 deletions test/com/google/javascript/jscomp/CommonJSIntegrationTest.java
View file
Expand Up @@ -108,9 +108,17 @@ public void testMultipleExportAssignments1() {
"var Foobar = require('./i0');", "var Foobar = require('./i0');",
"var show = new Foobar();", "var show = new Foobar();",
"show.foobar();")}, "show.foobar();")},
// TODO(tbreisacher): Add a clearer error for this. The user didn't type goog.provide new String[] {
// at all so mentioning goog.provide in the error is probably confusing. LINE_JOINER.join(
ProcessClosurePrimitives.DUPLICATE_DEFINITION_ERROR); "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() { public void testMultipleExportAssignments2() {
Expand Down
35 changes: 15 additions & 20 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 testProvidedNamespaceCannotBeReassigned1() { public void testProvidedNamespaceIsConst() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
options.setClosurePass(true); options.setClosurePass(true);
options.setInlineConstantVars(true); options.setInlineConstantVars(true);
Expand All @@ -1276,38 +1276,27 @@ public void testProvidedNamespaceCannotBeReassigned1() {
"var goog = {};", "var goog = {};",
"goog.provide('foo');", "goog.provide('foo');",
"function f() { 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( LINE_JOINER.join(
"var goog = {};", "var foo = {};",
"goog.provide('foo.bar');", "function f() { foo = {}; }"),
"foo.bar = function() {};", ConstCheck.CONST_REASSIGNED_VALUE_ERROR);
"function f() { foo.bar = {};}"),
ProcessClosurePrimitives.DEFINITION_NOT_IN_GLOBAL_SCOPE);
} }


public void testProvidedNamespaceCannotBeReassigned3() { public void testProvidedNamespaceIsConst2() {
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');",
"function f() { 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() { public void testProvidedNamespaceIsConst3() {
Expand Down Expand Up @@ -1349,6 +1338,12 @@ 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: 93 additions & 39 deletions test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java
View file
Expand Up @@ -19,8 +19,6 @@
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 @@ -237,9 +235,15 @@ public void testRemovalOfProvidedObjLit() {
"foo.bar={};", "foo.bar={};",
"foo.bar.moo={E:1,S:2};")); "foo.bar.moo={E:1,S:2};"));


testError( test(
"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};",
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 {}"); testEs6("goog.provide('foo'); var foo = class {}", "var foo = class {}");
} }
Expand All @@ -252,42 +256,35 @@ public void testProvidedDeclaredClassError() {
testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR); testErrorEs6("goog.provide('foo'); class foo {}", CLASS_NAMESPACE_ERROR);
} }


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


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

"let foo = 0; let foo = 1;");
testErrorEs6(
"goog.provide('foo'); let foo = 0; let foo = 1",
DUPLICATE_DEFINITION_ERROR);
} }


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

"foo = 0; let foo = 1;");
testErrorEs6(
"goog.provide('foo'); foo = 0; let foo = 1",
DUPLICATE_DEFINITION_ERROR);
} }


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


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


public void testMultipleDeclarationError2() { public void testMultipleDeclarationError2() {
testError( test(
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 }"),
DUPLICATE_DEFINITION_ERROR); LINE_JOINER.join(
"var foo = {};",
"if (true) {",
" var foo = {}; foo.bar = 0",
"} else {",
" foo.bar = 1",
"}"));
} }


public void testMultipleDeclarationError3() { public void testMultipleDeclarationError3() {
testError( test(
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 }"),
DUPLICATE_DEFINITION_ERROR); LINE_JOINER.join(
"var foo = {};",
"if (true) {",
" foo.bar = 0",
"} else {",
" var foo = {}; foo.bar = 1",
"}"));
} }


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


public void testProvideOrder4() { public void testProvideOrder4a() {
testError( 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 @@ -725,7 +761,25 @@ public void testProvideOrder4() {
"} else {", "} else {",
" goog.a.b = 2;", " 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() { public void testInvalidProvide() {
Expand Down

0 comments on commit 8b154ff

Please sign in to comment.