Skip to content

Commit bfa1744

Browse files
authored
Merge pull request #37894 from microsoft/always-error-on-property-override-accessor
Always error on property override accessor
2 parents 6ac6a04 + e466bb9 commit bfa1744

15 files changed

+504
-268
lines changed

src/compiler/checker.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33824,8 +33824,7 @@ namespace ts {
3382433824
const derivedPropertyFlags = derived.flags & SymbolFlags.PropertyOrAccessor;
3382533825
if (basePropertyFlags && derivedPropertyFlags) {
3382633826
// property/accessor is overridden with property/accessor
33827-
if (!compilerOptions.useDefineForClassFields
33828-
|| baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
33827+
if (baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
3382933828
|| base.valueDeclaration && base.valueDeclaration.parent.kind === SyntaxKind.InterfaceDeclaration
3383033829
|| derived.valueDeclaration && isBinaryExpression(derived.valueDeclaration)) {
3383133830
// when the base property is abstract or from an interface, base/derived flags don't need to match
@@ -33841,7 +33840,7 @@ namespace ts {
3384133840
Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor;
3384233841
error(getNameOfDeclaration(derived.valueDeclaration) || derived.valueDeclaration, errorMessage, symbolToString(base), typeToString(baseType), typeToString(type));
3384333842
}
33844-
else {
33843+
else if (compilerOptions.useDefineForClassFields) {
3384533844
const uninitialized = find(derived.declarations, d => d.kind === SyntaxKind.PropertyDeclaration && !(d as PropertyDeclaration).initializer);
3384633845
if (uninitialized
3384733846
&& !(derived.flags & SymbolFlags.Transient)

src/compiler/diagnosticMessages.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5705,6 +5705,10 @@
57055705
"category": "Message",
57065706
"code": 95118
57075707
},
5708+
"Generate 'get' and 'set' accessors for all overriding properties": {
5709+
"category": "Message",
5710+
"code": 95119
5711+
},
57085712

57095713
"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
57105714
"category": "Error",

src/services/codefixes/fixAddMissingMember.ts

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -78,19 +78,6 @@ namespace ts.codefix {
7878
},
7979
});
8080

81-
function getAllSupers(decl: ClassOrInterface | undefined, checker: TypeChecker): readonly ClassOrInterface[] {
82-
const res: ClassLikeDeclaration[] = [];
83-
while (decl) {
84-
const superElement = getClassExtendsHeritageElement(decl);
85-
const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression);
86-
const superDecl = superSymbol && find(superSymbol.declarations, isClassLike);
87-
if (superDecl) { res.push(superDecl); }
88-
decl = superDecl;
89-
}
90-
return res;
91-
}
92-
93-
type ClassOrInterface = ClassLikeDeclaration | InterfaceDeclaration;
9481
const enum InfoKind { Enum, ClassOrInterface }
9582
interface EnumInfo {
9683
readonly kind: InfoKind.Enum;
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/* @internal */
2+
namespace ts.codefix {
3+
const errorCodes = [
4+
Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code,
5+
Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code,
6+
];
7+
const fixId = "fixPropertyOverrideAccessor";
8+
registerCodeFix({
9+
errorCodes,
10+
getCodeActions(context) {
11+
const edits = doChange(context.sourceFile, context.span.start, context.span.length, context.errorCode, context);
12+
if (edits) {
13+
return [createCodeFixAction(fixId, edits, Diagnostics.Generate_get_and_set_accessors, fixId, Diagnostics.Generate_get_and_set_accessors_for_all_overriding_properties)];
14+
}
15+
},
16+
fixIds: [fixId],
17+
18+
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
19+
const edits = doChange(diag.file, diag.start, diag.length, diag.code, context);
20+
if (edits) {
21+
for (const edit of edits) {
22+
changes.pushRaw(context.sourceFile, edit);
23+
}
24+
}
25+
}),
26+
});
27+
28+
function doChange(file: SourceFile, start: number, length: number, code: number, context: CodeFixContext | CodeFixAllContext) {
29+
let startPosition: number;
30+
let endPosition: number;
31+
if (code === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) {
32+
startPosition = start;
33+
endPosition = start + length;
34+
}
35+
else if (code === Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code) {
36+
const checker = context.program.getTypeChecker();
37+
const node = getTokenAtPosition(file, start).parent;
38+
Debug.assert(isAccessor(node), "error span of fixPropertyOverrideAccessor should only be on an accessor");
39+
const containingClass = node.parent;
40+
Debug.assert(isClassLike(containingClass), "erroneous accessors should only be inside classes");
41+
const base = singleOrUndefined(getAllSupers(containingClass, checker));
42+
if (!base) return [];
43+
44+
const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name));
45+
const baseProp = checker.getPropertyOfType(checker.getTypeAtLocation(base), name);
46+
if (!baseProp || !baseProp.valueDeclaration) return [];
47+
48+
startPosition = baseProp.valueDeclaration.pos;
49+
endPosition = baseProp.valueDeclaration.end;
50+
file = getSourceFileOfNode(baseProp.valueDeclaration);
51+
}
52+
else {
53+
Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + code);
54+
}
55+
return generateAccessorFromProperty(file, startPosition, endPosition, context, Diagnostics.Generate_get_and_set_accessors.message);
56+
}
57+
}
Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
/* @internal */
2+
namespace ts.codefix {
3+
type AcceptedDeclaration = ParameterPropertyDeclaration | PropertyDeclaration | PropertyAssignment;
4+
type AcceptedNameType = Identifier | StringLiteral;
5+
type ContainerDeclaration = ClassLikeDeclaration | ObjectLiteralExpression;
6+
7+
interface Info {
8+
readonly container: ContainerDeclaration;
9+
readonly isStatic: boolean;
10+
readonly isReadonly: boolean;
11+
readonly type: TypeNode | undefined;
12+
readonly declaration: AcceptedDeclaration;
13+
readonly fieldName: AcceptedNameType;
14+
readonly accessorName: AcceptedNameType;
15+
readonly originalName: string;
16+
readonly renameAccessor: boolean;
17+
}
18+
19+
export function generateAccessorFromProperty(file: SourceFile, start: number, end: number, context: textChanges.TextChangesContext, _actionName: string): FileTextChanges[] | undefined {
20+
const fieldInfo = getAccessorConvertiblePropertyAtPosition(file, start, end);
21+
if (!fieldInfo) return undefined;
22+
23+
const changeTracker = textChanges.ChangeTracker.fromContext(context);
24+
const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration } = fieldInfo;
25+
26+
suppressLeadingAndTrailingTrivia(fieldName);
27+
suppressLeadingAndTrailingTrivia(accessorName);
28+
suppressLeadingAndTrailingTrivia(declaration);
29+
suppressLeadingAndTrailingTrivia(container);
30+
31+
let accessorModifiers: ModifiersArray | undefined;
32+
let fieldModifiers: ModifiersArray | undefined;
33+
if (isClassLike(container)) {
34+
const modifierFlags = getEffectiveModifierFlags(declaration);
35+
if (isSourceFileJS(file)) {
36+
const modifiers = createModifiers(modifierFlags);
37+
accessorModifiers = modifiers;
38+
fieldModifiers = modifiers;
39+
}
40+
else {
41+
accessorModifiers = createModifiers(prepareModifierFlagsForAccessor(modifierFlags));
42+
fieldModifiers = createModifiers(prepareModifierFlagsForField(modifierFlags));
43+
}
44+
}
45+
46+
updateFieldDeclaration(changeTracker, file, declaration, fieldName, fieldModifiers);
47+
48+
const getAccessor = generateGetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
49+
suppressLeadingAndTrailingTrivia(getAccessor);
50+
insertAccessor(changeTracker, file, getAccessor, declaration, container);
51+
52+
if (isReadonly) {
53+
// readonly modifier only existed in classLikeDeclaration
54+
const constructor = getFirstConstructorWithBody(<ClassLikeDeclaration>container);
55+
if (constructor) {
56+
updateReadonlyPropertyInitializerStatementConstructor(changeTracker, file, constructor, fieldName.text, originalName);
57+
}
58+
}
59+
else {
60+
const setAccessor = generateSetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
61+
suppressLeadingAndTrailingTrivia(setAccessor);
62+
insertAccessor(changeTracker, file, setAccessor, declaration, container);
63+
}
64+
65+
return changeTracker.getChanges();
66+
}
67+
68+
function isConvertibleName(name: DeclarationName): name is AcceptedNameType {
69+
return isIdentifier(name) || isStringLiteral(name);
70+
}
71+
72+
function isAcceptedDeclaration(node: Node): node is AcceptedDeclaration {
73+
return isParameterPropertyDeclaration(node, node.parent) || isPropertyDeclaration(node) || isPropertyAssignment(node);
74+
}
75+
76+
function createPropertyName(name: string, originalName: AcceptedNameType) {
77+
return isIdentifier(originalName) ? createIdentifier(name) : createLiteral(name);
78+
}
79+
80+
function createAccessorAccessExpression(fieldName: AcceptedNameType, isStatic: boolean, container: ContainerDeclaration) {
81+
const leftHead = isStatic ? (<ClassLikeDeclaration>container).name! : createThis(); // TODO: GH#18217
82+
return isIdentifier(fieldName) ? createPropertyAccess(leftHead, fieldName) : createElementAccess(leftHead, createLiteral(fieldName));
83+
}
84+
85+
function createModifiers(modifierFlags: ModifierFlags): ModifiersArray | undefined {
86+
return modifierFlags ? createNodeArray(createModifiersFromModifierFlags(modifierFlags)) : undefined;
87+
}
88+
89+
function prepareModifierFlagsForAccessor(modifierFlags: ModifierFlags): ModifierFlags {
90+
modifierFlags &= ~ModifierFlags.Readonly; // avoid Readonly modifier because it will convert to get accessor
91+
modifierFlags &= ~ModifierFlags.Private;
92+
93+
if (!(modifierFlags & ModifierFlags.Protected)) {
94+
modifierFlags |= ModifierFlags.Public;
95+
}
96+
97+
return modifierFlags;
98+
}
99+
100+
function prepareModifierFlagsForField(modifierFlags: ModifierFlags): ModifierFlags {
101+
modifierFlags &= ~ModifierFlags.Public;
102+
modifierFlags &= ~ModifierFlags.Protected;
103+
modifierFlags |= ModifierFlags.Private;
104+
return modifierFlags;
105+
}
106+
107+
export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, start: number, end: number): Info | undefined {
108+
const node = getTokenAtPosition(file, start);
109+
const declaration = findAncestor(node.parent, isAcceptedDeclaration);
110+
// make sure declaration have AccessibilityModifier or Static Modifier or Readonly Modifier
111+
const meaning = ModifierFlags.AccessibilityModifier | ModifierFlags.Static | ModifierFlags.Readonly;
112+
if (!declaration || !nodeOverlapsWithStartEnd(declaration.name, file, start, end)
113+
|| !isConvertibleName(declaration.name) || (getEffectiveModifierFlags(declaration) | meaning) !== meaning) return undefined;
114+
115+
const name = declaration.name.text;
116+
const startWithUnderscore = startsWithUnderscore(name);
117+
const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file), declaration.name);
118+
const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file) : name, declaration.name);
119+
return {
120+
isStatic: hasStaticModifier(declaration),
121+
isReadonly: hasEffectiveReadonlyModifier(declaration),
122+
type: getTypeAnnotationNode(declaration),
123+
container: declaration.kind === SyntaxKind.Parameter ? declaration.parent.parent : declaration.parent,
124+
originalName: (<AcceptedNameType>declaration.name).text,
125+
declaration,
126+
fieldName,
127+
accessorName,
128+
renameAccessor: startWithUnderscore
129+
};
130+
}
131+
132+
function generateGetAccessor(fieldName: AcceptedNameType, accessorName: AcceptedNameType, type: TypeNode | undefined, modifiers: ModifiersArray | undefined, isStatic: boolean, container: ContainerDeclaration) {
133+
return createGetAccessor(
134+
/*decorators*/ undefined,
135+
modifiers,
136+
accessorName,
137+
/*parameters*/ undefined!, // TODO: GH#18217
138+
type,
139+
createBlock([
140+
createReturn(
141+
createAccessorAccessExpression(fieldName, isStatic, container)
142+
)
143+
], /*multiLine*/ true)
144+
);
145+
}
146+
147+
function generateSetAccessor(fieldName: AcceptedNameType, accessorName: AcceptedNameType, type: TypeNode | undefined, modifiers: ModifiersArray | undefined, isStatic: boolean, container: ContainerDeclaration) {
148+
return createSetAccessor(
149+
/*decorators*/ undefined,
150+
modifiers,
151+
accessorName,
152+
[createParameter(
153+
/*decorators*/ undefined,
154+
/*modifiers*/ undefined,
155+
/*dotDotDotToken*/ undefined,
156+
createIdentifier("value"),
157+
/*questionToken*/ undefined,
158+
type
159+
)],
160+
createBlock([
161+
createStatement(
162+
createAssignment(
163+
createAccessorAccessExpression(fieldName, isStatic, container),
164+
createIdentifier("value")
165+
)
166+
)
167+
], /*multiLine*/ true)
168+
);
169+
}
170+
171+
function updatePropertyDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: PropertyDeclaration, fieldName: AcceptedNameType, modifiers: ModifiersArray | undefined) {
172+
const property = updateProperty(
173+
declaration,
174+
declaration.decorators,
175+
modifiers,
176+
fieldName,
177+
declaration.questionToken || declaration.exclamationToken,
178+
declaration.type,
179+
declaration.initializer
180+
);
181+
changeTracker.replaceNode(file, declaration, property);
182+
}
183+
184+
function updatePropertyAssignmentDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: PropertyAssignment, fieldName: AcceptedNameType) {
185+
const assignment = updatePropertyAssignment(declaration, fieldName, declaration.initializer);
186+
changeTracker.replacePropertyAssignment(file, declaration, assignment);
187+
}
188+
189+
function updateFieldDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: AcceptedDeclaration, fieldName: AcceptedNameType, modifiers: ModifiersArray | undefined) {
190+
if (isPropertyDeclaration(declaration)) {
191+
updatePropertyDeclaration(changeTracker, file, declaration, fieldName, modifiers);
192+
}
193+
else if (isPropertyAssignment(declaration)) {
194+
updatePropertyAssignmentDeclaration(changeTracker, file, declaration, fieldName);
195+
}
196+
else {
197+
changeTracker.replaceNode(file, declaration,
198+
updateParameter(declaration, declaration.decorators, modifiers, declaration.dotDotDotToken, cast(fieldName, isIdentifier), declaration.questionToken, declaration.type, declaration.initializer));
199+
}
200+
}
201+
202+
function insertAccessor(changeTracker: textChanges.ChangeTracker, file: SourceFile, accessor: AccessorDeclaration, declaration: AcceptedDeclaration, container: ContainerDeclaration) {
203+
isParameterPropertyDeclaration(declaration, declaration.parent) ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor) :
204+
isPropertyAssignment(declaration) ? changeTracker.insertNodeAfterComma(file, declaration, accessor) :
205+
changeTracker.insertNodeAfter(file, declaration, accessor);
206+
}
207+
208+
function updateReadonlyPropertyInitializerStatementConstructor(changeTracker: textChanges.ChangeTracker, file: SourceFile, constructor: ConstructorDeclaration, fieldName: string, originalName: string) {
209+
if (!constructor.body) return;
210+
constructor.body.forEachChild(function recur(node) {
211+
if (isElementAccessExpression(node) &&
212+
node.expression.kind === SyntaxKind.ThisKeyword &&
213+
isStringLiteral(node.argumentExpression) &&
214+
node.argumentExpression.text === originalName &&
215+
isWriteAccess(node)) {
216+
changeTracker.replaceNode(file, node.argumentExpression, createStringLiteral(fieldName));
217+
}
218+
if (isPropertyAccessExpression(node) && node.expression.kind === SyntaxKind.ThisKeyword && node.name.text === originalName && isWriteAccess(node)) {
219+
changeTracker.replaceNode(file, node.name, createIdentifier(fieldName));
220+
}
221+
if (!isFunctionLike(node) && !isClassLike(node)) {
222+
node.forEachChild(recur);
223+
}
224+
});
225+
}
226+
227+
export function getAllSupers(decl: ClassOrInterface | undefined, checker: TypeChecker): readonly ClassOrInterface[] {
228+
const res: ClassLikeDeclaration[] = [];
229+
while (decl) {
230+
const superElement = getClassExtendsHeritageElement(decl);
231+
const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression);
232+
if (!superSymbol) break;
233+
const symbol = superSymbol.flags & SymbolFlags.Alias ? checker.getAliasedSymbol(superSymbol) : superSymbol;
234+
const superDecl = find(symbol.declarations, isClassLike);
235+
if (!superDecl) break;
236+
res.push(superDecl);
237+
decl = superDecl;
238+
}
239+
return res;
240+
}
241+
242+
export type ClassOrInterface = ClassLikeDeclaration | InterfaceDeclaration;
243+
}

0 commit comments

Comments
 (0)