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

Bundle did_you_mean gem #3480

Closed
headius opened this Issue Nov 20, 2015 · 23 comments

Comments

Projects
None yet
4 participants
@headius
Member

headius commented Nov 20, 2015

This is from https://bugs.ruby-lang.org/issues/11252. See #3479 for full Ruby 2.3 checklist.

The did_you_mean gem uses TracePoint to intercept NameError (I think) and report a more useful message, guessing at what error you might have meant.

JRuby implements TracePoint, but most tracing is disabled by default for performance. Exception tracing might be ok, though...we need to confirm. And we may need to modify the "tracing is disabled" warning when the requested trace is 100% ok (if that's the case for all tracing did_you_mean uses).

I started the process by adding the beta3 version of the gem to lib/pom.rb, but it didn't seem to be able to find it via our proxies. Perhaps the proxy doesn't support beta gems?

diff --git a/lib/pom.rb b/lib/pom.rb
index b9a110a..03202f6 100644
--- a/lib/pom.rb
+++ b/lib/pom.rb
@@ -20,7 +20,8 @@ default_gems =
    ImportedGem.new( 'psych', '2.0.15' ),
    ImportedGem.new( 'json', '${json.version}' ),
    ImportedGem.new( 'jar-dependencies', '${jar-dependencies.version}' ),
-   ImportedGem.new( 'racc', '${racc.version}')
+   ImportedGem.new( 'racc', '${racc.version}' ),
+   ImportedGem.new( 'did_you_mean', '1.0.0.beta3' )
   ]

 project 'JRuby Lib Setup' do

@headius headius added this to the JRuby 9.1.0.0 milestone Nov 20, 2015

@headius headius referenced this issue Nov 20, 2015

Closed

Ruby 2.3 support #3479

50 of 58 tasks complete

kares added a commit to kares/jruby that referenced this issue Jan 29, 2016

