Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vscode-references-view does not de-duplicate results like the built in ReferencesModel #117095

Closed
atscott opened this issue Feb 19, 2021 · 7 comments · Fixed by #117424
Closed

vscode-references-view does not de-duplicate results like the built in ReferencesModel #117095

atscott opened this issue Feb 19, 2021 · 7 comments · Fixed by #117424
Assignees
@jrieken
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders references-viewlet verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded
Milestone
March 2021

Comments

@atscott
Copy link
Contributor

atscott commented Feb 19, 2021

The built-in "go to references" command in vscode uses the ReferencesModel to de-duplicate reference results:

vscode/src/vs/editor/contrib/gotoSymbol/referencesModel.ts

Lines 171 to 182 in 36dd567

if (current.children.length === 0 || ReferencesModel._compareReferences(link, current.children[current.children.length - 1]) !== 0) {
const oneRef = new OneReference(
providersFirst === link,
current,
link.uri,
link.targetSelectionRange || link.range,
ref => this._onDidChangeReferenceRange.fire(ref)
);
this.references.push(oneRef);
current.children.push(oneRef);
}

Equal uri's and ranges constitute duplicate results:

vscode/src/vs/editor/contrib/gotoSymbol/referencesModel.ts

Lines 293 to 295 in 36dd567

private static _compareReferences(a: Location, b: Location): number {
return extUri.compare(a.uri, b.uri) || Range.compareRangesUsingStarts(a.range, b.range);
}

The ReferencesModel of the extension does not de-duplicate results at all, but instead always pushes each location to the containing FileItem's references list: https://github.com/microsoft/vscode-references-view/blob/f1b526b39c97981c81d52a98d6e66f8f699dc38a/src/references/model.ts#L73
@atscott atscott changed the title vscode-references-view does not de-duplicate results the same way as built in ReferencesModel vscode-references-view does not de-duplicate results like the built in ReferencesModel Feb 19, 2021
atscott added a commit to atscott/vscode-references-view that referenced this issue Feb 19, 2021
@atscott
fix microsoft/vscode#117095 de-duplicate reference results.
a208a8d 
This commit de-duplicates reference location results in the same way that the built-in
'go to references'/peek references behavior does.

The built-in "go to references" command in vscode uses the `ReferencesModel`
to de-duplicate reference results: https://github.com/microsoft/vscode/blob/36dd567011eda09317a76d5dcb820fa9961eb786/src/vs/editor/contrib/gotoSymbol/referencesModel.ts#L171-L182
Equal uri's and ranges constitute duplicate results:
https://github.com/microsoft/vscode/blob/36dd567011eda09317a76d5dcb820fa9961eb786/src/vs/editor/contrib/gotoSymbol/referencesModel.ts#L293-L295

fix microsoft/vscode#117095
This was referenced Feb 19, 2021
Closed
Closed
@jrieken jrieken added this to the March 2021 milestone Feb 22, 2021
@atscott atscott mentioned this issue Feb 23, 2021
Merged
atscott added a commit to atscott/vscode that referenced this issue Feb 23, 2021
@atscott
fix microsoft#117095 de-duplicate location results
f5d9bdc 
This commit de-duplicates reference and other location results from
API endpoints `executeDefinitionProvider`, `executeDeclarationProvider`,
`executeImplementationProvider`, `executeTypeDefinitionProvider`, and `executeReferenceProvider`
(all commands registered in `goToSymbol.ts` which share the
`getLocationsLinks` helper).

Previously, this work was only done in the `ReferencesModel` which is used by
the `editor.action.findReferences` command. Extensions which called the
`vscode.executeReferenceProvider` would still get the duplicate results.

fix microsoft#117095
@atscott
Copy link
Contributor Author

atscott commented Mar 1, 2021

