Skip to content

Commit

Permalink
Warn on early variable references in different scopes in the same fun…
Browse files Browse the repository at this point in the history 
…ction

We used to warn on cases like
function f() {
  if (true) { x; }
  var x;
}
but seem to have accidentally stopped warning when we introduced ES6 block scopes.

This does still back off on references inside function scopes, to handle cases where functions definitions are hoisted to the beginning of a block but called later.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=210153436
  • Loading branch information
@lauraharker @blickly
lauraharker authored and blickly committed Aug 27, 2018
1 parent 148ba99 commit bdf047c
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 46 deletions.
17 changes: 12 additions & 5 deletions src/com/google/javascript/jscomp/VariableReferenceCheck.java
View file
Expand Up @@ -264,7 +264,10 @@ private void checkVar(Var v, List<Reference> references) {
} }


// Check for temporal dead zone of let / const declarations in for-in and for-of loops // Check for temporal dead zone of let / const declarations in for-in and for-of loops
if ((v.isLet() || v.isConst()) && v.getScope() == reference.getScope() // TODO(b/111441110): Fix this check. it causes spurious warnings on `b = a` in
// for (const [a, b = a] of []) {}
if ((v.isLet() || v.isConst())
&& v.getScope() == reference.getScope()
&& NodeUtil.isEnhancedFor(reference.getScope().getRootNode())) { && NodeUtil.isEnhancedFor(reference.getScope().getRootNode())) {
compiler.report(JSError.make(referenceNode, EARLY_REFERENCE_ERROR, v.name)); compiler.report(JSError.make(referenceNode, EARLY_REFERENCE_ERROR, v.name));
} }
Expand Down Expand Up @@ -391,7 +394,7 @@ private boolean checkRedeclaration(
* @return If an early reference has been found * @return If an early reference has been found
*/ */
private boolean checkEarlyReference(Var v, Reference reference, Node referenceNode) { private boolean checkEarlyReference(Var v, Reference reference, Node referenceNode) {
// Don't check the order of refer in externs files. // Don't check the order of references in externs files.
if (!referenceNode.isFromExterns()) { if (!referenceNode.isFromExterns()) {
// Special case to deal with var goog = goog || {}. Note that // Special case to deal with var goog = goog || {}. Note that
// let x = x || {} is illegal, just like var y = x || {}; let x = y; // let x = x || {} is illegal, just like var y = x || {}; let x = y;
Expand All @@ -405,10 +408,14 @@ private boolean checkEarlyReference(Var v, Reference reference, Node referenceNo
} }
} }


// Only generate warnings if the scopes do not match in order // Only generate warnings for early references in the same function scope/global scope in
// to deal with possible forward declarations and recursion // order to deal with possible forward declarations and recursion
// e.g. don't warn on:
// function f() { return x; } f(); let x = 5;
// We don't track where `f` is called, just where it's defined, and don't want to warn for
// function f() { return x; } let x = 5; f();
// TODO(moz): See if we can remove the bypass for "goog" // TODO(moz): See if we can remove the bypass for "goog"
if (reference.getScope() == v.scope && !v.getName().equals("goog")) { if (reference.getScope().hasSameContainerScope(v.scope) && !v.getName().equals("goog")) {
compiler.report( compiler.report(
JSError.make( JSError.make(
reference.getNode(), reference.getNode(),
Expand Down
100 changes: 59 additions & 41 deletions test/com/google/javascript/jscomp/VariableReferenceCheckTest.java
View file
Expand Up @@ -228,11 +228,11 @@ public void testIssue166f() {
} }


public void testEarlyReference() { public void testEarlyReference() {
assertUndeclared("function f() { a = 2; var a = 3; }"); assertEarlyReferenceWarning("function f() { a = 2; var a = 3; }");
} }


public void testEarlyReference_withES6Modules() { public void testEarlyReference_withES6Modules() {
assertUndeclared("export function f() { a = 2; var a = 3; }"); assertEarlyReferenceWarning("export function f() { a = 2; var a = 3; }");
} }


public void testCorrectEarlyReference() { public void testCorrectEarlyReference() {
Expand Down Expand Up @@ -297,7 +297,7 @@ public void testHoistedFunction_withES6Modules() {
} }


public void testNonHoistedFunction() { public void testNonHoistedFunction() {
assertUndeclared("if (true) { f(); function f() {} }"); assertEarlyReferenceWarning("if (true) { f(); function f() {} }");
} }


public void testNonHoistedFunction2() { public void testNonHoistedFunction2() {
Expand All @@ -317,15 +317,15 @@ public void testNonHoistedFunction5() {
} }


public void testNonHoistedFunction6() { public void testNonHoistedFunction6() {
assertUndeclared("if (false) { f(); function f() {} }"); assertEarlyReferenceWarning("if (false) { f(); function f() {} }");
} }


public void testNonHoistedFunction7() { public void testNonHoistedFunction7() {
assertUndeclared("function g() { if (false) { f(); function f() {} }}"); assertEarlyReferenceWarning("function g() { if (false) { f(); function f() {} }}");
} }


public void testNonHoistedFunction_withES6Modules() { public void testNonHoistedFunction_withES6Modules() {
assertUndeclared("export function g() { if (false) { f(); function f() {} }}"); assertEarlyReferenceWarning("export function g() { if (false) { f(); function f() {} }}");
} }


public void testNonHoistedRecursiveFunction1() { public void testNonHoistedRecursiveFunction1() {
Expand Down Expand Up @@ -552,15 +552,15 @@ public void testUsedInInnerFunction_withES6Modules() {


public void testUsedInShorthandObjLit() { public void testUsedInShorthandObjLit() {
enableUnusedLocalAssignmentCheck = true; enableUnusedLocalAssignmentCheck = true;
assertUndeclared("var z = {x}; z(); var x;"); assertEarlyReferenceWarning("var z = {x}; z(); var x;");
testSame("var {x} = foo();"); testSame("var {x} = foo();");
testSame("var {x} = {};"); // TODO(moz): Maybe add a warning for this case testSame("var {x} = {};"); // TODO(moz): Maybe add a warning for this case
testSame("function f() { var x = 1; return {x}; }"); testSame("function f() { var x = 1; return {x}; }");
} }


public void testUsedInShorthandObjLit_withES6Modules() { public void testUsedInShorthandObjLit_withES6Modules() {
enableUnusedLocalAssignmentCheck = true; enableUnusedLocalAssignmentCheck = true;
assertUndeclared("export var z = {x}; z(); var x;"); assertEarlyReferenceWarning("export var z = {x}; z(); var x;");
testSame("export var {x} = foo();"); testSame("export var {x} = foo();");
} }


Expand Down Expand Up @@ -916,7 +916,7 @@ public void testLetConstNotDirectlyInBlock() {
} }


public void testFunctionHoisting() { public void testFunctionHoisting() {
assertUndeclared("if (true) { f(); function f() {} }"); assertEarlyReferenceWarning("if (true) { f(); function f() {} }");
} }


public void testFunctionHoistingRedeclaration1() { public void testFunctionHoistingRedeclaration1() {
Expand Down Expand Up @@ -1035,11 +1035,11 @@ public void testClassExtend_withES6Modules() {
public void testArrayPattern() { public void testArrayPattern() {
assertNoWarning("var [a] = [1];"); assertNoWarning("var [a] = [1];");
assertNoWarning("var [a, b] = [1, 2];"); assertNoWarning("var [a, b] = [1, 2];");
assertUndeclared("alert(a); var [a] = [1];"); assertEarlyReferenceWarning("alert(a); var [a] = [1];");
assertUndeclared("alert(b); var [a, b] = [1, 2];"); assertEarlyReferenceWarning("alert(b); var [a, b] = [1, 2];");


assertUndeclared("[a] = [1]; var a;"); assertEarlyReferenceWarning("[a] = [1]; var a;");
assertUndeclared("[a, b] = [1]; var b;"); assertEarlyReferenceWarning("[a, b] = [1]; var b;");
} }


public void testArrayPattern_withES6Modules01() { public void testArrayPattern_withES6Modules01() {
Expand All @@ -1049,11 +1049,11 @@ public void testArrayPattern_withES6Modules01() {
public void testArrayPattern_defaultValue() { public void testArrayPattern_defaultValue() {
assertNoWarning("var [a = 1] = [2];"); assertNoWarning("var [a = 1] = [2];");
assertNoWarning("var [a = 1] = [];"); assertNoWarning("var [a = 1] = [];");
assertUndeclared("alert(a); var [a = 1] = [2];"); assertEarlyReferenceWarning("alert(a); var [a = 1] = [2];");
assertUndeclared("alert(a); var [a = 1] = [];"); assertEarlyReferenceWarning("alert(a); var [a = 1] = [];");


assertUndeclared("alert(a); var [a = b] = [1];"); assertEarlyReferenceWarning("alert(a); var [a = b] = [1];");
assertUndeclared("alert(a); var [a = b] = [];"); assertEarlyReferenceWarning("alert(a); var [a = b] = [];");
} }


public void testArrayPattern_defaultValue_withES6Modules01() { public void testArrayPattern_defaultValue_withES6Modules01() {
Expand All @@ -1070,10 +1070,10 @@ public void testObjectPattern() {
assertNoWarning("alert(a); var {a: b} = {};"); assertNoWarning("alert(a); var {a: b} = {};");
assertNoWarning("alert(a); var {a: {a: b}} = {};"); assertNoWarning("alert(a); var {a: {a: b}} = {};");


assertUndeclared("alert(b); var {a: b} = {a: 1};"); assertEarlyReferenceWarning("alert(b); var {a: b} = {a: 1};");
assertUndeclared("alert(a); var {a} = {a: 1};"); assertEarlyReferenceWarning("alert(a); var {a} = {a: 1};");


assertUndeclared("({a: b} = {}); var a, b;"); assertEarlyReferenceWarning("({a: b} = {}); var a, b;");
} }


public void testObjectPatternRest() { public void testObjectPatternRest() {
Expand All @@ -1083,27 +1083,27 @@ public void testObjectPatternRest() {
assertNoWarning("var {a, ...r} = {a: 1};"); assertNoWarning("var {a, ...r} = {a: 1};");


assertNoWarning("alert(r);"); assertNoWarning("alert(r);");
assertUndeclared("alert(r); var {...r} = {a: 1};"); assertEarlyReferenceWarning("alert(r); var {...r} = {a: 1};");


assertNoWarning("({...a} = {});"); assertNoWarning("({...a} = {});");
assertUndeclared("({...a} = {}); var a;"); assertEarlyReferenceWarning("({...a} = {}); var a;");
} }


public void testObjectPattern_withES6Modules01() { public void testObjectPattern_withES6Modules01() {
assertNoWarning("export var {a: b} = {a: 1};"); assertNoWarning("export var {a: b} = {a: 1};");
} }


public void testObjectPattern_defaultValue() { public void testObjectPattern_defaultValue() {
assertUndeclared("alert(b); var {a: b = c} = {a: 1};"); assertEarlyReferenceWarning("alert(b); var {a: b = c} = {a: 1};");
assertUndeclared("alert(b); var c; var {a: b = c} = {a: 1};"); assertEarlyReferenceWarning("alert(b); var c; var {a: b = c} = {a: 1};");
assertUndeclared("var {a: b = c} = {a: 1}; var c;"); assertEarlyReferenceWarning("var {a: b = c} = {a: 1}; var c;");
assertUndeclared("alert(b); var {a: b = c} = {};"); assertEarlyReferenceWarning("alert(b); var {a: b = c} = {};");
assertUndeclared("alert(a); var {a = c} = {a: 1};"); assertEarlyReferenceWarning("alert(a); var {a = c} = {a: 1};");
assertUndeclared("alert(a); var {a = c} = {};"); assertEarlyReferenceWarning("alert(a); var {a = c} = {};");
} }


public void testObjectPattern_defaultValue_withES6Modules() { public void testObjectPattern_defaultValue_withES6Modules() {
assertUndeclared("export var {a: b = c} = {a: 1}; var c;"); assertEarlyReferenceWarning("export var {a: b = c} = {a: 1}; var c;");
} }


/** /**
Expand Down Expand Up @@ -1158,12 +1158,11 @@ public void testDestructuring() {
" var c = b;", " var c = b;",
"}")); "}"));


assertUndeclared( assertEarlyReferenceWarning(
lines( lines("function f() { ", " var {a:c, b:d} = obj;", " var obj = {a:1, b:2};", "}"));
"function f() { ", " var {a:c, b:d} = obj;", " var obj = {a:1, b:2};", "}")); assertEarlyReferenceWarning(
assertUndeclared(
lines("function f() { ", " var {a, b} = obj;", " var obj = {a:1, b:2};", "}")); lines("function f() { ", " var {a, b} = obj;", " var obj = {a:1, b:2};", "}"));
assertUndeclared( assertEarlyReferenceWarning(
lines("function f() { ", " var e = c;", " var {a:c, b:d} = {a:1, b:2};", "}")); lines("function f() { ", " var e = c;", " var {a:c, b:d} = {a:1, b:2};", "}"));
} }


Expand All @@ -1180,9 +1179,8 @@ public void testDestructuring_withES6Modules() {
" var c = b;", " var c = b;",
"}")); "}"));


assertUndeclared( assertEarlyReferenceWarning(
lines( lines("export function f() { ", " var {a:c, b:d} = obj;", " var obj = {a:1, b:2};", "}"));
"export function f() { ", " var {a:c, b:d} = obj;", " var obj = {a:1, b:2};", "}"));
} }


public void testDestructuringInLoop() { public void testDestructuringInLoop() {
Expand All @@ -1191,6 +1189,28 @@ public void testDestructuringInLoop() {
testSame("for (let [{length: z}, w] in obj) {}"); testSame("for (let [{length: z}, w] in obj) {}");
} }


public void testReferencingPreviouslyDeclaredVariableInConst() {
testSame("const [a, b = a] = [];");
// TODO(b/111441110): don't error on this. it's valid code.
assertEarlyReferenceError("for (const [a, b = a] of []);");
}

public void testEarlyReferenceInInnerBlock() {
assertEarlyReferenceError("for (x of [1, 2, 3]) {} let x;");
assertEarlyReferenceError("{ x; } let x;");
assertEarlyReferenceError("{ C; } class C {}");
assertEarlyReferenceWarning("{ x; } var x;");
}

public void testEarlyVariableReferenceInsideFunction() {
testSame("function f() { x; } let x; f(); ");
testSame("function f() { const f = () => x; let x = 3; return f; }");
// NOTE: this will cause an error at runtime, but we don't report it because we don't track
// where `f` is being called.
testSame("function f() { x; } f(); let x;");
testSame("function f() { x; } f(); var x;");
}

public void testEnhancedForLoopTemporalDeadZone() { public void testEnhancedForLoopTemporalDeadZone() {
assertEarlyReferenceError("for (let x of [x]);"); assertEarlyReferenceError("for (let x of [x]);");
assertEarlyReferenceError("for (let x in [x]);"); assertEarlyReferenceError("for (let x in [x]);");
Expand Down Expand Up @@ -1246,10 +1266,8 @@ private void assertRedeclareGlobal(String js) {
testError(js, VarCheck.VAR_MULTIPLY_DECLARED_ERROR); testError(js, VarCheck.VAR_MULTIPLY_DECLARED_ERROR);
} }


/** /** Expects the JS to generate one bad-write warning. */
* Expects the JS to generate one bad-write warning. private void assertEarlyReferenceWarning(String js) {
*/
private void assertUndeclared(String js) {
testWarning(js, EARLY_REFERENCE); testWarning(js, EARLY_REFERENCE);
} }


Expand Down

0 comments on commit bdf047c

Please sign in to comment.