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.

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

[LSP] Add handler for workspace/symbol requests [#4619] #4620

Merged

Conversation

dschoepe
Copy link
Collaborator

@dschoepe dschoepe commented Oct 5, 2023

Description

This implements a response for workspace/symbol queries as documented here:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_symbol

This allows jumping to symbols by name across a workspace (e.g. in VSCode, typing #foo in the navigation prompt allows jumping to an identifier containing foo as a substring).

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

How has this been tested

  • Added unit tests in DafnyLanguageServer.Test.
  • Manually tested on some projects.

Addresses #4619

@dschoepe dschoepe force-pushed the support-workspace-symbol-query branch from 1a36c16 to 910a427 Compare October 5, 2023 18:29
@dschoepe dschoepe force-pushed the support-workspace-symbol-query branch from 910a427 to 1dc7ed5 Compare October 6, 2023 09:44
@dschoepe dschoepe marked this pull request as draft October 6, 2023 10:03
This implements a response for workspace/symbol queries
as documented here:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_symbol

This allows jumping to symbols by name across a workspace
(e.g. in VSCode, typing `#foo` in the navigation prompt allows jumping to an identifier containing
foo as a substring).

By submitting this pull request, I confirm that my contribution is made
under the terms of the MIT license.
@dschoepe dschoepe force-pushed the support-workspace-symbol-query branch from 1dc7ed5 to ebeaf57 Compare October 6, 2023 10:12
@dschoepe dschoepe marked this pull request as ready for review October 6, 2023 12:03
@@ -46,6 +46,18 @@
<None Update="Lookup\TestFiles\find-refs-b.dfy">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Update="Lookup\TestFiles\defines-foo.dfy">
Copy link
Member

@keyboardDrummer keyboardDrummer Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is for the test files to be inline in the tests. This is similar to what Roslyn's (C# compiler) tests look like. I don't have a good argument for one or the other though, so feel free to ignore this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was followed this approach based on ReferencesTest but I'm fine either way. I'll update the PR and inline them instead.

Copy link
Member

@keyboardDrummer keyboardDrummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, especially the tests, thanks a lot !

public override async Task<Container<SymbolInformation>?> Handle(WorkspaceSymbolParams request, CancellationToken cancellationToken) {
var queryText = request.Query.ToLower();
Dictionary<SymbolInformation, int> result = new Dictionary<SymbolInformation, int>();
// Using a LINQ traversal here would be more readable, but to be as responsive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code makes it clear that LINQ can't be used, so IMO the comment is unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll remove that.

// as it includes things like keywords and the signature of the
// the definitions. However, extracting just the name from
// an ISymbol requires explicit case splitting for each definition
var description = def.GetDescription(dafnyOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't def.NameToken.val give you the name?

Also, this doesn't match on qualified names, does it? (could be a future feature)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I must have missed that. I'll change it. Good callout about the qualified names. I'll leave a note here about it.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) October 6, 2023 14:18
keyboardDrummer
keyboardDrummer previously approved these changes Oct 9, 2023
@keyboardDrummer keyboardDrummer merged commit cc2d154 into dafny-lang:master Oct 9, 2023
18 checks passed
robin-aws pushed a commit to robin-aws/dafny that referenced this pull request Oct 11, 2023
…fny-lang#4620)

### Description

This implements a response for workspace/symbol queries as documented
here:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_symbol

This allows jumping to symbols by name across a workspace (e.g. in
VSCode, typing `#foo` in the navigation prompt allows jumping to an
identifier containing foo as a substring).

By submitting this pull request, I confirm that my contribution is made
under the terms of the MIT license.

### How has this been tested
- Added unit tests in DafnyLanguageServer.Test.
- Manually tested on some projects.

Addresses [dafny-lang#4619](dafny-lang#4619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants