Fix command-t on when using RVM

[ssgelm]

This should fix the various issues that have been reported when using RVM with a different version locally than the system version. Command-T still needs to be compiled with the system ruby but this prevents the error that occurs when opening vim.

Specifically I believe this fixes #175 and #76.

Sorry for the double pull requests, I realized #261 had an incorrect author on the commit so I created this pull request in its stead.

[wincent]

Thanks for this. Can you explain a little bit about how/why this fixes things?

[ssgelm]

For sure. There is a bug that exists at least with ruby 2.2.1 running with RVM on Debian Jessie that causes the following error when running :CommandT:

Error detected while processing vim/plugged/command-t/autoload/commandt.vim:
line  183:
Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
Error detected while processing function commandt#FileFinder:
line    2:
NoMethodError: undefined method `show_file_finder' for nil:NilClass

In short, the issue happens because ruby runs using some of RVM's gem monkeypatching which causes a Gem::Ext::BuildError instead of a LoadError. Given that this only catches a second type of exception I believe that the change it low risk. The worst case would be that the plugin would not crash when it otherwise should but I can't imagine a scenario in which that would occur. Anecdotally, we have been running with this patch for about a year and a half with zero issues.

[wincent]

Cool, thanks for the explanation. Before we merge this, I think we need to modify it so that it works in environments that aren't monkey-patched. For example, on my system Ruby, Gem::Ext::BuildError is not defined, so applying this patch would be a breaking change. We probably need a check with const_defined? of some sort.

[wincent]

Ha, I take it back. Ruby doesn't work the way I think it should.

begin
rescue Gem::Ext::RandomGarbage
end

Works.

[wincent]

Actually, we're going to have to try again. Had to revert this because it was blowing up for me in Ruby 2.3.3-p222 (commit).

[ssgelm]

That's unfortunate. I'll see what I can do to fix it.

[ssgelm]

@wincent I pushed a new commit that fixes this as far as I can tell. I didn't squash the commits but I am happy to if you'd prefer that.

[ssgelm]

Actually, this now doesn't fix the problem for some reason. ☹️ Back to the drawing board!

[ssgelm]

@wincent OK, now I believe it's fixed. Turns out Gem::Ext::BuildError isn't defined until rubygems/ext is autoloaded so I needed to require it before I could check if it's defined.

[ssgelm]

For what it's worth, I believe that e710003 is what broke it. Ruby stops evaluating the constants once it matches one so in the original code when LoadError is hit it doesn't matter whether Gem::Ext::BuildError is defined. Having said that I think that this new commit is a more robust way of handling it anyway.

For example:

2.3.3 :001 > begin
2.3.3 :002 >   require 'asdf'
2.3.3 :003?>   rescue LoadError, RandomOtherError
2.3.3 :004?>     puts 'worked'
2.3.3 :005?>   end
worked
 => nil
2.3.3 :006 > begin
2.3.3 :007 >   require 'asdf'
2.3.3 :008?>   rescue RandomOtherError, LoadError
2.3.3 :009?>     puts 'worked'
2.3.3 :010?>   end
NameError: uninitialized constant RandomOtherError
Did you mean?  RangeError
	from (irb):9:in `rescue in irb_binding'
	from (irb):7

[wincent]

Sigh, well that is subtle. I had tested both:

begin
  rescue Gem::Ext::RandomGarbage
end

And:

begin
  raise 'something'
rescue RuntimeError, RandomCrap
  puts 'rescued'
end

which also worked...

So much for the principle of least surprise. I was already surprised when the first example worked, but when the second one worked but this one doesn't that's pretty surprising indeed:

begin
  raise 'something'
rescue RandomCrap, RuntimeError
  puts 'rescued'
end

Sorry for the churn. I'll pull in your new fix.

[wincent]

Hm, so thinking about this, I am not so keen on the idea of pulling in RubyGems, which could slow down startup. (Nowhere else in Command-T references RubyGems, nor does it depend on it.)

This would obviously be a code-smell, but could we avoid this with something like this:

begin
  # ...
rescue Exception => e
  if LoadError === e || e.class.to_s === 'Gem::Ext::BuildError'
    # ...
  else
    raise
  end
end

(I even put a code-smell inside the code-smell, by doing the exception-to-string conversion.)

[ssgelm]

Yeah, that would work. Do you think it is at least cleaner to do something like the following?

begin
  # ...
rescue Exception => e
  EXCEPTIONS = [LoadError]
  EXCEPTIONS << Gem::Ext::BuildError if defined?(Gem::Ext::BuildError)
  if EXCEPTIONS.include? e.class
    # ...
  else
    raise
  end
end

[wincent]

Yeah that looks ok, I think.

[ssgelm]

OK, change is made. Did you want me to squash these commits?

[wincent]

Thanks. No need to squash.

[ssgelm]

Does this look ok to merge now?

[wincent]

Given the big rewrite for v6.0.x, older PRs will need to be re-rolled against main and target the new version, so I'm closing this one out. Feedback issue for 6.0.x is here:

• 🆕 Command-T 6.0.0 release — compendium issue #393