Skip to content

Commit f6dc0ad

Browse files
author
Andy
authored
Check for unused getter/setter in classes (microsoft#21013)
* Check for unused getter/setter in classes * Write-only use of setter is still a reference; and don't error on setter if getter exists
1 parent 4eb633e commit f6dc0ad

16 files changed

+174
-41
lines changed

‎src/compiler/checker.ts

Lines changed: 42 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -16034,27 +16034,27 @@ namespace ts {
1603416034
}
1603516035

1603616036
function markPropertyAsReferenced(prop: Symbol, nodeForCheckWriteOnly: Node | undefined, isThisAccess: boolean) {
16037-
if (prop &&
16038-
noUnusedIdentifiers &&
16039-
(prop.flags & SymbolFlags.ClassMember) &&
16040-
prop.valueDeclaration && hasModifier(prop.valueDeclaration, ModifierFlags.Private)
16041-
&& !(nodeForCheckWriteOnly && isWriteOnlyAccess(nodeForCheckWriteOnly))) {
16042-
16043-
if (isThisAccess) {
16044-
// Find any FunctionLikeDeclaration because those create a new 'this' binding. But this should only matter for methods (or getters/setters).
16045-
const containingMethod = findAncestor(nodeForCheckWriteOnly, isFunctionLikeDeclaration);
16046-
if (containingMethod && containingMethod.symbol === prop) {
16047-
return;
16048-
}
16049-
}
16037+
if (!prop || !noUnusedIdentifiers || !(prop.flags & SymbolFlags.ClassMember) || !prop.valueDeclaration || !hasModifier(prop.valueDeclaration, ModifierFlags.Private)) {
16038+
return;
16039+
}
16040+
if (nodeForCheckWriteOnly && isWriteOnlyAccess(nodeForCheckWriteOnly) && !(prop.flags & SymbolFlags.SetAccessor && !(prop.flags & SymbolFlags.GetAccessor))) {
16041+
return;
16042+
}
1605016043

16051-
if (getCheckFlags(prop) & CheckFlags.Instantiated) {
16052-
getSymbolLinks(prop).target.isReferenced = true;
16053-
}
16054-
else {
16055-
prop.isReferenced = true;
16044+
if (isThisAccess) {
16045+
// Find any FunctionLikeDeclaration because those create a new 'this' binding. But this should only matter for methods (or getters/setters).
16046+
const containingMethod = findAncestor(nodeForCheckWriteOnly, isFunctionLikeDeclaration);
16047+
if (containingMethod && containingMethod.symbol === prop) {
16048+
return;
1605616049
}
1605716050
}
16051+
16052+
if (getCheckFlags(prop) & CheckFlags.Instantiated) {
16053+
getSymbolLinks(prop).target.isReferenced = true;
16054+
}
16055+
else {
16056+
prop.isReferenced = true;
16057+
}
1605816058
}
1605916059

1606016060
function isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName, propertyName: __String): boolean {
@@ -21222,17 +21222,31 @@ namespace ts {
2122221222
if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) {
2122321223
if (node.members) {
2122421224
for (const member of node.members) {
21225-
if (member.kind === SyntaxKind.MethodDeclaration || member.kind === SyntaxKind.PropertyDeclaration) {
21226-
if (!member.symbol.isReferenced && hasModifier(member, ModifierFlags.Private)) {
21227-
error(member.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(member.symbol));
21228-
}
21229-
}
21230-
else if (member.kind === SyntaxKind.Constructor) {
21231-
for (const parameter of (<ConstructorDeclaration>member).parameters) {
21232-
if (!parameter.symbol.isReferenced && hasModifier(parameter, ModifierFlags.Private)) {
21233-
error(parameter.name, Diagnostics.Property_0_is_declared_but_its_value_is_never_read, symbolName(parameter.symbol));
21225+
switch (member.kind) {
21226+
case SyntaxKind.MethodDeclaration:
21227+
case SyntaxKind.PropertyDeclaration:
21228+
case SyntaxKind.GetAccessor:
21229+
case SyntaxKind.SetAccessor:
21230+
if (member.kind === SyntaxKind.SetAccessor && member.symbol.flags & SymbolFlags.GetAccessor) {
21231+
// Already would have reported an error on the getter.
21232+
break;
2123421233
}
21235-
}
21234+
if (!member.symbol.isReferenced && hasModifier(member, ModifierFlags.Private)) {
21235+
error(member.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(member.symbol));
21236+
}
21237+
break;
21238+
case SyntaxKind.Constructor:
21239+
for (const parameter of (<ConstructorDeclaration>member).parameters) {
21240+
if (!parameter.symbol.isReferenced && hasModifier(parameter, ModifierFlags.Private)) {
21241+
error(parameter.name, Diagnostics.Property_0_is_declared_but_its_value_is_never_read, symbolName(parameter.symbol));
21242+
}
21243+
}
21244+
break;
21245+
case SyntaxKind.IndexSignature:
21246+
// Can't be private
21247+
break;
21248+
default:
21249+
Debug.fail();
2123621250
}
2123721251
}
2123821252
}

‎tests/baselines/reference/unusedGetterInClass.errors.txt

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
tests/cases/compiler/unusedGetterInClass.ts(4,17): error TS6133: 'fullName' is declared but its value is never read.
2+
3+
4+
==== tests/cases/compiler/unusedGetterInClass.ts (1 errors) ====
5+
class Employee {
6+
private _fullName: string;
7+
8+
private get fullName(): string {
9+
~~~~~~~~
10+
!!! error TS6133: 'fullName' is declared but its value is never read.
11+
return this._fullName;
12+
}
13+
// Will not also error on the setter
14+
private set fullName(_: string) {}
15+
}

‎tests/baselines/reference/unusedGetterInClass.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
class Employee {
33
private _fullName: string;
44

5-
get fullName(): string {
5+
private get fullName(): string {
66
return this._fullName;
77
}
8+
// Will not also error on the setter
9+
private set fullName(_: string) {}
810
}
911

1012
//// [unusedGetterInClass.js]
@@ -15,6 +17,8 @@ var Employee = /** @class */ (function () {
1517
get: function () {
1618
return this._fullName;
1719
},
20+
// Will not also error on the setter
21+
set: function (_) { },
1822
enumerable: true,
1923
configurable: true
2024
});

‎tests/baselines/reference/unusedGetterInClass.symbols

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,16 @@ class Employee {
55
private _fullName: string;
66
>_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16))
77

8-
get fullName(): string {
9-
>fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30))
8+
private get fullName(): string {
9+
>fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30), Decl(unusedGetterInClass.ts, 5, 5))
1010

1111
return this._fullName;
1212
>this._fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16))
1313
>this : Symbol(Employee, Decl(unusedGetterInClass.ts, 0, 0))
1414
>_fullName : Symbol(Employee._fullName, Decl(unusedGetterInClass.ts, 0, 16))
1515
}
16+
// Will not also error on the setter
17+
private set fullName(_: string) {}
18+
>fullName : Symbol(Employee.fullName, Decl(unusedGetterInClass.ts, 1, 30), Decl(unusedGetterInClass.ts, 5, 5))
19+
>_ : Symbol(_, Decl(unusedGetterInClass.ts, 7, 25))
1620
}

