Support Sorbet typed: false files for completion requests
#1245
Conversation
Co-authored-by: Joshua Scharf <[email protected]>
Co-authored-by: Joshua Scharf <[email protected]>
|
I have signed the CLA! |
andyw8
changed the title
lib/ruby_lsp/requests/completion.rb
Outdated
| @@ -63,7 +65,7 @@ def on_string_node_enter(node) | |||
| # Handle completion on regular constant references (e.g. `Bar`) | |||
| sig { params(node: Prism::ConstantReadNode).void } | |||
| def on_constant_read_node_enter(node) | |||
| return if DependencyDetector.instance.typechecker | |||
| return if @typechecker_enabled | |||
There was a problem hiding this comment.
Seeing these changes for constants made me wonder why Sorbet wasn't providing hover or completion for them on typed: false files. It can accurately report diagnostics for them, so I thought it should just work.
After talking to the team, hover now works for constants in typed: false files and I opened a PR for completion. Definition already worked for constants.
If the PR is approved, we can maintain the same check we had before, since Sorbet will always be able to handle completion for constants.
There was a problem hiding this comment.
Oh nice yeah I mean if sorbet can cover this then that works! I had sorta considered that but it was way easier for me to understand how to change this code base.
Will we get 1:1 support for typed: false from the sorbet LS as this one though?
I know constants will be covered but I think you all are working on more support for methods in here as well right? Still seems like it could be valuable to know at the document level what's going on with typechecking.
Obviously this is kind of an edge case - It's on my roadmap to go get us past typed: false but it may take some time still. We need to dedicate some time to supporting that push properly and have a few other priorities before we can take the time to do that. We kinda yolo'd into it on first pass and it was messy / got in the way of devs quite a bit so we had to regress for a time. Hence why we have sorbet installed, but are largely typed: false for now.
Anywhoo context aside I still think this could be valuable but I'm also happy to wait for a concrete use-case if the current one gets covered by sorbet!
There was a problem hiding this comment.
Yeah, this change still makes perfect sense for methods. Sorbet does not even attempt to typecheck typed: false files, so we can definitely fallback method hover, completion, signature help and definition to the Ruby LSP.
For constants though, we can fully delegate to Sorbet. It's always able to discover and handle constants properly (meta-programming excluded of course).
So let's just revert the changes to the early returns for constant reads and constant path reads and leave it only for call nodes. Then we should be good to go.
There was a problem hiding this comment.
Right yeah that makes sense. I just pushed those changes up now 👍
andyw8
changed the title
typed: false files for completion requests
Motivation
related issue #1240
This is makes it so that the LS will still return completion results in
typed: falsefiles. In these files sorbet provides no support.We also still intend to address this in some other requests like hover and definition but figured we'd just start with this one. More PRs coming soon for those!
Credit also goes to @jscharf who paired on this with me :)
Implementation
Pretty much just followed the directions given here
Automated Tests
Yep!
Manual Tests
Tested these changes on our internal project which has many typed: false files and it all works as expected!