diff --git a/src/rules/arrayTypeRule.ts b/src/rules/arrayTypeRule.ts index e5a3a136741..763cf8a4342 100644 --- a/src/rules/arrayTypeRule.ts +++ b/src/rules/arrayTypeRule.ts @@ -97,8 +97,7 @@ function walk(ctx: Lint.WalkContext): void { const failureString = option === "array" ? Rule.FAILURE_STRING_ARRAY : Rule.FAILURE_STRING_ARRAY_SIMPLE; if (typeArguments === undefined || typeArguments.length === 0) { // Create an 'any' array - const fix = Lint.Replacement.replaceFromTo(node.getStart(), node.getEnd(), "any[]"); - ctx.addFailureAtNode(node, failureString, fix); + ctx.addFailureAtNode(node, failureString, Lint.Replacement.replaceFromTo(node.getStart(), node.getEnd(), "any[]")); return; } @@ -108,13 +107,12 @@ function walk(ctx: Lint.WalkContext): void { const type = typeArguments[0]; const parens = typeNeedsParentheses(type); - const fix = [ + ctx.addFailureAtNode(node, failureString, [ // Delete 'Array<' Lint.Replacement.replaceFromTo(node.getStart(), type.getStart(), parens ? "(" : ""), // Delete '>' and replace with '[] Lint.Replacement.replaceFromTo(type.getEnd(), node.getEnd(), parens ? ")[]" : "[]"), - ]; - ctx.addFailureAtNode(node, failureString, fix); + ]); } } diff --git a/src/rules/memberAccessRule.ts b/src/rules/memberAccessRule.ts index d3eed54bf21..4901d935825 100644 --- a/src/rules/memberAccessRule.ts +++ b/src/rules/memberAccessRule.ts @@ -129,12 +129,12 @@ function walk(ctx: Lint.WalkContext) { ? getChildOfKind(node, ts.SyntaxKind.ConstructorKeyword, ctx.sourceFile)! : node.name !== undefined ? node.name : node; const memberName = node.name !== undefined && node.name.kind === ts.SyntaxKind.Identifier ? node.name.text : undefined; - ctx.addFailureAtNode(nameNode, Rule.FAILURE_STRING_FACTORY(memberType(node), memberName)); + ctx.addFailureAtNode(nameNode, Rule.FAILURE_STRING_FACTORY(typeToString(node), memberName)); } } } -function memberType(node: ts.ClassElement): string { +function typeToString(node: ts.ClassElement): string { switch (node.kind) { case ts.SyntaxKind.MethodDeclaration: return "class method"; diff --git a/src/rules/noShadowedVariableRule.ts b/src/rules/noShadowedVariableRule.ts index d425cba5c54..9da22cd5033 100644 --- a/src/rules/noShadowedVariableRule.ts +++ b/src/rules/noShadowedVariableRule.ts @@ -15,9 +15,9 @@ * limitations under the License. */ -// tslint:disable deprecation -// (https://github.com/palantir/tslint/pull/2598) - +import { + isBlockScopedVariableDeclarationList, isFunctionExpression, isFunctionWithBody, isScopeBoundary, isThisParameter, ScopeBoundary, +} from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -37,121 +37,165 @@ export class Rule extends Lint.Rules.AbstractRule { /* tslint:enable:object-literal-sort-keys */ public static FAILURE_STRING_FACTORY(name: string) { - return `Shadowed variable: '${name}'`; + return `Shadowed name: '${name}'`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - return this.applyWithWalker(new NoShadowedVariableWalker(sourceFile, this.getOptions())); + return this.applyWithWalker(new NoShadowedVariableWalker(sourceFile, this.ruleName, undefined)); } } -class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker, Set> { - public createScope() { - return new Set(); - } - - public createBlockScope() { - return new Set(); - } - - public visitBindingElement(node: ts.BindingElement) { - const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier; - if (isSingleVariable) { - const name = node.name as ts.Identifier; - const variableDeclaration = Lint.getBindingElementVariableDeclaration(node); - const isBlockScopedVariable = variableDeclaration !== null && Lint.isBlockScopedVariable(variableDeclaration); - this.handleSingleVariableIdentifier(name, isBlockScopedVariable); +class Scope { + public functionScope: Scope; + public variables = new Map(); + public variablesSeen = new Map(); + public reassigned = new Set(); + constructor(functionScope?: Scope) { + // if no functionScope is provided we are in the process of creating a new function scope, which for consistency links to itself + this.functionScope = functionScope !== undefined ? functionScope : this; + } + + public addVariable(identifier: ts.Identifier, blockScoped = true) { + // block scoped variables go to the block scope, function scoped variables to the containing function scope + const scope = blockScoped ? this : this.functionScope; + const list = scope.variables.get(identifier.text); + if (list === undefined) { + scope.variables.set(identifier.text, [identifier]); + } else { + list.push(identifier); } - - super.visitBindingElement(node); - } - - public visitCatchClause(node: ts.CatchClause) { - // don't visit the catch clause variable declaration, just visit the block - // the catch clause variable declaration has its own special scoping rules - this.visitBlock(node.block); - } - - public visitCallSignature(_node: ts.SignatureDeclaration) { - // don't call super, we don't need to check parameter names in call signatures } +} - public visitFunctionType(_node: ts.FunctionOrConstructorTypeNode) { - // don't call super, we don't need to check names in function types - } - - public visitConstructorType(_node: ts.FunctionOrConstructorTypeNode) { - // don't call super, we don't need to check names in constructor types - } - - public visitIndexSignatureDeclaration(_node: ts.SignatureDeclaration) { - // don't call super, we don't want to walk index signatures - } - - public visitMethodSignature(_node: ts.SignatureDeclaration) { - // don't call super, we don't want to walk method signatures either - } - - public visitParameterDeclaration(node: ts.ParameterDeclaration) { - const isSingleParameter = node.name.kind === ts.SyntaxKind.Identifier; - - if (isSingleParameter) { - this.handleSingleVariableIdentifier(node.name as ts.Identifier, false); - } - - super.visitParameterDeclaration(node); - } - - public visitTypeLiteral(_node: ts.TypeLiteralNode) { - // don't call super, we don't want to walk the inside of type nodes - } - - public visitVariableDeclaration(node: ts.VariableDeclaration) { - const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier; - - if (isSingleVariable) { - this.handleSingleVariableIdentifier(node.name as ts.Identifier, Lint.isBlockScopedVariable(node)); +class NoShadowedVariableWalker extends Lint.AbstractWalker { + private scope: Scope; + public walk(sourceFile: ts.SourceFile) { + this.scope = new Scope(); + + const cb = (node: ts.Node): void => { + const parentScope = this.scope; + if (isFunctionExpression(node) && node.name !== undefined) { + /* special handling for named function expressions: + technically the name of the function is only visible inside of it, + but variables with the same name declared inside don't cause compiler errors. + Therefore we add an additional function scope only for the function name to avoid merging with other declarations */ + const functionScope = new Scope(); + functionScope.addVariable(node.name, false); + this.scope = new Scope(); + ts.forEachChild(node, cb); + this.onScopeEnd(functionScope); + this.scope = functionScope; + this.onScopeEnd(parentScope); + this.scope = parentScope; + return; + } + const boundary = isScopeBoundary(node); + if (boundary === ScopeBoundary.Block) { + this.scope = new Scope(parentScope.functionScope); + } else if (boundary === ScopeBoundary.Function) { + this.scope = new Scope(); + } + switch (node.kind) { + case ts.SyntaxKind.VariableDeclarationList: + this.handleVariableDeclarationList(node as ts.VariableDeclarationList); + break; + case ts.SyntaxKind.ClassExpression: + if ((node as ts.ClassExpression).name !== undefined) { + this.scope.addVariable((node as ts.ClassExpression).name!); + } + break; + case ts.SyntaxKind.TypeParameter: + this.scope.addVariable((node as ts.TypeParameterDeclaration).name); + break; + case ts.SyntaxKind.FunctionDeclaration: + case ts.SyntaxKind.ClassDeclaration: + if ((node as ts.FunctionDeclaration | ts.ClassDeclaration).name !== undefined) { + parentScope.addVariable((node as ts.FunctionDeclaration | ts.ClassDeclaration).name!, + node.kind !== ts.SyntaxKind.FunctionDeclaration); + } + break; + case ts.SyntaxKind.TypeAliasDeclaration: + case ts.SyntaxKind.EnumDeclaration: + case ts.SyntaxKind.InterfaceDeclaration: + parentScope.addVariable((node as ts.TypeAliasDeclaration | ts.EnumDeclaration | ts.InterfaceDeclaration).name); + break; + case ts.SyntaxKind.Parameter: + if (!isThisParameter(node as ts.ParameterDeclaration) && isFunctionWithBody(node.parent!)) { + this.handleBindingName((node as ts.ParameterDeclaration).name, false); + } + break; + case ts.SyntaxKind.ModuleDeclaration: + if (node.parent!.kind !== ts.SyntaxKind.ModuleDeclaration && + (node as ts.ModuleDeclaration).name.kind === ts.SyntaxKind.Identifier) { + parentScope.addVariable((node as ts.NamespaceDeclaration).name, false); + } + break; + case ts.SyntaxKind.ImportClause: + if ((node as ts.ImportClause).name !== undefined) { + this.scope.addVariable((node as ts.ImportClause).name!, false); + } + break; + case ts.SyntaxKind.NamespaceImport: + case ts.SyntaxKind.ImportSpecifier: + case ts.SyntaxKind.ImportEqualsDeclaration: + this.scope.addVariable((node as ts.NamespaceImport | ts.ImportSpecifier | ts.ImportEqualsDeclaration).name, false); + } + if (boundary !== ScopeBoundary.None) { + ts.forEachChild(node, cb); + this.onScopeEnd(parentScope); + this.scope = parentScope; + } else { + return ts.forEachChild(node, cb); + } + }; + + ts.forEachChild(sourceFile, cb); + this.onScopeEnd(); + } + + private handleVariableDeclarationList(node: ts.VariableDeclarationList) { + const blockScoped = isBlockScopedVariableDeclarationList(node); + for (const variable of node.declarations) { + this.handleBindingName(variable.name, blockScoped); } - - super.visitVariableDeclaration(node); } - private handleSingleVariableIdentifier(variableIdentifier: ts.Identifier, isBlockScoped: boolean) { - const variableName = variableIdentifier.text; - - if (this.isVarInCurrentScope(variableName) && !this.inCurrentBlockScope(variableName)) { - // shadowing if there's already a `var` of the same name in the scope AND - // it's not in the current block (handled by the 'no-duplicate-variable' rule) - this.addFailureOnIdentifier(variableIdentifier); - } else if (this.inPreviousBlockScope(variableName)) { - // shadowing if there is a `var`, `let`, 'const`, or parameter in a previous block scope - this.addFailureOnIdentifier(variableIdentifier); - } - - if (!isBlockScoped) { - // `var` variables go on the scope - this.getCurrentScope().add(variableName); + private handleBindingName(node: ts.BindingName, blockScoped: boolean) { + if (node.kind === ts.SyntaxKind.Identifier) { + this.scope.addVariable(node, blockScoped); + } else { + for (const element of node.elements) { + if (element.kind !== ts.SyntaxKind.OmittedExpression) { + this.handleBindingName(element.name, blockScoped); + } + } } - // all variables go on block scope, including `var` - this.getCurrentBlockScope().add(variableName); - } - - private isVarInCurrentScope(varName: string) { - return this.getCurrentScope().has(varName); } - private inCurrentBlockScope(varName: string) { - return this.getCurrentBlockScope().has(varName); - } - - private inPreviousBlockScope(varName: string) { - return this.getAllBlockScopes().some((scopeInfo) => { - return scopeInfo !== this.getCurrentBlockScope() && scopeInfo.has(varName); + private onScopeEnd(parent?: Scope) { + const {variables, variablesSeen} = this.scope; + variablesSeen.forEach((identifiers, name) => { + if (variables.has(name)) { + for (const identifier of identifiers) { + this.addFailureAtNode(identifier, Rule.FAILURE_STRING_FACTORY(name)); + } + } else if (parent !== undefined) { + addToList(parent.variablesSeen, name, identifiers); + } }); + if (parent !== undefined) { + variables.forEach((identifiers, name) => { + addToList(parent.variablesSeen, name, identifiers); + }); + } } +} - private addFailureOnIdentifier(ident: ts.Identifier) { - const failureString = Rule.FAILURE_STRING_FACTORY(ident.text); - this.addFailureAtNode(ident, failureString); +function addToList(map: Map, name: string, identifiers: ts.Identifier[]) { + const list = map.get(name); + if (list === undefined) { + map.set(name, identifiers); + } else { + list.push(...identifiers); } } diff --git a/src/rules/strictBooleanExpressionsRule.ts b/src/rules/strictBooleanExpressionsRule.ts index 366b8db9d97..f38275bfddb 100644 --- a/src/rules/strictBooleanExpressionsRule.ts +++ b/src/rules/strictBooleanExpressionsRule.ts @@ -422,12 +422,12 @@ function showLocation(n: Location): string { } } -function showFailure(location: Location, ty: TypeFailure, isUnionType: boolean, options: Options): string { +function showFailure(location: Location, ty: TypeFailure, unionType: boolean, options: Options): string { const expectedTypes = showExpectedTypes(options); const expected = expectedTypes.length === 1 ? `Only ${expectedTypes[0]}s are allowed` : `Allowed types are ${stringOr(expectedTypes)}`; - const tyFail = showTypeFailure(ty, isUnionType, options.strictNullChecks); + const tyFail = showTypeFailure(ty, unionType, options.strictNullChecks); return `This type is not allowed in the ${showLocation(location)} because it ${tyFail}. ${expected}.`; } @@ -441,8 +441,8 @@ function showExpectedTypes(options: Options): string[] { return parts; } -function showTypeFailure(ty: TypeFailure, isUnionType: boolean, strictNullChecks: boolean) { - const is = isUnionType ? "could be" : "is"; +function showTypeFailure(ty: TypeFailure, unionType: boolean, strictNullChecks: boolean) { + const is = unionType ? "could be" : "is"; switch (ty) { case TypeFailure.AlwaysTruthy: return strictNullChecks diff --git a/src/rules/whitespaceRule.ts b/src/rules/whitespaceRule.ts index 4b06b7c8d45..75bdfa86d31 100644 --- a/src/rules/whitespaceRule.ts +++ b/src/rules/whitespaceRule.ts @@ -144,7 +144,7 @@ function walk(ctx: Lint.WalkContext) { // an import clause can have _both_ named bindings and a name (the latter for the default import) // but the named bindings always come last, so we only need to check that for whitespace let position: number | undefined; - const { name, namedBindings } = importClause; + const { namedBindings } = importClause; if (namedBindings !== undefined) { if (namedBindings.kind !== ts.SyntaxKind.NamespaceImport) { namedBindings.elements.forEach((element, idx, arr) => { @@ -162,8 +162,8 @@ function walk(ctx: Lint.WalkContext) { }); } position = namedBindings.getEnd(); - } else if (name !== undefined) { - position = name.getEnd(); + } else if (importClause.name !== undefined) { + position = importClause.name.getEnd(); } if (position !== undefined) { diff --git a/src/test.ts b/src/test.ts index 7a0775bec61..9a7fa22e43e 100644 --- a/src/test.ts +++ b/src/test.ts @@ -117,7 +117,7 @@ export function runTest(testDirectory: string, rulesDirectory?: string | string[ getCanonicalFileName: (filename) => filename, getCurrentDirectory: () => process.cwd(), getDefaultLibFileName: () => ts.getDefaultLibFileName(compilerOptions), - getDirectories: (path) => fs.readdirSync(path), + getDirectories: (dir) => fs.readdirSync(dir), getNewLine: () => "\n", getSourceFile(filenameToGet) { const target = compilerOptions.target === undefined ? ts.ScriptTarget.ES5 : compilerOptions.target; @@ -223,8 +223,8 @@ export function consoleTestResultHandler(testResult: TestResult): boolean { } else { const markupDiffResults = diff.diffLines(results.markupFromMarkup, results.markupFromLinter); const fixesDiffResults = diff.diffLines(results.fixesFromLinter, results.fixesFromMarkup); - const didMarkupTestPass = !markupDiffResults.some((diff) => diff.added === true || diff.removed === true); - const didFixesTestPass = !fixesDiffResults.some((diff) => diff.added === true || diff.removed === true); + const didMarkupTestPass = !markupDiffResults.some((hunk) => hunk.added === true || hunk.removed === true); + const didFixesTestPass = !fixesDiffResults.some((hunk) => hunk.added === true || hunk.removed === true); if (didMarkupTestPass && didFixesTestPass) { console.log(colors.green(" Passed")); diff --git a/test/rules/no-shadowed-variable/test.ts.lint b/test/rules/no-shadowed-variable/test.ts.lint index f859db3c943..6da537b3f71 100644 --- a/test/rules/no-shadowed-variable/test.ts.lint +++ b/test/rules/no-shadowed-variable/test.ts.lint @@ -1,36 +1,53 @@ +import { Foo } from './foo'; +import * as Bar from '.bar'; +import Baz from './baz'; +import Bas = require('./bas'); +{ + const Foo = 'bar'; + ~~~ [err % ('Foo')] + const Bar = Foo; + ~~~ [err % ('Bar')] + const Baz = Bar; + ~~~ [err % ('Baz')] + const Bas = Baz; + ~~~ [err % ('Bas')] +} function letTesting() { var a = 1; + ~ [err % ('a')] let b = 1; if (true) { let a = 2; // failure - ~ [Shadowed variable: 'a'] + ~ [err % ('a')] let b = 2; // failure - ~ [Shadowed variable: 'b'] + ~ [err % ('b')] let c = 2; var e = 2; } else { let b = 3; // failure - ~ [Shadowed variable: 'b'] + ~ [err % ('b')] let c = 3; let e = 3; // failure - ~ [Shadowed variable: 'e'] + ~ [err % ('e')] let f = 3; + ~ [err % ('f')] } var f = 4; + ~ [err % ('f')] } let a = 1; if (true) { let a = 2; // failure - ~ [Shadowed variable: 'a'] + ~ [err % ('a')] } var g = 1; for (var index in [0, 1, 2]) { - var g = 2; // failure - ~ [Shadowed variable: 'g'] + let g = 2; // failure + ~ [err % ('g')] } function constTesting() { @@ -39,24 +56,21 @@ function constTesting() { if (true) { const h = 2; // failure - ~ [Shadowed variable: 'h'] + ~ [err % ('h')] const i = 2; // failure - ~ [Shadowed variable: 'i'] + ~ [err % ('i')] } } function testArguments(x: number, y: number): void { var x = 1; // failure - ~ [Shadowed variable: 'x'] let y = 2; // tsc error - ~ [Shadowed variable: 'y'] } let j = 1; -for (var index in [0, 1, 2]) { // failure - ~~~~~ [Shadowed variable: 'index'] +for (var index in [0, 1, 2]) { let j = 2; // failure - ~ [Shadowed variable: 'j'] + ~ [err % ('j')] } function testTryStatement() { @@ -69,7 +83,7 @@ function testTryStatement() { } finally { let foo = 3; let bar = 3; // failure - ~~~ [Shadowed variable: 'bar'] + ~~~ [err % ('bar')] } } @@ -78,7 +92,7 @@ function testWhileStatement() { while (true) { let foo = 2; // failure - ~~~ [Shadowed variable: 'foo'] + ~~~ [err % ('foo')] } } @@ -87,7 +101,7 @@ function testDoStatement() { do { let foo = 2; // failure - ~~~ [Shadowed variable: 'foo'] + ~~~ [err % ('foo')] } while (true); } @@ -101,21 +115,21 @@ function testDestructuring(x: number) { function innerFunc() { var [foo] = myFunc(); var [x] = myFunc(); // failure - ~ [Shadowed variable: 'x'] + ~ [err % ('x')] let [y] = myFunc(); // failure - ~ [Shadowed variable: 'y'] + ~ [err % ('y')] const [z] = myFunc(); // failure - ~ [Shadowed variable: 'z'] + ~ [err % ('z')] } function anotherInnerFunc() { var [{x}] = [{x: 1}]; // failure - ~ [Shadowed variable: 'x'] + ~ [err % ('x')] let [[y]] = [[2]]; // failure - ~ [Shadowed variable: 'y'] + ~ [err % ('y')] var [foo, ...bar] = [1, 2, 3, 4]; var [...z] = [1, 2, 3, 4]; // failure - ~ [Shadowed variable: 'z'] + ~ [err % ('z')] } } @@ -139,17 +153,16 @@ interface FalsePositive4 { let p = 1; function testParameterDestructuring( { pos: p }: FalsePositive3, // failure - ~ [Shadowed variable: 'p'] + ~ [err % ('p')] { pos: q, specular: r }: FalsePositive3, { pos }: FalsePositive3 ) { p = 2; - var q = 1; // failure - ~ [Shadowed variable: 'q'] + var q = 1; // merges with parameter, should use no-duplicate-variable to catch this function someInnerFunc() { let pos = 3; // failure - ~~~ [Shadowed variable: 'pos'] + ~~~ [err % ('pos')] } } @@ -198,8 +211,10 @@ function constructorsWithArgsParamConst( derivedCtor: new (...args: any[]) => any, baseCtors: Array any>, args: any[]): void { - const args = []; - ~~~~ [Shadowed variable: 'args'] + { + const args = []; + ~~~~ [err % ('args')] + } } class MyClass { }; @@ -215,5 +230,105 @@ function creatorFunction(constructor: MyConstructor, filter: MyFunction): void { for (let i = 0; i < 3; i++) { let i = 34; - ~ [Shadowed variable: 'i'] + ~ [err % ('i')] +} + +(function f1() { + let f1; + ~~ [err % ('f1')] +})(); +function f2() { + const f2 = 0; + ~~ [err % ('f2')] +} +(function f3() { + var f3; + ~~ [err % ('f3')] +})(); +(function f4(f4) { + ~~ [err % ('f4')] +})(); +let f5; +(function f5(){})() + ~~ [err % ('f5')] + +class C { + ~ [err % ('C')] + C(C: C): C { + ~ [err % ('C')] + ~ [err % ('C')] + type C = C; + ~ [err % ('C')] + namespace C { + ~ [err % ('C')] + interface C {} + ~ [err % ('C')] + ~ [err % ('C')] + } + } +} + +// allow nested namespaces +namespace ns.C {} +namespace ns { + export namespace C {} + ~ [err % ('C')] +} +namespace ns { + namespace C {} + ~ [err % ('C')] +} + +type Mapped = { + [K in T]: K + ~ [err % ('K')] } + +// Allow parameters in signatures +{ + let param; + class TestClass { + myMethod(param): void; + myMethod(): void; + myMethod(param): void {} + ~~~~~ [err % ('param')] + } + interface TestInterface { + (param): void; + myMethod(param): void + myMethod(): void; + ~~~~~ [err % ('param')] + } +} + +// Handle variables in multiple child scopes +{ + { + { + let test; + ~~~~ [err % ('test')] + } + let test; + ~~~~ [err % ('test')] + { + let test; + ~~~~ [err % ('test')] + } + } + { + let test; + ~~~~ [err % ('test')] + } + let test; +} + +// Allow mergeable declarations. +interface I {} +interface I {} + +// Exempt 'this' +function f(this): void { + function inner(this): void {} +} + +[err]: Shadowed name: '%s' \ No newline at end of file