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

Fatal is not a module #5648

Closed
ioquatix opened this Issue Mar 12, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@ioquatix
Copy link
Contributor

ioquatix commented Mar 12, 2019

In MRI, Fatal is not defined.

But in JRuby, it is.

Code that expects to be able to create it's own module Fatal or otherwise will fail.

In MRI, fatal is not exposed to user. But you can access it by using ObjectSpace to find it.

I suggest that initially, you remove class Fatal from global namespace. Then, perhaps with agreement from MRI, we can make something like FatalError, perhaps defined under RubyVM or something else.

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Mar 12, 2019

koyoko% irb
jruby-9.2.5.0 :001 > module Fatal; end;
Traceback (most recent call last):
        6: from /home/samuel/.rvm/rubies/jruby-9.2.5.0/bin/irb:13:in `<main>'
        5: from org/jruby/RubyKernel.java:1181:in `catch'
        4: from org/jruby/RubyKernel.java:1181:in `catch'
        3: from org/jruby/RubyKernel.java:1415:in `loop'
        2: from org/jruby/RubyKernel.java:1046:in `eval'
        1: from (irb):1:in `evaluate'
TypeError (Fatal is not a module)
jruby-9.2.5.0 :002 > 
koyoko% rvm use 2.6.1
Using /home/samuel/.rvm/gems/ruby-2.6.1
koyoko% irb          
2.6.1 :001 > module Fatal; end
 => nil 
2.6.1 :002 > 
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Mar 12, 2019

@ioquatix we can talk about whether or not we can hide this name since it is somewhat likely to hit user code which may also end up using global namespace for a common word. We usually try to avoid this practice but generally mostly out of fear MRI will introduce it.

It looks like 9.2.x has this since feb 2018. We maybe can deprecate and 9.3.x can not use global namespace. We will talk about the complexities of making the change. Once people use code it is more difficult to just remove it.

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 12, 2019

It doesn't look like any Ruby code tries to access this exception, which makes sense since it's not accessible in MRI. They appear to be defining it as a lower-case "fatal" so it doesn't show up in constant lists. I don't like the practice, but this isn't the only class or module they define this way. However simply changing our code to use the lower-case name causes Object.constants to have an entry for :fatal. I'm not sure why this doesn't appear in MRI yet.

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 12, 2019

Short term workaround would be to undef_const the offending name before you try to open your own class called Fatal.

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Mar 12, 2019

I only mentioned this because I think it's a consistency issue. I personally agree, there probably should be a constant.

In MRI:

Fatal = ObjectSpace.each_object(Class).find{|klass| klass < Exception && klass.inspect == 'fatal'}

finds the right constant.

headius added a commit to headius/jruby that referenced this issue Mar 13, 2019

Remove the "Fatal" constant since it's hidden in MRI.
This allows users to define their own Fatal class or module
without conflicting.

Fixes jruby#5648.
@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 13, 2019

I have done a PR to remove the constant from Object. I'm not sure how well our internals would handle the lower-case class name, so I have not made that change.

Are you expecting the ObjectSpace mechanism to work as well, or just hoping to get rid of the conflicting constant? Given that this class is intended to be a non-recoverable error, I'm inclined to not worry about "Fatal" versus "fatal".

@headius headius added this to the JRuby 9.2.7.0 milestone Mar 13, 2019

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Mar 13, 2019

I'm honestly not that concerned, the ObjectSpace lookup is a hack :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.