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

[READY] Add support for didChangeWorkspaceFolders #1731

Merged
merged 7 commits into from
Feb 17, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Jan 21, 2024

Perhaps it's finally time to add this capability...

There are a couple missing pieces:

  1. So far, this does not take into account the extra confs.
  2. Clangd completer does not have GetProjectRootFiles() implemented, but also clangd does not want to listen to this notification. I guess it knows better than us?
  3. Java completer also does not have GetProjectRootFiles() implemented, but actually wants this notification.
  4. Do we need _project_directory and _server_workspace_dirs?
  5. Tests... after we figure out the answers to the above!

This change is Reviewable

Copy link

codecov bot commented Jan 21, 2024

Codecov Report

Merging #1731 (3de31d7) into master (f11e293) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1731      +/-   ##
==========================================
+ Coverage   95.43%   95.52%   +0.09%     
==========================================
  Files          83       83              
  Lines        8168     8185      +17     
  Branches      163      163              
==========================================
+ Hits         7795     7819      +24     
+ Misses        323      316       -7     
  Partials       50       50              

@bstaletic
Copy link
Collaborator Author

This pull request actually allows rust-analyzer to report diagnostics for two projects opened in the same vim instance. For example:

  1. ycmd/tests/rust/testdata/common
  2. ycmd/tests/rust/testdata/macro

@bstaletic bstaletic force-pushed the workspace-dirs branch 8 times, most recently from b659707 to 6c1457d Compare January 28, 2024 07:15
@bstaletic
Copy link
Collaborator Author

Rebased.

This pull request is behaving correctly for rust, go and java. C++ server does not support this notification and we respect that.

Other than that, I have tried it with haskell and it did what I expected, but haskell server does not really do anything with the notification.

@bstaletic bstaletic changed the title [RFC] Add support for didChangeWorkspaceFolders [READY] Add support for didChangeWorkspaceFolders Feb 14, 2024
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Looks good!

Answers

  1. extra cones... I think you added this?
  2. clangd does its own copile_commands.json scanning and does a good job of associating files with dbs, let's leave them to it!
  3. java completer is special, and I can see that you've handled it like the special sausage that it is
  4. I think we keep both. The project root is still special in that it's the rootUri in the init and it's the CWD of the server
  5. I would like to see some tests for the other engines if we can. The java one was interesting that it shows the server is bonkers and seems to duplicate the messages :(

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/completers/java/java_completer.py line 421 at r1 (raw file):

  def _OpenProject( self, request_data, args ):

I wonder if we can just ditch this now? I doubt anyone uses it. I added it for a specific purpose to work around this limitation, but TBH YcmCompleter RestartServer works just as well, and this is just a bit of a hack on top of that.

I vote remove. WDYT?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Right, I should have updated the opening comment.

extra cones... I think you added this?

Extra confs can report statically known workspaces. I am unsure if we will need something like extra_conf.GetWorkspaceForFile(file). I am happy to defer that to a point after we get a lot more field experience and thus know whether we need that or not.

clangd

Agreed.

java [snip] special sausage

Loved that!

I would like to see some tests for the other engines if we can.

Originally, I tried to write a test for rust. But it tends to publish one set of diagnostics, change its mind, then publish another set of diagnostics. But not always. Our tests for the diagnostic poll don't deal well with that, so I gave up on testing with rust.

I can try with go...

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)


ycmd/completers/java/java_completer.py line 421 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I wonder if we can just ditch this now? I doubt anyone uses it. I added it for a specific purpose to work around this limitation, but TBH YcmCompleter RestartServer works just as well, and this is just a bit of a hack on top of that.

I vote remove. WDYT?

I initially stayed away from that because it is technically a breaking change. I can remove it, sure.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Ha! Gopls works great!

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)


ycmd/completers/java/java_completer.py line 421 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I initially stayed away from that because it is technically a breaking change. I can remove it, sure.

Removed.

Perhaps it's finally time to add this capability...

There are a couple missing pieces:

1. We're being naughty and not respecting that some servers do not
   support didChangeWorkspaceFolders - like clangd.
2. Tests... some are present, but maybe should be tested more
   thoroughly.
3. Extra confs are tough to figure out. Currently the only thing planned
   to be implemented is specifying up-front which workspaces should be
   opened.
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Okay, rust test is there too, although it needed a bit of secret jank sauce.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

@bstaletic bstaletic force-pushed the workspace-dirs branch 3 times, most recently from 0b3e46a to 3de31d7 Compare February 15, 2024 10:25
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Wiped off the jank sauce. 100% diff coverage hit.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 9 of 12 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Feb 17, 2024
Copy link
Contributor

mergify bot commented Feb 17, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 0ce532e into ycm-core:master Feb 17, 2024
17 of 18 checks passed
@bstaletic bstaletic deleted the workspace-dirs branch February 17, 2024 12:30
bstaletic added a commit to ycm-core/YouCompleteMe that referenced this pull request Feb 18, 2024
Changes since the last update:

ycm-core/ycmd#1728 YcmShowDetailedDiagnostic should now match what is echoed on the command line.
ycm-core/ycmd#1731 Add support for `workspace/didChangeWorkspaceFolders` LSP notification.
ycm-core/ycmd#1730 LSP servers are now updated with latest server state afer reset
ycm-core/ycmd#1727 Update JDT to version 1.31.0
ycm-core/ycmd#1726 C# symbol search filtering fix
ycm-core/ycmd#1724 C# GoToDocumentOutline consistency patch
ycm-core/ycmd#1723 Implement GoToDocumentOutline in C#
ycm-core/ycmd#1716 Display tsserver tags in detailed info and GetDoc

`workspace/didChangeWorkspaceFolders` seems to be working fine for java,
rust and go, but should be considered experimental. Definitely more
field experience is needed.
What it should do is allow LSP servers to understand multiple projects
in the same vim instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants