Skip to content

Commit 18d8ad1

Browse files
mhegazymhegazy
authored
Merge pull request microsoft#25372 from Microsoft/fixAddMissingMember_all_dedup
fixAddMissingMember: Improve deduplication in code-fix-all
2 parents c58e298 + 271bbb0 commit 18d8ad1

File tree

5 files changed

+166
-40
lines changed

5 files changed

+166
-40
lines changed

‎src/services/codeFixProvider.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ namespace ts {
7171
return fixIdToRegistration.get(cast(context.fixId, isString))!.getAllCodeActions!(context);
7272
}
7373

74-
function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions {
74+
export function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions {
7575
return { changes, commands };
7676
}
7777

@@ -89,7 +89,7 @@ namespace ts {
8989
return createCombinedCodeActions(changes, commands.length === 0 ? undefined : commands);
9090
}
9191

92-
function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void {
92+
export function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void {
9393
for (const diag of program.getSemanticDiagnostics(sourceFile, cancellationToken).concat(computeSuggestionDiagnostics(sourceFile, program, cancellationToken))) {
9494
if (contains(errorCodes, diag.code)) {
9595
cb(diag as DiagnosticWithLocation);

‎src/services/codefixes/fixAddMissingMember.ts

Lines changed: 82 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,55 +13,103 @@ namespace ts.codefix {
1313
if (!info) return undefined;
1414

1515
if (info.kind === InfoKind.enum) {
16-
const { token, enumDeclaration } = info;
17-
const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, enumDeclaration));
16+
const { token, parentDeclaration } = info;
17+
const changes = textChanges.ChangeTracker.with(context, t => addEnumMemberDeclaration(t, context.program.getTypeChecker(), token, parentDeclaration));
1818
return [createCodeFixAction(fixName, changes, [Diagnostics.Add_missing_enum_member_0, token.text], fixId, Diagnostics.Add_all_missing_members)];
1919
}
20-
const { classDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info;
21-
const methodCodeAction = call && getActionForMethodDeclaration(context, classDeclarationSourceFile, classDeclaration, token, call, makeStatic, inJs, context.preferences);
20+
const { parentDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info;
21+
const methodCodeAction = call && getActionForMethodDeclaration(context, classDeclarationSourceFile, parentDeclaration, token, call, makeStatic, inJs, context.preferences);
2222
const addMember = inJs ?
23-
singleElementArray(getActionsForAddMissingMemberInJavaScriptFile(context, classDeclarationSourceFile, classDeclaration, token.text, makeStatic)) :
24-
getActionsForAddMissingMemberInTypeScriptFile(context, classDeclarationSourceFile, classDeclaration, token, makeStatic);
23+
singleElementArray(getActionsForAddMissingMemberInJavaScriptFile(context, classDeclarationSourceFile, parentDeclaration, token.text, makeStatic)) :
24+
getActionsForAddMissingMemberInTypeScriptFile(context, classDeclarationSourceFile, parentDeclaration, token, makeStatic);
2525
return concatenate(singleElementArray(methodCodeAction), addMember);
2626
},
2727
fixIds: [fixId],
2828
getAllCodeActions: context => {
29-
const seenNames = createMap<true>();
30-
return codeFixAll(context, errorCodes, (changes, diag) => {
31-
const { program, preferences } = context;
32-
const checker = program.getTypeChecker();
33-
const info = getInfo(diag.file, diag.start, checker);
34-
if (!info || !addToSeen(seenNames, info.token.text)) {
35-
return;
36-
}
37-
38-
if (info.kind === InfoKind.enum) {
39-
const { token, enumDeclaration } = info;
40-
addEnumMemberDeclaration(changes, checker, token, enumDeclaration);
41-
}
42-
else {
43-
const { classDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info;
44-
// Always prefer to add a method declaration if possible.
45-
if (call) {
46-
addMethodDeclaration(context, changes, classDeclarationSourceFile, classDeclaration, token, call, makeStatic, inJs, preferences);
29+
const { program, preferences } = context;
30+
const checker = program.getTypeChecker();
31+
const seen = createMap<true>();
32+
33+
const classToMembers = new NodeMap<ClassLikeDeclaration, ClassInfo[]>();
34+
35+
return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => {
36+
eachDiagnostic(context, errorCodes, diag => {
37+
const info = getInfo(diag.file, diag.start, checker);
38+
if (!info || !addToSeen(seen, getNodeId(info.parentDeclaration) + "#" + info.token.text)) {
39+
return;
40+
}
41+
42+
if (info.kind === InfoKind.enum) {
43+
const { token, parentDeclaration } = info;
44+
addEnumMemberDeclaration(changes, checker, token, parentDeclaration);
4745
}
4846
else {
49-
if (inJs) {
50-
addMissingMemberInJs(changes, classDeclarationSourceFile, classDeclaration, token.text, makeStatic);
47+
const { parentDeclaration, token } = info;
48+
const infos = classToMembers.getOrUpdate(parentDeclaration, () => []);
49+
if (!infos.some(i => i.token.text === token.text)) infos.push(info);
50+
}
51+
});
52+
53+
classToMembers.forEach((infos, classDeclaration) => {
54+
const superClasses = getAllSuperClasses(classDeclaration, checker);
55+
for (const info of infos) {
56+
// If some superclass added this property, don't add it again.
57+
if (superClasses.some(superClass => {
58+
const superInfos = classToMembers.get(superClass);
59+
return !!superInfos && superInfos.some(({ token }) => token.text === info.token.text);
60+
})) continue;
61+
62+
const { parentDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info;
63+
64+
// Always prefer to add a method declaration if possible.
65+
if (call) {
66+
addMethodDeclaration(context, changes, classDeclarationSourceFile, parentDeclaration, token, call, makeStatic, inJs, preferences);
5167
}
5268
else {
53-
const typeNode = getTypeNode(program.getTypeChecker(), classDeclaration, token);
54-
addPropertyDeclaration(changes, classDeclarationSourceFile, classDeclaration, token.text, typeNode, makeStatic);
69+
if (inJs) {
70+
addMissingMemberInJs(changes, classDeclarationSourceFile, parentDeclaration, token.text, makeStatic);
71+
}
72+
else {
73+
const typeNode = getTypeNode(program.getTypeChecker(), parentDeclaration, token);
74+
addPropertyDeclaration(changes, classDeclarationSourceFile, parentDeclaration, token.text, typeNode, makeStatic);
75+
}
5576
}
5677
}
57-
}
58-
});
78+
});
79+
}));
5980
},
6081
});
6182

83+
function getAllSuperClasses(cls: ClassLikeDeclaration | undefined, checker: TypeChecker): ReadonlyArray<ClassLikeDeclaration> {
84+
const res: ClassLikeDeclaration[] = [];
85+
while (cls) {
86+
const superElement = getClassExtendsHeritageElement(cls);
87+
const superSymbol = superElement && checker.getSymbolAtLocation(superElement.expression);
88+
const superDecl = superSymbol && find(superSymbol.declarations, isClassLike);
89+
if (superDecl) { res.push(superDecl); }
90+
cls = superDecl;
91+
}
92+
return res;
93+
}
94+
95+
interface InfoBase {
96+
readonly kind: InfoKind;
97+
readonly token: Identifier;
98+
readonly parentDeclaration: EnumDeclaration | ClassLikeDeclaration;
99+
}
62100
enum InfoKind { enum, class }
63-
interface EnumInfo { kind: InfoKind.enum; token: Identifier; enumDeclaration: EnumDeclaration; }
64-
interface ClassInfo { kind: InfoKind.class; token: Identifier; classDeclaration: ClassLikeDeclaration; makeStatic: boolean; classDeclarationSourceFile: SourceFile; inJs: boolean; call: CallExpression | undefined; }
101+
interface EnumInfo extends InfoBase {
102+
readonly kind: InfoKind.enum;
103+
readonly parentDeclaration: EnumDeclaration;
104+
}
105+
interface ClassInfo extends InfoBase {
106+
readonly kind: InfoKind.class;
107+
readonly parentDeclaration: ClassLikeDeclaration;
108+
readonly makeStatic: boolean;
109+
readonly classDeclarationSourceFile: SourceFile;
110+
readonly inJs: boolean;
111+
readonly call: CallExpression | undefined;
112+
}
65113
type Info = EnumInfo | ClassInfo;
66114

67115
function getInfo(tokenSourceFile: SourceFile, tokenPos: number, checker: TypeChecker): Info | undefined {
@@ -86,11 +134,11 @@ namespace ts.codefix {
86134
const classDeclarationSourceFile = classDeclaration.getSourceFile();
87135
const inJs = isSourceFileJavaScript(classDeclarationSourceFile);
88136
const call = tryCast(parent.parent, isCallExpression);
89-
return { kind: InfoKind.class, token, classDeclaration, makeStatic, classDeclarationSourceFile, inJs, call };
137+
return { kind: InfoKind.class, token, parentDeclaration: classDeclaration, makeStatic, classDeclarationSourceFile, inJs, call };
90138
}
91139
const enumDeclaration = find(symbol.declarations, isEnumDeclaration);
92140
if (enumDeclaration) {
93-
return { kind: InfoKind.enum, token, enumDeclaration };
141+
return { kind: InfoKind.enum, token, parentDeclaration: enumDeclaration };
94142
}
95143
return undefined;
96144
}

‎src/services/utilities.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,6 +1351,40 @@ namespace ts {
13511351
}
13521352
}
13531353

1354+
export interface ReadonlyNodeMap<TNode extends Node, TValue> {
1355+
get(node: TNode): TValue | undefined;
1356+
has(node: TNode): boolean;
1357+
}
1358+
1359+
export class NodeMap<TNode extends Node, TValue> implements ReadonlyNodeMap<TNode, TValue> {
1360+
private map = createMap<{ node: TNode, value: TValue }>();
1361+
1362+
get(node: TNode): TValue | undefined {
1363+
const res = this.map.get(String(getNodeId(node)));
1364+
return res && res.value;
1365+
}
1366+
1367+
getOrUpdate(node: TNode, setValue: () => TValue): TValue {
1368+
const res = this.get(node);
1369+
if (res) return res;
1370+
const value = setValue();
1371+
this.set(node, value);
1372+
return value;
1373+
}
1374+
1375+
set(node: TNode, value: TValue): void {
1376+
this.map.set(String(getNodeId(node)), { node, value });
1377+
}
1378+
1379+
has(node: TNode): boolean {
1380+
return this.map.has(String(getNodeId(node)));
1381+
}
1382+
1383+
forEach(cb: (value: TValue, node: TNode) => void): void {
1384+
this.map.forEach(({ node, value }) => cb(value, node));
1385+
}
1386+
}
1387+
13541388
export function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined {
13551389
if (!node) return undefined;
13561390

‎tests/baselines/reference/api/tsserverlibrary.d.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10830,6 +10830,18 @@ declare namespace ts {
1083010830
forEach(cb: (node: Node) => void): void;
1083110831
some(pred: (node: Node) => boolean): boolean;
1083210832
}
10833+
interface ReadonlyNodeMap<TNode extends Node, TValue> {
10834+
get(node: TNode): TValue | undefined;
10835+
has(node: TNode): boolean;
10836+
}
10837+
class NodeMap<TNode extends Node, TValue> implements ReadonlyNodeMap<TNode, TValue> {
10838+
private map;
10839+
get(node: TNode): TValue | undefined;
10840+
getOrUpdate(node: TNode, setValue: () => TValue): TValue;
10841+
set(node: TNode, value: TValue): void;
10842+
has(node: TNode): boolean;
10843+
forEach(cb: (value: TValue, node: TNode) => void): void;
10844+
}
1083310845
function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined;
1083410846
function findModifier(node: Node, kind: Modifier["kind"]): Modifier | undefined;
1083510847
function insertImport(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDecl: Statement): void;
@@ -11590,8 +11602,10 @@ declare namespace ts {
1159011602
function getSupportedErrorCodes(): string[];
1159111603
function getFixes(context: CodeFixContext): CodeFixAction[];
1159211604
function getAllFixes(context: CodeFixAllContext): CombinedCodeActions;
11605+
function createCombinedCodeActions(changes: FileTextChanges[], commands?: CodeActionCommand[]): CombinedCodeActions;
1159311606
function createFileTextChanges(fileName: string, textChanges: TextChange[]): FileTextChanges;
1159411607
function codeFixAll(context: CodeFixAllContext, errorCodes: number[], use: (changes: textChanges.ChangeTracker, error: DiagnosticWithLocation, commands: Push<CodeActionCommand>) => void): CombinedCodeActions;
11608+
function eachDiagnostic({ program, sourceFile, cancellationToken }: CodeFixAllContext, errorCodes: number[], cb: (diag: DiagnosticWithLocation) => void): void;
1159511609
}
1159611610
}
1159711611
declare namespace ts {

‎tests/cases/fourslash/codeFixAddMissingMember_all.ts

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,22 @@
88
//// }
99
////}
1010
////
11-
////enum E {}
12-
////E.A;
11+
////class D extends C {}
12+
////class E extends D {
13+
//// method() {
14+
//// this.x = 0;
15+
//// this.ex = 0;
16+
//// }
17+
////}
18+
////
19+
////class Unrelated {
20+
//// method() {
21+
//// this.x = 0;
22+
//// }
23+
////}
24+
////
25+
////enum En {}
26+
////En.A;
1327

1428
verify.codeFixAll({
1529
fixId: "addMissingMember",
@@ -27,8 +41,24 @@ verify.codeFixAll({
2741
}
2842
}
2943
30-
enum E {
44+
class D extends C {}
45+
class E extends D {
46+
ex: number;
47+
method() {
48+
this.x = 0;
49+
this.ex = 0;
50+
}
51+
}
52+
53+
class Unrelated {
54+
x: number;
55+
method() {
56+
this.x = 0;
57+
}
58+
}
59+
60+
enum En {
3161
A
3262
}
33-
E.A;`,
63+
En.A;`,
3464
});

0 commit comments

Comments
 (0)