Hi @jrieken, I was wondering what the ETA for a fix for this would be (#117424 if approved). We're getting a lot of reports in the Angular Language Service extension repo for duplicate results in "find all references". If this will take some time to agree on a fix and get it into a release, I think we'll want to implement a workaround on our end for now. Thanks for your help!

atscott added a commit to atscott/angular that referenced this issue Mar 1, 2021
@atscott
fix(language-service): only provide template results on reference req…
77e8d03 
…uests

VSCode only de-duplicates references results for "go to references" requests
but does not de-duplicate them for "find all references" requests. The
result is that users see duplicate references for results in TypeScript
files - one from the built-in TS extension and one from us.
While this is an issue in VSCode (see microsoft/vscode#117095)
this commit provides a quick workaround on our end until it can be addressed there.

This commit should be reverted when microsoft/vscode/issues/117095 is resolved.

fixes angular/vscode-ng-language-service#1124
atscott added a commit to atscott/angular that referenced this issue Mar 1, 2021
@atscott
fix(language-service): only provide template results on reference req…
0419960 
…uests

VSCode only de-duplicates references results for "go to references" requests
but does not de-duplicate them for "find all references" requests. The
result is that users see duplicate references for results in TypeScript
files - one from the built-in TS extension and one from us.
While this is an issue in VSCode (see microsoft/vscode#117095)
this commit provides a quick workaround on our end until it can be addressed there.

This commit should be reverted when microsoft/vscode/issues/117095 is resolved.

fixes angular/vscode-ng-language-service#1124
@atscott atscott mentioned this issue Mar 1, 2021
Closed
14 tasks
@jrieken
Copy link
Member

jrieken commented Mar 2, 2021

Hi @jrieken, I was wondering what the ETA for a fix for this would be (#117424 if approved).

This is scheduled for March, meaning it will ship as stable early April and in insiders as soon as it is merged

@jrieken jrieken added feature-request Request for new features or functionality references-viewlet labels Mar 2, 2021
zarend pushed a commit to angular/angular that referenced this issue Mar 3, 2021
@atscott @zarend
fix(language-service): only provide template results on reference req…
ef87953 
…uests (#41041)

VSCode only de-duplicates references results for "go to references" requests
but does not de-duplicate them for "find all references" requests. The
result is that users see duplicate references for results in TypeScript
files - one from the built-in TS extension and one from us.
While this is an issue in VSCode (see microsoft/vscode#117095)
this commit provides a quick workaround on our end until it can be addressed there.

This commit should be reverted when microsoft/vscode/issues/117095 is resolved.

fixes angular/vscode-ng-language-service#1124

PR Close #41041
zarend pushed a commit to angular/angular that referenced this issue Mar 3, 2021
@atscott @zarend
fix(language-service): only provide template results on reference req…
10aa564 
…uests (#41041)

VSCode only de-duplicates references results for "go to references" requests
but does not de-duplicate them for "find all references" requests. The
result is that users see duplicate references for results in TypeScript
files - one from the built-in TS extension and one from us.
While this is an issue in VSCode (see microsoft/vscode#117095)
this commit provides a quick workaround on our end until it can be addressed there.

This commit should be reverted when microsoft/vscode/issues/117095 is resolved.

fixes angular/vscode-ng-language-service#1124

PR Close #41041
atscott added a commit to atscott/vscode that referenced this issue Mar 3, 2021
@atscott
fixup! fix microsoft#117095 de-duplicate location results
19b7444
jrieken added a commit that referenced this issue Mar 4, 2021
@jrieken
Merge pull request #117424 from atscott/dedupereferences
73c01a5 
fix #117095 de-duplicate reference results
@jrieken jrieken added author-verification-requested Issues potentially verifiable by issue author verification-needed Verification of issue is requested labels Mar 21, 2021
@RMacfarlane RMacfarlane added the verification-steps-needed Steps to verify are needed for verification label Mar 26, 2021
@RMacfarlane
Copy link
Contributor

@jrieken what's a good way to verify this?

@atscott
Copy link
Contributor Author

atscott commented Mar 26, 2021

Hi @RMacfarlane - I can verify this today. You'd need to know about an extension that provides reference results that would be duplicates of another extension and then run the "find all references" action from the built in Reference Search View extension (which does not de-duplicate results)

I can verify this with the angular language service by reverting angular/angular@10aa564

@atscott
Copy link
Contributor Author

atscott commented Mar 26, 2021

/verified

@RMacfarlane
Copy link
Contributor

Thanks @atscott!

@jrieken jrieken added the verified Verification succeeded label Mar 29, 2021
jrieken added a commit that referenced this issue Apr 6, 2021
@jrieken
better fix for #117095 and also fixes #119086
ac35a5b
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jrieken jrieken

Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders references-viewlet verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded
Projects
None yet
Milestone
March 2021
4 participants
@atscott @jrieken @RMacfarlane and others