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

Always error on property override accessor #37894

Merged
merged 10 commits into from
May 26, 2020
5 changes: 2 additions & 3 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33373,8 +33373,7 @@ namespace ts {
const derivedPropertyFlags = derived.flags & SymbolFlags.PropertyOrAccessor;
if (basePropertyFlags && derivedPropertyFlags) {
// property/accessor is overridden with property/accessor
if (!compilerOptions.useDefineForClassFields
|| baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
if (baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
|| base.valueDeclaration && base.valueDeclaration.parent.kind === SyntaxKind.InterfaceDeclaration
|| derived.valueDeclaration && isBinaryExpression(derived.valueDeclaration)) {
// when the base property is abstract or from an interface, base/derived flags don't need to match
Expand All @@ -33390,7 +33389,7 @@ namespace ts {
Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor;
error(getNameOfDeclaration(derived.valueDeclaration) || derived.valueDeclaration, errorMessage, symbolToString(base), typeToString(baseType), typeToString(type));
}
else {
else if (compilerOptions.useDefineForClassFields) {
const uninitialized = find(derived.declarations, d => d.kind === SyntaxKind.PropertyDeclaration && !(d as PropertyDeclaration).initializer);
if (uninitialized
&& !(derived.flags & SymbolFlags.Transient)
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -5641,7 +5641,11 @@
"category": "Message",
"code": 95116
},

"Generate 'get' and 'set' accessors for all overriding properties": {
"category": "Message",
"code": 95117
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
"code": 18004
Expand Down
13 changes: 0 additions & 13 deletions src/services/codefixes/fixAddMissingMember.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,19 +83,6 @@ namespace ts.codefix {
},
});

function getAllSupers(decl: ClassOrInterface | undefined, checker: TypeChecker): readonly ClassOrInterface[] {
const res: ClassLikeDeclaration[] = [];
while (decl) {
const superElement = getClassExtendsHeritageElement(decl);
const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression);
const superDecl = superSymbol && find(superSymbol.declarations, isClassLike);
if (superDecl) { res.push(superDecl); }
decl = superDecl;
}
return res;
}

type ClassOrInterface = ClassLikeDeclaration | InterfaceDeclaration;
const enum InfoKind { Enum, ClassOrInterface }
interface EnumInfo {
readonly kind: InfoKind.Enum;
Expand Down
56 changes: 56 additions & 0 deletions src/services/codefixes/fixPropertyOverrideAccessor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/* @internal */
namespace ts.codefix {
const errorCodes = [
Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code,
Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code,
];
const fixId = "fixPropertyOverrideAccessor";
registerCodeFix({
errorCodes,
getCodeActions(context) {
const edits = doChange(context.sourceFile, context.span.start, context.span.length, context.errorCode, context);
if (edits) {
return [createCodeFixAction(fixId, edits, Diagnostics.Generate_get_and_set_accessors, fixId, Diagnostics.Generate_get_and_set_accessors_for_all_overriding_properties)];
}
},
fixIds: [fixId],

getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
const edits = doChange(diag.file, diag.start, diag.length, diag.code, context);
if (edits) {
for (const edit of edits) {
changes.pushRaw(context.sourceFile, edit);
}
}
}),
});

function doChange(file: SourceFile, start: number, length: number, code: number, context: CodeFixContext | CodeFixAllContext) {
let startPosition: number;
let endPosition: number;
if (code === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) {
startPosition = start;
endPosition = start + length;
}
else if (code === Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code) {
const checker = context.program.getTypeChecker();
const node = getTokenAtPosition(file, start).parent;
Debug.assert(isAccessor(node), "error span of fixPropertyOverrideAccessor should only be on an accessor");
const containingClass = node.parent;
Debug.assert(isClassLike(containingClass), "erroneous accessors should only be inside classes");
const base = singleOrUndefined(getAllSupers(containingClass, checker));
if (!base) return [];

const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name));
const baseProp = checker.getPropertyOfType(checker.getTypeAtLocation(base), name);
if (!baseProp || !baseProp.valueDeclaration) return [];

startPosition = baseProp.valueDeclaration.pos;
endPosition = baseProp.valueDeclaration.end;
Comment on lines +48 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Uhm, how do you know this valueDeclaration is in file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't! I need to update file to be the file with the superclass, not the file with the error.

Additionally, getSupers doesn't follow aliases, so doesn't work for imported classes either.

}
else {
Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + code);
}
return generateAccessorFromProperty(file, startPosition, endPosition, context, Diagnostics.Generate_get_and_set_accessors.message);
}
}
222 changes: 222 additions & 0 deletions src/services/codefixes/generateAccessors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
/* @internal */
namespace ts.codefix {
type AcceptedDeclaration = ParameterPropertyDeclaration | PropertyDeclaration | PropertyAssignment;
type AcceptedNameType = Identifier | StringLiteral;
type ContainerDeclaration = ClassLikeDeclaration | ObjectLiteralExpression;

interface Info {
readonly container: ContainerDeclaration;
readonly isStatic: boolean;
readonly isReadonly: boolean;
readonly type: TypeNode | undefined;
readonly declaration: AcceptedDeclaration;
readonly fieldName: AcceptedNameType;
readonly accessorName: AcceptedNameType;
readonly originalName: string;
readonly renameAccessor: boolean;
}

export function generateAccessorFromProperty(file: SourceFile, start: number, end: number, context: textChanges.TextChangesContext, _actionName: string): FileTextChanges[] | undefined {
const fieldInfo = getAccessorConvertiblePropertyAtPosition(file, start, end);
if (!fieldInfo) return undefined;

const isJS = isSourceFileJS(file);
const changeTracker = textChanges.ChangeTracker.fromContext(context);
const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration } = fieldInfo;

suppressLeadingAndTrailingTrivia(fieldName);
suppressLeadingAndTrailingTrivia(accessorName);
suppressLeadingAndTrailingTrivia(declaration);
suppressLeadingAndTrailingTrivia(container);

const isInClassLike = isClassLike(container);
// avoid Readonly modifier because it will convert to get accessor
const modifierFlags = getModifierFlags(declaration) & ~ModifierFlags.Readonly;
const accessorModifiers = isInClassLike
? !modifierFlags || modifierFlags & ModifierFlags.Private
? getModifiers(isJS, isStatic, SyntaxKind.PublicKeyword)
: createNodeArray(createModifiersFromModifierFlags(modifierFlags))
: undefined;
const fieldModifiers = isInClassLike ? getModifiers(isJS, isStatic, SyntaxKind.PrivateKeyword) : undefined;

updateFieldDeclaration(changeTracker, file, declaration, fieldName, fieldModifiers);

const getAccessor = generateGetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
suppressLeadingAndTrailingTrivia(getAccessor);
insertAccessor(changeTracker, file, getAccessor, declaration, container);

if (isReadonly) {
// readonly modifier only existed in classLikeDeclaration
const constructor = getFirstConstructorWithBody(<ClassLikeDeclaration>container);
if (constructor) {
updateReadonlyPropertyInitializerStatementConstructor(changeTracker, file, constructor, fieldName.text, originalName);
}
}
else {
const setAccessor = generateSetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
suppressLeadingAndTrailingTrivia(setAccessor);
insertAccessor(changeTracker, file, setAccessor, declaration, container);
}

return changeTracker.getChanges();
}

function isConvertibleName(name: DeclarationName): name is AcceptedNameType {
return isIdentifier(name) || isStringLiteral(name);
}

function isAcceptedDeclaration(node: Node): node is AcceptedDeclaration {
return isParameterPropertyDeclaration(node, node.parent) || isPropertyDeclaration(node) || isPropertyAssignment(node);
}

function createPropertyName(name: string, originalName: AcceptedNameType) {
return isIdentifier(originalName) ? createIdentifier(name) : createLiteral(name);
}

function createAccessorAccessExpression(fieldName: AcceptedNameType, isStatic: boolean, container: ContainerDeclaration) {
const leftHead = isStatic ? (<ClassLikeDeclaration>container).name! : createThis(); // TODO: GH#18217
return isIdentifier(fieldName) ? createPropertyAccess(leftHead, fieldName) : createElementAccess(leftHead, createLiteral(fieldName));
}

function getModifiers(isJS: boolean, isStatic: boolean, accessModifier: SyntaxKind.PublicKeyword | SyntaxKind.PrivateKeyword): NodeArray<Modifier> | undefined {
const modifiers = append<Modifier>(
!isJS ? [createToken(accessModifier) as Token<SyntaxKind.PublicKeyword> | Token<SyntaxKind.PrivateKeyword>] : undefined,
isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined
);
return modifiers && createNodeArray(modifiers);
}

export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, start: number, end: number): Info | undefined {
const node = getTokenAtPosition(file, start);
const declaration = findAncestor(node.parent, isAcceptedDeclaration);
// make sure declaration have AccessibilityModifier or Static Modifier or Readonly Modifier
const meaning = ModifierFlags.AccessibilityModifier | ModifierFlags.Static | ModifierFlags.Readonly;
if (!declaration || !nodeOverlapsWithStartEnd(declaration.name, file, start, end)
|| !isConvertibleName(declaration.name) || (getModifierFlags(declaration) | meaning) !== meaning) return undefined;

const name = declaration.name.text;
const startWithUnderscore = startsWithUnderscore(name);
const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file), declaration.name);
const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file) : name, declaration.name);
return {
isStatic: hasStaticModifier(declaration),
isReadonly: hasReadonlyModifier(declaration),
type: getTypeAnnotationNode(declaration),
container: declaration.kind === SyntaxKind.Parameter ? declaration.parent.parent : declaration.parent,
originalName: (<AcceptedNameType>declaration.name).text,
declaration,
fieldName,
accessorName,
renameAccessor: startWithUnderscore
};
}

function generateGetAccessor(fieldName: AcceptedNameType, accessorName: AcceptedNameType, type: TypeNode | undefined, modifiers: ModifiersArray | undefined, isStatic: boolean, container: ContainerDeclaration) {
return createGetAccessor(
/*decorators*/ undefined,
modifiers,
accessorName,
/*parameters*/ undefined!, // TODO: GH#18217
type,
createBlock([
createReturn(
createAccessorAccessExpression(fieldName, isStatic, container)
)
], /*multiLine*/ true)
);
}

function generateSetAccessor(fieldName: AcceptedNameType, accessorName: AcceptedNameType, type: TypeNode | undefined, modifiers: ModifiersArray | undefined, isStatic: boolean, container: ContainerDeclaration) {
return createSetAccessor(
/*decorators*/ undefined,
modifiers,
accessorName,
[createParameter(
/*decorators*/ undefined,
/*modifiers*/ undefined,
/*dotDotDotToken*/ undefined,
createIdentifier("value"),
/*questionToken*/ undefined,
type
)],
createBlock([
createStatement(
createAssignment(
createAccessorAccessExpression(fieldName, isStatic, container),
createIdentifier("value")
)
)
], /*multiLine*/ true)
);
}

function updatePropertyDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: PropertyDeclaration, fieldName: AcceptedNameType, modifiers: ModifiersArray | undefined) {
const property = updateProperty(
declaration,
declaration.decorators,
modifiers,
fieldName,
declaration.questionToken || declaration.exclamationToken,
declaration.type,
declaration.initializer
);
changeTracker.replaceNode(file, declaration, property);
}

function updatePropertyAssignmentDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: PropertyAssignment, fieldName: AcceptedNameType) {
const assignment = updatePropertyAssignment(declaration, fieldName, declaration.initializer);
changeTracker.replacePropertyAssignment(file, declaration, assignment);
}

function updateFieldDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: AcceptedDeclaration, fieldName: AcceptedNameType, modifiers: ModifiersArray | undefined) {
if (isPropertyDeclaration(declaration)) {
updatePropertyDeclaration(changeTracker, file, declaration, fieldName, modifiers);
}
else if (isPropertyAssignment(declaration)) {
updatePropertyAssignmentDeclaration(changeTracker, file, declaration, fieldName);
}
else {
changeTracker.replaceNode(file, declaration,
updateParameter(declaration, declaration.decorators, modifiers, declaration.dotDotDotToken, cast(fieldName, isIdentifier), declaration.questionToken, declaration.type, declaration.initializer));
}
}

function insertAccessor(changeTracker: textChanges.ChangeTracker, file: SourceFile, accessor: AccessorDeclaration, declaration: AcceptedDeclaration, container: ContainerDeclaration) {
isParameterPropertyDeclaration(declaration, declaration.parent) ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor) :
isPropertyAssignment(declaration) ? changeTracker.insertNodeAfterComma(file, declaration, accessor) :
changeTracker.insertNodeAfter(file, declaration, accessor);
}

function updateReadonlyPropertyInitializerStatementConstructor(changeTracker: textChanges.ChangeTracker, file: SourceFile, constructor: ConstructorDeclaration, fieldName: string, originalName: string) {
if (!constructor.body) return;
constructor.body.forEachChild(function recur(node) {
if (isElementAccessExpression(node) &&
node.expression.kind === SyntaxKind.ThisKeyword &&
isStringLiteral(node.argumentExpression) &&
node.argumentExpression.text === originalName &&
isWriteAccess(node)) {
changeTracker.replaceNode(file, node.argumentExpression, createStringLiteral(fieldName));
}
if (isPropertyAccessExpression(node) && node.expression.kind === SyntaxKind.ThisKeyword && node.name.text === originalName && isWriteAccess(node)) {
changeTracker.replaceNode(file, node.name, createIdentifier(fieldName));
}
if (!isFunctionLike(node) && !isClassLike(node)) {
node.forEachChild(recur);
}
});
}

export function getAllSupers(decl: ClassOrInterface | undefined, checker: TypeChecker): readonly ClassOrInterface[] {
const res: ClassLikeDeclaration[] = [];
while (decl) {
const superElement = getClassExtendsHeritageElement(decl);
const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression);
const superDecl = superSymbol && find(superSymbol.declarations, isClassLike);
if (superDecl) { res.push(superDecl); }
decl = superDecl;
}
return res;
}

export type ClassOrInterface = ClassLikeDeclaration | InterfaceDeclaration;
}
Loading