Skip to content

Allow accessors to override non-class or abstract properties #41994

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
May 31, 2022
Merged

Allow accessors to override non-class or abstract properties #41994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
53373ab
remove too-late fix
sandersn Dec 15, 2020
e638d6b
Allow any property from a mapped type
sandersn Dec 16, 2020
af0233f
Merge branch 'master' into allow-override-transient-properties
sandersn Feb 9, 2021
63fd359
turn off error for any non-class base
sandersn Feb 9, 2021
80112a8
Also handle synthetic properties more laxly
sandersn Feb 10, 2021
b049784
Merge branch 'main' into allow-override-transient-properties
sandersn Feb 4, 2022
f70a448
update baselines
sandersn Feb 4, 2022
bb91a9a
Merge branch 'main' into allow-override-transient-properties
sandersn May 16, 2022
0528e73
Update baselines
sandersn May 16, 2022
fee6a0e
createUnionProperty of accessors creates an accessor
sandersn May 17, 2022
c8a0ee8
Merge branch 'main' into allow-override-transient-properties
sandersn May 24, 2022
0eac002
Fix computation of write type of accessors
sandersn May 24, 2022
341229c
Calculate property-vs-accessor in existing loop
sandersn May 24, 2022
632853b
Undo synthetic accessor change
sandersn May 25, 2022
6addf09
Minimise diff
sandersn May 26, 2022
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
16 changes: 12 additions & 4 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7555,7 +7555,7 @@ namespace ts {
...!length(baseTypes) ? [] : [factory.createHeritageClause(SyntaxKind.ExtendsKeyword, map(baseTypes, b => serializeBaseType(b, staticBaseType, localName)))],
...!length(implementsExpressions) ? [] : [factory.createHeritageClause(SyntaxKind.ImplementsKeyword, implementsExpressions)]
];
const symbolProps = getNonInterhitedProperties(classType, baseTypes, getPropertiesOfType(classType));
const symbolProps = getNonInheritedProperties(classType, baseTypes, getPropertiesOfType(classType));
const publicSymbolProps = filter(symbolProps, s => {
// `valueDeclaration` could be undefined if inherited from
// a union/intersection base type, but inherited properties
Expand Down Expand Up @@ -39949,10 +39949,13 @@ namespace ts {
const derivedPropertyFlags = derived.flags & SymbolFlags.PropertyOrAccessor;
if (basePropertyFlags && derivedPropertyFlags) {
// property/accessor is overridden with property/accessor
if (baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
|| base.valueDeclaration && base.valueDeclaration.parent.kind === SyntaxKind.InterfaceDeclaration
if ((getCheckFlags(base) & CheckFlags.Synthetic
? base.declarations?.some(d => isPropertyAbstractOrInterface(d, baseDeclarationFlags))
: base.declarations?.every(d => isPropertyAbstractOrInterface(d, baseDeclarationFlags)))
|| getCheckFlags(base) & CheckFlags.Mapped
|| derived.valueDeclaration && isBinaryExpression(derived.valueDeclaration)) {
// when the base property is abstract or from an interface, base/derived flags don't need to match
// for intersection properties, this must be true of *any* of the declarations, for others it must be true of *all*
// same when the derived property is from an assignment
continue;
}
Expand Down Expand Up @@ -40010,7 +40013,12 @@ namespace ts {
}
}

function getNonInterhitedProperties(type: InterfaceType, baseTypes: BaseType[], properties: Symbol[]) {
function isPropertyAbstractOrInterface(declaration: Declaration, baseDeclarationFlags: ModifierFlags) {
return baseDeclarationFlags & ModifierFlags.Abstract && (!isPropertyDeclaration(declaration) || !declaration.initializer)
|| isInterfaceDeclaration(declaration.parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we only care that the declaration is an interface? I find it interesting that the following isn't currently an error:

declare class X1 {
    get y(): number;
}

class Y1 extends X1 {
    y = 1; // error (correct)
}


interface X2 {
    get y(): number;
}

declare var X2: {
    new (): X2;
    prototype: X2;
}

class Y2 extends X2 {
    y = 1; // no error (incorrect?)
}

Maybe we can be weaker when the property on the interface is just PropertySignature because of back-compat, but we probably should be erroring if the property on the interface is a GetAccessorDeclaration or SetAccessorDeclaration and you're trying to override it with a field.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, but without the commit with create-unions-of-accessors-as-accessors, it adds a weird wrinkle when synthetic properties are all accessors: specifically, if you modify accessorsOverrideProperty9 so that all bases use accessors (as in your other comment), then the error shows up again: the base symbol is synthetic, but all 3 bases are accessors, so the exemption no longer applies.

Instead, execution falls through to the flags check, but the flags are wrong, so the error message is wrong.

It's probably more correct to change the way synthetic accessors are created, but it's a much further-reaching change, and most of this code is still needed. I'd rather have a more pressing reason than this to change the code that creates synthetic accessors.

}

function getNonInheritedProperties(type: InterfaceType, baseTypes: BaseType[], properties: Symbol[]) {
if (!length(baseTypes)) {
return properties;
}
Expand Down
50 changes: 50 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty8.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
//// [accessorsOverrideProperty8.ts]
type Types = 'boolean' | 'unknown' | 'string';

type Properties<T extends { [key: string]: Types }> = {
readonly [key in keyof T]: T[key] extends 'boolean' ? boolean : T[key] extends 'string' ? string : unknown
}

type AnyCtor<P extends object> = new (...a: any[]) => P

declare function classWithProperties<T extends { [key: string]: Types }, P extends object>(properties: T, klass: AnyCtor<P>): {
new(): P & Properties<T>;
prototype: P & Properties<T>
};

const Base = classWithProperties({
get x() { return 'boolean' as const },
y: 'string',
}, class Base {
});

class MyClass extends Base {
get x() {
return false;
}
get y() {
return 'hi'
}
}

const mine = new MyClass();
const value = mine.x;



//// [accessorsOverrideProperty8.js]
const Base = classWithProperties({
get x() { return 'boolean'; },
y: 'string',
}, class Base {
});
class MyClass extends Base {
get x() {
return false;
}
get y() {
return 'hi';
}
}
const mine = new MyClass();
const value = mine.x;
93 changes: 93 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty8.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty8.ts ===
type Types = 'boolean' | 'unknown' | 'string';
>Types : Symbol(Types, Decl(accessorsOverrideProperty8.ts, 0, 0))

type Properties<T extends { [key: string]: Types }> = {
>Properties : Symbol(Properties, Decl(accessorsOverrideProperty8.ts, 0, 46))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 2, 16))
>key : Symbol(key, Decl(accessorsOverrideProperty8.ts, 2, 29))
>Types : Symbol(Types, Decl(accessorsOverrideProperty8.ts, 0, 0))

readonly [key in keyof T]: T[key] extends 'boolean' ? boolean : T[key] extends 'string' ? string : unknown
>key : Symbol(key, Decl(accessorsOverrideProperty8.ts, 3, 14))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 2, 16))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 2, 16))
>key : Symbol(key, Decl(accessorsOverrideProperty8.ts, 3, 14))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 2, 16))
>key : Symbol(key, Decl(accessorsOverrideProperty8.ts, 3, 14))
}

