Skip to content

Commit

Permalink
Updates from design review + fix ancient bug
Browse files Browse the repository at this point in the history
1. Don't error when overriding properties from interfaces.
2. Fix error when overriding methods with other things. This had no
tests so I assume that the code was always dead and never worked.
  • Loading branch information
sandersn committed Sep 13, 2019
1 parent c26785e commit c4f334a
Show file tree
Hide file tree
Showing 19 changed files with 361 additions and 7 deletions.
19 changes: 12 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29513,19 +29513,20 @@ namespace ts {
// either base or derived property is private - not override, skip it
continue;
}
if (isPrototypeProperty(base)) {
// method is overridden with method - correct case
continue;
}
let errorMessage: DiagnosticMessage;
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) {
if (baseDeclarationFlags & ModifierFlags.Abstract
|| getObjectFlags(getTargetType(baseType)) & ObjectFlags.Interface) {
// when the base property is abstract or from an interface, base/derived flags don't need to match
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 (!(baseDeclarationFlags & ModifierFlags.Abstract) && basePropertyFlags === SymbolFlags.Property && derivedPropertyFlags !== SymbolFlags.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 {
Expand All @@ -29534,7 +29535,11 @@ namespace ts {
}
}
else if (isPrototypeProperty(base)) {
if (derived.flags & SymbolFlags.Accessor) {
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
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
}

25 changes: 25 additions & 0 deletions tests/baselines/reference/checkJsFiles_noErrorLocation.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
tests/cases/compiler/a.js(14,10): error TS2424: Class 'A' defines instance member function 'foo', but extended class 'B' defines it as instance member property.


==== tests/cases/compiler/a.js (1 errors) ====
// @ts-check
class A {
constructor() {

}
foo() {
return 4;
}
}

class B extends A {
constructor() {
super();
this.foo = () => 3;
~~~
!!! error TS2424: Class 'A' defines instance member function 'foo', but extended class 'B' defines it as instance member property.
}
}

const i = new B();
i.foo();
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
tests/cases/compiler/inheritanceMemberAccessorOverridingMethod.ts(8,9): error TS2423: Class 'a' defines instance member function 'x', but extended class 'b' defines it as instance member accessor.


==== tests/cases/compiler/inheritanceMemberAccessorOverridingMethod.ts (1 errors) ====
class a {
x() {
return "20";
}
}

class b extends a {
get x() {
~
!!! error TS2423: Class 'a' defines instance member function 'x', but extended class 'b' defines it as instance member accessor.
return () => "20";
}
set x(aValue) {

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
tests/cases/compiler/inheritanceMemberPropertyOverridingMethod.ts(8,5): error TS2424: Class 'a' defines instance member function 'x', but extended class 'b' defines it as instance member property.


==== tests/cases/compiler/inheritanceMemberPropertyOverridingMethod.ts (1 errors) ====
class a {
x() {
return "20";
}
}

class b extends a {
x: () => string;
~
!!! error TS2424: Class 'a' defines instance member function 'x', but extended class 'b' defines it as instance member property.
}
31 changes: 31 additions & 0 deletions tests/baselines/reference/overrideInterfaceProperty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//// [overrideInterfaceProperty.ts]
interface Mup<K, V> {
readonly size: number;
}
interface MupConstructor {
new(): Mup<any, any>;
new<K, V>(entries?: readonly (readonly [K, V])[] | null): Mup<K, V>;
readonly prototype: Mup<any, any>;
}
declare var Mup: MupConstructor;

class Sizz extends Mup {
// ok, because Mup is an interface
get size() { return 0 }
}
class Kasizz extends Mup {
size = -1
}


//// [overrideInterfaceProperty.js]
class Sizz extends Mup {
// ok, because Mup is an interface
get size() { return 0; }
}
class Kasizz extends Mup {
constructor() {
super(...arguments);
this.size = -1;
}
}
49 changes: 49 additions & 0 deletions tests/baselines/reference/overrideInterfaceProperty.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/overrideInterfaceProperty.ts ===
interface Mup<K, V> {
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))
>K : Symbol(K, Decl(overrideInterfaceProperty.ts, 0, 14))
>V : Symbol(V, Decl(overrideInterfaceProperty.ts, 0, 16))

readonly size: number;
>size : Symbol(Mup.size, Decl(overrideInterfaceProperty.ts, 0, 21))
}
interface MupConstructor {
>MupConstructor : Symbol(MupConstructor, Decl(overrideInterfaceProperty.ts, 2, 1))

new(): Mup<any, any>;
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))

new<K, V>(entries?: readonly (readonly [K, V])[] | null): Mup<K, V>;
>K : Symbol(K, Decl(overrideInterfaceProperty.ts, 5, 8))
>V : Symbol(V, Decl(overrideInterfaceProperty.ts, 5, 10))
>entries : Symbol(entries, Decl(overrideInterfaceProperty.ts, 5, 14))
>K : Symbol(K, Decl(overrideInterfaceProperty.ts, 5, 8))
>V : Symbol(V, Decl(overrideInterfaceProperty.ts, 5, 10))
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))
>K : Symbol(K, Decl(overrideInterfaceProperty.ts, 5, 8))
>V : Symbol(V, Decl(overrideInterfaceProperty.ts, 5, 10))

readonly prototype: Mup<any, any>;
>prototype : Symbol(MupConstructor.prototype, Decl(overrideInterfaceProperty.ts, 5, 72))
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))
}
declare var Mup: MupConstructor;
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))
>MupConstructor : Symbol(MupConstructor, Decl(overrideInterfaceProperty.ts, 2, 1))

