Fix hasShadowedGlobals check to properly ignore top-level declarations (

#367)

We'll want top-level declaration detection for #228 anyway, and
hasShadowedGlobals was buggy because it was treating export declarations as
shadowing themselves, so this adds some bookkeeping so we know when a
declaration is top-level or not.

This doesn't affect correctness, it was just an optimization that wasn't kicking
in when it was supposed to.

Tracking scope depth throughout the parser is a little scary and adds another
field to the state, so there may be a way to improve it in the future, but it
certainly should work for now.

src/identifyShadowedGlobals.ts

@@ -1,7 +1,7 @@
 import {
   isBlockScopedDeclaration,
-  isDeclaration,
   isFunctionScopedDeclaration,
+  isNonTopLevelDeclaration,
 } from "./parser/tokenizer";
 import {Scope} from "./parser/tokenizer/state";
 import {TokenType as tt} from "./parser/tokenizer/types";
@@ -26,11 +26,12 @@ export default function identifyShadowedGlobals(
  * We can do a fast up-front check to see if there are any declarations to global names. If not,
  * then there's no point in computing scope assignments.
  */
-function hasShadowedGlobals(tokens: TokenProcessor, globalNames: Set<string>): boolean {
+// Exported for testing.
+export function hasShadowedGlobals(tokens: TokenProcessor, globalNames: Set<string>): boolean {
   for (const token of tokens.tokens) {
     if (
       token.type === tt.name &&
-      isDeclaration(token) &&
+      isNonTopLevelDeclaration(token) &&
       globalNames.has(tokens.identifierNameForToken(token))
     ) {
       return true;

src/parser/plugins/flow.ts

@@ -1037,6 +1037,7 @@ export function flowParseSubscripts(startPos: number, noCalls: boolean = false):
 
 // Returns true if there was an arrow function here.
 function parseAsyncArrowWithTypeParameters(startPos: number): boolean {
+  state.scopeDepth++;
   const startTokenIndex = state.tokens.length;
   parseFunctionParams();
   if (!parseArrow()) {

src/parser/tokenizer/index.ts

@@ -12,15 +12,29 @@ import {TokenType, TokenType as tt} from "./types";
 export const enum IdentifierRole {
   Access,
   ExportAccess,
+  TopLevelDeclaration,
   FunctionScopedDeclaration,
   BlockScopedDeclaration,
+  ObjectShorthandTopLevelDeclaration,
   ObjectShorthandFunctionScopedDeclaration,
   ObjectShorthandBlockScopedDeclaration,
   ObjectShorthand,
   ObjectKey,
 }
 
 export function isDeclaration(token: Token): boolean {
+  const role = token.identifierRole;
+  return (
+    role === IdentifierRole.TopLevelDeclaration ||
+    role === IdentifierRole.FunctionScopedDeclaration ||
+    role === IdentifierRole.BlockScopedDeclaration ||
+    role === IdentifierRole.ObjectShorthandTopLevelDeclaration ||
+    role === IdentifierRole.ObjectShorthandFunctionScopedDeclaration ||
+    role === IdentifierRole.ObjectShorthandBlockScopedDeclaration
+  );
+}
+
+export function isNonTopLevelDeclaration(token: Token): boolean {
   const role = token.identifierRole;
   return (
     role === IdentifierRole.FunctionScopedDeclaration ||
@@ -32,8 +46,11 @@ export function isDeclaration(token: Token): boolean {
 
 export function isBlockScopedDeclaration(token: Token): boolean {
   const role = token.identifierRole;
+  // Treat top-level declarations as block scope since the distinction doesn't matter here.
   return (
+    role === IdentifierRole.TopLevelDeclaration ||
     role === IdentifierRole.BlockScopedDeclaration ||
+    role === IdentifierRole.ObjectShorthandTopLevelDeclaration ||
     role === IdentifierRole.ObjectShorthandBlockScopedDeclaration
   );
 }
@@ -46,6 +63,14 @@ export function isFunctionScopedDeclaration(token: Token): boolean {
   );
 }
 
+export function isObjectShorthandDeclaration(token: Token): boolean {
+  return (
+    token.identifierRole === IdentifierRole.ObjectShorthandTopLevelDeclaration ||
+    token.identifierRole === IdentifierRole.ObjectShorthandBlockScopedDeclaration ||
+    token.identifierRole === IdentifierRole.ObjectShorthandFunctionScopedDeclaration
+  );
+}
+
 // Object type used to represent tokens. Note that normally, tokens
 // simply exist as properties on the parser object. This is only
 // used for the onToken callback and the external tokenizer.

src/parser/tokenizer/state.ts

@@ -26,6 +26,7 @@ export class StateSnapshot {
     readonly start: number,
     readonly end: number,
     readonly isType: boolean,
+    readonly scopeDepth: number,
     readonly error: Error | null,
   ) {}
 }
@@ -53,6 +54,7 @@ export default class State {
   end: number = 0;
 
   isType: boolean = false;
+  scopeDepth: number = 0;
 
   /**
    * If the parser is in an error state, then the token is always tt.eof and all functions can
@@ -76,6 +78,7 @@ export default class State {
       this.start,
       this.end,
       this.isType,
+      this.scopeDepth,
       this.error,
     );
   }
@@ -91,6 +94,7 @@ export default class State {
     this.start = snapshot.start;
     this.end = snapshot.end;
     this.isType = snapshot.isType;
+    this.scopeDepth = snapshot.scopeDepth;
     this.error = snapshot.error;
   }
 }

src/parser/traverser/expression.ts

@@ -321,6 +321,7 @@ export function baseParseSubscript(startPos: number, noCalls: boolean, stopState
         // We hit an arrow, so backtrack and start again parsing function parameters.
         state.restoreFromSnapshot(snapshot);
         stopState.stop = true;
+        state.scopeDepth++;
 
         parseFunctionParams();
         parseAsyncArrowFromCallExpression(startPos, startTokenIndex);
@@ -452,6 +453,7 @@ export function parseExprAtom(): boolean {
         contextualKeyword === ContextualKeyword._async &&
         match(tt.name)
       ) {
+        state.scopeDepth++;
         parseBindingIdentifier(false);
         expect(tt.arrow);
         // let foo = async bar => {};
@@ -460,6 +462,7 @@ export function parseExprAtom(): boolean {
       }
 
       if (canBeArrow && !canInsertSemicolon() && match(tt.arrow)) {
+        state.scopeDepth++;
         markPriorBindingIdentifier(false);
         expect(tt.arrow);
         parseArrowExpression(functionStart, startTokenIndex);
@@ -596,6 +599,7 @@ function parseParenAndDistinguishExpression(canBeArrow: boolean): boolean {
       // It was an arrow function this whole time, so start over and parse it as params so that we
       // get proper token annotations.
       state.restoreFromSnapshot(snapshot);
+      state.scopeDepth++;
       // We don't need to worry about functionStart for arrow functions, so just use something.
       const functionStart = state.start;
       // Don't specify a context ID because arrow functions don't need a context ID.
@@ -700,9 +704,7 @@ export function parseObj(isPattern: boolean, isBlockScope: boolean): void {
       if (isPattern) {
         // Mark role when the only thing being spread over is an identifier.
         if (state.tokens.length === previousIndex + 2) {
-          state.tokens[state.tokens.length - 1].identifierRole = isBlockScope
-            ? IdentifierRole.BlockScopedDeclaration
-            : IdentifierRole.FunctionScopedDeclaration;
+          markPriorBindingIdentifier(isBlockScope);
         }
         if (eat(tt.braceR)) {
           break;
@@ -854,6 +856,7 @@ export function parseMethod(
 ): void {
   const funcContextId = getNextContextId();
 
+  state.scopeDepth++;
   const startTokenIndex = state.tokens.length;
   const allowModifiers = isConstructor; // For TypeScript parameter properties
   parseFunctionParams(allowModifiers, funcContextId);
@@ -865,6 +868,7 @@ export function parseMethod(
   );
   const endTokenIndex = state.tokens.length;
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, true));
+  state.scopeDepth--;
 }
 
 // Parse arrow function expression.
@@ -874,6 +878,7 @@ export function parseArrowExpression(functionStart: number, startTokenIndex: num
   parseFunctionBody(functionStart, false /* isGenerator */, true);
   const endTokenIndex = state.tokens.length;
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, true));
+  state.scopeDepth--;
 }
 
 export function parseFunctionBodyAndFinish(

src/parser/traverser/lval.ts

@@ -34,9 +34,13 @@ export function parseBindingIdentifier(isBlockScope: boolean): void {
 }
 
 export function markPriorBindingIdentifier(isBlockScope: boolean): void {
-  state.tokens[state.tokens.length - 1].identifierRole = isBlockScope
-    ? IdentifierRole.BlockScopedDeclaration
-    : IdentifierRole.FunctionScopedDeclaration;
+  if (state.scopeDepth === 0) {
+    state.tokens[state.tokens.length - 1].identifierRole = IdentifierRole.TopLevelDeclaration;
+  } else {
+    state.tokens[state.tokens.length - 1].identifierRole = isBlockScope
+      ? IdentifierRole.BlockScopedDeclaration
+      : IdentifierRole.FunctionScopedDeclaration;
+  }
 }
 
 // Parses lvalue (assignable) atom.

src/parser/traverser/statement.ts

@@ -75,6 +75,9 @@ import {
 export function parseTopLevel(): File {
   parseBlockBody(tt.eof);
   state.scopes.push(new Scope(0, state.tokens.length, true));
+  if (state.scopeDepth !== 0) {
+    throw new Error(`Invalid scope depth at end of file: ${state.scopeDepth}`);
+  }
   return new File(state.tokens, state.scopes);
 }
 
@@ -283,10 +286,12 @@ function parseDoStatement(): void {
 }
 
 function parseForStatement(): void {
+  state.scopeDepth++;
   const startTokenIndex = state.tokens.length;
   parseAmbiguousForStatement();
   const endTokenIndex = state.tokens.length;
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, false));
+  state.scopeDepth--;
 }
 
 // Disambiguating between a `for` and a `for`/`in` or `for`/`of`
@@ -368,6 +373,7 @@ function parseReturnStatement(): void {
 function parseSwitchStatement(): void {
   next();
   parseParenExpression();
+  state.scopeDepth++;
   const startTokenIndex = state.tokens.length;
   expect(tt.braceL);
 
@@ -387,6 +393,7 @@ function parseSwitchStatement(): void {
   next(); // Closing brace
   const endTokenIndex = state.tokens.length;
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, false));
+  state.scopeDepth--;
 }
 
 function parseThrowStatement(): void {
@@ -404,6 +411,7 @@ function parseTryStatement(): void {
     next();
     let catchBindingStartTokenIndex = null;
     if (match(tt.parenL)) {
+      state.scopeDepth++;
       catchBindingStartTokenIndex = state.tokens.length;
       expect(tt.parenL);
       parseBindingAtom(true /* isBlockScope */);
@@ -415,6 +423,7 @@ function parseTryStatement(): void {
       // catch block.
       const endTokenIndex = state.tokens.length;
       state.scopes.push(new Scope(catchBindingStartTokenIndex, endTokenIndex, false));
+      state.scopeDepth--;
     }
   }
   if (eat(tt._finally)) {
@@ -466,6 +475,7 @@ export function parseBlock(
   contextId: number = 0,
 ): void {
   const startTokenIndex = state.tokens.length;
+  state.scopeDepth++;
   expect(tt.braceL);
   if (contextId) {
     state.tokens[state.tokens.length - 1].contextId = contextId;
@@ -476,6 +486,7 @@ export function parseBlock(
   }
   const endTokenIndex = state.tokens.length;
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, isFunctionScope));
+  state.scopeDepth--;
 }
 
 export function parseBlockBody(end: TokenType): void {
@@ -567,19 +578,23 @@ export function parseFunction(
     // a new function scope to enforce that.
     if (!isStatement) {
       nameScopeStartTokenIndex = state.tokens.length;
+      state.scopeDepth++;
     }
     parseBindingIdentifier(false);
   }
 
   const startTokenIndex = state.tokens.length;
+  state.scopeDepth++;
   parseFunctionParams();
   parseFunctionBodyAndFinish(functionStart, isGenerator, allowExpressionBody);
   const endTokenIndex = state.tokens.length;
   // In addition to the block scope of the function body, we need a separate function-style scope
   // that includes the params.
   state.scopes.push(new Scope(startTokenIndex, endTokenIndex, true));
+  state.scopeDepth--;
   if (nameScopeStartTokenIndex !== null) {
     state.scopes.push(new Scope(nameScopeStartTokenIndex, endTokenIndex, true));
+    state.scopeDepth--;
   }
 }
 
@@ -620,6 +635,7 @@ export function parseClass(isStatement: boolean, optionalId: boolean = false): v
   let nameScopeStartTokenIndex = null;
   if (!isStatement) {
     nameScopeStartTokenIndex = state.tokens.length;
+    state.scopeDepth++;
   }
   parseClassId(isStatement, optionalId);
   parseClassSuper();
@@ -633,6 +649,7 @@ export function parseClass(isStatement: boolean, optionalId: boolean = false): v
   if (nameScopeStartTokenIndex !== null) {
     const endTokenIndex = state.tokens.length;
     state.scopes.push(new Scope(nameScopeStartTokenIndex, endTokenIndex, false));
+    state.scopeDepth--;
   }
 }
 
@@ -806,8 +823,7 @@ function parseClassId(isStatement: boolean, optionalId: boolean = false): void {
   }
 
   if (match(tt.name)) {
-    parseIdentifier();
-    state.tokens[state.tokens.length - 1].identifierRole = IdentifierRole.BlockScopedDeclaration;
+    parseBindingIdentifier(true);
   }
 
   if (isTypeScriptEnabled) {

src/transformers/CJSImportTransformer.ts

@@ -1,5 +1,5 @@
 import CJSImportProcessor from "../CJSImportProcessor";
-import {IdentifierRole, isDeclaration} from "../parser/tokenizer";
+import {IdentifierRole, isDeclaration, isObjectShorthandDeclaration} from "../parser/tokenizer";
 import {ContextualKeyword} from "../parser/tokenizer/keywords";
 import {TokenType as tt} from "../parser/tokenizer/types";
 import TokenProcessor from "../TokenProcessor";
@@ -444,10 +444,7 @@ export default class CJSImportTransformer extends Transformer {
           if (replacement === null) {
             throw new Error(`Expected a replacement for ${name} in \`export var\` syntax.`);
           }
-          if (
-            token.identifierRole === IdentifierRole.ObjectShorthandBlockScopedDeclaration ||
-            token.identifierRole === IdentifierRole.ObjectShorthandFunctionScopedDeclaration
-          ) {
+          if (isObjectShorthandDeclaration(token)) {
             replacement = `${name}: ${replacement}`;
           }
           this.tokens.replaceToken(replacement);

test/identifyShadowedGlobals-test.ts

@@ -0,0 +1,45 @@
+import * as assert from "assert";
+import CJSImportProcessor from "../src/CJSImportProcessor";
+import {hasShadowedGlobals} from "../src/identifyShadowedGlobals";
+import NameManager from "../src/NameManager";
+import {parse} from "../src/parser";
+import TokenProcessor from "../src/TokenProcessor";
+
+function assertHasShadowedGlobals(code: string, expected: boolean): void {
+  const file = parse(code, false, false, false);
+  const tokenProcessor = new TokenProcessor(code, file.tokens, false);
+  const nameManager = new NameManager(tokenProcessor);
+  nameManager.preprocessNames();
+  const importProcessor = new CJSImportProcessor(nameManager, tokenProcessor, false);
+  importProcessor.preprocessTokens();
+  assert.strictEqual(
+    hasShadowedGlobals(tokenProcessor, importProcessor.getGlobalNames()),
+    expected,
+  );
+}
+
+describe("identifyShadowedGlobals", () => {
+  it("properly does an up-front that there are any shadowed globals", () => {
+    assertHasShadowedGlobals(
+      `
+      import a from 'a';
+      function foo() {
+        const a = 3;
+        console.log(a);
+      }
+    `,
+      true,
+    );
+  });
+
+  it("properly detects when there are no shadowed globals", () => {
+    assertHasShadowedGlobals(
+      `
+      import a from 'a';
+      
+      export const b = 3;
+    `,
+      false,
+    );
+  });
+});