‎tests/baselines/reference/unusedGetterInClass.types

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,16 @@ class Employee {
55
private _fullName: string;
66
>_fullName : string
77

8-
get fullName(): string {
8+
private get fullName(): string {
99
>fullName : string
1010

1111
return this._fullName;
1212
>this._fullName : string
1313
>this : this
1414
>_fullName : string
1515
}
16+
// Will not also error on the setter
17+
private set fullName(_: string) {}
18+
>fullName : string
19+
>_ : string
1620
}

‎tests/baselines/reference/unusedSetterInClass.errors.txt

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,16 @@
11
tests/cases/compiler/unusedSetterInClass.ts(2,13): error TS6133: '_fullName' is declared but its value is never read.
2+
tests/cases/compiler/unusedSetterInClass.ts(4,17): error TS6133: 'fullName' is declared but its value is never read.
23

34

4-
==== tests/cases/compiler/unusedSetterInClass.ts (1 errors) ====
5+
==== tests/cases/compiler/unusedSetterInClass.ts (2 errors) ====
56
class Employee {
67
private _fullName: string;
78
~~~~~~~~~
89
!!! error TS6133: '_fullName' is declared but its value is never read.
910

10-
set fullName(newName: string) {
11+
private set fullName(newName: string) {
12+
~~~~~~~~
13+
!!! error TS6133: 'fullName' is declared but its value is never read.
1114
this._fullName = newName;
1215
}
1316
}

‎tests/baselines/reference/unusedSetterInClass.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
class Employee {
33
private _fullName: string;
44

5-
set fullName(newName: string) {
5+
private set fullName(newName: string) {
66
this._fullName = newName;
77
}
88
}

‎tests/baselines/reference/unusedSetterInClass.symbols

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ class Employee {
55
private _fullName: string;
66
>_fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16))
77

8-
set fullName(newName: string) {
8+
private set fullName(newName: string) {
99
>fullName : Symbol(Employee.fullName, Decl(unusedSetterInClass.ts, 1, 30))
10-
>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 17))
10+
>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 25))
1111

