From b7f36508c12fa1b670c5349cc065d21916f04f43 Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Mon, 24 Dec 2018 06:50:57 -0800 Subject: [PATCH] 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 | 7 ++-- src/parser/plugins/flow.ts | 1 + src/parser/tokenizer/index.ts | 25 +++++++++++++ src/parser/tokenizer/state.ts | 4 +++ src/parser/traverser/expression.ts | 11 ++++-- src/parser/traverser/lval.ts | 10 ++++-- src/parser/traverser/statement.ts | 20 +++++++++-- src/transformers/CJSImportTransformer.ts | 7 ++-- test/identifyShadowedGlobals-test.ts | 45 ++++++++++++++++++++++++ test/tokens-test.ts | 32 ++++++++++++----- 10 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 test/identifyShadowedGlobals-test.ts diff --git a/src/identifyShadowedGlobals.ts b/src/identifyShadowedGlobals.ts index d5edb82b..632de34f 100644 --- a/src/identifyShadowedGlobals.ts +++ b/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): boolean { +// Exported for testing. +export function hasShadowedGlobals(tokens: TokenProcessor, globalNames: Set): boolean { for (const token of tokens.tokens) { if ( token.type === tt.name && - isDeclaration(token) && + isNonTopLevelDeclaration(token) && globalNames.has(tokens.identifierNameForToken(token)) ) { return true; diff --git a/src/parser/plugins/flow.ts b/src/parser/plugins/flow.ts index 03ee16c5..d9dec92a 100644 --- a/src/parser/plugins/flow.ts +++ b/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()) { diff --git a/src/parser/tokenizer/index.ts b/src/parser/tokenizer/index.ts index 483731ab..eb09384a 100644 --- a/src/parser/tokenizer/index.ts +++ b/src/parser/tokenizer/index.ts @@ -12,8 +12,10 @@ import {TokenType, TokenType as tt} from "./types"; export const enum IdentifierRole { Access, ExportAccess, + TopLevelDeclaration, FunctionScopedDeclaration, BlockScopedDeclaration, + ObjectShorthandTopLevelDeclaration, ObjectShorthandFunctionScopedDeclaration, ObjectShorthandBlockScopedDeclaration, ObjectShorthand, @@ -21,6 +23,18 @@ export const enum IdentifierRole { } 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. diff --git a/src/parser/tokenizer/state.ts b/src/parser/tokenizer/state.ts index 106c1f09..8888b5ec 100644 --- a/src/parser/tokenizer/state.ts +++ b/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; } } diff --git a/src/parser/traverser/expression.ts b/src/parser/traverser/expression.ts index a38cd162..a591456c 100644 --- a/src/parser/traverser/expression.ts +++ b/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( diff --git a/src/parser/traverser/lval.ts b/src/parser/traverser/lval.ts index b3985e25..7f84fe67 100644 --- a/src/parser/traverser/lval.ts +++ b/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. diff --git a/src/parser/traverser/statement.ts b/src/parser/traverser/statement.ts index 2ba225bd..ea97ed9d 100644 --- a/src/parser/traverser/statement.ts +++ b/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) { diff --git a/src/transformers/CJSImportTransformer.ts b/src/transformers/CJSImportTransformer.ts index 9f6b5e26..40bd34ce 100644 --- a/src/transformers/CJSImportTransformer.ts +++ b/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); diff --git a/test/identifyShadowedGlobals-test.ts b/test/identifyShadowedGlobals-test.ts new file mode 100644 index 00000000..a0e77d86 --- /dev/null +++ b/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, + ); + }); +}); diff --git a/test/tokens-test.ts b/test/tokens-test.ts index 426b84d5..7e5f0967 100644 --- a/test/tokens-test.ts +++ b/test/tokens-test.ts @@ -8,7 +8,7 @@ type TokenExpectation = {[K in keyof SimpleToken]?: SimpleToken[K]}; function assertTokens(code: string, expectedTokens: Array): void { const tokens: Array = parse(code, true, false, false).tokens; - assert.equal(tokens.length, expectedTokens.length); + assert.strictEqual(tokens.length, expectedTokens.length); const projectedTokens = tokens.map((token, i) => { const result = {}; for (const key of Object.keys(expectedTokens[i])) { @@ -16,18 +16,25 @@ function assertTokens(code: string, expectedTokens: Array): vo } return result; }); - assert.deepEqual(projectedTokens, expectedTokens); + assert.deepStrictEqual(projectedTokens, expectedTokens); } describe("tokens", () => { it("properly provides identifier roles for const, let, and var", () => { assertTokens( ` - const x = 1; - let y = 2; - var z = 3; + function f() { + const x = 1; + let y = 2; + var z = 3; + } `, [ + {type: tt._function}, + {type: tt.name, identifierRole: IdentifierRole.TopLevelDeclaration}, + {type: tt.parenL}, + {type: tt.parenR}, + {type: tt.braceL}, {type: tt._const}, {type: tt.name, identifierRole: IdentifierRole.BlockScopedDeclaration}, {type: tt.eq}, @@ -43,6 +50,7 @@ describe("tokens", () => { {type: tt.eq}, {type: tt.num}, {type: tt.semi}, + {type: tt.braceR}, {type: tt.eof}, ], ); @@ -94,12 +102,19 @@ describe("tokens", () => { it("treats functions as function-scoped and classes as block-scoped", () => { assertTokens( ` - function f() { - } - class C { + function wrapper() { + function f() { + } + class C { + } } `, [ + {type: tt._function}, + {type: tt.name, identifierRole: IdentifierRole.TopLevelDeclaration}, + {type: tt.parenL}, + {type: tt.parenR}, + {type: tt.braceL}, {type: tt._function}, {type: tt.name, identifierRole: IdentifierRole.FunctionScopedDeclaration}, {type: tt.parenL}, @@ -110,6 +125,7 @@ describe("tokens", () => { {type: tt.name, identifierRole: IdentifierRole.BlockScopedDeclaration}, {type: tt.braceL}, {type: tt.braceR}, + {type: tt.braceR}, {type: tt.eof}, ], );