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

Handle nil nd_parent in spot_colon2 #16

Closed

Conversation

jeremyevans
Copy link

This fixes an issue in Sequel's tests on ruby 3.1.0-preview1, where a
test was showing the wrong output in an exception message. The
related code rescues NameError to provide more information, then
reraises. Here's the output of Sequel's test without changes to
error_highlight:

  1) Failure:
Sequel::Model::Associations::AssociationReflection::#associated_class#test_0006_should include association inspect output if an exception would be raised [/data/code/sequel/spec/model/associ
ation_reflection_spec.rb:75]:
Expected "undefined method `last_lineno' for nil:NilClass\n\n      if nd_parent.last_lineno == @node.last_lineno\n                  ^^^^^^^^^^^^" to include # encoding: UTF-8
"#<Sequel::Model::Associations::ManyToOneAssociationReflection #<Class:0x000002594112e458>.many_to_one :c>".

This change makes the Sequel test pass. There probably is a better
fix.

I tried to reproduce this outside of Sequel's tests in order to create
a test for error_highlight, but I wasn't able to do so quickly.

This fixes an issue in Sequel's tests on ruby 3.1.0-preview1, where a
test was showing the wrong output in an exception message.  The
related code rescues NameError to provide more information, then
reraises.  Here's the output of Sequel's test without changes to
error_highlight:

```
  1) Failure:
Sequel::Model::Associations::AssociationReflection::#associated_class#test_0006_should include association inspect output if an exception would be raised [/data/code/sequel/spec/model/associ
ation_reflection_spec.rb:75]:
Expected "undefined method `last_lineno' for nil:NilClass\n\n      if nd_parent.last_lineno == @node.last_lineno\n                  ^^^^^^^^^^^^" to include # encoding: UTF-8
"#<Sequel::Model::Associations::ManyToOneAssociationReflection #<Class:0x000002594112e458>.many_to_one :c>".
```

This change makes the Sequel test pass. There probably is a better
fix.

I tried to reproduce this outside of Sequel's tests in order to create
a test for error_highlight, but I wasn't able to do so quickly.
@mame
Copy link
Member

mame commented Dec 18, 2021

Thank you for your report. I think I got a clue. The following code will trigger an unknown bug of error_highlight (or RubyVM::AST or something).

module Foo
  Object.module_eval("::Bar", __FILE__, __LINE__)
end

The error message changes if error_highlight is enabled:

$ ruby --disable-error_highlight t.rb
t.rb:2:in `<module:Foo>': uninitialized constant Bar (NameError)
        from t.rb:2:in `module_eval'
        from t.rb:2:in `<module:Foo>'
        from t.rb:1:in `<main>'

$ ruby t.rb
t.rb:2:in `<module:Foo>': NameError
        from t.rb:2:in `module_eval'
        from t.rb:2:in `<module:Foo>'
        from t.rb:1:in `<main>'

I'll continue to investigate the issue tonight

matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 18, 2021
This check is needed to fix a bug of error_highlight when NameError
occurred in eval'ed code.
ruby/error_highlight#16

The same check for proc/method has been already introduced since
64ac984.
@mame mame closed this in d2140d7 Dec 18, 2021
@mame
Copy link
Member

mame commented Dec 18, 2021

@jeremyevans I think the issue was fixed by the combination of d2140d7 and ruby/ruby@6a51c3e. Could you try ruby/ruby master with your code base? Let me know if it is not fixed yet.

@mame
Copy link
Member

mame commented Dec 18, 2021

Ah, no, the issue is not fixed yet. Continue to investigate...

@mame
Copy link
Member

mame commented Dec 18, 2021

Argh, a pilot error. I used the older ruby to check if the issue is fixed or not. I think it's fixed after all.

@jeremyevans
Copy link
Author

I've built and installed ruby-head locally and checked and confirmed the problem is fixed. Thank you very much for fixing this!

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

Successfully merging this pull request may close these issues.

2 participants