From a133cec24691d72b9dbf7eaf7b778284a02b277b Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 15 Feb 2018 09:30:58 -0800 Subject: [PATCH] Fix bug: Interface type parameter merged with property is not unused (#21966) --- src/compiler/checker.ts | 30 ++++++------- .../noUnusedLocals_selfReference.errors.txt | 21 ++++++++- .../reference/noUnusedLocals_selfReference.js | 13 ++++++ .../noUnusedLocals_selfReference.symbols | 45 ++++++++++++++----- .../noUnusedLocals_selfReference.types | 23 ++++++++++ .../compiler/noUnusedLocals_selfReference.ts | 9 ++++ .../nounusedTypeParameterConstraint.ts | 3 +- 7 files changed, 115 insertions(+), 29 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e682c18ea95..df69b4d8efe 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -1159,7 +1159,7 @@ namespace ts { const originalLocation = location; // needed for did-you-mean error reporting, which gathers candidates starting from the original location let result: Symbol; let lastLocation: Node; - let lastNonBlockLocation: Node; + let lastSelfReferenceLocation: Node; let propertyWithInvalidInitializer: Node; const errorLocation = location; let grandparent: Node; @@ -1383,17 +1383,17 @@ namespace ts { } break; } - if (isNonBlockLocation(location)) { - lastNonBlockLocation = location; + if (isSelfReferenceLocation(location)) { + lastSelfReferenceLocation = location; } lastLocation = location; location = location.parent; } // We just climbed up parents looking for the name, meaning that we started in a descendant node of `lastLocation`. - // If `result === lastNonBlockLocation.symbol`, that means that we are somewhere inside `lastNonBlockLocation` looking up a name, and resolving to `lastLocation` itself. + // If `result === lastSelfReferenceLocation.symbol`, that means that we are somewhere inside `lastSelfReferenceLocation` looking up a name, and resolving to `lastLocation` itself. // That means that this is a self-reference of `lastLocation`, and shouldn't count this when considering whether `lastLocation` is used. - if (isUse && result && nameNotFoundMessage && noUnusedIdentifiers && result !== lastNonBlockLocation.symbol) { + if (isUse && result && nameNotFoundMessage && noUnusedIdentifiers && (!lastSelfReferenceLocation || result !== lastSelfReferenceLocation.symbol)) { result.isReferenced = true; } @@ -1476,17 +1476,17 @@ namespace ts { return result; } - function isNonBlockLocation({ kind }: Node): boolean { - switch (kind) { - case SyntaxKind.Block: - case SyntaxKind.ModuleBlock: - case SyntaxKind.SwitchStatement: - case SyntaxKind.CaseBlock: - case SyntaxKind.CaseClause: - case SyntaxKind.DefaultClause: - return false; - default: + function isSelfReferenceLocation(node: Node): boolean { + switch (node.kind) { + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.ClassDeclaration: + case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.EnumDeclaration: + case SyntaxKind.TypeAliasDeclaration: + case SyntaxKind.ModuleDeclaration: // For `namespace N { N; }` return true; + default: + return false; } } diff --git a/tests/baselines/reference/noUnusedLocals_selfReference.errors.txt b/tests/baselines/reference/noUnusedLocals_selfReference.errors.txt index e1d198ff559..a3aef6ced68 100644 --- a/tests/baselines/reference/noUnusedLocals_selfReference.errors.txt +++ b/tests/baselines/reference/noUnusedLocals_selfReference.errors.txt @@ -2,10 +2,13 @@ tests/cases/compiler/noUnusedLocals_selfReference.ts(3,10): error TS6133: 'f' is tests/cases/compiler/noUnusedLocals_selfReference.ts(5,14): error TS6133: 'g' is declared but its value is never read. tests/cases/compiler/noUnusedLocals_selfReference.ts(9,7): error TS6133: 'C' is declared but its value is never read. tests/cases/compiler/noUnusedLocals_selfReference.ts(12,6): error TS6133: 'E' is declared but its value is never read. -tests/cases/compiler/noUnusedLocals_selfReference.ts(14,19): error TS6133: 'm' is declared but its value is never read. +tests/cases/compiler/noUnusedLocals_selfReference.ts(13,11): error TS6133: 'I' is declared but its value is never read. +tests/cases/compiler/noUnusedLocals_selfReference.ts(14,6): error TS6133: 'T' is declared but its value is never read. +tests/cases/compiler/noUnusedLocals_selfReference.ts(15,11): error TS6133: 'N' is declared but its value is never read. +tests/cases/compiler/noUnusedLocals_selfReference.ts(22,19): error TS6133: 'm' is declared but its value is never read. -==== tests/cases/compiler/noUnusedLocals_selfReference.ts (5 errors) ==== +==== tests/cases/compiler/noUnusedLocals_selfReference.ts (8 errors) ==== export {}; // Make this a module scope, so these are local variables. function f() { @@ -26,6 +29,20 @@ tests/cases/compiler/noUnusedLocals_selfReference.ts(14,19): error TS6133: 'm' i enum E { A = 0, B = E.A } ~ !!! error TS6133: 'E' is declared but its value is never read. + interface I { x: I }; + ~ +!!! error TS6133: 'I' is declared but its value is never read. + type T = { x: T }; + ~ +!!! error TS6133: 'T' is declared but its value is never read. + namespace N { N; } + ~ +!!! error TS6133: 'N' is declared but its value is never read. + + // Avoid a false positive. + // Previously `T` was considered unused due to merging with the property, + // back when all non-blocks were checked for recursion. + export interface A { T: T } class P { private m() { this.m; } } ~ diff --git a/tests/baselines/reference/noUnusedLocals_selfReference.js b/tests/baselines/reference/noUnusedLocals_selfReference.js index c23d8295340..c676560b64a 100644 --- a/tests/baselines/reference/noUnusedLocals_selfReference.js +++ b/tests/baselines/reference/noUnusedLocals_selfReference.js @@ -11,6 +11,14 @@ class C { m() { C; } } enum E { A = 0, B = E.A } +interface I { x: I }; +type T = { x: T }; +namespace N { N; } + +// Avoid a false positive. +// Previously `T` was considered unused due to merging with the property, +// back when all non-blocks were checked for recursion. +export interface A { T: T } class P { private m() { this.m; } } P; @@ -40,6 +48,11 @@ var E; E[E["A"] = 0] = "A"; E[E["B"] = 0] = "B"; })(E || (E = {})); +; +var N; +(function (N) { + N; +})(N || (N = {})); var P = /** @class */ (function () { function P() { } diff --git a/tests/baselines/reference/noUnusedLocals_selfReference.symbols b/tests/baselines/reference/noUnusedLocals_selfReference.symbols index cd4195094fe..5074fb840a4 100644 --- a/tests/baselines/reference/noUnusedLocals_selfReference.symbols +++ b/tests/baselines/reference/noUnusedLocals_selfReference.symbols @@ -29,23 +29,46 @@ enum E { A = 0, B = E.A } >E : Symbol(E, Decl(noUnusedLocals_selfReference.ts, 10, 1)) >A : Symbol(E.A, Decl(noUnusedLocals_selfReference.ts, 11, 8)) +interface I { x: I }; +>I : Symbol(I, Decl(noUnusedLocals_selfReference.ts, 11, 25)) +>x : Symbol(I.x, Decl(noUnusedLocals_selfReference.ts, 12, 13)) +>I : Symbol(I, Decl(noUnusedLocals_selfReference.ts, 11, 25)) + +type T = { x: T }; +>T : Symbol(T, Decl(noUnusedLocals_selfReference.ts, 12, 21)) +>x : Symbol(x, Decl(noUnusedLocals_selfReference.ts, 13, 10)) +>T : Symbol(T, Decl(noUnusedLocals_selfReference.ts, 12, 21)) + +namespace N { N; } +>N : Symbol(N, Decl(noUnusedLocals_selfReference.ts, 13, 18)) +>N : Symbol(N, Decl(noUnusedLocals_selfReference.ts, 13, 18)) + +// Avoid a false positive. +// Previously `T` was considered unused due to merging with the property, +// back when all non-blocks were checked for recursion. +export interface A { T: T } +>A : Symbol(A, Decl(noUnusedLocals_selfReference.ts, 14, 18)) +>T : Symbol(T, Decl(noUnusedLocals_selfReference.ts, 19, 19), Decl(noUnusedLocals_selfReference.ts, 19, 23)) +>T : Symbol(T, Decl(noUnusedLocals_selfReference.ts, 19, 19), Decl(noUnusedLocals_selfReference.ts, 19, 23)) +>T : Symbol(T, Decl(noUnusedLocals_selfReference.ts, 19, 19), Decl(noUnusedLocals_selfReference.ts, 19, 23)) + class P { private m() { this.m; } } ->P : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 11, 25)) ->m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 13, 9)) ->this.m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 13, 9)) ->this : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 11, 25)) ->m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 13, 9)) +>P : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 19, 30)) +>m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 21, 9)) +>this.m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 21, 9)) +>this : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 19, 30)) +>m : Symbol(P.m, Decl(noUnusedLocals_selfReference.ts, 21, 9)) P; ->P : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 11, 25)) +>P : Symbol(P, Decl(noUnusedLocals_selfReference.ts, 19, 30)) // Does not detect mutual recursion. function g() { D; } ->g : Symbol(g, Decl(noUnusedLocals_selfReference.ts, 14, 2)) ->D : Symbol(D, Decl(noUnusedLocals_selfReference.ts, 17, 19)) +>g : Symbol(g, Decl(noUnusedLocals_selfReference.ts, 22, 2)) +>D : Symbol(D, Decl(noUnusedLocals_selfReference.ts, 25, 19)) class D { m() { g; } } ->D : Symbol(D, Decl(noUnusedLocals_selfReference.ts, 17, 19)) ->m : Symbol(D.m, Decl(noUnusedLocals_selfReference.ts, 18, 9)) ->g : Symbol(g, Decl(noUnusedLocals_selfReference.ts, 14, 2)) +>D : Symbol(D, Decl(noUnusedLocals_selfReference.ts, 25, 19)) +>m : Symbol(D.m, Decl(noUnusedLocals_selfReference.ts, 26, 9)) +>g : Symbol(g, Decl(noUnusedLocals_selfReference.ts, 22, 2)) diff --git a/tests/baselines/reference/noUnusedLocals_selfReference.types b/tests/baselines/reference/noUnusedLocals_selfReference.types index cbaa413c651..de904d98513 100644 --- a/tests/baselines/reference/noUnusedLocals_selfReference.types +++ b/tests/baselines/reference/noUnusedLocals_selfReference.types @@ -30,6 +30,29 @@ enum E { A = 0, B = E.A } >E : typeof E >A : E +interface I { x: I }; +>I : I +>x : I +>I : I + +type T = { x: T }; +>T : { x: T; } +>x : { x: T; } +>T : { x: T; } + +namespace N { N; } +>N : typeof N +>N : typeof N + +// Avoid a false positive. +// Previously `T` was considered unused due to merging with the property, +// back when all non-blocks were checked for recursion. +export interface A { T: T } +>A : A +>T : T +>T : T +>T : T + class P { private m() { this.m; } } >P : P >m : () => void diff --git a/tests/cases/compiler/noUnusedLocals_selfReference.ts b/tests/cases/compiler/noUnusedLocals_selfReference.ts index 10ec9ebf782..9ae8244d0f8 100644 --- a/tests/cases/compiler/noUnusedLocals_selfReference.ts +++ b/tests/cases/compiler/noUnusedLocals_selfReference.ts @@ -1,4 +1,5 @@ // @noUnusedLocals: true +// @noUnusedParameters: true export {}; // Make this a module scope, so these are local variables. @@ -12,6 +13,14 @@ class C { m() { C; } } enum E { A = 0, B = E.A } +interface I { x: I }; +type T = { x: T }; +namespace N { N; } + +// Avoid a false positive. +// Previously `T` was considered unused due to merging with the property, +// back when all non-blocks were checked for recursion. +export interface A { T: T } class P { private m() { this.m; } } P; diff --git a/tests/cases/compiler/nounusedTypeParameterConstraint.ts b/tests/cases/compiler/nounusedTypeParameterConstraint.ts index d2c3a1677ee..8108d01d68f 100644 --- a/tests/cases/compiler/nounusedTypeParameterConstraint.ts +++ b/tests/cases/compiler/nounusedTypeParameterConstraint.ts @@ -1,4 +1,5 @@ -//@noUnusedLocals:true +// @noUnusedLocals: true +// @noUnusedParameters:true //@filename: bar.ts export interface IEventSourcedEntity { }