Skip to content

Commit

Permalink
Disallow property/accessor overrides
Browse files Browse the repository at this point in the history
Unless the base property or accessor is abstract
  • Loading branch information
sandersn committed Sep 12, 2019
1 parent bc7bde3 commit f76e01f
Show file tree
Hide file tree
Showing 45 changed files with 1,182 additions and 8 deletions.
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)) {
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

0 comments on commit f76e01f

Please sign in to comment.