class Sizz extends Mup {
>Sizz : Symbol(Sizz, Decl(overrideInterfaceProperty.ts, 8, 32))
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))

// ok, because Mup is an interface
get size() { return 0 }
>size : Symbol(Sizz.size, Decl(overrideInterfaceProperty.ts, 10, 24))
}
class Kasizz extends Mup {
>Kasizz : Symbol(Kasizz, Decl(overrideInterfaceProperty.ts, 13, 1))
>Mup : Symbol(Mup, Decl(overrideInterfaceProperty.ts, 0, 0), Decl(overrideInterfaceProperty.ts, 8, 11))

size = -1
>size : Symbol(Kasizz.size, Decl(overrideInterfaceProperty.ts, 14, 26))
}

36 changes: 36 additions & 0 deletions tests/baselines/reference/overrideInterfaceProperty.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/overrideInterfaceProperty.ts ===
interface Mup<K, V> {
readonly size: number;
>size : number
}
interface MupConstructor {
new(): Mup<any, any>;
new<K, V>(entries?: readonly (readonly [K, V])[] | null): Mup<K, V>;
>entries : readonly (readonly [K, V])[]
>null : null

readonly prototype: Mup<any, any>;
>prototype : Mup<any, any>
}
declare var Mup: MupConstructor;
>Mup : MupConstructor

class Sizz extends Mup {
>Sizz : Sizz
>Mup : Mup<any, any>

// ok, because Mup is an interface
get size() { return 0 }
>size : number
>0 : 0
}
class Kasizz extends Mup {
>Kasizz : Kasizz
>Mup : Mup<any, any>

size = -1
>size : number
>-1 : -1
>1 : 1
}

13 changes: 13 additions & 0 deletions tests/baselines/reference/propertyOverridesMethod.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
tests/cases/conformance/classes/propertyMemberDeclarations/propertyOverridesMethod.ts(5,5): error TS2424: Class 'A' defines instance member function 'm', but extended class 'B' defines it as instance member property.


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

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


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

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

m = () => 1
>m : Symbol(B.m, Decl(propertyOverridesMethod.ts, 3, 19))
}

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

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

m = () => 1
>m : () => number
>() => 1 : () => number
>1 : 1
}

16 changes: 16 additions & 0 deletions tests/baselines/reference/propertyOverridingPrototype.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
tests/cases/compiler/propertyOverridingPrototype.ts(7,5): error TS2424: Class 'Base' defines instance member function 'foo', but extended class 'Derived' defines it as instance member property.


==== tests/cases/compiler/propertyOverridingPrototype.ts (1 errors) ====
class Base {
foo() {
}
}

class Derived extends Base {
foo: () => { };
~~~
!!! error TS2424: Class 'Base' defines instance member function 'foo', but extended class 'Derived' defines it as instance member property.
}


Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// @target: esnext
class A {
m() { }
}
class B extends A {
get m() { return () => 1 }
}
Loading

0 comments on commit c4f334a

Please sign in to comment.