refactor(language-service): de-duplicate rename and reference results (#40454)

The initial implementation assumed that the consuming editors would
de-duplicate rename locations. In fact, vscode treats overlapping rename
locations as distinct and errors when trying to preview the renames.

This commit updates the language service to de-duplicate exact file+span
matches before returning rename and reference locations.

While vscode _does_ de-duplicate reference results, it still makes sense
to de-duplicate them on our side when possible to make tests more
understandable. If a template has 3 instances of a variable, it makes
sense to get get 3 reference results rather than 4+ with some duplicates.

PR Close #40454
This commit is contained in:
Andrew Scott 2021-01-15 10:47:39 -08:00 committed by Andrew Kushnir
parent 5e95153d44
commit 402e2e6189
2 changed files with 40 additions and 20 deletions

View File

@ -166,7 +166,7 @@ export class ReferencesAndRenameBuilder {
return undefined; return undefined;
} }
const entries: ts.RenameLocation[] = []; const entries: Map<string, ts.RenameLocation> = new Map();
for (const location of locations) { for (const location of locations) {
// TODO(atscott): Determine if a file is a shim file in a more robust way and make the API // TODO(atscott): Determine if a file is a shim file in a more robust way and make the API
// available in an appropriate location. // available in an appropriate location.
@ -177,17 +177,17 @@ export class ReferencesAndRenameBuilder {
if (entry === null) { if (entry === null) {
return undefined; return undefined;
} }
entries.push(entry); entries.set(createLocationKey(entry), entry);
} else { } else {
// Ensure we only allow renaming a TS result with matching text // Ensure we only allow renaming a TS result with matching text
const refNode = this.getTsNodeAtPosition(location.fileName, location.textSpan.start); const refNode = this.getTsNodeAtPosition(location.fileName, location.textSpan.start);
if (refNode === null || refNode.getText() !== originalNodeText) { if (refNode === null || refNode.getText() !== originalNodeText) {
return undefined; return undefined;
} }
entries.push(location); entries.set(createLocationKey(location), location);
} }
} }
return entries; return Array.from(entries.values());
} }
getReferencesAtPosition(filePath: string, position: number): ts.ReferenceEntry[]|undefined { getReferencesAtPosition(filePath: string, position: number): ts.ReferenceEntry[]|undefined {
@ -344,18 +344,18 @@ export class ReferencesAndRenameBuilder {
return undefined; return undefined;
} }
const entries: ts.ReferenceEntry[] = []; const entries: Map<string, ts.ReferenceEntry> = new Map();
for (const ref of refs) { for (const ref of refs) {
if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) { if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
const entry = this.convertToTemplateDocumentSpan(ref, this.ttc); const entry = this.convertToTemplateDocumentSpan(ref, this.ttc);
if (entry !== null) { if (entry !== null) {
entries.push(entry); entries.set(createLocationKey(entry), entry);
} }
} else { } else {
entries.push(ref); entries.set(createLocationKey(ref), ref);
} }
} }
return entries; return Array.from(entries.values());
} }
private convertToTemplateDocumentSpan<T extends ts.DocumentSpan>( private convertToTemplateDocumentSpan<T extends ts.DocumentSpan>(
@ -432,3 +432,13 @@ function getRenameTextAndSpanAtPosition(node: TmplAstNode|AST, position: number)
return null; return null;
} }
/**
* Creates a "key" for a rename/reference location by concatenating file name, span start, and span
* length. This allows us to de-duplicate template results when an item may appear several times
* in the TCB but map back to the same template location.
*/
function createLocationKey(ds: ts.DocumentSpan) {
return ds.fileName + ds.textSpan.start + ds.textSpan.length;
}

View File

@ -554,33 +554,43 @@ describe('find references and rename locations', () => {
let cursor: number; let cursor: number;
beforeEach(() => { beforeEach(() => {
const cursorInfo = extractCursorInfo(` const cursorInfo = extractCursorInfo(`
<div *ngFor="let hero of heroes">
<span *ngIf="hero">
{{her¦o}}
</span>
</div>
`);
cursor = cursorInfo.cursor;
const appFile = {
name: _('/app.ts'),
contents: `
import {Component} from '@angular/core'; import {Component} from '@angular/core';
@Component({template: '<div *ngFor="let hero of heroes">{{her¦o}}</div>'}) @Component({templateUrl: './template.ng.html'})
export class AppCmp { export class AppCmp {
heroes: string[] = []; heroes: string[] = [];
}`); }`
cursor = cursorInfo.cursor; };
const appFile = {name: _('/app.ts'), contents: cursorInfo.text}; const templateFile = {name: _('/template.ng.html'), contents: cursorInfo.text};
env = createModuleWithDeclarations([appFile]); env = createModuleWithDeclarations([appFile], [templateFile]);
}); });
it('should find references', () => { it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; const refs = getReferencesAtPosition(_('/template.ng.html'), cursor)!;
expect(refs.length).toBe(2); expect(refs.length).toBe(3);
assertFileNames(refs, ['app.ts']); assertFileNames(refs, ['template.ng.html']);
assertTextSpans(refs, ['hero']); assertTextSpans(refs, ['hero']);
const originalRefs = env.ngLS.getReferencesAtPosition(_('/app.ts'), cursor)!; const originalRefs = env.ngLS.getReferencesAtPosition(_('/template.ng.html'), cursor)!;
// Get the declaration by finding the reference that appears first in the template // Get the declaration by finding the reference that appears first in the template
originalRefs.sort((a, b) => a.textSpan.start - b.textSpan.start); originalRefs.sort((a, b) => a.textSpan.start - b.textSpan.start);
expect(originalRefs[0].isDefinition).toBe(true); expect(originalRefs[0].isDefinition).toBe(true);
}); });
it('should find rename locations', () => { it('should find rename locations', () => {
const renameLocations = getRenameLocationsAtPosition(_('/app.ts'), cursor)!; const renameLocations = getRenameLocationsAtPosition(_('/template.ng.html'), cursor)!;
expect(renameLocations.length).toBe(2); expect(renameLocations.length).toBe(3);
assertFileNames(renameLocations, ['app.ts']); assertFileNames(renameLocations, ['template.ng.html']);
assertTextSpans(renameLocations, ['hero']); assertTextSpans(renameLocations, ['hero']);
}); });
}); });