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

Index core classes using RBS #2132

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jun 5, 2024

Step towards #1335

To test:

  • Disable the Sorbet extension and hover over a core class name, e.g. String.
  • Verify you see the docs in the hover
  • Use Go To Definition and verify it takes you to the RBS file with the String definitions.

After this we can move on to methods.

@andyw8 andyw8 force-pushed the andyw8/use-rbs-for-indexing-core-classes branch 2 times, most recently from eec49af to 8958b22 Compare June 6, 2024 18:50
lib/ruby_lsp/server.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 changed the title WIP: Index core classes using RBS Index core classes using RBS Jun 6, 2024
@andyw8 andyw8 force-pushed the andyw8/use-rbs-for-indexing-core-classes branch from 8958b22 to 4136b05 Compare June 6, 2024 18:52
@andyw8 andyw8 marked this pull request as ready for review June 6, 2024 18:52
@andyw8 andyw8 requested a review from a team as a code owner June 6, 2024 18:52
@andyw8 andyw8 requested review from st0012 and vinistock June 6, 2024 18:52
assert_equal(0, entry.location.start_column)
assert_operator(entry.location.end_column, :>, 0)

assert(@index.instance_variable_get(:@entries).key?("String"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(not really needed, will delete)

@andyw8 andyw8 added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Jun 6, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 6, 2024

(seeing CI failures for older Rubies, looking into...)

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Once this is merged, I have a branch that will introduce Object, Kernel, and BasicObject to the ancestors list.

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.

This can be handled in the next PR, but we need to extract includes, prepends and extends and insert those as well.

lib/ruby_indexer/lib/ruby_indexer/index_rbs.rb Outdated Show resolved Hide resolved
Comment on lines 27 to 29
declarations.each do |declaration|
process_declaration(declaration, pathname)
end
Copy link
Member

Choose a reason for hiding this comment

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

Should we inline this inside the loop? Is there a need for the process_signature method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I prefer it this way for readability.

lib/ruby_indexer/lib/ruby_indexer/index_rbs.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index_rbs.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/test/configuration_test.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/server.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index_rbs.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index_rbs.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/location.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/use-rbs-for-indexing-core-classes branch 2 times, most recently from 0f078ed to 8a3229a Compare June 7, 2024 17:47
@@ -20,6 +20,7 @@ Gem::Specification.new do |s|

s.add_dependency("language_server-protocol", "~> 3.17.0")
s.add_dependency("prism", ">= 0.29.0", "< 0.30")
s.add_dependency("rbs", ">= 3", "< 4")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified it on 3.0.0. With 2.0.0 it fails because "each_signature" isn't defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'm open to removing the upper constraint)

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the upper constraint. We can't guarantee that a major version won't break the LSP.

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ Gem::Specification.new do |s|

s.add_dependency("language_server-protocol", "~> 3.17.0")
s.add_dependency("prism", ">= 0.29.0", "< 0.30")
s.add_dependency("rbs", ">= 3", "< 4")
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the upper constraint. We can't guarantee that a major version won't break the LSP.

lib/ruby_lsp/server.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/use-rbs-for-indexing-core-classes branch from 4787ae6 to 2c07153 Compare June 7, 2024 19:44
@andyw8 andyw8 force-pushed the andyw8/use-rbs-for-indexing-core-classes branch from 2c07153 to e3cdd2b Compare June 7, 2024 19:45
@andyw8 andyw8 enabled auto-merge (squash) June 7, 2024 19:46
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.

Great work! 🚀

@andyw8 andyw8 merged commit ad2e42c into main Jun 7, 2024
35 checks passed
@andyw8 andyw8 deleted the andyw8/use-rbs-for-indexing-core-classes branch June 7, 2024 19:58
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 11, 2024

@soutaro I thought you may be interested to see the start of this work.

@hsw15192617273
Copy link

hi, jruby maybe not be able to install rbs.

❯ gem install rbs                                                                                                                                                                                                                        (base)
Building native extensions. This could take a while...
ERROR:  Error installing rbs:
	ERROR: Failed to build gem native extension.

    current directory: /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/gems/shared/gems/rbs-3.5.1/ext/rbs_extension
/foo/bar/.rbenv/versions/jruby-9.4.7.0/bin/jruby -I /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib extconf.rb
checking for whether -std=gnu99 is accepted as CFLAGS... *** extconf.rb failed ***
Could not create Makefile due to some reason, probably lack of necessary
libraries and/or headers.  Check the mkmf.log file for more details.  You may
need configuration options.

Provided configuration options:
	--with-opt-dir
	--without-opt-dir
	--with-opt-include
	--without-opt-include=${opt-dir}/include
	--with-opt-lib
	--without-opt-lib=${opt-dir}/lib
	--with-make-prog
	--without-make-prog
	--srcdir=.
	--curdir
	--ruby=/foo/bar/.rbenv/versions/jruby-9.4.7.0/bin/jruby
RuntimeError: The compiler failed to generate an executable file.
You have to install development tools first.

         try_do at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:456
    try_compile at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:571
    with_werror at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:522
    try_compile at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:571
     try_cflags at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:635
  append_cflags at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:641
   checking_for at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:942
       postpone at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:350
           open at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:320
       postpone at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:350
           open at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:320
       postpone at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:346
   checking_for at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:941
  append_cflags at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:640
           each at org/jruby/RubyArray.java:1981
  append_cflags at /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/stdlib/mkmf.rb:639
         <main> at extconf.rb:3

To see why this extension failed to compile, please check the mkmf.log which can be found here:

  /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/gems/shared/extensions/universal-java-17/3.1.0/rbs-3.5.1/mkmf.log

extconf failed, exit code 1

Gem files will remain installed in /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/gems/shared/gems/rbs-3.5.1 for inspection.
Results logged to /foo/bar/.rbenv/versions/jruby-9.4.7.0/lib/ruby/gems/shared/extensions/universal-java-17/3.1.0/rbs-3.5.1/gem_make.out

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 13, 2024

@hsw15192617273 we don't currently support JRuby, but feel free to note this on #1257

@vinistock
Copy link
Member

@andyw8 sorry, I don't think that's accurate. The Ruby LSP does work on JRuby, we just didn't add it to CI.

Is it possible to add a gemspec constraint to only use rbs on non JRuby platforms?

@st0012
Copy link
Member

st0012 commented Jun 13, 2024

Is it possible to add a gemspec constraint to only use rbs on non JRuby platforms?

I don't think it's possible

The Ruby LSP does work on JRuby, we just didn't add it to CI.

Wasn't it practically impossible because Sorbet can't run on it?

@vinistock
Copy link
Member

The issue blocking JRuby was solved November of last year #1263. My understanding is that it has been working since then.

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 13, 2024

@@hsw15192617273 was Ruby LSP working for you previously? (i.e. before RBS was introduced).

@hsw15192617273
Copy link

hsw15192617273 commented Jun 13, 2024

@@hsw15192617273 was Ruby LSP working for you previously? (i.e. before RBS was introduced).

yeah, if install ruby-lsp 0.16.7, it's working fine. (I started using JRuby today)

@hsw15192617273
Copy link

but i not use sorbet in ruby code. just run ruby-lsp for vscode extension. JRuby install sorbet maybe not raise exception, I will confirm it tomorrow.

@soutaro
Copy link
Contributor

soutaro commented Jun 14, 2024

I planned to implement a pure Ruby RBS parser to make it compatible to other Ruby implementations, but not finished yet...

@vinistock
Copy link
Member

@andyw8 for the time being, to unblock our JRuby friends, let's remove the gemspec dependency from RBS and check for the presence of the gem instead.

begin
  gem("rbs", ">= 3", "< 4")
rescue LoadError
  return
end

module RubyLsp
  class RBSIndexer
    # ...

And then where we use it

index_rbs if defined?(RubyLsp::RBSIndexer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants