From ff10e76c81a297e607825d5ae52984945ebf847b Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 4 Jan 2018 14:29:38 -0800 Subject: [PATCH 1/2] Check for unused getter/setter in classes --- src/compiler/checker.ts | 30 ++++++++++++------- .../reference/unusedGetterInClass.errors.txt | 17 +++++++++++ .../reference/unusedGetterInClass.js | 8 ++++- .../reference/unusedGetterInClass.symbols | 5 +++- .../reference/unusedGetterInClass.types | 5 +++- tests/cases/compiler/unusedGetterInClass.ts | 3 +- 6 files changed, 54 insertions(+), 14 deletions(-) create mode 100644 tests/baselines/reference/unusedGetterInClass.errors.txt diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 49dde4ea6e4c4..c4660e709398e 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -21222,17 +21222,27 @@ namespace ts { if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) { if (node.members) { for (const member of node.members) { - if (member.kind === SyntaxKind.MethodDeclaration || member.kind === SyntaxKind.PropertyDeclaration) { - if (!member.symbol.isReferenced && hasModifier(member, ModifierFlags.Private)) { - error(member.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(member.symbol)); - } - } - else if (member.kind === SyntaxKind.Constructor) { - for (const parameter of (member).parameters) { - if (!parameter.symbol.isReferenced && hasModifier(parameter, ModifierFlags.Private)) { - error(parameter.name, Diagnostics.Property_0_is_declared_but_its_value_is_never_read, symbolName(parameter.symbol)); + switch (member.kind) { + case SyntaxKind.MethodDeclaration: + case SyntaxKind.PropertyDeclaration: + case SyntaxKind.GetAccessor: + case SyntaxKind.SetAccessor: + if (!member.symbol.isReferenced && hasModifier(member, ModifierFlags.Private)) { + error(member.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(member.symbol)); } - } + break; + case SyntaxKind.Constructor: + for (const parameter of (member).parameters) { + if (!parameter.symbol.isReferenced && hasModifier(parameter, ModifierFlags.Private)) { + error(parameter.name, Diagnostics.Property_0_is_declared_but_its_value_is_never_read, symbolName(parameter.symbol)); + } + } + break; + case SyntaxKind.IndexSignature: + // Can't be private + break; + default: + Debug.fail(); } } } diff --git a/tests/baselines/reference/unusedGetterInClass.errors.txt b/tests/baselines/reference/unusedGetterInClass.errors.txt new file mode 100644 index 0000000000000..55c20fced2817 --- /dev/null +++ b/tests/baselines/reference/unusedGetterInClass.errors.txt @@ -0,0 +1,17 @@ +tests/cases/compiler/unusedGetterInClass.ts(4,17): error TS6133: 'fullName' is declared but its value is never read. +tests/cases/compiler/unusedGetterInClass.ts(7,17): error TS6133: 'setter' is declared but its value is never read. + + +==== tests/cases/compiler/unusedGetterInClass.ts (2 errors) ==== + class Employee { + private _fullName: string; + + private get fullName(): string { + ~~~~~~~~ +!!! error TS6133: 'fullName' is declared but its value is never read. + return this._fullName; + } + private set setter(_: string) {} + ~~~~~~ +!!! error TS6133: 'setter' is declared but its value is never read. + } \ No newline at end of file diff --git a/tests/baselines/reference/unusedGetterInClass.js b/tests/baselines/reference/unusedGetterInClass.js index 0bdbf0066086c..88a648e4b7fe1 100644 --- a/tests/baselines/reference/unusedGetterInClass.js +++ b/tests/baselines/reference/unusedGetterInClass.js @@ -2,9 +2,10 @@ class Employee { private _fullName: string; - get fullName(): string { + private get fullName(): string { return this._fullName; } + private set setter(_: string) {} } //// [unusedGetterInClass.js] @@ -18,5 +19,10 @@ var Employee = /** @class */ (function () { enumerable: true, configurable: true }); + Object.defineProperty(Employee.prototype, "setter", { + set: function (_) { }, + enumerable: true, + configurable: true + }); return Employee; }()); diff --git a/tests/baselines/reference/unusedGetterInClass.symbols b/tests/baselines/reference/unusedGetterInClass.symbols index 7bab42f6743a2..52a0351cbbf03 100644 --- a/tests/baselines/reference/unusedGetterInClass.symbols +++ b/tests/baselines/reference/unusedGetterInClass.symbols @@ -5,7 +5,7 @@ class Employee { private _fullName: string; >_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16)) - get fullName(): string { + private get fullName(): string { >fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30)) return this._fullName; @@ -13,4 +13,7 @@ class Employee { >this : Symbol(Employee, Decl(unusedGetterInClass.ts, 0, 0)) >_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16)) } + private set setter(_: string) {} +>setter : Symbol(Employee.setter, Decl(unusedGetterInClass.ts, 5, 5)) +>_ : Symbol(_, Decl(unusedGetterInClass.ts, 6, 23)) } diff --git a/tests/baselines/reference/unusedGetterInClass.types b/tests/baselines/reference/unusedGetterInClass.types index 648bed6ac5d20..7f25263beb500 100644 --- a/tests/baselines/reference/unusedGetterInClass.types +++ b/tests/baselines/reference/unusedGetterInClass.types @@ -5,7 +5,7 @@ class Employee { private _fullName: string; >_fullName : string - get fullName(): string { + private get fullName(): string { >fullName : string return this._fullName; @@ -13,4 +13,7 @@ class Employee { >this : this >_fullName : string } + private set setter(_: string) {} +>setter : string +>_ : string } diff --git a/tests/cases/compiler/unusedGetterInClass.ts b/tests/cases/compiler/unusedGetterInClass.ts index e5259f91cdc42..c02a9b4be2bb7 100644 --- a/tests/cases/compiler/unusedGetterInClass.ts +++ b/tests/cases/compiler/unusedGetterInClass.ts @@ -5,7 +5,8 @@ class Employee { private _fullName: string; - get fullName(): string { + private get fullName(): string { return this._fullName; } + private set setter(_: string) {} } \ No newline at end of file From ee752c100d80d8c4a9385243b925fc29b8cf588e Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 4 Jan 2018 15:04:56 -0800 Subject: [PATCH 2/2] Write-only use of setter is still a reference; and don't error on setter if getter exists --- src/compiler/checker.ts | 40 ++++++++++--------- .../reference/unusedGetterInClass.errors.txt | 8 ++-- .../reference/unusedGetterInClass.js | 8 ++-- .../reference/unusedGetterInClass.symbols | 9 +++-- .../reference/unusedGetterInClass.types | 5 ++- .../reference/unusedSetterInClass.errors.txt | 7 +++- .../reference/unusedSetterInClass.js | 2 +- .../reference/unusedSetterInClass.symbols | 6 +-- .../reference/unusedSetterInClass.types | 2 +- .../reference/unusedSetterInClass2.errors.txt | 14 +++++++ .../reference/unusedSetterInClass2.js | 25 ++++++++++++ .../reference/unusedSetterInClass2.symbols | 18 +++++++++ .../reference/unusedSetterInClass2.types | 20 ++++++++++ tests/cases/compiler/unusedGetterInClass.ts | 3 +- tests/cases/compiler/unusedSetterInClass.ts | 2 +- tests/cases/compiler/unusedSetterInClass2.ts | 10 +++++ 16 files changed, 136 insertions(+), 43 deletions(-) create mode 100644 tests/baselines/reference/unusedSetterInClass2.errors.txt create mode 100644 tests/baselines/reference/unusedSetterInClass2.js create mode 100644 tests/baselines/reference/unusedSetterInClass2.symbols create mode 100644 tests/baselines/reference/unusedSetterInClass2.types create mode 100644 tests/cases/compiler/unusedSetterInClass2.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index c4660e709398e..c90904d6655ef 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -16034,27 +16034,27 @@ namespace ts { } function markPropertyAsReferenced(prop: Symbol, nodeForCheckWriteOnly: Node | undefined, isThisAccess: boolean) { - if (prop && - noUnusedIdentifiers && - (prop.flags & SymbolFlags.ClassMember) && - prop.valueDeclaration && hasModifier(prop.valueDeclaration, ModifierFlags.Private) - && !(nodeForCheckWriteOnly && isWriteOnlyAccess(nodeForCheckWriteOnly))) { - - if (isThisAccess) { - // Find any FunctionLikeDeclaration because those create a new 'this' binding. But this should only matter for methods (or getters/setters). - const containingMethod = findAncestor(nodeForCheckWriteOnly, isFunctionLikeDeclaration); - if (containingMethod && containingMethod.symbol === prop) { - return; - } - } + if (!prop || !noUnusedIdentifiers || !(prop.flags & SymbolFlags.ClassMember) || !prop.valueDeclaration || !hasModifier(prop.valueDeclaration, ModifierFlags.Private)) { + return; + } + if (nodeForCheckWriteOnly && isWriteOnlyAccess(nodeForCheckWriteOnly) && !(prop.flags & SymbolFlags.SetAccessor && !(prop.flags & SymbolFlags.GetAccessor))) { + return; + } - if (getCheckFlags(prop) & CheckFlags.Instantiated) { - getSymbolLinks(prop).target.isReferenced = true; - } - else { - prop.isReferenced = true; + if (isThisAccess) { + // Find any FunctionLikeDeclaration because those create a new 'this' binding. But this should only matter for methods (or getters/setters). + const containingMethod = findAncestor(nodeForCheckWriteOnly, isFunctionLikeDeclaration); + if (containingMethod && containingMethod.symbol === prop) { + return; } } + + if (getCheckFlags(prop) & CheckFlags.Instantiated) { + getSymbolLinks(prop).target.isReferenced = true; + } + else { + prop.isReferenced = true; + } } function isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName, propertyName: __String): boolean { @@ -21227,6 +21227,10 @@ namespace ts { case SyntaxKind.PropertyDeclaration: case SyntaxKind.GetAccessor: case SyntaxKind.SetAccessor: + if (member.kind === SyntaxKind.SetAccessor && member.symbol.flags & SymbolFlags.GetAccessor) { + // Already would have reported an error on the getter. + break; + } if (!member.symbol.isReferenced && hasModifier(member, ModifierFlags.Private)) { error(member.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(member.symbol)); } diff --git a/tests/baselines/reference/unusedGetterInClass.errors.txt b/tests/baselines/reference/unusedGetterInClass.errors.txt index 55c20fced2817..7ab8d33fd9579 100644 --- a/tests/baselines/reference/unusedGetterInClass.errors.txt +++ b/tests/baselines/reference/unusedGetterInClass.errors.txt @@ -1,8 +1,7 @@ tests/cases/compiler/unusedGetterInClass.ts(4,17): error TS6133: 'fullName' is declared but its value is never read. -tests/cases/compiler/unusedGetterInClass.ts(7,17): error TS6133: 'setter' is declared but its value is never read. -==== tests/cases/compiler/unusedGetterInClass.ts (2 errors) ==== +==== tests/cases/compiler/unusedGetterInClass.ts (1 errors) ==== class Employee { private _fullName: string; @@ -11,7 +10,6 @@ tests/cases/compiler/unusedGetterInClass.ts(7,17): error TS6133: 'setter' is dec !!! error TS6133: 'fullName' is declared but its value is never read. return this._fullName; } - private set setter(_: string) {} - ~~~~~~ -!!! error TS6133: 'setter' is declared but its value is never read. + // Will not also error on the setter + private set fullName(_: string) {} } \ No newline at end of file diff --git a/tests/baselines/reference/unusedGetterInClass.js b/tests/baselines/reference/unusedGetterInClass.js index 88a648e4b7fe1..3cdac7b63df58 100644 --- a/tests/baselines/reference/unusedGetterInClass.js +++ b/tests/baselines/reference/unusedGetterInClass.js @@ -5,7 +5,8 @@ class Employee { private get fullName(): string { return this._fullName; } - private set setter(_: string) {} + // Will not also error on the setter + private set fullName(_: string) {} } //// [unusedGetterInClass.js] @@ -16,10 +17,7 @@ var Employee = /** @class */ (function () { get: function () { return this._fullName; }, - enumerable: true, - configurable: true - }); - Object.defineProperty(Employee.prototype, "setter", { + // Will not also error on the setter set: function (_) { }, enumerable: true, configurable: true diff --git a/tests/baselines/reference/unusedGetterInClass.symbols b/tests/baselines/reference/unusedGetterInClass.symbols index 52a0351cbbf03..f80229b197205 100644 --- a/tests/baselines/reference/unusedGetterInClass.symbols +++ b/tests/baselines/reference/unusedGetterInClass.symbols @@ -6,14 +6,15 @@ class Employee { >_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16)) private get fullName(): string { ->fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30)) +>fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30), Decl(unusedGetterInClass.ts, 5, 5)) return this._fullName; >this._fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16)) >this : Symbol(Employee, Decl(unusedGetterInClass.ts, 0, 0)) >_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16)) } - private set setter(_: string) {} ->setter : Symbol(Employee.setter, Decl(unusedGetterInClass.ts, 5, 5)) ->_ : Symbol(_, Decl(unusedGetterInClass.ts, 6, 23)) + // Will not also error on the setter + private set fullName(_: string) {} +>fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30), Decl(unusedGetterInClass.ts, 5, 5)) +>_ : Symbol(_, Decl(unusedGetterInClass.ts, 7, 25)) } diff --git a/tests/baselines/reference/unusedGetterInClass.types b/tests/baselines/reference/unusedGetterInClass.types index 7f25263beb500..822d47e695231 100644 --- a/tests/baselines/reference/unusedGetterInClass.types +++ b/tests/baselines/reference/unusedGetterInClass.types @@ -13,7 +13,8 @@ class Employee { >this : this >_fullName : string } - private set setter(_: string) {} ->setter : string + // Will not also error on the setter + private set fullName(_: string) {} +>fullName : string >_ : string } diff --git a/tests/baselines/reference/unusedSetterInClass.errors.txt b/tests/baselines/reference/unusedSetterInClass.errors.txt index d7cd764b2934e..d5380cdc1ddca 100644 --- a/tests/baselines/reference/unusedSetterInClass.errors.txt +++ b/tests/baselines/reference/unusedSetterInClass.errors.txt @@ -1,13 +1,16 @@ tests/cases/compiler/unusedSetterInClass.ts(2,13): error TS6133: '_fullName' is declared but its value is never read. +tests/cases/compiler/unusedSetterInClass.ts(4,17): error TS6133: 'fullName' is declared but its value is never read. -==== tests/cases/compiler/unusedSetterInClass.ts (1 errors) ==== +==== tests/cases/compiler/unusedSetterInClass.ts (2 errors) ==== class Employee { private _fullName: string; ~~~~~~~~~ !!! error TS6133: '_fullName' is declared but its value is never read. - set fullName(newName: string) { + private set fullName(newName: string) { + ~~~~~~~~ +!!! error TS6133: 'fullName' is declared but its value is never read. this._fullName = newName; } } \ No newline at end of file diff --git a/tests/baselines/reference/unusedSetterInClass.js b/tests/baselines/reference/unusedSetterInClass.js index 30911d8ef2ad8..9b7a65eb449f4 100644 --- a/tests/baselines/reference/unusedSetterInClass.js +++ b/tests/baselines/reference/unusedSetterInClass.js @@ -2,7 +2,7 @@ class Employee { private _fullName: string; - set fullName(newName: string) { + private set fullName(newName: string) { this._fullName = newName; } } diff --git a/tests/baselines/reference/unusedSetterInClass.symbols b/tests/baselines/reference/unusedSetterInClass.symbols index cf04d2da65f72..da5d62bdc85a5 100644 --- a/tests/baselines/reference/unusedSetterInClass.symbols +++ b/tests/baselines/reference/unusedSetterInClass.symbols @@ -5,14 +5,14 @@ class Employee { private _fullName: string; >_fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16)) - set fullName(newName: string) { + private set fullName(newName: string) { >fullName : Symbol(Employee.fullName, Decl(unusedSetterInClass.ts, 1, 30)) ->newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 17)) +>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 25)) this._fullName = newName; >this._fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16)) >this : Symbol(Employee, Decl(unusedSetterInClass.ts, 0, 0)) >_fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16)) ->newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 17)) +>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 25)) } } diff --git a/tests/baselines/reference/unusedSetterInClass.types b/tests/baselines/reference/unusedSetterInClass.types index 1f6cf0d25a0e3..1a4c5c8a76cad 100644 --- a/tests/baselines/reference/unusedSetterInClass.types +++ b/tests/baselines/reference/unusedSetterInClass.types @@ -5,7 +5,7 @@ class Employee { private _fullName: string; >_fullName : string - set fullName(newName: string) { + private set fullName(newName: string) { >fullName : string >newName : string diff --git a/tests/baselines/reference/unusedSetterInClass2.errors.txt b/tests/baselines/reference/unusedSetterInClass2.errors.txt new file mode 100644 index 0000000000000..c3be0fc0302d9 --- /dev/null +++ b/tests/baselines/reference/unusedSetterInClass2.errors.txt @@ -0,0 +1,14 @@ +tests/cases/compiler/unusedSetterInClass2.ts(3,17): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. + + +==== tests/cases/compiler/unusedSetterInClass2.ts (1 errors) ==== + // Unlike everything else, a setter without a getter is used by a write access. + class Employee { + private set p(_: number) {} + ~ +!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. + + m() { + this.p = 0; + } + } \ No newline at end of file diff --git a/tests/baselines/reference/unusedSetterInClass2.js b/tests/baselines/reference/unusedSetterInClass2.js new file mode 100644 index 0000000000000..9c3111982a9e5 --- /dev/null +++ b/tests/baselines/reference/unusedSetterInClass2.js @@ -0,0 +1,25 @@ +//// [unusedSetterInClass2.ts] +// Unlike everything else, a setter without a getter is used by a write access. +class Employee { + private set p(_: number) {} + + m() { + this.p = 0; + } +} + +//// [unusedSetterInClass2.js] +// Unlike everything else, a setter without a getter is used by a write access. +var Employee = /** @class */ (function () { + function Employee() { + } + Object.defineProperty(Employee.prototype, "p", { + set: function (_) { }, + enumerable: true, + configurable: true + }); + Employee.prototype.m = function () { + this.p = 0; + }; + return Employee; +}()); diff --git a/tests/baselines/reference/unusedSetterInClass2.symbols b/tests/baselines/reference/unusedSetterInClass2.symbols new file mode 100644 index 0000000000000..1092f85ff9e7c --- /dev/null +++ b/tests/baselines/reference/unusedSetterInClass2.symbols @@ -0,0 +1,18 @@ +=== tests/cases/compiler/unusedSetterInClass2.ts === +// Unlike everything else, a setter without a getter is used by a write access. +class Employee { +>Employee : Symbol(Employee, Decl(unusedSetterInClass2.ts, 0, 0)) + + private set p(_: number) {} +>p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16)) +>_ : Symbol(_, Decl(unusedSetterInClass2.ts, 2, 18)) + + m() { +>m : Symbol(Employee.m, Decl(unusedSetterInClass2.ts, 2, 31)) + + this.p = 0; +>this.p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16)) +>this : Symbol(Employee, Decl(unusedSetterInClass2.ts, 0, 0)) +>p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16)) + } +} diff --git a/tests/baselines/reference/unusedSetterInClass2.types b/tests/baselines/reference/unusedSetterInClass2.types new file mode 100644 index 0000000000000..0237db7ce8c57 --- /dev/null +++ b/tests/baselines/reference/unusedSetterInClass2.types @@ -0,0 +1,20 @@ +=== tests/cases/compiler/unusedSetterInClass2.ts === +// Unlike everything else, a setter without a getter is used by a write access. +class Employee { +>Employee : Employee + + private set p(_: number) {} +>p : number +>_ : number + + m() { +>m : () => void + + this.p = 0; +>this.p = 0 : 0 +>this.p : number +>this : this +>p : number +>0 : 0 + } +} diff --git a/tests/cases/compiler/unusedGetterInClass.ts b/tests/cases/compiler/unusedGetterInClass.ts index c02a9b4be2bb7..cc08954eb75ce 100644 --- a/tests/cases/compiler/unusedGetterInClass.ts +++ b/tests/cases/compiler/unusedGetterInClass.ts @@ -8,5 +8,6 @@ class Employee { private get fullName(): string { return this._fullName; } - private set setter(_: string) {} + // Will not also error on the setter + private set fullName(_: string) {} } \ No newline at end of file diff --git a/tests/cases/compiler/unusedSetterInClass.ts b/tests/cases/compiler/unusedSetterInClass.ts index a8e45bab43434..634f2b2b08ea5 100644 --- a/tests/cases/compiler/unusedSetterInClass.ts +++ b/tests/cases/compiler/unusedSetterInClass.ts @@ -5,7 +5,7 @@ class Employee { private _fullName: string; - set fullName(newName: string) { + private set fullName(newName: string) { this._fullName = newName; } } \ No newline at end of file diff --git a/tests/cases/compiler/unusedSetterInClass2.ts b/tests/cases/compiler/unusedSetterInClass2.ts new file mode 100644 index 0000000000000..2e6ee32e0e68f --- /dev/null +++ b/tests/cases/compiler/unusedSetterInClass2.ts @@ -0,0 +1,10 @@ +// @noUnusedLocals:true + +// Unlike everything else, a setter without a getter is used by a write access. +class Employee { + private set p(_: number) {} + + m() { + this.p = 0; + } +} \ No newline at end of file