search

dgutov / robe

Code navigation, documentation lookup and completion for Ruby
584 stars 37 forks source link

Robe may throw exception if a gem overrides `Class#instance_methods` #54

Closed aifreedom closed 9 years ago

aifreedom commented 9 years ago

I got errors like below when using robe as a company backend. I looked into this case and it seems that it's because exifr overrides Class#instance_methods to include additional entries in the return of instance_methods but didn't include that in instance_method. https://github.com/remvee/exifr/blob/90f7734233f2a37cdc101bcc347265714fd0f02e/lib/exifr/tiff.rb#L429-L431

My proposed solution to this is to rescue NameError for the candidates << mod.instance_method(sym). But I still have a question about why EXIFR::JPEG is scanned when scanning candidates for an instance of a totally unrelated class.

Request failed: /complete_method/refer/rop/ReferralsController/-. Please file an issue.
undefined method `reference_black_white' for class `EXIFR::JPEG'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:43:in `instance_method'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:43:in `block in scan_methods'
/usr/local/opt/rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/awesome_print-1.0.2/lib/awesome_print/core_ext/array.rb:62:in `block in grep'
/usr/local/opt/rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/awesome_print-1.0.2/lib/awesome_print/core_ext/array.rb:60:in `each'
/usr/local/opt/rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/awesome_print-1.0.2/lib/awesome_print/core_ext/array.rb:60:in `grep'
/usr/local/opt/rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/awesome_print-1.0.2/lib/awesome_print/core_ext/array.rb:60:in `grep'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:42:in `scan_methods'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:20:in `block in scan'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:13:in `each_object'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:13:in `each'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/scanners.rb:13:in `scan'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/sash.rb:132:in `complete_method'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/sash.rb:176:in `public_send'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/sash.rb:176:in `call'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/server.rb:40:in `block in start'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/server.rb:28:in `loop'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe/server.rb:28:in `start'
/Users/song_xie/.emacs.d/.cask/24.5.1/elpa/robe-20150222.1644/lib/robe.rb:18:in `block in start'
dgutov commented 9 years ago

Hi!

I looked into this case and it seems that it's because exifr overrides Class#instance_methods to include additional entries in the return of instance_methods but didn't include that in instance_method.

That sounds like a bug: it violates the contract of those methods, seems like. Have you tried reporting it?

My proposed solution to this is to rescue NameError for the candidates mod.instance_method(sym).

I'm not sure it's the right approach: if the methods we call are allowed to do whatever (and essentially, they do), then rescuing NameError may not be enough as well. instance_method may raise a different error, or instance_methods may start raising errors or return unexpected values.

Maybe that calls for a circumvention of said override, like we do for a few methods already: https://github.com/dgutov/robe/commit/9a33e54e2572a6fb7fbac674f0cb5e378a5cab44

But I still have a question about why EXIFR::JPEG is scanned when scanning candidates for an instance of a totally unrelated class.

Because Robe performs almost no type inference, and when it does not know the type, it scans all classes and modules. It's explained in README.md.

aifreedom commented 9 years ago

Hi Dmitry, thanks for the prompt reply! I agree that this is a bug in that gem. I'll report it to them.

I'm not sure if my approach is the right one. But I think that rope shouldn't be broken when scanning a gem with bug. Using begin ... rescue ... end is one way to protect rope from being affected. There might be better ways to do it of course. I'm not familiar enough with robe's code. Do you have better ways to make it more robust?

dgutov commented 9 years ago

I'll report it to them.

Let me know how it goes.

There might be better ways to do it of course. I'm not familiar enough with robe's code.

Have you looked at the linked commit?

ypresto commented 9 years ago

I have same issue on EXIFR::JPEG

dgutov commented 9 years ago

Ok, let's try the workaround.

ypresto commented 9 years ago

Now works well, thanks!!!