prepare for bundling did_you_mean gem (#3480)
need to disable Ruby version validation at RubyGems due :
```
Gem::InstallError: did_you_mean requires Ruby version >= 2.3.0
```

@headius headius removed this from the JRuby 9.1.0.0 milestone Apr 15, 2016

@headius headius referenced this issue Apr 20, 2016

Open

Remaining 2.3 checklist items not in JRuby 9.1 #3816

8 of 23 tasks complete
@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Apr 27, 2016

Contributor

The creator of the did_you_mean gem here. I've started working on this to help make this happen. TracePoint is actually no longer necessary, so it shouldn't be too hard to make it work on JRuby. Also, I've found a bug in NameError#receiver which is breaking some of the did_you_mean features. I'm working on a fix now and will send a pull request shortly.

Contributor

yuki24 commented Apr 27, 2016

The creator of the did_you_mean gem here. I've started working on this to help make this happen. TracePoint is actually no longer necessary, so it shouldn't be too hard to make it work on JRuby. Also, I've found a bug in NameError#receiver which is breaking some of the did_you_mean features. I'm working on a fix now and will send a pull request shortly.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 27, 2016

Member

@yuki24 great. Thanks for the update!

Member

enebo commented Apr 27, 2016

@yuki24 great. Thanks for the update!

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Apr 27, 2016

Contributor

Ok, now the only change we need to add to JRuby is this change: https://bugs.ruby-lang.org/issues/11777. It's a little weird change as this is a special case just for the did_you_mean gem. I'm not actually sure if it makes sense to introduce this change to JRuby as it breaks the consistency of the behavior of the #local_variables method.

@headius @enebo WDYT?

Contributor

yuki24 commented Apr 27, 2016

Ok, now the only change we need to add to JRuby is this change: https://bugs.ruby-lang.org/issues/11777. It's a little weird change as this is a special case just for the did_you_mean gem. I'm not actually sure if it makes sense to introduce this change to JRuby as it breaks the consistency of the behavior of the #local_variables method.

@headius @enebo WDYT?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 27, 2016

Member

@yuki24 We are not in a position to make that change right now, since it would require significant rework of how we do method invocation. Method call logic does not have access to the local state of the caller, in order to allow us to optimize the caller independent of the call.

I notice matz wanted it called "internal" but these things have a way of becoming official APIs.

We won't be making any changes for this in 9.1. It may be something we can add afterwards, since we have already been forced to provide more caller data to support refinements. The structures here are basically the same.

Member

headius commented Apr 27, 2016

@yuki24 We are not in a position to make that change right now, since it would require significant rework of how we do method invocation. Method call logic does not have access to the local state of the caller, in order to allow us to optimize the caller independent of the call.

I notice matz wanted it called "internal" but these things have a way of becoming official APIs.

We won't be making any changes for this in 9.1. It may be something we can add afterwards, since we have already been forced to provide more caller data to support refinements. The structures here are basically the same.

@headius headius added this to the JRuby 9.1.1.0 milestone Apr 27, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 27, 2016

Member

#3816 is the ongoing bug for missing 2.3 features, including did_you_mean.

Member

headius commented Apr 27, 2016

#3816 is the ongoing bug for missing 2.3 features, including did_you_mean.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 27, 2016

Member

@yuki24 we do have a path for adding this information so don't be discouraged. I think @headius saying 'may' should be changed to 'will'. It just depends on how long it takes for us to get to this work. Informally we came up with some ways to do this but none of them are a small amount of work.

Member

enebo commented Apr 27, 2016

@yuki24 we do have a path for adding this information so don't be discouraged. I think @headius saying 'may' should be changed to 'will'. It just depends on how long it takes for us to get to this work. Informally we came up with some ways to do this but none of them are a small amount of work.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Apr 28, 2016

Contributor

I'm actually ok with giving up on that particular feature. It's the only feature that is not enabled yet and everything else is green: https://travis-ci.org/yuki24/did_you_mean/jobs/126229792#L311-L321

I don't think we should treat it as a blocker. We can just disable it for the time being, and when JRuby is ready we can just turn it back on.

Contributor

yuki24 commented Apr 28, 2016

I'm actually ok with giving up on that particular feature. It's the only feature that is not enabled yet and everything else is green: https://travis-ci.org/yuki24/did_you_mean/jobs/126229792#L311-L321

I don't think we should treat it as a blocker. We can just disable it for the time being, and when JRuby is ready we can just turn it back on.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 2, 2016

Member

@yuki24 Due to other issues (we don't like how DYM gets loaded by RubyGems right now) we will do this in 9.1.1.0. Thank you for your help getting DYM working on JRuby!

Member

headius commented May 2, 2016

@yuki24 Due to other issues (we don't like how DYM gets loaded by RubyGems right now) we will do this in 9.1.1.0. Thank you for your help getting DYM working on JRuby!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 2, 2016

Member

Oh FWIW, we still have the logic to load DYM on startup in JRuby 9.1, but it is disabled by default. If you are a user who really wants DYM on by default in your environment, you'll need to install it manually and add --enable:did_you_mean to JRUBY_OPTS or RUBYOPT.

Member

headius commented May 2, 2016

Oh FWIW, we still have the logic to load DYM on startup in JRuby 9.1, but it is disabled by default. If you are a user who really wants DYM on by default in your environment, you'll need to install it manually and add --enable:did_you_mean to JRUBY_OPTS or RUBYOPT.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 May 9, 2016

Contributor

@headius Thank you for the update! So is it safe to assume JRuby 9.1.1.0 will come with this change?

Contributor

yuki24 commented May 9, 2016

@headius Thank you for the update! So is it safe to assume JRuby 9.1.1.0 will come with this change?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

@yuki24 We might be able to enable it for very limited use (i.e. interpreter-only) but I don't think any NameError#local_variables feature will make it into 9.1.1.0.

The problem is that we don't always have access to the local variable names for an arbitrary executing method. In many cases we can optimize Ruby code down to straightforward JVM bytecode without any Ruby runtime structures required to execute.

I may spike something this week that just embeds the variable names into each call site.

The other problem with us including DYM into 9.1.1.0 is #3828...loading DYM on every boot as a normal gem causes RubyGems to initialize many internal structures before we have an opportunity to tweak them for JRuby's more exotic deployment modes. This needs to be addressed in RubyGems, most likely.

Member

headius commented May 9, 2016

@yuki24 We might be able to enable it for very limited use (i.e. interpreter-only) but I don't think any NameError#local_variables feature will make it into 9.1.1.0.

The problem is that we don't always have access to the local variable names for an arbitrary executing method. In many cases we can optimize Ruby code down to straightforward JVM bytecode without any Ruby runtime structures required to execute.

I may spike something this week that just embeds the variable names into each call site.

The other problem with us including DYM into 9.1.1.0 is #3828...loading DYM on every boot as a normal gem causes RubyGems to initialize many internal structures before we have an opportunity to tweak them for JRuby's more exotic deployment modes. This needs to be addressed in RubyGems, most likely.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 May 10, 2016

Contributor

@headius Thank you for the clarification! I'll just kill the local variable name correction on JRuby for the time being.

Does the use of --enable:did_you_mean (or JRUBY_OPTS or RUBYOPT) cause the same trouble? Or is it smart enough to load DYM after tweaking RubyGems?

Contributor

yuki24 commented May 10, 2016

@headius Thank you for the clarification! I'll just kill the local variable name correction on JRuby for the time being.

Does the use of --enable:did_you_mean (or JRUBY_OPTS or RUBYOPT) cause the same trouble? Or is it smart enough to load DYM after tweaking RubyGems?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 10, 2016

Member

@yuki24 Currently all that enable/disable of did_you_mean does is define a toplevel DidYouMean constant to indicate that the gem should be loaded. There's more needed here to get it to load without booting RubyGems.

We could make it a "default gem" that's "installed" directly into stdlib, but MRI has been reluctant to introduce more default gems.

I'm hoping @tenderlove or the folks currently maintaining RubyGems will have some ideas for us.

Member

headius commented May 10, 2016

@yuki24 Currently all that enable/disable of did_you_mean does is define a toplevel DidYouMean constant to indicate that the gem should be loaded. There's more needed here to get it to load without booting RubyGems.

We could make it a "default gem" that's "installed" directly into stdlib, but MRI has been reluctant to introduce more default gems.

I'm hoping @tenderlove or the folks currently maintaining RubyGems will have some ideas for us.

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian May 11, 2016

Member

@headius default gem might not work, as rubygems need to look for newer versions of the default gem inside the installed gems first, i.e. the Gem.path and Gem.dir are already set even using default gems.

but we just could require 'did_you_mean' and then reset rubygems Gem.clear_paths. there might be still side-effects but not on Gem.path and Gem.dir which is main thing of this issue here.

Member

mkristian commented May 11, 2016

@headius default gem might not work, as rubygems need to look for newer versions of the default gem inside the installed gems first, i.e. the Gem.path and Gem.dir are already set even using default gems.

but we just could require 'did_you_mean' and then reset rubygems Gem.clear_paths. there might be still side-effects but not on Gem.path and Gem.dir which is main thing of this issue here.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 11, 2016

Member

@mkristian Ahh that's pretty clever. Gem.clear_paths will reset everything we're worried about?

Member

headius commented May 11, 2016

@mkristian Ahh that's pretty clever. Gem.clear_paths will reset everything we're worried about?

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian May 11, 2016

Member

@headius it will reset all we know gives problems so far :)

Member

mkristian commented May 11, 2016

@headius it will reset all we know gives problems so far :)

@headius headius modified the milestones: JRuby 9.1.1.0, JRuby 9.1.2.0 May 11, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 13, 2016

Member

Worth giving it a try, but I want to do it in 9.1.2.0 and let it bake for a while.

Member

headius commented May 13, 2016

Worth giving it a try, but I want to do it in 9.1.2.0 and let it bake for a while.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 May 15, 2016

Contributor

Speaking of the NameError#local_variables issue, It's also worth mentioning that what's really required to get a list of local variable names is not the TracePoint, but a binding object. TracePoint is just one way of capturing it. I used the interception gem before, and how it captures a binding object is to use EventHook in the Java level, although I'm not sure if it causes a performance/memory problem. If it doesn't, I could just add a similar implementation to DYM.

Contributor

yuki24 commented May 15, 2016

Speaking of the NameError#local_variables issue, It's also worth mentioning that what's really required to get a list of local variable names is not the TracePoint, but a binding object. TracePoint is just one way of capturing it. I used the interception gem before, and how it captures a binding object is to use EventHook in the Java level, although I'm not sure if it causes a performance/memory problem. If it doesn't, I could just add a similar implementation to DYM.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 May 15, 2016

Contributor

I've managed to add an event hook that captures a binding object for the JRuby runtime. This way we wouldn't have to wait for NameError#local_variables to be implemented and could avoid using TracePoint as well. But again, I'm totally unsure whether or not it would potentially cause a problem: yuki24/did_you_mean@1-0-stable...1-0-stable-with-jruby-support

I'm not very familiar with RubyGems and JRuby, so there isn't much I can do for the other issue... But I would be more than happy to wait until 9.1.2.0.

Contributor

yuki24 commented May 15, 2016

I've managed to add an event hook that captures a binding object for the JRuby runtime. This way we wouldn't have to wait for NameError#local_variables to be implemented and could avoid using TracePoint as well. But again, I'm totally unsure whether or not it would potentially cause a problem: yuki24/did_you_mean@1-0-stable...1-0-stable-with-jruby-support

I'm not very familiar with RubyGems and JRuby, so there isn't much I can do for the other issue... But I would be more than happy to wait until 9.1.2.0.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 16, 2016

Member

@yuki24 Clever! Unfortunately this suffers from a different issue: normally we don't trigger all event hook trace points without --debug, since it would add needless overhead even when there's no hook installed.

The code you wrote should work fine with --debug, however.

I'm not sure what our ETA is for NameError#local_variables. Thoughts on that and this hook, @enebo?

Member

headius commented May 16, 2016

@yuki24 Clever! Unfortunately this suffers from a different issue: normally we don't trigger all event hook trace points without --debug, since it would add needless overhead even when there's no hook installed.

The code you wrote should work fine with --debug, however.

I'm not sure what our ETA is for NameError#local_variables. Thoughts on that and this hook, @enebo?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 16, 2016

Member

@headius @yuki24 I think as far as this goes it will not work without --debug and perhaps that is ok but at the same time I think longer term we should be passing something through our call path which contains (or can access static scope). So perhaps we can land this short term and the long term fix will be much much larger and in the future?

Member

enebo commented May 16, 2016

@headius @yuki24 I think as far as this goes it will not work without --debug and perhaps that is ok but at the same time I think longer term we should be passing something through our call path which contains (or can access static scope). So perhaps we can land this short term and the long term fix will be much much larger and in the future?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 16, 2016

Member

I just realized this hook patch is still dependent on us not throwing away any dynamic scopes, so it probably won't help enough to merge.

Member

headius commented May 16, 2016

I just realized this hook patch is still dependent on us not throwing away any dynamic scopes, so it probably won't help enough to merge.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 16, 2016

Member

An additional snag: we don't seem to have the appropriate plumbing in our build to install a gem without making it a default gem. I'm working on that now and will push a branch/PR with did_you_mean installed soon.

Member

headius commented May 16, 2016

An additional snag: we don't seem to have the appropriate plumbing in our build to install a gem without making it a default gem. I'm working on that now and will push a branch/PR with did_you_mean installed soon.

@headius headius closed this in #3892 May 20, 2016

@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment