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

FIX: make sure to specify sorbet-runtime version #1027

Merged
merged 1 commit into from
Sep 21, 2023
Merged

FIX: make sure to specify sorbet-runtime version #1027

merged 1 commit into from
Sep 21, 2023

Conversation

rreckonerr
Copy link
Contributor

Motivation

I ran into an issue when older version of sorbet-runtime in my project's Gemfile.lock caused an error described here

Implementation

The error stack trace pointed to sorbet-runtime

/Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/lib/core_ext/uri.rb:10:in `from_path': wrong number of arguments (given 1, expected 0; required keyword: path) (ArgumentError)
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/sorbet-runtime-0.5.5510/lib/types/private/methods/_methods.rb:240:in `call'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/sorbet-runtime-0.5.5510/lib/types/private/methods/_methods.rb:240:in `block in _on_method_added'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/lib/ruby_lsp/utils.rb:9:in `<module:RubyLsp>'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/lib/ruby_lsp/utils.rb:4:in `<top (required)>'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/lib/ruby_lsp/internal.rb:16:in `require'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/lib/ruby_lsp/internal.rb:16:in `<top (required)>'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/exe/ruby-lsp:80:in `require_relative'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/gems/ruby-lsp-0.9.4/exe/ruby-lsp:80:in `<top (required)>'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/bin/ruby-lsp:25:in `load'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/bin/ruby-lsp:25:in `<main>'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/bin/ruby_executable_hooks:22:in `eval'
	from /Users/__redacted__/.rvm/gems/ruby-3.1.2/bin/ruby_executable_hooks:22:in `<main>'
[Error - 10:46:20] Server initialization failed.
  Message: Pending response rejected since connection got disposed
  Code: -32097 
[Info  - 10:46:20] Connection to server got closed. Server will restart.
true
[Error - 10:46:20] Ruby LSP client: couldn't create connection to server.
  Message: Pending response rejected since connection got disposed
  Code: -32097

Automated Tests

I believe there are no tests covering this

Manual Tests

You should have a Gemfile.lock with older sorbet-runtime version specified. In my case it was 0.5.5510. Manually upgrading it to 0.5.11010 solved the issue for me. I see multiple versions specified in your Gemfile.lock, so this commit upgrades it to the latest one specified

@rreckonerr rreckonerr requested a review from a team as a code owner September 19, 2023 21:51
@rreckonerr rreckonerr force-pushed the fix/set-min-sorbet-runtime-version branch from f9e91ec to f2112c3 Compare September 19, 2023 21:52
@rreckonerr rreckonerr mentioned this pull request Sep 19, 2023
Closed
6 tasks
@rreckonerr rreckonerr force-pushed the fix/set-min-sorbet-runtime-version branch from f2112c3 to 8fe6190 Compare September 19, 2023 22:06
KaanOzkan
KaanOzkan reviewed Sep 20, 2023
View reviewed changes
ruby-lsp.gemspec Outdated
@@ -18,7 +18,7 @@ Gem::Specification.new do |s|
s.require_paths = ["lib"]

s.add_dependency("language_server-protocol", "~> 3.17.0")
s.add_dependency("sorbet-runtime")
s.add_dependency("sorbet-runtime", "~> 0.5.11026")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about going this route and if we did, we'd have to specify a minimum version (>) and we'd have to find an earlier version that fixed this change. I took a quick look through issues on Sorbet repo but nothing jumped out.

Also you were using sorbet-runtime of 3 and a half years ago so a minimum version can possibly cause more annoyance to users than good. I assume not many people will run into this problem.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we should, at worst, have a minimum version as @KaanOzkan states above, but one that is not super recent so that folks are inconvenienced by it.

@vinistock
Copy link
Member

We definitely don't want to lock the LSP into a specific sorbet-runtime version.

I don't fully understand the issue. Why would an old sorbet-runtime cause that ArgumentError?

The URI::Generic.from_path is created by us, it's not a patch to an existing method. Did Sorbet have a bug handling arguments?

@rreckonerr
Copy link
Contributor Author

rreckonerr commented Sep 20, 2023 •
edited
Loading

The last sorbet-runtime version that breaks vscode-ruby-lsp is 0.5.5676, next version 0.5.5685 works. Here's the comparison between the two versions, but I don't see what exactly causes the error. See rubygems.org for the versions release sequences.

0.5.5676
image

0.5.5685
image

@rreckonerr
FIX: make sure to specify sorbet-runtime version
635a122 
older versions, e.g 0.5.5510 result in
"Server initialization failed" error

Refs: https://github.com/Shopify/vscode-ruby-lsp/issues/778
@rreckonerr rreckonerr force-pushed the fix/set-min-sorbet-runtime-version branch from 8fe6190 to 635a122 Compare September 20, 2023 20:01
vinistock
vinistock approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

I'm still unsure as to why the bug happens, but given that version 0.5.5685 is quite old, I think we can move forward

@vinistock vinistock enabled auto-merge (squash) September 21, 2023 15:40
andyw8
andyw8 reviewed Sep 21, 2023
View reviewed changes
Gemfile.lock
@@ -3,7 +3,7 @@ PATH
specs:
ruby-lsp (0.11.0)
language_server-protocol (~> 3.17.0)
sorbet-runtime
sorbet-runtime (>= 0.5.5685)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for reference: https://rubygems.org/gems/sorbet-runtime/versions/0.5.5685

@vinistock vinistock merged commit f8b16ef into Shopify:main Sep 21, 2023
14 of 15 checks passed
@rreckonerr rreckonerr deleted the fix/set-min-sorbet-runtime-version branch September 21, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KaanOzkan KaanOzkan KaanOzkan left review comments

@paracycle paracycle paracycle left review comments

@vinistock vinistock vinistock approved these changes

@andyw8 andyw8 andyw8 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rreckonerr @vinistock @andyw8 @paracycle @KaanOzkan