Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow property/accessor overrides #33401

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29513,15 +29513,35 @@ namespace ts {
// either base or derived property is private - not override, skip it
continue;
}

if (isPrototypeProperty(base) || base.flags & SymbolFlags.PropertyOrAccessor && derived.flags & SymbolFlags.PropertyOrAccessor) {
// method is overridden with method or property/accessor is overridden with property/accessor - correct case
continue;
}

let errorMessage: DiagnosticMessage;
if (isPrototypeProperty(base)) {
if (derived.flags & SymbolFlags.Accessor) {
const basePropertyFlags = base.flags & SymbolFlags.PropertyOrAccessor;
const derivedPropertyFlags = derived.flags & SymbolFlags.PropertyOrAccessor;
if (basePropertyFlags && derivedPropertyFlags) {
// property/accessor is overridden with property/accessor
if (baseDeclarationFlags & ModifierFlags.Abstract
|| base.valueDeclaration && base.valueDeclaration.parent.kind === SyntaxKind.InterfaceDeclaration
|| derived.valueDeclaration && isBinaryExpression(derived.valueDeclaration)) {
// when the base property is abstract or from an interface, base/derived flags don't need to match
// same when the derived property is from an assignment
continue;
}
if (basePropertyFlags !== SymbolFlags.Property && derivedPropertyFlags === SymbolFlags.Property) {
errorMessage = Diagnostics.Class_0_defines_instance_member_accessor_1_but_extended_class_2_defines_it_as_instance_member_property;
}
else if (basePropertyFlags === SymbolFlags.Property && derivedPropertyFlags !== SymbolFlags.Property) {
errorMessage = Diagnostics.Class_0_defines_instance_member_property_1_but_extended_class_2_defines_it_as_instance_member_accessor;
}
else {
// correct case
continue;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change makes it obvious that this line is always false and its contents are dead code. I'll remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errr, instead I tried reviving it. We'll see how much code has unknowingly been missing this error.

}
}
else if (isPrototypeProperty(base)) {
if (isPrototypeProperty(derived)) {
// method is overridden with method -- correct case
continue;
}
else if (derived.flags & SymbolFlags.Accessor) {
errorMessage = Diagnostics.Class_0_defines_instance_member_function_1_but_extended_class_2_defines_it_as_instance_member_accessor;
}
else {
Expand Down
9 changes: 9 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2204,6 +2204,15 @@
"category": "Error",
"code": 2609
},
"Class '{0}' defines instance member accessor '{1}', but extended class '{2}' defines it as instance member property.": {
"category": "Error",
"code": 2610
},
"Class '{0}' defines instance member property '{1}', but extended class '{2}' defines it as instance member accessor.": {
"category": "Error",
"code": 2611
},

"Cannot augment module '{0}' with value exports because it resolves to a non-module entity.": {
"category": "Error",
"code": 2649
Expand Down
1 change: 0 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3972,7 +3972,6 @@ namespace ts {
}

export function getModifierFlagsNoCache(node: Node): ModifierFlags {

let flags = ModifierFlags.None;
if (node.modifiers) {
for (const modifier of node.modifiers) {
Expand Down
6 changes: 2 additions & 4 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,6 @@ namespace ts {
}

class TokenObject<TKind extends SyntaxKind> extends TokenOrIdentifierObject implements Token<TKind> {
public symbol!: Symbol;
public kind: TKind;

constructor(kind: TKind, pos: number, end: number) {
Expand All @@ -349,9 +348,8 @@ namespace ts {
}

class IdentifierObject extends TokenOrIdentifierObject implements Identifier {
public kind!: SyntaxKind.Identifier;
public kind: SyntaxKind.Identifier = SyntaxKind.Identifier;
public escapedText!: __String;
public symbol!: Symbol;
public autoGenerateFlags!: GeneratedIdentifierFlags;
_primaryExpressionBrand: any;
_memberExpressionBrand: any;
Expand Down Expand Up @@ -541,7 +539,7 @@ namespace ts {
}

class SourceFileObject extends NodeObject implements SourceFile {
public kind!: SyntaxKind.SourceFile;
public kind: SyntaxKind.SourceFile = SyntaxKind.SourceFile;
public _declarationBrand: any;
public fileName!: string;
public path!: Path;
Expand Down
13 changes: 13 additions & 0 deletions tests/baselines/reference/accessorsOverrideMethod.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideMethod.ts(5,9): error TS2423: Class 'A' defines instance member function 'm', but extended class 'B' defines it as instance member accessor.


==== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideMethod.ts (1 errors) ====
class A {
m() { }
}
class B extends A {
get m() { return () => 1 }
~
!!! error TS2423: Class 'A' defines instance member function 'm', but extended class 'B' defines it as instance member accessor.
}

16 changes: 16 additions & 0 deletions tests/baselines/reference/accessorsOverrideMethod.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//// [accessorsOverrideMethod.ts]
class A {
m() { }
}
class B extends A {
get m() { return () => 1 }
}


//// [accessorsOverrideMethod.js]
class A {
m() { }
}
class B extends A {
get m() { return () => 1; }
}
15 changes: 15 additions & 0 deletions tests/baselines/reference/accessorsOverrideMethod.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideMethod.ts ===
class A {
>A : Symbol(A, Decl(accessorsOverrideMethod.ts, 0, 0))

m() { }
>m : Symbol(A.m, Decl(accessorsOverrideMethod.ts, 0, 9))
}
class B extends A {
>B : Symbol(B, Decl(accessorsOverrideMethod.ts, 2, 1))
>A : Symbol(A, Decl(accessorsOverrideMethod.ts, 0, 0))

get m() { return () => 1 }
>m : Symbol(B.m, Decl(accessorsOverrideMethod.ts, 3, 19))
}

17 changes: 17 additions & 0 deletions tests/baselines/reference/accessorsOverrideMethod.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideMethod.ts ===
class A {
>A : A

m() { }
>m : () => void
}
class B extends A {
>B : B
>A : A

get m() { return () => 1 }
>m : () => number
>() => 1 : () => number
>1 : 1
}

24 changes: 24 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty.ts(5,9): error TS2611: Class 'A' defines instance member property 'p', but extended class 'B' defines it as instance member accessor.
tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty.ts(12,9): error TS2611: Class 'C' defines instance member property 'p', but extended class 'D' defines it as instance member accessor.


==== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty.ts (2 errors) ====
class A {
p = 'yep'
}
class B extends A {
get p() { return 'oh no' } // error
~
!!! error TS2611: Class 'A' defines instance member property 'p', but extended class 'B' defines it as instance member accessor.
}
class C {
p = 101
}
class D extends C {
_secret = 11
get p() { return this._secret } // error
~
!!! error TS2611: Class 'C' defines instance member property 'p', but extended class 'D' defines it as instance member accessor.
set p(value) { this._secret = value } // error
}

39 changes: 39 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//// [accessorsOverrideProperty.ts]
class A {
p = 'yep'
}
class B extends A {
get p() { return 'oh no' } // error
}
class C {
p = 101
}
class D extends C {
_secret = 11
get p() { return this._secret } // error
set p(value) { this._secret = value } // error
}


//// [accessorsOverrideProperty.js]
class A {
constructor() {
this.p = 'yep';
}
}
class B extends A {
get p() { return 'oh no'; } // error
}
class C {
constructor() {
this.p = 101;
}
}
class D extends C {
constructor() {
super(...arguments);
this._secret = 11;
}
get p() { return this._secret; } // error
set p(value) { this._secret = value; } // error
}
42 changes: 42 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty.ts ===
class A {
>A : Symbol(A, Decl(accessorsOverrideProperty.ts, 0, 0))

p = 'yep'
>p : Symbol(A.p, Decl(accessorsOverrideProperty.ts, 0, 9))
}
class B extends A {
>B : Symbol(B, Decl(accessorsOverrideProperty.ts, 2, 1))
>A : Symbol(A, Decl(accessorsOverrideProperty.ts, 0, 0))

get p() { return 'oh no' } // error
>p : Symbol(B.p, Decl(accessorsOverrideProperty.ts, 3, 19))
}
class C {
>C : Symbol(C, Decl(accessorsOverrideProperty.ts, 5, 1))

p = 101
>p : Symbol(C.p, Decl(accessorsOverrideProperty.ts, 6, 9))
}
class D extends C {
>D : Symbol(D, Decl(accessorsOverrideProperty.ts, 8, 1))
>C : Symbol(C, Decl(accessorsOverrideProperty.ts, 5, 1))

_secret = 11
>_secret : Symbol(D._secret, Decl(accessorsOverrideProperty.ts, 9, 19))

get p() { return this._secret } // error
>p : Symbol(D.p, Decl(accessorsOverrideProperty.ts, 10, 17), Decl(accessorsOverrideProperty.ts, 11, 35))
>this._secret : Symbol(D._secret, Decl(accessorsOverrideProperty.ts, 9, 19))
>this : Symbol(D, Decl(accessorsOverrideProperty.ts, 8, 1))
>_secret : Symbol(D._secret, Decl(accessorsOverrideProperty.ts, 9, 19))

set p(value) { this._secret = value } // error
>p : Symbol(D.p, Decl(accessorsOverrideProperty.ts, 10, 17), Decl(accessorsOverrideProperty.ts, 11, 35))
>value : Symbol(value, Decl(accessorsOverrideProperty.ts, 12, 10))
>this._secret : Symbol(D._secret, Decl(accessorsOverrideProperty.ts, 9, 19))
>this : Symbol(D, Decl(accessorsOverrideProperty.ts, 8, 1))
>_secret : Symbol(D._secret, Decl(accessorsOverrideProperty.ts, 9, 19))
>value : Symbol(value, Decl(accessorsOverrideProperty.ts, 12, 10))
}

47 changes: 47 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty.ts ===
class A {
>A : A

p = 'yep'
>p : string
>'yep' : "yep"
}
class B extends A {
>B : B
>A : A

get p() { return 'oh no' } // error
>p : string
>'oh no' : "oh no"
}
class C {
>C : C

p = 101
>p : number
>101 : 101
}
class D extends C {
>D : D
>C : C

_secret = 11
>_secret : number
>11 : 11

get p() { return this._secret } // error
>p : number
>this._secret : number
>this : this
>_secret : number

set p(value) { this._secret = value } // error
>p : number
>value : number
>this._secret = value : number
>this._secret : number
>this : this
>_secret : number
>value : number
}

18 changes: 18 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty2.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty2.ts(6,7): error TS2611: Class 'Base' defines instance member property 'x', but extended class 'Derived' defines it as instance member accessor.


==== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty2.ts (1 errors) ====
class Base {
x = 1;
}

class Derived extends Base {
get x() { return 2; } // should be an error
~
!!! error TS2611: Class 'Base' defines instance member property 'x', but extended class 'Derived' defines it as instance member accessor.
set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1

26 changes: 26 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//// [accessorsOverrideProperty2.ts]
class Base {
x = 1;
}

class Derived extends Base {
get x() { return 2; } // should be an error
set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1


//// [accessorsOverrideProperty2.js]
class Base {
constructor() {
this.x = 1;
}
}
class Derived extends Base {
get x() { return 2; } // should be an error
set x(value) { console.log(`x was set to ${value}`); }
}
const obj = new Derived(); // nothing printed
console.log(obj.x); // 1
Loading