type AnyCtor<P extends object> = new (...a: any[]) => P
>AnyCtor : Symbol(AnyCtor, Decl(accessorsOverrideProperty8.ts, 4, 1))
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 6, 13))
>a : Symbol(a, Decl(accessorsOverrideProperty8.ts, 6, 38))
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 6, 13))

declare function classWithProperties<T extends { [key: string]: Types }, P extends object>(properties: T, klass: AnyCtor<P>): {
>classWithProperties : Symbol(classWithProperties, Decl(accessorsOverrideProperty8.ts, 6, 55))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 8, 37))
>key : Symbol(key, Decl(accessorsOverrideProperty8.ts, 8, 50))
>Types : Symbol(Types, Decl(accessorsOverrideProperty8.ts, 0, 0))
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 8, 72))
>properties : Symbol(properties, Decl(accessorsOverrideProperty8.ts, 8, 91))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 8, 37))
>klass : Symbol(klass, Decl(accessorsOverrideProperty8.ts, 8, 105))
>AnyCtor : Symbol(AnyCtor, Decl(accessorsOverrideProperty8.ts, 4, 1))
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 8, 72))

new(): P & Properties<T>;
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 8, 72))
>Properties : Symbol(Properties, Decl(accessorsOverrideProperty8.ts, 0, 46))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 8, 37))

prototype: P & Properties<T>
>prototype : Symbol(prototype, Decl(accessorsOverrideProperty8.ts, 9, 29))
>P : Symbol(P, Decl(accessorsOverrideProperty8.ts, 8, 72))
>Properties : Symbol(Properties, Decl(accessorsOverrideProperty8.ts, 0, 46))
>T : Symbol(T, Decl(accessorsOverrideProperty8.ts, 8, 37))

};

const Base = classWithProperties({
>Base : Symbol(Base, Decl(accessorsOverrideProperty8.ts, 13, 5))
>classWithProperties : Symbol(classWithProperties, Decl(accessorsOverrideProperty8.ts, 6, 55))

get x() { return 'boolean' as const },
>x : Symbol(x, Decl(accessorsOverrideProperty8.ts, 13, 34))
>const : Symbol(const)

y: 'string',
>y : Symbol(y, Decl(accessorsOverrideProperty8.ts, 14, 42))

}, class Base {
>Base : Symbol(Base, Decl(accessorsOverrideProperty8.ts, 16, 2))

});

class MyClass extends Base {
>MyClass : Symbol(MyClass, Decl(accessorsOverrideProperty8.ts, 17, 3))
>Base : Symbol(Base, Decl(accessorsOverrideProperty8.ts, 13, 5))

get x() {
>x : Symbol(MyClass.x, Decl(accessorsOverrideProperty8.ts, 19, 28))

return false;
}
get y() {
>y : Symbol(MyClass.y, Decl(accessorsOverrideProperty8.ts, 22, 5))

return 'hi'
}
}

const mine = new MyClass();
>mine : Symbol(mine, Decl(accessorsOverrideProperty8.ts, 28, 5))
>MyClass : Symbol(MyClass, Decl(accessorsOverrideProperty8.ts, 17, 3))

const value = mine.x;
>value : Symbol(value, Decl(accessorsOverrideProperty8.ts, 29, 5))
>mine.x : Symbol(MyClass.x, Decl(accessorsOverrideProperty8.ts, 19, 28))
>mine : Symbol(mine, Decl(accessorsOverrideProperty8.ts, 28, 5))
>x : Symbol(MyClass.x, Decl(accessorsOverrideProperty8.ts, 19, 28))


78 changes: 78 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty8.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty8.ts ===
type Types = 'boolean' | 'unknown' | 'string';
>Types : "string" | "boolean" | "unknown"

type Properties<T extends { [key: string]: Types }> = {
>Properties : Properties<T>
>key : string

readonly [key in keyof T]: T[key] extends 'boolean' ? boolean : T[key] extends 'string' ? string : unknown
}

type AnyCtor<P extends object> = new (...a: any[]) => P
>AnyCtor : AnyCtor<P>
>a : any[]

declare function classWithProperties<T extends { [key: string]: Types }, P extends object>(properties: T, klass: AnyCtor<P>): {
>classWithProperties : <T extends { [key: string]: Types; }, P extends object>(properties: T, klass: AnyCtor<P>) => { new (): P & Properties<T>; prototype: P & Properties<T>;}
>key : string
>properties : T
>klass : AnyCtor<P>

new(): P & Properties<T>;
prototype: P & Properties<T>
>prototype : P & Properties<T>

};

const Base = classWithProperties({
>Base : { new (): Base & Properties<{ readonly x: "boolean"; y: "string"; }>; prototype: Base & Properties<{ readonly x: "boolean"; y: "string"; }>; }
>classWithProperties({ get x() { return 'boolean' as const }, y: 'string',}, class Base {}) : { new (): Base & Properties<{ readonly x: "boolean"; y: "string"; }>; prototype: Base & Properties<{ readonly x: "boolean"; y: "string"; }>; }
>classWithProperties : <T extends { [key: string]: Types; }, P extends object>(properties: T, klass: AnyCtor<P>) => { new (): P & Properties<T>; prototype: P & Properties<T>; }
>{ get x() { return 'boolean' as const }, y: 'string',} : { readonly x: "boolean"; y: "string"; }

get x() { return 'boolean' as const },
>x : "boolean"
>'boolean' as const : "boolean"
>'boolean' : "boolean"

y: 'string',
>y : "string"
>'string' : "string"

}, class Base {
>class Base {} : typeof Base
>Base : typeof Base

});

class MyClass extends Base {
>MyClass : MyClass
>Base : Base & Properties<{ readonly x: "boolean"; y: "string"; }>

get x() {
>x : boolean

return false;
>false : false
}
get y() {
>y : string

return 'hi'
>'hi' : "hi"
}
}

const mine = new MyClass();
>mine : MyClass
>new MyClass() : MyClass
>MyClass : typeof MyClass

const value = mine.x;
>value : boolean
>mine.x : boolean
>mine : MyClass
>x : boolean


79 changes: 79 additions & 0 deletions tests/baselines/reference/accessorsOverrideProperty9.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
//// [accessorsOverrideProperty9.ts]
// #41347, based on microsoft/rushstack

// Mixin utilities
export type Constructor<T = {}> = new (...args: any[]) => T;
export type PropertiesOf<T> = { [K in keyof T]: T[K] };

interface IApiItemConstructor extends Constructor<ApiItem>, PropertiesOf<typeof ApiItem> {}

// Base class
class ApiItem {
public get members(): ReadonlyArray<ApiItem> {
return [];
}
}

// Normal subclass
class ApiEnumMember extends ApiItem {
}

// Mixin base class
interface ApiItemContainerMixin extends ApiItem {
readonly members: ReadonlyArray<ApiItem>;
}

function ApiItemContainerMixin<TBaseClass extends IApiItemConstructor>(
baseClass: TBaseClass
): TBaseClass & (new (...args: any[]) => ApiItemContainerMixin) {
abstract class MixedClass extends baseClass implements ApiItemContainerMixin {
public constructor(...args: any[]) {
super(...args);
}

public get members(): ReadonlyArray<ApiItem> {
return [];
}
}

return MixedClass;
}

// Subclass inheriting from mixin
export class ApiEnum extends ApiItemContainerMixin(ApiItem) {
// This worked prior to TypeScript 4.0:
public get members(): ReadonlyArray<ApiEnumMember> {
return [];
}
}


//// [accessorsOverrideProperty9.js]
// #41347, based on microsoft/rushstack
// Base class
class ApiItem {
get members() {
return [];
}
}
// Normal subclass
class ApiEnumMember extends ApiItem {
}
function ApiItemContainerMixin(baseClass) {
class MixedClass extends baseClass {
constructor(...args) {
super(...args);
}
get members() {
return [];
}
}
return MixedClass;
}
// Subclass inheriting from mixin
export class ApiEnum extends ApiItemContainerMixin(ApiItem) {
// This worked prior to TypeScript 4.0:
get members() {
return [];
}
}
Loading