-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
3ffe253
Always error when property overrides accessor or vice versa
sandersn 71b7695
add fourslash tests for codefix
sandersn e287c83
Codefix invokes generate get-set accessor refactor
sandersn 3030cd8
refactoring done except for deduping
sandersn 6eacc9c
move into new, centrally (?) located file
sandersn 3428e27
Reorder tsconfig and move one more function
sandersn 4d541d2
Minor cleanup
sandersn 802e87b
Merge branch 'master' into always-error-on-property-override-accessor
sandersn 82d9576
Merge branch 'master' into always-error-on-property-override-accessor
sandersn e466bb9
Fix multi-file usage
sandersn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} | ||
else { | ||
Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + code); | ||
} | ||
return generateAccessorFromProperty(file, startPosition, endPosition, context, Diagnostics.Generate_get_and_set_accessors.message); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 infile
?There was a problem hiding this comment.
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.