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

'typecheck' command fails on Ruby 2.7.1 / Solargraph 0.39.13 #350

Closed
gj opened this issue Aug 6, 2020 · 6 comments
Closed

'typecheck' command fails on Ruby 2.7.1 / Solargraph 0.39.13 #350

gj opened this issue Aug 6, 2020 · 6 comments

Comments

@gj
Copy link

gj commented Aug 6, 2020

Apologies if this has been asked-and-answered. I looked through issues and found a few related to Ruby 2.7 that seem to have been addressed and a few others with similar-looking errors (#329 / #337) that seem to have been addressed as well.

Problem

Running bundle exec solargraph typecheck on Ruby 2.4.10 works perfectly fine. Running it on 2.7.1 does not.

Versions

  • Ruby 2.7.1
  • Solargraph 0.39.13

Expected

❯ bundle exec solargraph typecheck
0 problems found in 0 of 18 files.

Actual

❯ bundle exec solargraph typecheck
bundler: failed to load command: solargraph (/home/gj/.asdf/installs/ruby/2.7.1/bin/solargraph)
NoMethodError: undefined method `each' for nil:NilClass
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/parser/rubyvm/node_methods.rb:254:in `reduce_to_value_nodes'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/parser/rubyvm/node_methods.rb:201:in `block in get_return_nodes'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/parser/rubyvm/node_methods.rb:200:in `each'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/parser/rubyvm/node_methods.rb:200:in `get_return_nodes'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/parser/rubyvm/node_methods.rb:58:in `returns_from'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/pin/base_variable.rb:42:in `probe'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/type_checker.rb:176:in `block in variable_type_tag_problems'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/type_checker.rb:148:in `each'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/type_checker.rb:148:in `variable_type_tag_problems'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/type_checker.rb:44:in `problems'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/shell.rb:148:in `block in typecheck'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/shell.rb:146:in `each'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/lib/solargraph/shell.rb:146:in `typecheck'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/thor-1.0.1/lib/thor/command.rb:27:in `run'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/thor-1.0.1/lib/thor/invocation.rb:127:in `invoke_command'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/thor-1.0.1/lib/thor.rb:392:in `dispatch'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/thor-1.0.1/lib/thor/base.rb:485:in `start'
  /home/gj/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/solargraph-0.39.13/bin/solargraph:5:in `<top (required)>'
  /home/gj/.asdf/installs/ruby/2.7.1/bin/solargraph:23:in `load'
  /home/gj/.asdf/installs/ruby/2.7.1/bin/solargraph:23:in `<top (required)>'

Code that blows up

node.children.each do |cc|
result.concat reduce_to_value_nodes(cc.children[1..-1])
end

Tiny patch that makes it not blow up

node.children.each do |cc|
-  result.concat reduce_to_value_nodes(cc.children[1..-1])
+  result.concat reduce_to_value_nodes(cc.children[1..-1] || [])
end
@castwide
Copy link
Owner

castwide commented Aug 8, 2020

Thanks for the info. I'm working on a fix, but I haven't been able to reproduce the error yet. I'd like to pinpoint the root cause before we patch it.

It looks like the problem occurs when Solargraph tries to infer the return value of a case statement. Since it happens in Ruby 2.7, I thought it might be due to a hole in support pattern matching.

One thing that might help is if you run solargraph scan -v. With any luck, it'll tell you the specific method in your code where the static analysis fails.

@gj
Copy link
Author

gj commented Aug 9, 2020

Thanks for the info. I'm working on a fix, but I haven't been able to reproduce the error yet. I'd like to pinpoint the root cause before we patch it.

Reproduction steps:

  1. Clone https://github.com/osohq/oso
  2. cd languages/ruby
  3. bundle
  4. bundle exec solargraph typecheck

One thing that might help is if you run solargraph scan -v. With any luck, it'll tell you the specific method in your code where the static analysis fails.

Output doesn't include any obvious errors (though, to be fair, I'm not entirely sure what I'm looking for 🙂) and ends with Scanned /path/to/project (4206 pins) in 2.109445258975029 seconds.

@castwide
Copy link
Owner

Thanks. I was able to reproduce the error using that repo. It happens when Solargraph tries to infer a return value from this case statement: https://github.com/osohq/oso/blob/28c7e0a0a8a0bd257d9b92dadb0be94241a5af87/languages/ruby/lib/oso/polar/host.rb#L189

This is the minimal reproducible example I made in a scratch project:

def foo
  value = case true
  when value
    true
  else
    false
  end
end

It looks like it only happens when the case condition is a literal boolean, i.e., true or false. I should be able to fix it in the next patch release.

Output doesn't include any obvious errors (though, to be fair, I'm not entirely sure what I'm looking for 🙂) and ends with Scanned /path/to/project (4206 pins) in 2.109445258975029 seconds.

Yeah, the scan succeeded. It didn't hit that error because it didn't try to infer the value of the local variable being assigned from the case statement.

@gj
Copy link
Author

gj commented Aug 10, 2020

It looks like it only happens when the case condition is a literal boolean, i.e., true or false. I should be able to fix it in the next patch release.

Awesome, thanks for your work on this project!

@castwide
Copy link
Owner

Released in 0.39.14.

@gj
Copy link
Author

gj commented Aug 13, 2020

0.39.14 working like a charm on Ruby 2.7.1 🥳

@gj gj closed this as completed Aug 13, 2020
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

No branches or pull requests

2 participants