From 3ffe25316682ab1c95a1d9fef56d9dbcb5a2453c Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 9 Apr 2020 08:27:08 -0700 Subject: [PATCH 1/8] Always error when property overrides accessor or vice versa Previously this was only an error when useDefineForClassFields: true, but now it's an error for all code. This is a rare mistake to make, and usually only occurs in code written before `readonly` was available. Codefix to come in subsequent commits. --- src/compiler/checker.ts | 5 ++-- .../accessorsOverrideProperty6.errors.txt | 24 +++++++++++++++++++ ...emberAccessorOverridingProperty.errors.txt | 5 +++- ...emberPropertyOverridingAccessor.errors.txt | 5 +++- ...it-when-useDefineForClassFields-changes.js | 8 ++++++- 5 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 tests/baselines/reference/accessorsOverrideProperty6.errors.txt diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 98c059ca3060e..18c03b8b3040a 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -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 @@ -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) diff --git a/tests/baselines/reference/accessorsOverrideProperty6.errors.txt b/tests/baselines/reference/accessorsOverrideProperty6.errors.txt new file mode 100644 index 0000000000000..ad3c13e6849e9 --- /dev/null +++ b/tests/baselines/reference/accessorsOverrideProperty6.errors.txt @@ -0,0 +1,24 @@ +tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty6.ts(5,9): error TS2611: 'p' is defined as a property in class 'A', but is overridden here in 'B' as an accessor. +tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty6.ts(12,9): error TS2611: 'p' is defined as a property in class 'C', but is overridden here in 'D' as an accessor. + + +==== tests/cases/conformance/classes/propertyMemberDeclarations/accessorsOverrideProperty6.ts (2 errors) ==== + class A { + p = 'yep' + } + class B extends A { + get p() { return 'oh no' } // error + ~ +!!! error TS2611: 'p' is defined as a property in class 'A', but is overridden here in 'B' as an accessor. + } + class C { + p = 101 + } + class D extends C { + _secret = 11 + get p() { return this._secret } // error + ~ +!!! error TS2611: 'p' is defined as a property in class 'C', but is overridden here in 'D' as an accessor. + set p(value) { this._secret = value } // error + } + \ No newline at end of file diff --git a/tests/baselines/reference/inheritanceMemberAccessorOverridingProperty.errors.txt b/tests/baselines/reference/inheritanceMemberAccessorOverridingProperty.errors.txt index 98b9d349d64fe..0872ab47f2ec6 100644 --- a/tests/baselines/reference/inheritanceMemberAccessorOverridingProperty.errors.txt +++ b/tests/baselines/reference/inheritanceMemberAccessorOverridingProperty.errors.txt @@ -1,8 +1,9 @@ tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts(6,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. +tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts(6,9): error TS2611: 'x' is defined as a property in class 'a', but is overridden here in 'b' as an accessor. tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts(9,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. -==== tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts (2 errors) ==== +==== tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts (3 errors) ==== class a { x: string; } @@ -11,6 +12,8 @@ tests/cases/compiler/inheritanceMemberAccessorOverridingProperty.ts(9,9): error get x() { ~ !!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. + ~ +!!! error TS2611: 'x' is defined as a property in class 'a', but is overridden here in 'b' as an accessor. return "20"; } set x(aValue: string) { diff --git a/tests/baselines/reference/inheritanceMemberPropertyOverridingAccessor.errors.txt b/tests/baselines/reference/inheritanceMemberPropertyOverridingAccessor.errors.txt index ec8ec05a8aeb5..61e7e78942d35 100644 --- a/tests/baselines/reference/inheritanceMemberPropertyOverridingAccessor.errors.txt +++ b/tests/baselines/reference/inheritanceMemberPropertyOverridingAccessor.errors.txt @@ -1,8 +1,9 @@ tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts(3,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts(6,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher. +tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts(12,5): error TS2610: 'x' is defined as an accessor in class 'a', but is overridden here in 'b' as an instance property. -==== tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts (2 errors) ==== +==== tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts (3 errors) ==== class a { private __x: () => string; get x() { @@ -19,4 +20,6 @@ tests/cases/compiler/inheritanceMemberPropertyOverridingAccessor.ts(6,9): error class b extends a { x: () => string; + ~ +!!! error TS2610: 'x' is defined as an accessor in class 'a', but is overridden here in 'b' as an instance property. } \ No newline at end of file diff --git a/tests/baselines/reference/tscWatch/programUpdates/updates-diagnostics-and-emit-when-useDefineForClassFields-changes.js b/tests/baselines/reference/tscWatch/programUpdates/updates-diagnostics-and-emit-when-useDefineForClassFields-changes.js index 721df9bc34376..0e2e3b2a407c6 100644 --- a/tests/baselines/reference/tscWatch/programUpdates/updates-diagnostics-and-emit-when-useDefineForClassFields-changes.js +++ b/tests/baselines/reference/tscWatch/programUpdates/updates-diagnostics-and-emit-when-useDefineForClassFields-changes.js @@ -37,7 +37,13 @@ Output:: [12:00:13 AM] Starting compilation in watch mode... -[12:00:16 AM] Found 0 errors. Watching for file changes. +a.ts:2:21 - error TS2610: 'prop' is defined as an accessor in class 'C', but is overridden here in 'D' as an instance property. + +2 class D extends C { prop = 1; } +   ~~~~ + + +[12:00:16 AM] Found 1 error. Watching for file changes. From 71b7695113e470c665f9258da903466f1eded348 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 9 Apr 2020 11:24:54 -0700 Subject: [PATCH 2/8] add fourslash tests for codefix --- .../codeFixPropertyOverrideAccess.ts | 27 +++++++++++++++++++ .../codeFixPropertyOverrideAccess2.ts | 27 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 tests/cases/fourslash/codeFixPropertyOverrideAccess.ts create mode 100644 tests/cases/fourslash/codeFixPropertyOverrideAccess2.ts diff --git a/tests/cases/fourslash/codeFixPropertyOverrideAccess.ts b/tests/cases/fourslash/codeFixPropertyOverrideAccess.ts new file mode 100644 index 0000000000000..d930f76e46838 --- /dev/null +++ b/tests/cases/fourslash/codeFixPropertyOverrideAccess.ts @@ -0,0 +1,27 @@ +/// + +// @strict: true + +//// class A { +//// get x() { return 1 } +//// } +//// class B extends A { +//// x = 2 +//// } + +verify.codeFix({ + description: `Generate 'get' and 'set' accessors`, + newFileContent: `class A { + get x() { return 1 } +} +class B extends A { + private _x = 2 + public get x() { + return this._x + } + public set x(value) { + this._x = value + } +}`, + index: 0 +}) diff --git a/tests/cases/fourslash/codeFixPropertyOverrideAccess2.ts b/tests/cases/fourslash/codeFixPropertyOverrideAccess2.ts new file mode 100644 index 0000000000000..3146d1364fece --- /dev/null +++ b/tests/cases/fourslash/codeFixPropertyOverrideAccess2.ts @@ -0,0 +1,27 @@ +/// + +// @strict: true + +//// class A { +//// x = 1 +//// } +//// class B extends A { +//// get x() { return 2 } +//// } + +verify.codeFix({ + description: `Generate 'get' and 'set' accessors`, + newFileContent: `class A { + private _x = 1 + public get x() { + return this._x + } + public set x(value) { + this._x = value + } +} +class B extends A { + get x() { return 2 } +}`, + index: 0 +}) From e287c8321aac8fc2a59172d9e6153791f633f44f Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 9 Apr 2020 15:06:45 -0700 Subject: [PATCH 3/8] Codefix invokes generate get-set accessor refactor 1. Add add-all test 2. Add codefix that delegates to get-set accessor refactor. Needs massive amounts of cleanup and deduplication. --- .../codefixes/fixPropertyOverrideAccessor.ts | 451 ++++++++++++++++++ .../generateGetAccessorAndSetAccessor.ts | 2 +- src/services/tsconfig.json | 21 +- .../codeFixPropertyOverrideAccess_all.ts | 45 ++ 4 files changed, 508 insertions(+), 11 deletions(-) create mode 100644 src/services/codefixes/fixPropertyOverrideAccessor.ts create mode 100644 tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts new file mode 100644 index 0000000000000..ac63dd8ac5b40 --- /dev/null +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -0,0 +1,451 @@ + + // export interface CodeAction { + // /** Description of the code action to display in the UI of the editor */ + // description: string; + // /** Text changes to apply to each file as part of the code action */ + // changes: FileTextChanges[]; + // /** + // * If the user accepts the code fix, the editor should send the action back in a `applyAction` request. + // * This allows the language service to have side effects (e.g. installing dependencies) upon a code fix. + // */ + // commands?: CodeActionCommand[]; + // } + + // export interface CodeFixAction extends CodeAction { + // /** Short name to identify the fix, for use by telemetry. */ + // fixName: string; + // /** + // * If present, one may call 'getCombinedCodeFix' with this fixId. + // * This may be omitted to indicate that the code fix can't be applied in a group. + // */ + // fixId?: {}; + // fixAllDescription?: string; + // } + + // export interface RefactorEditInfo { + // edits: FileTextChanges[]; + // renameFilename?: string ; + // renameLocation?: number; + // commands?: CodeActionCommand[]; + // } +// r.commands -> c.commands +// r.edits -> c.changes +// r.renameFilename ??? +// r.renameLocation ??? + +/* @internal */ +namespace ts.codefix { + const fixName = "fixPropertyOverrideAccessor"; + 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) { + // TODO: Also still need to support getAllCodeActions (most codefixes do this pretty simply) + if (context.errorCode === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { + const ctx = { ...context, file: context.sourceFile, startPosition: context.span.start, endPosition: context.span.start + context.span.length } + const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); + if (!r) Debug.fail("Couldn't get refactor edits"); + // createCodeFixAction(fixName, r.edits, [Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property, token.text], fixId, Diagnostics.Add_all_missing_members) + return [{ commands: r.commands, changes: r.edits, description: "Generate 'get' and 'set' accessors", fixName, fixAllDescription: "Generate 'get' and 'set' accessors", fixId }]; + } + else if (context.errorCode === 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() + // 1. find node + const node = getTokenAtPosition(context.sourceFile, context.span.start).parent; + // 2. make usre it's a accessor declaration, and if so record its name + Debug.assert(isAccessor(node), "it wasn't an accessor"); + const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name)) + // 3. find parent, make sure it's a class + const containingClass = node.parent; + Debug.assert(isClassLike(containingClass), "it wasn't classlike") + // 4. getAllSupers + const bases = getAllSupers(containingClass, checker) + // 4a. for now, just use the first one + const base = singleOrUndefined(bases) + Debug.assert(!!base, "Porbably more than one super:" + bases.length) + + // 5. getTypeOfNode + const baseType = checker.getTypeAtLocation(base) + // 6. getPropertyOfType + const baseProp = checker.getPropertyOfType(baseType, name) + Debug.assert(!!baseProp, "Couldn't find base property for " + name) + // 7. pass property.valueDeclaration.start,end into the other refactor + const ctx = { ...context, file: context.sourceFile, startPosition: baseProp.valueDeclaration.pos, endPosition: baseProp.valueDeclaration.end } + // TODO: This doesn't do the snazzy post-rename tho + const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); + if (!r) Debug.fail("Couldn't get refactor edits"); + // createCodeFixAction(fixName, r.edits, [Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property, token.text], fixId, Diagnostics.Add_all_missing_members) + return [{ commands: r.commands, changes: r.edits, description: "Generate 'get' and 'set' accessors", fixName, fixAllDescription: "Generate 'get' and 'set' accessors", fixId }]; + // 1. find location of base property + // 2. do the other thing here too + // TODO: Remember to look for readonly and skip emitting setter if found + // TODO: Maybe should convert base into getter/setter pair + // class A { x = 1 } class B { get x() { return 2 } + // ==> + // class A { private _x + } + else { + Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + context.errorCode); + } + // const info = getInfo(context.sourceFile, context.span.start, context.program.getTypeChecker(), context.program); + // if (!info) return undefined; + + // if (info.kind === InfoKind.Enum) { + // const { token, parentDeclaration } = info; + // const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, parentDeclaration)); + // return [createCodeFixAction(fixName, changes, [Diagnostics.Add_missing_enum_member_0, token.text], fixId, Diagnostics.Add_all_missing_members)]; + // } + // const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; + // const methodCodeAction = call && getActionForMethodDeclaration(context, declSourceFile, parentDeclaration, token, call, makeStatic, inJs, context.preferences); + // const addMember = inJs && !isInterfaceDeclaration(parentDeclaration) ? + // singleElementArray(getActionsForAddMissingMemberInJavascriptFile(context, declSourceFile, parentDeclaration, token, makeStatic)) : + // getActionsForAddMissingMemberInTypeScriptFile(context, declSourceFile, parentDeclaration, token, makeStatic); + // return concatenate(singleElementArray(methodCodeAction), addMember); + }, + fixIds: [fixId], + + getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { + if (diag.code === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { + const ctx = { ...context, file: context.sourceFile, startPosition: diag.start, endPosition: diag.start + diag.length } + const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); + if (!r) Debug.fail("Couldn't get refactor edits"); + for (const e of r.edits) { + changes.pushRaw(context.sourceFile, e) + } + } + }), + // getAllCodeActions: context => { + // const { program, preferences } = context; + // const checker = program.getTypeChecker(); + // const seen = createMap(); + + // const typeDeclToMembers = new NodeMap(); + + // return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => { + // eachDiagnostic(context, errorCodes, diag => { + // const info = getInfo(diag.file, diag.start, checker, context.program); + // if (!info || !addToSeen(seen, getNodeId(info.parentDeclaration) + "#" + info.token.text)) { + // return; + // } + + // if (info.kind === InfoKind.Enum) { + // const { token, parentDeclaration } = info; + // addEnumMemberDeclaration(changes, checker, token, parentDeclaration); + // } + // else { + // const { parentDeclaration, token } = info; + // const infos = typeDeclToMembers.getOrUpdate(parentDeclaration, () => []); + // if (!infos.some(i => i.token.text === token.text)) infos.push(info); + // } + // }); + + // typeDeclToMembers.forEach((infos, classDeclaration) => { + // const supers = getAllSupers(classDeclaration, checker); + // for (const info of infos) { + // // If some superclass added this property, don't add it again. + // if (supers.some(superClassOrInterface => { + // const superInfos = typeDeclToMembers.get(superClassOrInterface); + // return !!superInfos && superInfos.some(({ token }) => token.text === info.token.text); + // })) continue; + + // const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; + + // // Always prefer to add a method declaration if possible. + // if (call && !isPrivateIdentifier(token)) { + // addMethodDeclaration(context, changes, declSourceFile, parentDeclaration, token, call, makeStatic, inJs, preferences); + // } + // else { + // if (inJs && !isInterfaceDeclaration(parentDeclaration)) { + // addMissingMemberInJs(changes, declSourceFile, parentDeclaration, token, makeStatic); + // } + // else { + // const typeNode = getTypeNode(program.getTypeChecker(), parentDeclaration, token); + // addPropertyDeclaration(changes, declSourceFile, parentDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0); + // } + // } + // } + // }); + // })); + // }, + }); + + 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; + // readonly token: Identifier; + // readonly parentDeclaration: EnumDeclaration; + // } + // interface ClassOrInterfaceInfo { + // readonly kind: InfoKind.ClassOrInterface; + // readonly token: Identifier | PrivateIdentifier; + // readonly parentDeclaration: ClassOrInterface; + // readonly makeStatic: boolean; + // readonly declSourceFile: SourceFile; + // readonly inJs: boolean; + // readonly call: CallExpression | undefined; + // } + // type Info = EnumInfo | ClassOrInterfaceInfo; + + // function getInfo(tokenSourceFile: SourceFile, tokenPos: number, checker: TypeChecker, program: Program): Info | undefined { + // // The identifier of the missing property. eg: + // // this.missing = 1; + // // ^^^^^^^ + // const token = getTokenAtPosition(tokenSourceFile, tokenPos); + // if (!isIdentifier(token) && !isPrivateIdentifier(token)) { + // return undefined; + // } + + // const { parent } = token; + // if (!isPropertyAccessExpression(parent)) return undefined; + + // const leftExpressionType = skipConstraint(checker.getTypeAtLocation(parent.expression)); + // const { symbol } = leftExpressionType; + // if (!symbol || !symbol.declarations) return undefined; + + // const classDeclaration = find(symbol.declarations, isClassLike); + // // Don't suggest adding private identifiers to anything other than a class. + // if (!classDeclaration && isPrivateIdentifier(token)) { + // return undefined; + // } + + // // Prefer to change the class instead of the interface if they are merged + // const classOrInterface = classDeclaration || find(symbol.declarations, isInterfaceDeclaration); + // if (classOrInterface && !program.isSourceFileFromExternalLibrary(classOrInterface.getSourceFile())) { + // const makeStatic = ((leftExpressionType as TypeReference).target || leftExpressionType) !== checker.getDeclaredTypeOfSymbol(symbol); + // if (makeStatic && (isPrivateIdentifier(token) || isInterfaceDeclaration(classOrInterface))) { + // return undefined; + // } + + // const declSourceFile = classOrInterface.getSourceFile(); + // const inJs = isSourceFileJS(declSourceFile); + // const call = tryCast(parent.parent, isCallExpression); + // return { kind: InfoKind.ClassOrInterface, token, parentDeclaration: classOrInterface, makeStatic, declSourceFile, inJs, call }; + // } + + // const enumDeclaration = find(symbol.declarations, isEnumDeclaration); + // if (enumDeclaration && !isPrivateIdentifier(token) && !program.isSourceFileFromExternalLibrary(enumDeclaration.getSourceFile())) { + // return { kind: InfoKind.Enum, token, parentDeclaration: enumDeclaration }; + // } + // return undefined; + // } + + // function getActionsForAddMissingMemberInJavascriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction | undefined { + // const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, declSourceFile, classDeclaration, token, makeStatic)); + // if (changes.length === 0) { + // return undefined; + // } + + // const diagnostic = makeStatic ? Diagnostics.Initialize_static_property_0 : + // isPrivateIdentifier(token) ? Diagnostics.Declare_a_private_field_named_0 : Diagnostics.Initialize_property_0_in_the_constructor; + + // return createCodeFixAction(fixName, changes, [diagnostic, token.text], fixId, Diagnostics.Add_all_missing_members); + // } + + // function addMissingMemberInJs(changeTracker: textChanges.ChangeTracker, declSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier | PrivateIdentifier, makeStatic: boolean): void { + // const tokenName = token.text; + // if (makeStatic) { + // if (classDeclaration.kind === SyntaxKind.ClassExpression) { + // return; + // } + // const className = classDeclaration.name!.getText(); + // const staticInitialization = initializePropertyToUndefined(createIdentifier(className), tokenName); + // changeTracker.insertNodeAfter(declSourceFile, classDeclaration, staticInitialization); + // } + // else if (isPrivateIdentifier(token)) { + // const property = createProperty( + // /*decorators*/ undefined, + // /*modifiers*/ undefined, + // tokenName, + // /*questionToken*/ undefined, + // /*type*/ undefined, + // /*initializer*/ undefined); + + // const lastProp = getNodeToInsertPropertyAfter(classDeclaration); + // if (lastProp) { + // changeTracker.insertNodeAfter(declSourceFile, lastProp, property); + // } + // else { + // changeTracker.insertNodeAtClassStart(declSourceFile, classDeclaration, property); + // } + // } + // else { + // const classConstructor = getFirstConstructorWithBody(classDeclaration); + // if (!classConstructor) { + // return; + // } + // const propertyInitialization = initializePropertyToUndefined(createThis(), tokenName); + // changeTracker.insertNodeAtConstructorEnd(declSourceFile, classConstructor, propertyInitialization); + // } + // } + + // function initializePropertyToUndefined(obj: Expression, propertyName: string) { + // return createStatement(createAssignment(createPropertyAccess(obj, propertyName), createIdentifier("undefined"))); + // } + + // function getActionsForAddMissingMemberInTypeScriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction[] | undefined { + // const typeNode = getTypeNode(context.program.getTypeChecker(), classDeclaration, token); + // const actions: CodeFixAction[] = [createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0)]; + // if (makeStatic || isPrivateIdentifier(token)) { + // return actions; + // } + + // if (startsWithUnderscore(token.text)) { + // actions.unshift(createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, ModifierFlags.Private)); + // } + + // actions.push(createAddIndexSignatureAction(context, declSourceFile, classDeclaration, token.text, typeNode)); + // return actions; + // } + + // function getTypeNode(checker: TypeChecker, classDeclaration: ClassOrInterface, token: Node) { + // let typeNode: TypeNode | undefined; + // if (token.parent.parent.kind === SyntaxKind.BinaryExpression) { + // const binaryExpression = token.parent.parent as BinaryExpression; + // const otherExpression = token.parent === binaryExpression.left ? binaryExpression.right : binaryExpression.left; + // const widenedType = checker.getWidenedType(checker.getBaseTypeOfLiteralType(checker.getTypeAtLocation(otherExpression))); + // typeNode = checker.typeToTypeNode(widenedType, classDeclaration); + // } + // else { + // const contextualType = checker.getContextualType(token.parent as Expression); + // typeNode = contextualType ? checker.typeToTypeNode(contextualType) : undefined; + // } + // return typeNode || createKeywordTypeNode(SyntaxKind.AnyKeyword); + // } + + // function createAddPropertyDeclarationAction(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): CodeFixAction { + // const changes = textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, declSourceFile, classDeclaration, tokenName, typeNode, modifierFlags)); + // if (modifierFlags & ModifierFlags.Private) { + // return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Declare_private_property_0, tokenName]); + // } + // return createCodeFixAction(fixName, changes, [modifierFlags & ModifierFlags.Static ? Diagnostics.Declare_static_property_0 : Diagnostics.Declare_property_0, tokenName], fixId, Diagnostics.Add_all_missing_members); + // } + + // function addPropertyDeclaration(changeTracker: textChanges.ChangeTracker, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): void { + // const property = createProperty( + // /*decorators*/ undefined, + // /*modifiers*/ modifierFlags ? createNodeArray(createModifiersFromModifierFlags(modifierFlags)) : undefined, + // tokenName, + // /*questionToken*/ undefined, + // typeNode, + // /*initializer*/ undefined); + + // const lastProp = getNodeToInsertPropertyAfter(classDeclaration); + // if (lastProp) { + // changeTracker.insertNodeAfter(declSourceFile, lastProp, property); + // } + // else { + // changeTracker.insertNodeAtClassStart(declSourceFile, classDeclaration, property); + // } + // } + + // // Gets the last of the first run of PropertyDeclarations, or undefined if the class does not start with a PropertyDeclaration. + // function getNodeToInsertPropertyAfter(cls: ClassOrInterface): PropertyDeclaration | undefined { + // let res: PropertyDeclaration | undefined; + // for (const member of cls.members) { + // if (!isPropertyDeclaration(member)) break; + // res = member; + // } + // return res; + // } + + // function createAddIndexSignatureAction(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode): CodeFixAction { + // // Index signatures cannot have the static modifier. + // const stringTypeNode = createKeywordTypeNode(SyntaxKind.StringKeyword); + // const indexingParameter = createParameter( + // /*decorators*/ undefined, + // /*modifiers*/ undefined, + // /*dotDotDotToken*/ undefined, + // "x", + // /*questionToken*/ undefined, + // stringTypeNode, + // /*initializer*/ undefined); + // const indexSignature = createIndexSignature( + // /*decorators*/ undefined, + // /*modifiers*/ undefined, + // [indexingParameter], + // typeNode); + + // const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(declSourceFile, classDeclaration, indexSignature)); + // // No fixId here because code-fix-all currently only works on adding individual named properties. + // return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]); + // } + + // function getActionForMethodDeclaration( + // context: CodeFixContext, + // declSourceFile: SourceFile, + // classDeclaration: ClassOrInterface, + // token: Identifier | PrivateIdentifier, + // callExpression: CallExpression, + // makeStatic: boolean, + // inJs: boolean, + // preferences: UserPreferences, + // ): CodeFixAction | undefined { + // // Private methods are not implemented yet. + // if (isPrivateIdentifier(token)) { return undefined; } + // const changes = textChanges.ChangeTracker.with(context, t => addMethodDeclaration(context, t, declSourceFile, classDeclaration, token, callExpression, makeStatic, inJs, preferences)); + // return createCodeFixAction(fixName, changes, [makeStatic ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0, token.text], fixId, Diagnostics.Add_all_missing_members); + // } + + // function addMethodDeclaration( + // context: CodeFixContextBase, + // changeTracker: textChanges.ChangeTracker, + // declSourceFile: SourceFile, + // typeDecl: ClassOrInterface, + // token: Identifier, + // callExpression: CallExpression, + // makeStatic: boolean, + // inJs: boolean, + // preferences: UserPreferences, + // ): void { + // const methodDeclaration = createMethodFromCallExpression(context, callExpression, token.text, inJs, makeStatic, preferences, typeDecl); + // const containingMethodDeclaration = getAncestor(callExpression, SyntaxKind.MethodDeclaration); + + // if (containingMethodDeclaration && containingMethodDeclaration.parent === typeDecl) { + // changeTracker.insertNodeAfter(declSourceFile, containingMethodDeclaration, methodDeclaration); + // } + // else { + // changeTracker.insertNodeAtClassStart(declSourceFile, typeDecl, methodDeclaration); + // } + // } + + // function addEnumMemberDeclaration(changes: textChanges.ChangeTracker, checker: TypeChecker, token: Identifier, enumDeclaration: EnumDeclaration) { + // /** + // * create initializer only literal enum that has string initializer. + // * value of initializer is a string literal that equal to name of enum member. + // * numeric enum or empty enum will not create initializer. + // */ + // const hasStringInitializer = some(enumDeclaration.members, member => { + // const type = checker.getTypeAtLocation(member); + // return !!(type && type.flags & TypeFlags.StringLike); + // }); + + // const enumMember = createEnumMember(token, hasStringInitializer ? createStringLiteral(token.text) : undefined); + // changes.replaceNode(enumDeclaration.getSourceFile(), enumDeclaration, updateEnumDeclaration( + // enumDeclaration, + // enumDeclaration.decorators, + // enumDeclaration.modifiers, + // enumDeclaration.name, + // concatenate(enumDeclaration.members, singleElementArray(enumMember)) + // ), { + // leadingTriviaOption: textChanges.LeadingTriviaOption.IncludeAll, + // trailingTriviaOption: textChanges.TrailingTriviaOption.Exclude + // }); + // } +} diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/src/services/refactors/generateGetAccessorAndSetAccessor.ts index 038cd04cdeb45..78eb7c6f3e8bc 100644 --- a/src/services/refactors/generateGetAccessorAndSetAccessor.ts +++ b/src/services/refactors/generateGetAccessorAndSetAccessor.ts @@ -35,7 +35,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { }]; } - function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { + export function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { const { file } = context; const fieldInfo = getConvertibleFieldAtPosition(context); diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 117461bc1900c..7c0eda0773575 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -61,29 +61,30 @@ "codefixes/correctQualifiedNameToIndexedAccessType.ts", "codefixes/convertToTypeOnlyExport.ts", "codefixes/convertToTypeOnlyImport.ts", - "codefixes/fixClassIncorrectlyImplementsInterface.ts", - "codefixes/importFixes.ts", - "codefixes/fixImplicitThis.ts", - "codefixes/fixSpelling.ts", - "codefixes/returnValueCorrect.ts", "codefixes/fixAddMissingMember.ts", "codefixes/fixAddMissingNewOperator.ts", + "codefixes/fixAwaitInSyncFunction.ts", "codefixes/fixCannotFindModule.ts", "codefixes/fixClassDoesntImplementInheritedAbstractMember.ts", + "codefixes/fixClassIncorrectlyImplementsInterface.ts", "codefixes/fixClassSuperMustPrecedeThisAccess.ts", "codefixes/fixConstructorForDerivedNeedSuperCall.ts", "codefixes/fixEnableExperimentalDecorators.ts", "codefixes/fixEnableJsxFlag.ts", - "codefixes/fixModuleAndTargetOptions.ts", "codefixes/fixExtendsInterfaceBecomesImplements.ts", "codefixes/fixForgottenThisPropertyAccess.ts", + "codefixes/fixImplicitThis.ts", "codefixes/fixInvalidJsxCharacters.ts", - "codefixes/fixUnusedIdentifier.ts", - "codefixes/fixUnreachableCode.ts", - "codefixes/fixUnusedLabel.ts", "codefixes/fixJSDocTypes.ts", "codefixes/fixMissingCallParentheses.ts", - "codefixes/fixAwaitInSyncFunction.ts", + "codefixes/fixModuleAndTargetOptions.ts", + "codefixes/fixPropertyOverrideAccessor.ts", + "codefixes/fixSpelling.ts", + "codefixes/fixUnreachableCode.ts", + "codefixes/fixUnusedIdentifier.ts", + "codefixes/fixUnusedLabel.ts", + "codefixes/importFixes.ts", + "codefixes/returnValueCorrect.ts", "codefixes/inferFromUsage.ts", "codefixes/disableJsDiagnostics.ts", "codefixes/helpers.ts", diff --git a/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts b/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts new file mode 100644 index 0000000000000..9c6c3bac4e525 --- /dev/null +++ b/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts @@ -0,0 +1,45 @@ +/// + +// @strict: true + +//// class A { +//// get x() { return 1 } +//// } +//// class B extends A { +//// x = 2 +//// } +//// class C { +//// get x() { return 3 } +//// } +//// class D extends C { +//// x = 4 +//// } + +verify.codeFixAll({ + fixId: "fixPropertyOverrideAccessor", + fixAllDescription: "Generate 'get' and 'set' accessors", + newFileContent: `class A { + get x() { return 1 } +} +class B extends A { + private _x = 2 + public get x() { + return this._x + } + public set x(value) { + this._x = value + } +} +class C { + get x() { return 3 } +} +class D extends C { + private _x = 4 + public get x() { + return this._x + } + public set x(value) { + this._x = value + } +}`, +}) From 3030cd8eff237c00e2e6f9b433973c35d7120bf7 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 9 Apr 2020 16:43:31 -0700 Subject: [PATCH 4/8] refactoring done except for deduping --- src/compiler/diagnosticMessages.json | 6 +- .../codefixes/fixPropertyOverrideAccessor.ts | 453 ++---------------- .../codeFixPropertyOverrideAccess_all.ts | 2 +- 3 files changed, 44 insertions(+), 417 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 29bf61b2f752b..0110e2259959d 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -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 diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts index ac63dd8ac5b40..8154216a77ed6 100644 --- a/src/services/codefixes/fixPropertyOverrideAccessor.ts +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -1,41 +1,5 @@ - - // export interface CodeAction { - // /** Description of the code action to display in the UI of the editor */ - // description: string; - // /** Text changes to apply to each file as part of the code action */ - // changes: FileTextChanges[]; - // /** - // * If the user accepts the code fix, the editor should send the action back in a `applyAction` request. - // * This allows the language service to have side effects (e.g. installing dependencies) upon a code fix. - // */ - // commands?: CodeActionCommand[]; - // } - - // export interface CodeFixAction extends CodeAction { - // /** Short name to identify the fix, for use by telemetry. */ - // fixName: string; - // /** - // * If present, one may call 'getCombinedCodeFix' with this fixId. - // * This may be omitted to indicate that the code fix can't be applied in a group. - // */ - // fixId?: {}; - // fixAllDescription?: string; - // } - - // export interface RefactorEditInfo { - // edits: FileTextChanges[]; - // renameFilename?: string ; - // renameLocation?: number; - // commands?: CodeActionCommand[]; - // } -// r.commands -> c.commands -// r.edits -> c.changes -// r.renameFilename ??? -// r.renameLocation ??? - /* @internal */ namespace ts.codefix { - const fixName = "fixPropertyOverrideAccessor"; 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, @@ -44,135 +8,56 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - // TODO: Also still need to support getAllCodeActions (most codefixes do this pretty simply) - if (context.errorCode === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { - const ctx = { ...context, file: context.sourceFile, startPosition: context.span.start, endPosition: context.span.start + context.span.length } - const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); - if (!r) Debug.fail("Couldn't get refactor edits"); - // createCodeFixAction(fixName, r.edits, [Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property, token.text], fixId, Diagnostics.Add_all_missing_members) - return [{ commands: r.commands, changes: r.edits, description: "Generate 'get' and 'set' accessors", fixName, fixAllDescription: "Generate 'get' and 'set' accessors", fixId }]; - } - else if (context.errorCode === 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() - // 1. find node - const node = getTokenAtPosition(context.sourceFile, context.span.start).parent; - // 2. make usre it's a accessor declaration, and if so record its name - Debug.assert(isAccessor(node), "it wasn't an accessor"); - const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name)) - // 3. find parent, make sure it's a class - const containingClass = node.parent; - Debug.assert(isClassLike(containingClass), "it wasn't classlike") - // 4. getAllSupers - const bases = getAllSupers(containingClass, checker) - // 4a. for now, just use the first one - const base = singleOrUndefined(bases) - Debug.assert(!!base, "Porbably more than one super:" + bases.length) - - // 5. getTypeOfNode - const baseType = checker.getTypeAtLocation(base) - // 6. getPropertyOfType - const baseProp = checker.getPropertyOfType(baseType, name) - Debug.assert(!!baseProp, "Couldn't find base property for " + name) - // 7. pass property.valueDeclaration.start,end into the other refactor - const ctx = { ...context, file: context.sourceFile, startPosition: baseProp.valueDeclaration.pos, endPosition: baseProp.valueDeclaration.end } - // TODO: This doesn't do the snazzy post-rename tho - const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); - if (!r) Debug.fail("Couldn't get refactor edits"); - // createCodeFixAction(fixName, r.edits, [Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property, token.text], fixId, Diagnostics.Add_all_missing_members) - return [{ commands: r.commands, changes: r.edits, description: "Generate 'get' and 'set' accessors", fixName, fixAllDescription: "Generate 'get' and 'set' accessors", fixId }]; - // 1. find location of base property - // 2. do the other thing here too - // TODO: Remember to look for readonly and skip emitting setter if found - // TODO: Maybe should convert base into getter/setter pair - // class A { x = 1 } class B { get x() { return 2 } - // ==> - // class A { private _x + const r = doChange(context); + if (r) { + return [createCodeFixAction(fixId, r.edits, Diagnostics.Generate_get_and_set_accessors, fixId, Diagnostics.Generate_get_and_set_accessors_for_all_overriding_properties)]; } - else { - Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + context.errorCode); - } - // const info = getInfo(context.sourceFile, context.span.start, context.program.getTypeChecker(), context.program); - // if (!info) return undefined; - - // if (info.kind === InfoKind.Enum) { - // const { token, parentDeclaration } = info; - // const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, parentDeclaration)); - // return [createCodeFixAction(fixName, changes, [Diagnostics.Add_missing_enum_member_0, token.text], fixId, Diagnostics.Add_all_missing_members)]; - // } - // const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; - // const methodCodeAction = call && getActionForMethodDeclaration(context, declSourceFile, parentDeclaration, token, call, makeStatic, inJs, context.preferences); - // const addMember = inJs && !isInterfaceDeclaration(parentDeclaration) ? - // singleElementArray(getActionsForAddMissingMemberInJavascriptFile(context, declSourceFile, parentDeclaration, token, makeStatic)) : - // getActionsForAddMissingMemberInTypeScriptFile(context, declSourceFile, parentDeclaration, token, makeStatic); - // return concatenate(singleElementArray(methodCodeAction), addMember); }, fixIds: [fixId], getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { - if (diag.code === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { - const ctx = { ...context, file: context.sourceFile, startPosition: diag.start, endPosition: diag.start + diag.length } - const r = ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(ctx, "Generate 'get' and 'set' accessors"); - if (!r) Debug.fail("Couldn't get refactor edits"); + const codeFixContext = { ...context, span: { start: diag.start, length: diag.length }, errorCode: diag.code }; + const r = doChange(codeFixContext) + if (r) { for (const e of r.edits) { changes.pushRaw(context.sourceFile, e) } } }), - // getAllCodeActions: context => { - // const { program, preferences } = context; - // const checker = program.getTypeChecker(); - // const seen = createMap(); - - // const typeDeclToMembers = new NodeMap(); - - // return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => { - // eachDiagnostic(context, errorCodes, diag => { - // const info = getInfo(diag.file, diag.start, checker, context.program); - // if (!info || !addToSeen(seen, getNodeId(info.parentDeclaration) + "#" + info.token.text)) { - // return; - // } - - // if (info.kind === InfoKind.Enum) { - // const { token, parentDeclaration } = info; - // addEnumMemberDeclaration(changes, checker, token, parentDeclaration); - // } - // else { - // const { parentDeclaration, token } = info; - // const infos = typeDeclToMembers.getOrUpdate(parentDeclaration, () => []); - // if (!infos.some(i => i.token.text === token.text)) infos.push(info); - // } - // }); - - // typeDeclToMembers.forEach((infos, classDeclaration) => { - // const supers = getAllSupers(classDeclaration, checker); - // for (const info of infos) { - // // If some superclass added this property, don't add it again. - // if (supers.some(superClassOrInterface => { - // const superInfos = typeDeclToMembers.get(superClassOrInterface); - // return !!superInfos && superInfos.some(({ token }) => token.text === info.token.text); - // })) continue; - - // const { parentDeclaration, declSourceFile, inJs, makeStatic, token, call } = info; - - // // Always prefer to add a method declaration if possible. - // if (call && !isPrivateIdentifier(token)) { - // addMethodDeclaration(context, changes, declSourceFile, parentDeclaration, token, call, makeStatic, inJs, preferences); - // } - // else { - // if (inJs && !isInterfaceDeclaration(parentDeclaration)) { - // addMissingMemberInJs(changes, declSourceFile, parentDeclaration, token, makeStatic); - // } - // else { - // const typeNode = getTypeNode(program.getTypeChecker(), parentDeclaration, token); - // addPropertyDeclaration(changes, declSourceFile, parentDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0); - // } - // } - // } - // }); - // })); - // }, }); + function doChange(context: CodeFixContext) { + let startPosition: number + let endPosition: number + if (context.errorCode === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { + startPosition = context.span.start + endPosition = context.span.start + context.span.length + } + else if (context.errorCode === 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(context.sourceFile, context.span.start).parent; + Debug.assert(isAccessor(node), "it wasn't an accessor"); + const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name)) + const containingClass = node.parent; + Debug.assert(isClassLike(containingClass), "it wasn't classlike") + const bases = getAllSupers(containingClass, checker) + const base = singleOrUndefined(bases) + Debug.assert(!!base, "Porbably more than one super:" + bases.length) + const baseType = checker.getTypeAtLocation(base) + const baseProp = checker.getPropertyOfType(baseType, name) + Debug.assert(!!baseProp, "Couldn't find base property for " + name) + startPosition = baseProp.valueDeclaration.pos + endPosition = baseProp.valueDeclaration.end + } + else { + Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + context.errorCode); + } + const refactorContext = { ...context, file: context.sourceFile, startPosition, endPosition } + // TODO: Maybe just move most of this into a neutral area. + return ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(refactorContext, Diagnostics.Generate_get_and_set_accessors.message); + } + + // TODO: Stolen from a different codefix, should dedupe it somewhere. function getAllSupers(decl: ClassOrInterface | undefined, checker: TypeChecker): readonly ClassOrInterface[] { const res: ClassLikeDeclaration[] = []; while (decl) { @@ -186,266 +71,4 @@ namespace ts.codefix { } type ClassOrInterface = ClassLikeDeclaration | InterfaceDeclaration; - // const enum InfoKind { Enum, ClassOrInterface } - // interface EnumInfo { - // readonly kind: InfoKind.Enum; - // readonly token: Identifier; - // readonly parentDeclaration: EnumDeclaration; - // } - // interface ClassOrInterfaceInfo { - // readonly kind: InfoKind.ClassOrInterface; - // readonly token: Identifier | PrivateIdentifier; - // readonly parentDeclaration: ClassOrInterface; - // readonly makeStatic: boolean; - // readonly declSourceFile: SourceFile; - // readonly inJs: boolean; - // readonly call: CallExpression | undefined; - // } - // type Info = EnumInfo | ClassOrInterfaceInfo; - - // function getInfo(tokenSourceFile: SourceFile, tokenPos: number, checker: TypeChecker, program: Program): Info | undefined { - // // The identifier of the missing property. eg: - // // this.missing = 1; - // // ^^^^^^^ - // const token = getTokenAtPosition(tokenSourceFile, tokenPos); - // if (!isIdentifier(token) && !isPrivateIdentifier(token)) { - // return undefined; - // } - - // const { parent } = token; - // if (!isPropertyAccessExpression(parent)) return undefined; - - // const leftExpressionType = skipConstraint(checker.getTypeAtLocation(parent.expression)); - // const { symbol } = leftExpressionType; - // if (!symbol || !symbol.declarations) return undefined; - - // const classDeclaration = find(symbol.declarations, isClassLike); - // // Don't suggest adding private identifiers to anything other than a class. - // if (!classDeclaration && isPrivateIdentifier(token)) { - // return undefined; - // } - - // // Prefer to change the class instead of the interface if they are merged - // const classOrInterface = classDeclaration || find(symbol.declarations, isInterfaceDeclaration); - // if (classOrInterface && !program.isSourceFileFromExternalLibrary(classOrInterface.getSourceFile())) { - // const makeStatic = ((leftExpressionType as TypeReference).target || leftExpressionType) !== checker.getDeclaredTypeOfSymbol(symbol); - // if (makeStatic && (isPrivateIdentifier(token) || isInterfaceDeclaration(classOrInterface))) { - // return undefined; - // } - - // const declSourceFile = classOrInterface.getSourceFile(); - // const inJs = isSourceFileJS(declSourceFile); - // const call = tryCast(parent.parent, isCallExpression); - // return { kind: InfoKind.ClassOrInterface, token, parentDeclaration: classOrInterface, makeStatic, declSourceFile, inJs, call }; - // } - - // const enumDeclaration = find(symbol.declarations, isEnumDeclaration); - // if (enumDeclaration && !isPrivateIdentifier(token) && !program.isSourceFileFromExternalLibrary(enumDeclaration.getSourceFile())) { - // return { kind: InfoKind.Enum, token, parentDeclaration: enumDeclaration }; - // } - // return undefined; - // } - - // function getActionsForAddMissingMemberInJavascriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction | undefined { - // const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, declSourceFile, classDeclaration, token, makeStatic)); - // if (changes.length === 0) { - // return undefined; - // } - - // const diagnostic = makeStatic ? Diagnostics.Initialize_static_property_0 : - // isPrivateIdentifier(token) ? Diagnostics.Declare_a_private_field_named_0 : Diagnostics.Initialize_property_0_in_the_constructor; - - // return createCodeFixAction(fixName, changes, [diagnostic, token.text], fixId, Diagnostics.Add_all_missing_members); - // } - - // function addMissingMemberInJs(changeTracker: textChanges.ChangeTracker, declSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier | PrivateIdentifier, makeStatic: boolean): void { - // const tokenName = token.text; - // if (makeStatic) { - // if (classDeclaration.kind === SyntaxKind.ClassExpression) { - // return; - // } - // const className = classDeclaration.name!.getText(); - // const staticInitialization = initializePropertyToUndefined(createIdentifier(className), tokenName); - // changeTracker.insertNodeAfter(declSourceFile, classDeclaration, staticInitialization); - // } - // else if (isPrivateIdentifier(token)) { - // const property = createProperty( - // /*decorators*/ undefined, - // /*modifiers*/ undefined, - // tokenName, - // /*questionToken*/ undefined, - // /*type*/ undefined, - // /*initializer*/ undefined); - - // const lastProp = getNodeToInsertPropertyAfter(classDeclaration); - // if (lastProp) { - // changeTracker.insertNodeAfter(declSourceFile, lastProp, property); - // } - // else { - // changeTracker.insertNodeAtClassStart(declSourceFile, classDeclaration, property); - // } - // } - // else { - // const classConstructor = getFirstConstructorWithBody(classDeclaration); - // if (!classConstructor) { - // return; - // } - // const propertyInitialization = initializePropertyToUndefined(createThis(), tokenName); - // changeTracker.insertNodeAtConstructorEnd(declSourceFile, classConstructor, propertyInitialization); - // } - // } - - // function initializePropertyToUndefined(obj: Expression, propertyName: string) { - // return createStatement(createAssignment(createPropertyAccess(obj, propertyName), createIdentifier("undefined"))); - // } - - // function getActionsForAddMissingMemberInTypeScriptFile(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, token: Identifier | PrivateIdentifier, makeStatic: boolean): CodeFixAction[] | undefined { - // const typeNode = getTypeNode(context.program.getTypeChecker(), classDeclaration, token); - // const actions: CodeFixAction[] = [createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, makeStatic ? ModifierFlags.Static : 0)]; - // if (makeStatic || isPrivateIdentifier(token)) { - // return actions; - // } - - // if (startsWithUnderscore(token.text)) { - // actions.unshift(createAddPropertyDeclarationAction(context, declSourceFile, classDeclaration, token.text, typeNode, ModifierFlags.Private)); - // } - - // actions.push(createAddIndexSignatureAction(context, declSourceFile, classDeclaration, token.text, typeNode)); - // return actions; - // } - - // function getTypeNode(checker: TypeChecker, classDeclaration: ClassOrInterface, token: Node) { - // let typeNode: TypeNode | undefined; - // if (token.parent.parent.kind === SyntaxKind.BinaryExpression) { - // const binaryExpression = token.parent.parent as BinaryExpression; - // const otherExpression = token.parent === binaryExpression.left ? binaryExpression.right : binaryExpression.left; - // const widenedType = checker.getWidenedType(checker.getBaseTypeOfLiteralType(checker.getTypeAtLocation(otherExpression))); - // typeNode = checker.typeToTypeNode(widenedType, classDeclaration); - // } - // else { - // const contextualType = checker.getContextualType(token.parent as Expression); - // typeNode = contextualType ? checker.typeToTypeNode(contextualType) : undefined; - // } - // return typeNode || createKeywordTypeNode(SyntaxKind.AnyKeyword); - // } - - // function createAddPropertyDeclarationAction(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): CodeFixAction { - // const changes = textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, declSourceFile, classDeclaration, tokenName, typeNode, modifierFlags)); - // if (modifierFlags & ModifierFlags.Private) { - // return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Declare_private_property_0, tokenName]); - // } - // return createCodeFixAction(fixName, changes, [modifierFlags & ModifierFlags.Static ? Diagnostics.Declare_static_property_0 : Diagnostics.Declare_property_0, tokenName], fixId, Diagnostics.Add_all_missing_members); - // } - - // function addPropertyDeclaration(changeTracker: textChanges.ChangeTracker, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode, modifierFlags: ModifierFlags): void { - // const property = createProperty( - // /*decorators*/ undefined, - // /*modifiers*/ modifierFlags ? createNodeArray(createModifiersFromModifierFlags(modifierFlags)) : undefined, - // tokenName, - // /*questionToken*/ undefined, - // typeNode, - // /*initializer*/ undefined); - - // const lastProp = getNodeToInsertPropertyAfter(classDeclaration); - // if (lastProp) { - // changeTracker.insertNodeAfter(declSourceFile, lastProp, property); - // } - // else { - // changeTracker.insertNodeAtClassStart(declSourceFile, classDeclaration, property); - // } - // } - - // // Gets the last of the first run of PropertyDeclarations, or undefined if the class does not start with a PropertyDeclaration. - // function getNodeToInsertPropertyAfter(cls: ClassOrInterface): PropertyDeclaration | undefined { - // let res: PropertyDeclaration | undefined; - // for (const member of cls.members) { - // if (!isPropertyDeclaration(member)) break; - // res = member; - // } - // return res; - // } - - // function createAddIndexSignatureAction(context: CodeFixContext, declSourceFile: SourceFile, classDeclaration: ClassOrInterface, tokenName: string, typeNode: TypeNode): CodeFixAction { - // // Index signatures cannot have the static modifier. - // const stringTypeNode = createKeywordTypeNode(SyntaxKind.StringKeyword); - // const indexingParameter = createParameter( - // /*decorators*/ undefined, - // /*modifiers*/ undefined, - // /*dotDotDotToken*/ undefined, - // "x", - // /*questionToken*/ undefined, - // stringTypeNode, - // /*initializer*/ undefined); - // const indexSignature = createIndexSignature( - // /*decorators*/ undefined, - // /*modifiers*/ undefined, - // [indexingParameter], - // typeNode); - - // const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(declSourceFile, classDeclaration, indexSignature)); - // // No fixId here because code-fix-all currently only works on adding individual named properties. - // return createCodeFixActionWithoutFixAll(fixName, changes, [Diagnostics.Add_index_signature_for_property_0, tokenName]); - // } - - // function getActionForMethodDeclaration( - // context: CodeFixContext, - // declSourceFile: SourceFile, - // classDeclaration: ClassOrInterface, - // token: Identifier | PrivateIdentifier, - // callExpression: CallExpression, - // makeStatic: boolean, - // inJs: boolean, - // preferences: UserPreferences, - // ): CodeFixAction | undefined { - // // Private methods are not implemented yet. - // if (isPrivateIdentifier(token)) { return undefined; } - // const changes = textChanges.ChangeTracker.with(context, t => addMethodDeclaration(context, t, declSourceFile, classDeclaration, token, callExpression, makeStatic, inJs, preferences)); - // return createCodeFixAction(fixName, changes, [makeStatic ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0, token.text], fixId, Diagnostics.Add_all_missing_members); - // } - - // function addMethodDeclaration( - // context: CodeFixContextBase, - // changeTracker: textChanges.ChangeTracker, - // declSourceFile: SourceFile, - // typeDecl: ClassOrInterface, - // token: Identifier, - // callExpression: CallExpression, - // makeStatic: boolean, - // inJs: boolean, - // preferences: UserPreferences, - // ): void { - // const methodDeclaration = createMethodFromCallExpression(context, callExpression, token.text, inJs, makeStatic, preferences, typeDecl); - // const containingMethodDeclaration = getAncestor(callExpression, SyntaxKind.MethodDeclaration); - - // if (containingMethodDeclaration && containingMethodDeclaration.parent === typeDecl) { - // changeTracker.insertNodeAfter(declSourceFile, containingMethodDeclaration, methodDeclaration); - // } - // else { - // changeTracker.insertNodeAtClassStart(declSourceFile, typeDecl, methodDeclaration); - // } - // } - - // function addEnumMemberDeclaration(changes: textChanges.ChangeTracker, checker: TypeChecker, token: Identifier, enumDeclaration: EnumDeclaration) { - // /** - // * create initializer only literal enum that has string initializer. - // * value of initializer is a string literal that equal to name of enum member. - // * numeric enum or empty enum will not create initializer. - // */ - // const hasStringInitializer = some(enumDeclaration.members, member => { - // const type = checker.getTypeAtLocation(member); - // return !!(type && type.flags & TypeFlags.StringLike); - // }); - - // const enumMember = createEnumMember(token, hasStringInitializer ? createStringLiteral(token.text) : undefined); - // changes.replaceNode(enumDeclaration.getSourceFile(), enumDeclaration, updateEnumDeclaration( - // enumDeclaration, - // enumDeclaration.decorators, - // enumDeclaration.modifiers, - // enumDeclaration.name, - // concatenate(enumDeclaration.members, singleElementArray(enumMember)) - // ), { - // leadingTriviaOption: textChanges.LeadingTriviaOption.IncludeAll, - // trailingTriviaOption: textChanges.TrailingTriviaOption.Exclude - // }); - // } } diff --git a/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts b/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts index 9c6c3bac4e525..5cc9a7a8eaa75 100644 --- a/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts +++ b/tests/cases/fourslash/codeFixPropertyOverrideAccess_all.ts @@ -17,7 +17,7 @@ verify.codeFixAll({ fixId: "fixPropertyOverrideAccessor", - fixAllDescription: "Generate 'get' and 'set' accessors", + fixAllDescription: "Generate 'get' and 'set' accessors for all overriding properties", newFileContent: `class A { get x() { return 1 } } From 6eacc9c1e2835f9c18996a1c3c6d896fbcc762c9 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 9 Apr 2020 17:00:37 -0700 Subject: [PATCH 5/8] move into new, centrally (?) located file --- .../codefixes/fixPropertyOverrideAccessor.ts | 3 +- src/services/codefixes/generateAccessors.ts | 220 ++++++++++++++++++ .../generateGetAccessorAndSetAccessor.ts | 220 +----------------- src/services/tsconfig.json | 1 + 4 files changed, 224 insertions(+), 220 deletions(-) create mode 100644 src/services/codefixes/generateAccessors.ts diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts index 8154216a77ed6..7fdf18f1eec60 100644 --- a/src/services/codefixes/fixPropertyOverrideAccessor.ts +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -53,8 +53,7 @@ namespace ts.codefix { Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + context.errorCode); } const refactorContext = { ...context, file: context.sourceFile, startPosition, endPosition } - // TODO: Maybe just move most of this into a neutral area. - return ts.refactor.generateGetAccessorAndSetAccessor.getEditsForAction(refactorContext, Diagnostics.Generate_get_and_set_accessors.message); + return getEditsForAction(refactorContext, Diagnostics.Generate_get_and_set_accessors.message); } // TODO: Stolen from a different codefix, should dedupe it somewhere. diff --git a/src/services/codefixes/generateAccessors.ts b/src/services/codefixes/generateAccessors.ts new file mode 100644 index 0000000000000..5fb2cf0507761 --- /dev/null +++ b/src/services/codefixes/generateAccessors.ts @@ -0,0 +1,220 @@ +/* @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; + } + + // TODO: Use a general type instead of Refactor* types + // TODO: Rename this to show that it generates accessor + export function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { + const { file } = context; + + const fieldInfo = getConvertibleFieldAtPosition(context); + if (!fieldInfo) return undefined; + + const isJS = isSourceFileJS(file); + const changeTracker = textChanges.ChangeTracker.fromContext(context); + const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration, renameAccessor } = 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(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); + } + + const edits = changeTracker.getChanges(); + const renameFilename = file.fileName; + + const nameNeedRename = renameAccessor ? accessorName : fieldName; + const renameLocationOffset = isIdentifier(nameNeedRename) ? 0 : -1; + const renameLocation = renameLocationOffset + getRenameLocation(edits, renameFilename, nameNeedRename.text, /*preferLastLocation*/ isParameter(declaration)); + return { renameFilename, renameLocation, edits }; + } + + 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 ? (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 | undefined { + const modifiers = append( + !isJS ? [createToken(accessModifier) as Token | Token] : undefined, + isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined + ); + return modifiers && createNodeArray(modifiers); + } + + export function getConvertibleFieldAtPosition(context: RefactorContext): Info | undefined { + const { file, startPosition, endPosition } = context; + + const node = getTokenAtPosition(file, startPosition); + 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, startPosition, endPosition!) // TODO: GH#18217 + || !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: (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, 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); + } + }); + } +} diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/src/services/refactors/generateGetAccessorAndSetAccessor.ts index 78eb7c6f3e8bc..42108db74361d 100644 --- a/src/services/refactors/generateGetAccessorAndSetAccessor.ts +++ b/src/services/refactors/generateGetAccessorAndSetAccessor.ts @@ -2,26 +2,10 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { const actionName = "Generate 'get' and 'set' accessors"; const actionDescription = Diagnostics.Generate_get_and_set_accessors.message; - registerRefactor(actionName, { getEditsForAction, getAvailableActions }); - - 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; - } + registerRefactor(actionName, { getEditsForAction: codefix.getEditsForAction, getAvailableActions }); function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - if (!getConvertibleFieldAtPosition(context)) return emptyArray; + if (!codefix.getConvertibleFieldAtPosition(context)) return emptyArray; return [{ name: actionName, @@ -34,204 +18,4 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { ] }]; } - - export function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { - const { file } = context; - - const fieldInfo = getConvertibleFieldAtPosition(context); - if (!fieldInfo) return undefined; - - const isJS = isSourceFileJS(file); - const changeTracker = textChanges.ChangeTracker.fromContext(context); - const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration, renameAccessor } = 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(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); - } - - const edits = changeTracker.getChanges(); - const renameFilename = file.fileName; - - const nameNeedRename = renameAccessor ? accessorName : fieldName; - const renameLocationOffset = isIdentifier(nameNeedRename) ? 0 : -1; - const renameLocation = renameLocationOffset + getRenameLocation(edits, renameFilename, nameNeedRename.text, /*preferLastLocation*/ isParameter(declaration)); - return { renameFilename, renameLocation, edits }; - } - - 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 ? (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 | undefined { - const modifiers = append( - !isJS ? [createToken(accessModifier) as Token | Token] : undefined, - isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined - ); - return modifiers && createNodeArray(modifiers); - } - - function getConvertibleFieldAtPosition(context: RefactorContext): Info | undefined { - const { file, startPosition, endPosition } = context; - - const node = getTokenAtPosition(file, startPosition); - 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, startPosition, endPosition!) // TODO: GH#18217 - || !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: (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, 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); - } - }); - } } diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 7c0eda0773575..9777f53d95136 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -88,6 +88,7 @@ "codefixes/inferFromUsage.ts", "codefixes/disableJsDiagnostics.ts", "codefixes/helpers.ts", + "codefixes/generateAccessors.ts", "codefixes/fixInvalidImportSyntax.ts", "codefixes/fixStrictClassInitialization.ts", "codefixes/requireInTs.ts", From 3428e275e66eda357a1a94b33be1a7c1929a0f2c Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 10 Apr 2020 09:06:43 -0700 Subject: [PATCH 6/8] Reorder tsconfig and move one more function --- src/services/codefixes/fixAddMissingMember.ts | 13 ------------ .../codefixes/fixPropertyOverrideAccessor.ts | 16 +-------------- src/services/codefixes/generateAccessors.ts | 16 ++++++++++++++- src/services/tsconfig.json | 20 +++++++++---------- 4 files changed, 26 insertions(+), 39 deletions(-) diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index 7a5098d60655a..fbecac8f33afa 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -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; diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts index 7fdf18f1eec60..ac4b292b5bf4a 100644 --- a/src/services/codefixes/fixPropertyOverrideAccessor.ts +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -34,6 +34,7 @@ namespace ts.codefix { endPosition = context.span.start + context.span.length } else if (context.errorCode === Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code) { + // TODO: A lot of these should be bails instead of asserts const checker = context.program.getTypeChecker() const node = getTokenAtPosition(context.sourceFile, context.span.start).parent; Debug.assert(isAccessor(node), "it wasn't an accessor"); @@ -55,19 +56,4 @@ namespace ts.codefix { const refactorContext = { ...context, file: context.sourceFile, startPosition, endPosition } return getEditsForAction(refactorContext, Diagnostics.Generate_get_and_set_accessors.message); } - - // TODO: Stolen from a different codefix, should dedupe it somewhere. - 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; } diff --git a/src/services/codefixes/generateAccessors.ts b/src/services/codefixes/generateAccessors.ts index 5fb2cf0507761..067c47923dbcc 100644 --- a/src/services/codefixes/generateAccessors.ts +++ b/src/services/codefixes/generateAccessors.ts @@ -17,7 +17,7 @@ namespace ts.codefix { } // TODO: Use a general type instead of Refactor* types - // TODO: Rename this to show that it generates accessor + // TODO: Rename this to show that it generates accessors export function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { const { file } = context; @@ -217,4 +217,18 @@ namespace ts.codefix { } }); } + + 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; } diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 9777f53d95136..5aa8d835fb629 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -61,30 +61,30 @@ "codefixes/correctQualifiedNameToIndexedAccessType.ts", "codefixes/convertToTypeOnlyExport.ts", "codefixes/convertToTypeOnlyImport.ts", + "codefixes/fixClassIncorrectlyImplementsInterface.ts", + "codefixes/importFixes.ts", + "codefixes/fixImplicitThis.ts", + "codefixes/fixSpelling.ts", + "codefixes/returnValueCorrect.ts", "codefixes/fixAddMissingMember.ts", "codefixes/fixAddMissingNewOperator.ts", - "codefixes/fixAwaitInSyncFunction.ts", "codefixes/fixCannotFindModule.ts", "codefixes/fixClassDoesntImplementInheritedAbstractMember.ts", - "codefixes/fixClassIncorrectlyImplementsInterface.ts", "codefixes/fixClassSuperMustPrecedeThisAccess.ts", "codefixes/fixConstructorForDerivedNeedSuperCall.ts", "codefixes/fixEnableExperimentalDecorators.ts", "codefixes/fixEnableJsxFlag.ts", + "codefixes/fixModuleAndTargetOptions.ts", "codefixes/fixExtendsInterfaceBecomesImplements.ts", "codefixes/fixForgottenThisPropertyAccess.ts", - "codefixes/fixImplicitThis.ts", "codefixes/fixInvalidJsxCharacters.ts", + "codefixes/fixUnusedIdentifier.ts", + "codefixes/fixUnreachableCode.ts", + "codefixes/fixUnusedLabel.ts", "codefixes/fixJSDocTypes.ts", "codefixes/fixMissingCallParentheses.ts", - "codefixes/fixModuleAndTargetOptions.ts", + "codefixes/fixAwaitInSyncFunction.ts", "codefixes/fixPropertyOverrideAccessor.ts", - "codefixes/fixSpelling.ts", - "codefixes/fixUnreachableCode.ts", - "codefixes/fixUnusedIdentifier.ts", - "codefixes/fixUnusedLabel.ts", - "codefixes/importFixes.ts", - "codefixes/returnValueCorrect.ts", "codefixes/inferFromUsage.ts", "codefixes/disableJsDiagnostics.ts", "codefixes/helpers.ts", From 4d541d2b7430d4a6e85c9191654a8d83fd3d2019 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 10 Apr 2020 09:55:36 -0700 Subject: [PATCH 7/8] Minor cleanup 1. Fix lint. 2. Make code easier to read. 3. Turns some asserts into bails instead. --- .../codefixes/fixPropertyOverrideAccessor.ts | 115 +++-- src/services/codefixes/generateAccessors.ts | 456 +++++++++--------- .../generateGetAccessorAndSetAccessor.ts | 43 +- 3 files changed, 307 insertions(+), 307 deletions(-) diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts index ac4b292b5bf4a..4729042bedf12 100644 --- a/src/services/codefixes/fixPropertyOverrideAccessor.ts +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -1,59 +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 r = doChange(context); - if (r) { - return [createCodeFixAction(fixId, r.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 codeFixContext = { ...context, span: { start: diag.start, length: diag.length }, errorCode: diag.code }; - const r = doChange(codeFixContext) - if (r) { - for (const e of r.edits) { - changes.pushRaw(context.sourceFile, e) - } - } - }), - }); - - function doChange(context: CodeFixContext) { - let startPosition: number - let endPosition: number - if (context.errorCode === Diagnostics._0_is_defined_as_an_accessor_in_class_1_but_is_overridden_here_in_2_as_an_instance_property.code) { - startPosition = context.span.start - endPosition = context.span.start + context.span.length - } - else if (context.errorCode === Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor.code) { - // TODO: A lot of these should be bails instead of asserts - const checker = context.program.getTypeChecker() - const node = getTokenAtPosition(context.sourceFile, context.span.start).parent; - Debug.assert(isAccessor(node), "it wasn't an accessor"); - const name = unescapeLeadingUnderscores(getTextOfPropertyName(node.name)) - const containingClass = node.parent; - Debug.assert(isClassLike(containingClass), "it wasn't classlike") - const bases = getAllSupers(containingClass, checker) - const base = singleOrUndefined(bases) - Debug.assert(!!base, "Porbably more than one super:" + bases.length) - const baseType = checker.getTypeAtLocation(base) - const baseProp = checker.getPropertyOfType(baseType, name) - Debug.assert(!!baseProp, "Couldn't find base property for " + name) - startPosition = baseProp.valueDeclaration.pos - endPosition = baseProp.valueDeclaration.end - } - else { - Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + context.errorCode); - } - const refactorContext = { ...context, file: context.sourceFile, startPosition, endPosition } - return getEditsForAction(refactorContext, Diagnostics.Generate_get_and_set_accessors.message); - } -} +/* @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); + } +} diff --git a/src/services/codefixes/generateAccessors.ts b/src/services/codefixes/generateAccessors.ts index 067c47923dbcc..b234c490f779b 100644 --- a/src/services/codefixes/generateAccessors.ts +++ b/src/services/codefixes/generateAccessors.ts @@ -1,234 +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; - } - - // TODO: Use a general type instead of Refactor* types - // TODO: Rename this to show that it generates accessors - export function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { - const { file } = context; - - const fieldInfo = getConvertibleFieldAtPosition(context); - if (!fieldInfo) return undefined; - - const isJS = isSourceFileJS(file); - const changeTracker = textChanges.ChangeTracker.fromContext(context); - const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration, renameAccessor } = 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(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); - } - - const edits = changeTracker.getChanges(); - const renameFilename = file.fileName; - - const nameNeedRename = renameAccessor ? accessorName : fieldName; - const renameLocationOffset = isIdentifier(nameNeedRename) ? 0 : -1; - const renameLocation = renameLocationOffset + getRenameLocation(edits, renameFilename, nameNeedRename.text, /*preferLastLocation*/ isParameter(declaration)); - return { renameFilename, renameLocation, edits }; - } - - 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 ? (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 | undefined { - const modifiers = append( - !isJS ? [createToken(accessModifier) as Token | Token] : undefined, - isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined - ); - return modifiers && createNodeArray(modifiers); - } - - export function getConvertibleFieldAtPosition(context: RefactorContext): Info | undefined { - const { file, startPosition, endPosition } = context; - - const node = getTokenAtPosition(file, startPosition); - 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, startPosition, endPosition!) // TODO: GH#18217 - || !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: (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, 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; -} +/* @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(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 ? (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 | undefined { + const modifiers = append( + !isJS ? [createToken(accessModifier) as Token | Token] : 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: (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, 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; +} diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/src/services/refactors/generateGetAccessorAndSetAccessor.ts index 42108db74361d..e942305f6b92c 100644 --- a/src/services/refactors/generateGetAccessorAndSetAccessor.ts +++ b/src/services/refactors/generateGetAccessorAndSetAccessor.ts @@ -2,20 +2,35 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { const actionName = "Generate 'get' and 'set' accessors"; const actionDescription = Diagnostics.Generate_get_and_set_accessors.message; - registerRefactor(actionName, { getEditsForAction: codefix.getEditsForAction, getAvailableActions }); + registerRefactor(actionName, { + getEditsForAction(context, actionName) { + if (!context.endPosition) return undefined; + const info = codefix.getAccessorConvertiblePropertyAtPosition(context.file, context.startPosition, context.endPosition); + if (!info) return undefined; + const edits = codefix.generateAccessorFromProperty(context.file, context.startPosition, context.endPosition, context, actionName); + if (!edits) return undefined; - function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - if (!codefix.getConvertibleFieldAtPosition(context)) return emptyArray; + const renameFilename = context.file.fileName; + const nameNeedRename = info.renameAccessor ? info.accessorName : info.fieldName; + const renameLocationOffset = isIdentifier(nameNeedRename) ? 0 : -1; + const renameLocation = renameLocationOffset + getRenameLocation(edits, renameFilename, nameNeedRename.text, /*preferLastLocation*/ isParameter(info.declaration)); - return [{ - name: actionName, - description: actionDescription, - actions: [ - { - name: actionName, - description: actionDescription - } - ] - }]; - } + return { renameFilename, renameLocation, edits }; + }, + getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { + if (!context.endPosition) return emptyArray; + if (!codefix.getAccessorConvertiblePropertyAtPosition(context.file, context.startPosition, context.endPosition)) return emptyArray; + + return [{ + name: actionName, + description: actionDescription, + actions: [ + { + name: actionName, + description: actionDescription + } + ] + }]; + } + }); } From e466bb94c173b1eaaf697590538b0c47651f5efa Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 22 May 2020 09:06:05 -0700 Subject: [PATCH 8/8] Fix multi-file usage 1. Follow aliases with getSupers. 2. Make fix in file with superclass, not with the error. --- .../codefixes/fixPropertyOverrideAccessor.ts | 1 + src/services/codefixes/generateAccessors.ts | 7 +++-- .../codeFixPropertyOverrideAccess3.ts | 28 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/codeFixPropertyOverrideAccess3.ts diff --git a/src/services/codefixes/fixPropertyOverrideAccessor.ts b/src/services/codefixes/fixPropertyOverrideAccessor.ts index 4729042bedf12..1941b5ccd5826 100644 --- a/src/services/codefixes/fixPropertyOverrideAccessor.ts +++ b/src/services/codefixes/fixPropertyOverrideAccessor.ts @@ -47,6 +47,7 @@ namespace ts.codefix { startPosition = baseProp.valueDeclaration.pos; endPosition = baseProp.valueDeclaration.end; + file = getSourceFileOfNode(baseProp.valueDeclaration); } else { Debug.fail("fixPropertyOverrideAccessor codefix got unexpected error code " + code); diff --git a/src/services/codefixes/generateAccessors.ts b/src/services/codefixes/generateAccessors.ts index 4b281139f0f23..7fed793a2f1ac 100644 --- a/src/services/codefixes/generateAccessors.ts +++ b/src/services/codefixes/generateAccessors.ts @@ -229,8 +229,11 @@ namespace ts.codefix { 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); } + if (!superSymbol) break; + const symbol = superSymbol.flags & SymbolFlags.Alias ? checker.getAliasedSymbol(superSymbol) : superSymbol; + const superDecl = find(symbol.declarations, isClassLike); + if (!superDecl) break; + res.push(superDecl); decl = superDecl; } return res; diff --git a/tests/cases/fourslash/codeFixPropertyOverrideAccess3.ts b/tests/cases/fourslash/codeFixPropertyOverrideAccess3.ts new file mode 100644 index 0000000000000..cc83c8e067f09 --- /dev/null +++ b/tests/cases/fourslash/codeFixPropertyOverrideAccess3.ts @@ -0,0 +1,28 @@ +/// + +// @strict: true + +// @Filename: foo.ts +//// import { A } from './source' +//// class B extends A { +//// get x() { return 2 } +//// } +// @Filename: source.ts +//// export class A { +//// x = 1 +//// } + +verify.codeFix({ + description: `Generate 'get' and 'set' accessors`, + newFileContent: { + '/tests/cases/fourslash/source.ts': `export class A { + private _x = 1; + public get x() { + return this._x; + } + public set x(value) { + this._x = value; + } +}`}, + index: 0 +})