ruby / error_highlight

The gem enhances Exception#message by adding a short explanation where the exception is raised
MIT License
150 stars 23 forks source link

Doesn't highlight error for missing constant when const_missing is redefined for class #34

Open luke-gru opened 1 year ago

luke-gru commented 1 year ago

I'm working on new feature for ruby's test/unit tool and I define Test::Unit::TestCase.const_missing. It does some stuff then calls old const_missing like the following (pseudo code):

orig = self.method(:const_missing)
def const_missing(name)
  # do some stuff 
   orig.call(name)
end

This broke error_highlight tests for missing constant, it wasn't highlighting anymore. I'm not sure if this is fixable or not.

mame commented 1 year ago

Can you please write down the executable code and exactly what you expect?

class Foo
  $orig = self.method(:const_missing)
  def self.const_missing(name)
    $orig.call(name)
  end
end

Foo::Bar

Current:

$ ruby test.rb
test.rb:5:in `call': uninitialized constant Foo::Bar (NameError)

    $orig.call(name)
         ^^^^^
        from test.rb:5:in `const_missing'
        from test.rb:9:in `<main>'

Maybe what you expect:

$ ruby test.rb
test.rb:5:in `call': uninitialized constant Foo::Bar (NameError)

Foo::Bar
   ^^^^^
        from test.rb:5:in `const_missing'
        from test.rb:9:in `<main>'

or

$ ruby test.rb
test.rb:9:in `<main>': uninitialized constant Foo::Bar (NameError)

Foo::Bar
   ^^^^^

The former can be achieved with error_highlight. But Foo::Bar is not in test.rb:5, so it may be confusing. The latter is quite difficult to achieve with error_highlight.

(I was aware of a similar problem, but had not written about it before, so I have created #35.)

luke-gru commented 1 year ago

If you go in Ruby repository and at the top of the test_error_highlight.rb you put:

klass = Test::Unit::TestCase
def klass.const_missing(name)
  super
end

you will see failing tests.

mame commented 1 year ago

Sorry, I don't understand the context at all. Why do you need to run the test of error_highlight along with your hack for test-unit gem? Are you proposing to introduce your hack into the test-unit gem?

luke-gru commented 1 year ago

Yeah it's a bit confusing, I'll explain. The changes I'm making are not to the test-unit gem, but to ruby's internal testing framework that is used when running ruby's test-suite (make test-all). You can find the file that I'm changing in the ruby repository at tool/lib/test/unit/testcase.rb. When I was changing this file to include my hack, all the tests in ruby's test suite were still working except for error_highlight tests. I was thinking that maybe this is a bug in error_highlight itself, maybe it does not work properly when the class (or the parent class) has defined const_missing. That is why I reported the issue. To reproduce the issue, you would have to go to the ruby repository and run make test-all TESTOPTS="test/error_highlight/test_error_highlight.rb" with the changes in my 2nd comment in this thread.

mame commented 1 year ago

I don't know why you try to change tool/lib/test/unit/testcase.rb, but anyway.

In fact, error_highlight does not work well when const_missing is defined. This is a variant problem of #35. I think this is a limitation around exceptions in Ruby itself rather than an issue with error_highlight. If tool/lib/test/unit/testcase.rb defines Test::Unit::TestCase.const_missing, I will remove the method temporarily during error_highlight testing.

On another note, I don't think it's a good idea to define Test::Unit::TestCase.const_missing in tool/lib/test/unit/testcase.rb. make test-all should run the test suite in a bare Ruby environment as much as possible. This is one of the reasons why Ruby test suite has not migrated to rspec.

luke-gru commented 1 year ago

I was just messing around with this, and got it working.

in def self.spot in base.rb:

        return nil unless loc
        if NameError === obj
          while locs.any? && /in `const_missing'/ =~ loc.to_s
            locs.shift
            loc = locs.first
          end
        end

Kind of hacky, but gets the job done 😄

As for not changing test/unit/testcase.rb, I understand your concerns.

mame commented 1 year ago

Your patch is what I said as 2-1 in #35. Its problem is that the line number shown by the backtrace and the snippet shown by error_highlight are discrepant.

class Foo
  def self.const_missing(_)
    super
  end

  Bar
end
$ ruby -I lib/ test.rb
test.rb:3:in `const_missing': uninitialized constant Foo::Bar (NameError)

  Bar
  ^^^
        from test.rb:6:in `<class:Foo>'
        from test.rb:1:in `<main>'

Note that there is no Bar in test.rb:3.