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 1 commit
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
23 changes: 18 additions & 5 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29513,14 +29513,27 @@ 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
if (isPrototypeProperty(base)) {
// method is overridden with method - correct case
continue;
}

let errorMessage: DiagnosticMessage;
if (isPrototypeProperty(base)) {
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) && 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 (!(baseDeclarationFlags & ModifierFlags.Abstract) && 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;
}
}
else if (isPrototypeProperty(base)) {
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.

if (derived.flags & SymbolFlags.Accessor) {
errorMessage = Diagnostics.Class_0_defines_instance_member_function_1_but_extended_class_2_defines_it_as_instance_member_accessor;
}
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
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
36 changes: 36 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty2.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty2.ts ===
class Base {
>Base : Symbol(Base, Decl(accessorsOverrideProperty2.ts, 0, 0))

x = 1;
>x : Symbol(Base.x, Decl(accessorsOverrideProperty2.ts, 0, 12))
}

class Derived extends Base {
>Derived : Symbol(Derived, Decl(accessorsOverrideProperty2.ts, 2, 1))
>Base : Symbol(Base, Decl(accessorsOverrideProperty2.ts, 0, 0))

get x() { return 2; } // should be an error
>x : Symbol(Derived.x, Decl(accessorsOverrideProperty2.ts, 4, 28), Decl(accessorsOverrideProperty2.ts, 5, 23))

set x(value) { console.log(`x was set to ${value}`); }
>x : Symbol(Derived.x, Decl(accessorsOverrideProperty2.ts, 4, 28), Decl(accessorsOverrideProperty2.ts, 5, 23))
>value : Symbol(value, Decl(accessorsOverrideProperty2.ts, 6, 8))
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>value : Symbol(value, Decl(accessorsOverrideProperty2.ts, 6, 8))
}

const obj = new Derived(); // nothing printed
>obj : Symbol(obj, Decl(accessorsOverrideProperty2.ts, 9, 5))
>Derived : Symbol(Derived, Decl(accessorsOverrideProperty2.ts, 2, 1))

console.log(obj.x); // 1
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>obj.x : Symbol(Derived.x, Decl(accessorsOverrideProperty2.ts, 4, 28), Decl(accessorsOverrideProperty2.ts, 5, 23))
>obj : Symbol(obj, Decl(accessorsOverrideProperty2.ts, 9, 5))
>x : Symbol(Derived.x, Decl(accessorsOverrideProperty2.ts, 4, 28), Decl(accessorsOverrideProperty2.ts, 5, 23))

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

x = 1;
>x : number
>1 : 1
}

class Derived extends Base {
>Derived : Derived
>Base : Base

get x() { return 2; } // should be an error
>x : number
>2 : 2

set x(value) { console.log(`x was set to ${value}`); }
>x : number
>value : number
>console.log(`x was set to ${value}`) : void
>console.log : (message?: any, ...optionalParams: any[]) => void
>console : Console
>log : (message?: any, ...optionalParams: any[]) => void
>`x was set to ${value}` : string
>value : number
}

const obj = new Derived(); // nothing printed
>obj : Derived
>new Derived() : Derived
>Derived : typeof Derived

console.log(obj.x); // 1
>console.log(obj.x) : void
>console.log : (message?: any, ...optionalParams: any[]) => void
>console : Console
>log : (message?: any, ...optionalParams: any[]) => void
>obj.x : number
>obj : Derived
>x : number

15 changes: 15 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty3.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty3.ts(6,9): error TS2611: Class 'Animal' defines instance member property 'sound', but extended class 'Lion' defines it as instance member accessor.


==== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty3.ts (1 errors) ====
declare class Animal {
sound: string
}
class Lion extends Animal {
_sound = 'grrr'
get sound() { return this._sound } // error here
~~~~~
!!! error TS2611: Class 'Animal' defines instance member property 'sound', but extended class 'Lion' defines it as instance member accessor.
set sound(val) { this._sound = val }
}

20 changes: 20 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// [accessorsOverrideProperty3.ts]
declare class Animal {
sound: string
}
class Lion extends Animal {
_sound = 'grrr'
get sound() { return this._sound } // error here
set sound(val) { this._sound = val }
}


//// [accessorsOverrideProperty3.js]
class Lion extends Animal {
constructor() {
super(...arguments);
this._sound = 'grrr';
}
get sound() { return this._sound; } // error here
set sound(val) { this._sound = val; }
}
Loading