[Tests] Add coverage for ReferencesContext.includeDeclaration#2644
[Tests] Add coverage for ReferencesContext.includeDeclaration#2644YooGyeongMo wants to merge 3 commits into
Conversation
References should return use sites, not the definition itself. The textDocument/references request was including the symbol's own definition, adding noise when navigating from a definition. This change removes .definition from the roles set when includeDeclaration is true. The existing testReferencesInMacro test was also updated to apply the same principle consistently across the macro case. Fixes swiftlang#2638
YooGyeongMo
requested review from
ahoppen,
hamishknight and
rintaro
as code owners
| [ | ||
| Location(uri: project.fileURI, range: Range(project.positions["2️⃣"])), | ||
| Location(uri: project.fileURI, range: Range(project.positions["3️⃣"])), | ||
| ] |
There was a problem hiding this comment.
If I understood the LSP specification correctly, the declarartion should be part of this response if and only if includeDeclaration is true. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_references
You could check if this is already the behavior (write tests if not yet covered). Maybe the VSCode extension is sending the wrong request with includeDeclaration: true.
Revert the earlier behavior change and add regression test coverage for both modes of `includeDeclaration`. sourcekit-lsp already handles this per the LSP spec. These tests pin the behavior so it cannot regress. Related to swiftlang#2638
|
The changes in your PR do not seem to match your PR description. In particular, it currently only contains test changes. Could you please make sure that they match? |
YooGyeongMo
changed the title
|
Hi @ahoppen @tothambrus11 , Thanks for the feedback. Apologies for the mismatch — I've updated the description to match the current changes: test coverage only, with no server behavior change. For context on how I arrived here: I initially tried removing
The Based on this, SourceKit-LSP appears to already be following the LSP behavior for Does this direction make sense to you? |
|
@ahoppen Thanks for the update in #2638 I saw that this now appears to be related to the VS Code extension sending Given that, would it still be useful to keep this PR as test-only coverage for the existing |
|
Yeah, a test would be good here but one of the tests you added covers the default behavior, which should already be tested by other tests. Could you reduce this to only add a test that covers new behavior? |
|
Done, thanks @ahoppen ! I removed the redundant The Please let me know if this looks good or if anything else is needed. |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
3 participants
Description
Following the discussion in #2638, SourceKit-LSP's
textDocument/referencesbehavior appears to already comply with the LSP specification. WhenincludeDeclarationistrue, the declaration is included. WhenincludeDeclarationisfalse, the declaration is excluded.The behavior reported in #2638, where the definition appears in "Find All References" results in VS Code, appears to be caused by the VS Code Swift extension sending
includeDeclaration: truefor that command, rather than by a server-side bug (verified here).This PR reverts the earlier behavior change and instead adds test coverage for the
includeDeclaration: falsecase, which wasn't previously covered. TheincludeDeclaration: truecase is already covered by existing teststestReferencesInMacro,CopyDestinationTests,IndexTests,SwiftPMIntegrationTests.Tests added
testReferencesWithoutDeclaration: verifies thatincludeDeclaration: falsereturns only use sitesThe test passes against the current behavior.
Note on the original issue
The UX concern, where the definition appears in the results, is best addressed on the client side. For example, the VS Code Swift extension could send
includeDeclaration: falsefor "Find All References". I would be happy to follow up onvscode-swiftafter this PR is reviewed.Related to #2638