Skip to content

Commit

Permalink
refactor(language-service): de-duplicate rename and reference results
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
atscott committed Jan 15, 2021
1 parent 9a5ac47 commit 71f2ca0
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 20 deletions.
26 changes: 18 additions & 8 deletions packages/language-service/ivy/references.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class ReferencesAndRenameBuilder {
return undefined;
}

const entries: ts.RenameLocation[] = [];
const entries: Map<string, ts.RenameLocation> = new Map();
for (const location of locations) {
// TODO(atscott): Determine if a file is a shim file in a more robust way and make the API
// available in an appropriate location.
Expand All @@ -150,17 +150,17 @@ export class ReferencesAndRenameBuilder {
if (entry === null) {
return undefined;
}
entries.push(entry);
entries.set(createLocationKey(entry), entry);
} else {
// Ensure we only allow renaming a TS result with matching text
const refNode = this.getTsNodeAtPosition(location.fileName, location.textSpan.start);
if (refNode === null || refNode.getText() !== originalNodeText) {
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 {
Expand Down Expand Up @@ -309,18 +309,18 @@ export class ReferencesAndRenameBuilder {
return undefined;
}

const entries: ts.ReferenceEntry[] = [];
const entries: Map<string, ts.ReferenceEntry> = new Map();
for (const ref of refs) {
if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
const entry = this.convertToTemplateDocumentSpan(ref, this.ttc);
if (entry !== null) {
entries.push(entry);
entries.set(createLocationKey(entry), entry);
}
} else {
entries.push(ref);
entries.set(createLocationKey(ref), ref);
}
}
return entries;
return Array.from(entries.values());
}

private convertToTemplateDocumentSpan<T extends ts.DocumentSpan>(
Expand Down Expand Up @@ -399,3 +399,13 @@ function getTemplateNodeRenameTextAtPosition(node: TmplAstNode|AST, position: nu

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;
}
34 changes: 22 additions & 12 deletions packages/language-service/ivy/test/references_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -554,33 +554,43 @@ describe('find references and rename locations', () => {
let cursor: number;
beforeEach(() => {
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';
@Component({template: '<div *ngFor="let hero of heroes">{{her¦o}}</div>'})
@Component({templateUrl: './template.ng.html'})
export class AppCmp {
heroes: string[] = [];
}`);
cursor = cursorInfo.cursor;
const appFile = {name: _('/app.ts'), contents: cursorInfo.text};
env = createModuleWithDeclarations([appFile]);
}`
};
const templateFile = {name: _('/template.ng.html'), contents: cursorInfo.text};
env = createModuleWithDeclarations([appFile], [templateFile]);
});

it('should find references', () => {
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(2);
assertFileNames(refs, ['app.ts']);
const refs = getReferencesAtPosition(_('/template.ng.html'), cursor)!;
expect(refs.length).toBe(3);
assertFileNames(refs, ['template.ng.html']);
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
originalRefs.sort((a, b) => a.textSpan.start - b.textSpan.start);
expect(originalRefs[0].isDefinition).toBe(true);
});

it('should find rename locations', () => {
const renameLocations = getRenameLocationsAtPosition(_('/app.ts'), cursor)!;
expect(renameLocations.length).toBe(2);
assertFileNames(renameLocations, ['app.ts']);
const renameLocations = getRenameLocationsAtPosition(_('/template.ng.html'), cursor)!;
expect(renameLocations.length).toBe(3);
assertFileNames(renameLocations, ['template.ng.html']);
assertTextSpans(renameLocations, ['hero']);
});
});
Expand Down

0 comments on commit 71f2ca0

Please sign in to comment.