Skip to content

Commit

Permalink
Disallow creating a scope rooted at a function body, without first cr…
Browse files Browse the repository at this point in the history 
…eating the parent scope, which will be rooted at the function itself.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=147866686
  • Loading branch information
@dimvar
tbreisacher authored and dimvar committed Feb 19, 2017
1 parent 8150e12 commit 5a57189
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 43 deletions.
6 changes: 3 additions & 3 deletions src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java
View file
Expand Up @@ -120,8 +120,8 @@ private void scanRoot(Node n) {
n.isRoot() || NodeUtil.isFunctionBlock(n) || n.isModuleBody(); n.isRoot() || NodeUtil.isFunctionBlock(n) || n.isModuleBody();
scanVars(n, scanInnerBlocks, true); scanVars(n, scanInnerBlocks, true);
} else { } else {
// n is the global SCRIPT node // n is the global scope
checkState(scope.getParent() == null); checkState(scope.isGlobal(), scope);
scanVars(n, true, true); scanVars(n, true, true);
} }
} }
Expand Down Expand Up @@ -249,7 +249,7 @@ private void declareVar(Node n) {
CompilerInput input = compiler.getInput(inputId); CompilerInput input = compiler.getInput(inputId);
if (v != null if (v != null
|| isShadowingDisallowed(name) || isShadowingDisallowed(name)
|| (scope.isLocal() && name.equals(ARGUMENTS))) { || ((scope.isFunctionScope() || scope.isFunctionBlockScope()) && name.equals(ARGUMENTS))) {
redeclarationHandler.onRedeclaration(scope, name, n, input); redeclarationHandler.onRedeclaration(scope, name, n, input);
} else { } else {
scope.declare(name, n, input); scope.declare(name, n, input);
Expand Down
11 changes: 10 additions & 1 deletion src/com/google/javascript/jscomp/Scope.java
View file
Expand Up @@ -16,6 +16,8 @@


package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkArgument;

import com.google.common.base.Preconditions; import com.google.common.base.Preconditions;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.StaticScope; import com.google.javascript.rhino.StaticScope;
Expand All @@ -41,7 +43,7 @@ public class Scope implements StaticScope {
/** /**
* Creates a Scope given the parent Scope and the root node of the scope. * Creates a Scope given the parent Scope and the root node of the scope.
* @param parent The parent Scope. Cannot be null. * @param parent The parent Scope. Cannot be null.
* @param rootNode Typically the FUNCTION node. * @param rootNode
*/ */
Scope(Scope parent, Node rootNode) { Scope(Scope parent, Node rootNode) {
Preconditions.checkNotNull(parent); Preconditions.checkNotNull(parent);
Expand All @@ -67,6 +69,13 @@ public String toString() {
} }


static Scope createGlobalScope(Node rootNode) { static Scope createGlobalScope(Node rootNode) {
// TODO(tbreisacher): Can we tighten this to allow only ROOT nodes?
checkArgument(
rootNode.isRoot()
|| rootNode.isScript()
|| rootNode.isModuleBody()
|| rootNode.isFunction(),
rootNode);
return new Scope(rootNode); return new Scope(rootNode);
} }


Expand Down
10 changes: 4 additions & 6 deletions src/com/google/javascript/jscomp/VarCheck.java
View file
Expand Up @@ -23,7 +23,6 @@
import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.JSDocInfoBuilder; import com.google.javascript.rhino.JSDocInfoBuilder;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;

import java.util.ArrayList; import java.util.ArrayList;
import java.util.HashSet; import java.util.HashSet;
import java.util.Set; import java.util.Set;
Expand Down Expand Up @@ -117,9 +116,8 @@ class VarCheck extends AbstractPostOrderCallback implements
} }


/** /**
* Create a SyntacticScopeCreator. If not in sanity check mode, use a * Creates the scope creator used by this pass. If not in sanity check mode, use a {@link
* {@link RedeclarationCheckHandler} to check var redeclarations. * RedeclarationCheckHandler} to check var redeclarations.
* @return the SyntacticScopeCreator
*/ */
private ScopeCreator createScopeCreator() { private ScopeCreator createScopeCreator() {
if (sanityCheck) { if (sanityCheck) {
Expand Down Expand Up @@ -194,11 +192,11 @@ public void visit(NodeTraversal t, Node n, Node parent) {
Var var = scope.getVar(varName); Var var = scope.getVar(varName);
if (var == null) { if (var == null) {
if (NodeUtil.isFunctionExpression(parent) if (NodeUtil.isFunctionExpression(parent)
|| NodeUtil.isClassExpression(parent) && n == parent.getFirstChild()) { || (NodeUtil.isClassExpression(parent) && n == parent.getFirstChild())) {
// e.g. [ function foo() {} ], it's okay if "foo" isn't defined in the // e.g. [ function foo() {} ], it's okay if "foo" isn't defined in the
// current scope. // current scope.
} else { } else {
boolean isArguments = scope.isLocal() && ARGUMENTS.equals(varName); boolean isArguments = scope.isFunctionScope() && ARGUMENTS.equals(varName);
// The extern checks are stricter, don't report a second error. // The extern checks are stricter, don't report a second error.
if (!isArguments && !(strictExternCheck && t.getInput().isExtern())) { if (!isArguments && !(strictExternCheck && t.getInput().isExtern())) {
t.report(n, UNDEFINED_VAR_ERROR, varName); t.report(n, UNDEFINED_VAR_ERROR, varName);
Expand Down
84 changes: 56 additions & 28 deletions test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java
View file
Expand Up @@ -16,16 +16,16 @@


package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.javascript.jscomp.CompilerTestCase.LINE_JOINER;


import com.google.common.base.Joiner;
import com.google.common.collect.HashMultiset; import com.google.common.collect.HashMultiset;
import com.google.common.collect.Multiset; import com.google.common.collect.Multiset;
import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.SyntacticScopeCreator.RedeclarationHandler; import com.google.javascript.jscomp.SyntacticScopeCreator.RedeclarationHandler;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;

import junit.framework.TestCase; import junit.framework.TestCase;


/** /**
Expand Down Expand Up @@ -268,7 +268,7 @@ public void testArrayDestructuringVarInBlock() {
} }


public void testObjectDestructuring() { public void testObjectDestructuring() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function foo() {", "function foo() {",
" var {a, b} = bar();", " var {a, b} = bar();",
"}"); "}");
Expand All @@ -287,7 +287,7 @@ public void testObjectDestructuring() {
} }


public void testObjectDestructuring2() { public void testObjectDestructuring2() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function foo() {", "function foo() {",
" var {a: b = 1} = bar();", " var {a: b = 1} = bar();",
"}"); "}");
Expand All @@ -306,7 +306,7 @@ public void testObjectDestructuring2() {
} }


public void testObjectDestructuringComputedProp() { public void testObjectDestructuringComputedProp() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function foo() {", "function foo() {",
" var {['s']: a} = bar();", " var {['s']: a} = bar();",
"}"); "}");
Expand Down Expand Up @@ -335,7 +335,7 @@ public void testObjectDestructuringComputedPropParam() {
} }


public void testObjectDestructuringNested() { public void testObjectDestructuringNested() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function foo() {", "function foo() {",
" var {a:{b}} = bar();", " var {a:{b}} = bar();",
"}"); "}");
Expand All @@ -354,7 +354,7 @@ public void testObjectDestructuringNested() {
} }


public void testObjectDestructuringWithInitializer() { public void testObjectDestructuringWithInitializer() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function foo() {", "function foo() {",
" var {a=1} = bar();", " var {a=1} = bar();",
"}"); "}");
Expand Down Expand Up @@ -431,8 +431,12 @@ public void testScopeRootNode() {
assertFalse(globalScope.isBlockScope()); assertFalse(globalScope.isBlockScope());
assertEquals(globalScope, globalScope.getClosestHoistScope()); assertEquals(globalScope, globalScope.getClosestHoistScope());


Node fooBlockNode = root.getFirstChild().getLastChild(); Node function = root.getFirstChild();
Scope fooScope = scopeCreator.createScope(fooBlockNode, null); checkState(function.isFunction(), function);
Scope functionScope = scopeCreator.createScope(function, globalScope);

Node fooBlockNode = NodeUtil.getFunctionBody(function);
Scope fooScope = scopeCreator.createScope(fooBlockNode, functionScope);
assertEquals(fooBlockNode, fooScope.getRootNode()); assertEquals(fooBlockNode, fooScope.getRootNode());
assertTrue(fooScope.isBlockScope()); assertTrue(fooScope.isBlockScope());
assertEquals(fooScope, fooScope.getClosestHoistScope()); assertEquals(fooScope, fooScope.getClosestHoistScope());
Expand Down Expand Up @@ -522,22 +526,40 @@ public void testForOfLoopScope() {


public void testFunctionArgument() { public void testFunctionArgument() {
String js = "function f(x) { if (true) { let y = 3; } }"; String js = "function f(x) { if (true) { let y = 3; } }";
Node functionBlock = getRoot(js).getLastChild(); Node root = getRoot(js);
Scope functionScope = scopeCreator.createScope(functionBlock, null); Scope global = scopeCreator.createScope(root, null);
assertTrue(functionScope.isDeclared("x", false)); Node function = root.getLastChild();
assertFalse(functionScope.isDeclared("y", false)); checkState(function.isFunction(), function);
Scope functionScope = scopeCreator.createScope(function, global);


Node ifBlock = functionBlock.getLastChild().getLastChild().getLastChild(); Node functionBlock = NodeUtil.getFunctionBody(function);
Scope blockScope = scopeCreator.createScope(ifBlock, functionScope); Scope fBlockScope = scopeCreator.createScope(functionBlock, functionScope);
assertTrue(blockScope.isDeclared("x", false));
assertTrue(fBlockScope.isDeclared("x", false));
assertFalse(fBlockScope.isDeclared("y", false));

Node ifBlock = functionBlock.getLastChild().getLastChild();
checkState(ifBlock.isNormalBlock(), ifBlock);
Scope blockScope = scopeCreator.createScope(ifBlock, fBlockScope);
assertFalse(blockScope.isDeclared("x", false));
assertTrue(blockScope.isDeclared("x", true));
assertTrue(blockScope.isDeclared("y", false)); assertTrue(blockScope.isDeclared("y", false));
} }


public void testTheArgumentsVariable() { public void testTheArgumentsVariable() {
String js = "function f() { if (true) { let arguments = 3; } }"; String js = "function f() { if (true) { let arguments = 3; } }";
Node ifBlock = getRoot(js).getLastChild().getLastChild() Node root = getRoot(js);
.getLastChild().getLastChild(); Scope global = scopeCreator.createScope(root, null);
Scope blockScope = scopeCreator.createScope(ifBlock, null);
Node function = root.getFirstChild();
checkState(function.isFunction(), function);
Scope fScope = scopeCreator.createScope(function, global);

Node fBlock = NodeUtil.getFunctionBody(function);
Scope fBlockScope = scopeCreator.createScope(fBlock, fScope);

Node ifBlock = fBlock.getFirstChild().getLastChild();
Scope blockScope = scopeCreator.createScope(ifBlock, fBlockScope);
assertTrue(blockScope.isDeclared("arguments", false)); assertTrue(blockScope.isDeclared("arguments", false));
} }


Expand Down Expand Up @@ -588,17 +610,22 @@ public void testIsCatchBlockScoped() {
} }


public void testVarAfterLet() { public void testVarAfterLet() {
String js = Joiner.on('\n').join( String js = LINE_JOINER.join(
"function f() {", "function f() {",
" if (a) {", " if (a) {",
" let x;", " let x;",
" }", " }",
" var y;", " var y;",
"}" "}");
);
Node root = getRoot(js); Node root = getRoot(js);
Scope global = scopeCreator.createScope(root, null);
Node function = root.getFirstChild();
Scope fScope = scopeCreator.createScope(function, global);

Node fBlock = root.getFirstChild().getLastChild(); Node fBlock = root.getFirstChild().getLastChild();
Scope fBlockScope = scopeCreator.createScope(fBlock, null); Scope fBlockScope = scopeCreator.createScope(fBlock, fScope);
checkNotNull(fBlockScope);
assertFalse(fBlockScope.isDeclared("x", false)); assertFalse(fBlockScope.isDeclared("x", false));
assertTrue(fBlockScope.isDeclared("y", false)); assertTrue(fBlockScope.isDeclared("y", false));


Expand All @@ -611,15 +638,16 @@ public void testVarAfterLet() {
public void testSimpleFunctionParam() { public void testSimpleFunctionParam() {
String js = "function f(x) {}"; String js = "function f(x) {}";
Node root = getRoot(js); Node root = getRoot(js);
Node fNode = root.getFirstChild();

Scope globalScope = scopeCreator.createScope(root, null); Scope globalScope = scopeCreator.createScope(root, null);

Node fNode = root.getFirstChild();
checkState(fNode.isFunction(), fNode);
Scope fScope = scopeCreator.createScope(fNode, globalScope); Scope fScope = scopeCreator.createScope(fNode, globalScope);
assertTrue(fScope.isDeclared("x", false)); assertTrue(fScope.isDeclared("x", false));


Node fBlock = fNode.getLastChild(); Node fBlock = NodeUtil.getFunctionBody(fNode);
Scope fBlockScope = scopeCreator.createScope(fBlock, null); Scope fBlockScope = scopeCreator.createScope(fBlock, fScope);
assertFalse(fBlockScope.isDeclared("x", false)); assertTrue(fBlockScope.isDeclared("x", false));
} }


public void testOnlyOneDeclaration() { public void testOnlyOneDeclaration() {
Expand Down
2 changes: 1 addition & 1 deletion test/com/google/javascript/jscomp/GlobalVarReferenceMapTest.java
View file
Expand Up @@ -50,7 +50,7 @@ public final class GlobalVarReferenceMapTest extends TestCase {
private final GlobalVarReferenceMap map = new GlobalVarReferenceMap( private final GlobalVarReferenceMap map = new GlobalVarReferenceMap(
ImmutableList.of(INPUT1, INPUT2, INPUT3), ImmutableList.of(EXTERN1)); ImmutableList.of(INPUT1, INPUT2, INPUT3), ImmutableList.of(EXTERN1));
private final Map<Var, ReferenceCollection> globalMap = new HashMap<>(); private final Map<Var, ReferenceCollection> globalMap = new HashMap<>();
private final Node root = new Node(Token.BLOCK); private final Node root = new Node(Token.ROOT);
private final Scope globalScope = Scope.createGlobalScope(root); private final Scope globalScope = Scope.createGlobalScope(root);
private Node scriptRoot = new Node(Token.SCRIPT); private Node scriptRoot = new Node(Token.SCRIPT);


Expand Down
6 changes: 3 additions & 3 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -3051,11 +3051,11 @@ public void process(Node externs, Node root) {


public void testSuppressEs5StrictWarning() { public void testSuppressEs5StrictWarning() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
options.setWarningLevel(DiagnosticGroups.ES5_STRICT, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.ES5_STRICT, CheckLevel.WARNING);
test(options, test(options,
"/** @suppress{es5Strict} */\n" + "/** @suppress{es5Strict} */ function f() { var arguments; }",
"function f() { var arguments; }", "");
"function f() {}");
} }


public void testCheckProvidesWarning() { public void testCheckProvidesWarning() {
Expand Down
2 changes: 1 addition & 1 deletion test/com/google/javascript/jscomp/VarCheckTest.java
View file
Expand Up @@ -579,7 +579,7 @@ public void testFunctionScopeArguments() {


testError("var f = function arguments() {}", VarCheck.VAR_ARGUMENTS_SHADOWED_ERROR); testError("var f = function arguments() {}", VarCheck.VAR_ARGUMENTS_SHADOWED_ERROR);
testError("var f = function (arguments) {}", VarCheck.VAR_ARGUMENTS_SHADOWED_ERROR); testError("var f = function (arguments) {}", VarCheck.VAR_ARGUMENTS_SHADOWED_ERROR);
testError("function f() {try {} catch(arguments) {}}", VarCheck.VAR_ARGUMENTS_SHADOWED_ERROR); testSame("function f() {try {} catch(arguments) {}}");


sanityCheck = true; sanityCheck = true;
testSame("function f() {var arguments}"); testSame("function f() {var arguments}");
Expand Down

0 comments on commit 5a57189

Please sign in to comment.