1212
this._fullName = newName;
1313
>this._fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16))
1414
>this : Symbol(Employee, Decl(unusedSetterInClass.ts, 0, 0))
1515
>_fullName : Symbol(Employee._fullName, Decl(unusedSetterInClass.ts, 0, 16))
16-
>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 17))
16+
>newName : Symbol(newName, Decl(unusedSetterInClass.ts, 3, 25))
1717
}
1818
}

‎tests/baselines/reference/unusedSetterInClass.types

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ class Employee {
55
private _fullName: string;
66
>_fullName : string
77

8-
set fullName(newName: string) {
8+
private set fullName(newName: string) {
99
>fullName : string
1010
>newName : string
1111

‎tests/baselines/reference/unusedSetterInClass2.errors.txt

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
tests/cases/compiler/unusedSetterInClass2.ts(3,17): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
2+
3+
4+
==== tests/cases/compiler/unusedSetterInClass2.ts (1 errors) ====
5+
// Unlike everything else, a setter without a getter is used by a write access.
6+
class Employee {
7+
private set p(_: number) {}
8+
~
9+
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
10+
11+
m() {
12+
this.p = 0;
13+
}
14+
}

‎tests/baselines/reference/unusedSetterInClass2.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
//// [unusedSetterInClass2.ts]
2+
// Unlike everything else, a setter without a getter is used by a write access.
3+
class Employee {
4+
private set p(_: number) {}
5+
6+
m() {
7+
this.p = 0;
8+
}
9+
}
10+
11+
//// [unusedSetterInClass2.js]
12+
// Unlike everything else, a setter without a getter is used by a write access.
13+
var Employee = /** @class */ (function () {
14+
function Employee() {
15+
}
16+
Object.defineProperty(Employee.prototype, "p", {
17+
set: function (_) { },
18+
enumerable: true,
19+
configurable: true
20+
});
21+
Employee.prototype.m = function () {
22+
this.p = 0;
23+
};
24+
return Employee;
25+
}());

‎tests/baselines/reference/unusedSetterInClass2.symbols

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
=== tests/cases/compiler/unusedSetterInClass2.ts ===
2+
// Unlike everything else, a setter without a getter is used by a write access.
3+
class Employee {
4+
>Employee : Symbol(Employee, Decl(unusedSetterInClass2.ts, 0, 0))
5+
6+
private set p(_: number) {}
7+
>p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16))
8+
>_ : Symbol(_, Decl(unusedSetterInClass2.ts, 2, 18))
9+
10+
m() {
11+
>m : Symbol(Employee.m, Decl(unusedSetterInClass2.ts, 2, 31))
12+
13+
this.p = 0;
14+
>this.p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16))
15+
>this : Symbol(Employee, Decl(unusedSetterInClass2.ts, 0, 0))
16+
>p : Symbol(Employee.p, Decl(unusedSetterInClass2.ts, 1, 16))
17+
}
18+
}

‎tests/baselines/reference/unusedSetterInClass2.types

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
=== tests/cases/compiler/unusedSetterInClass2.ts ===
2+
// Unlike everything else, a setter without a getter is used by a write access.
3+
class Employee {
4+
>Employee : Employee
5+
6+
private set p(_: number) {}
7+
>p : number
8+
>_ : number
9+
10+
m() {
11+
>m : () => void
12+
13+
this.p = 0;
14+
>this.p = 0 : 0
15+
>this.p : number
16+
>this : this
17+
>p : number
18+
>0 : 0
19+
}
20+
}

‎tests/cases/compiler/unusedGetterInClass.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
class Employee {
66
private _fullName: string;
77

8-
get fullName(): string {
8+
private get fullName(): string {
99
return this._fullName;
1010
}
11+
// Will not also error on the setter
12+
private set fullName(_: string) {}
1113
}

‎tests/cases/compiler/unusedSetterInClass.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
class Employee {
66
private _fullName: string;
77

8-
set fullName(newName: string) {
8+
private set fullName(newName: string) {
99
this._fullName = newName;
1010
}
1111
}

‎tests/cases/compiler/unusedSetterInClass2.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// @noUnusedLocals:true
2+
3+
// Unlike everything else, a setter without a getter is used by a write access.
4+
class Employee {
5+
private set p(_: number) {}
6+
7+
m() {
8+
this.p = 0;
9+
}
10+
}

0 commit comments

Comments
 (0)