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

defined? with a colon2 fully resolves the left constant #3903

Closed
headius opened this Issue May 18, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@headius
Member

headius commented May 18, 2016

We are not compiling defined?(UndefinedConst::XYZ) properly.

Environment

JRuby 9.1, and probably all 9k.

Expected Behavior

In 1b6f75d, @eregon added an expectation that the above syntax would not trigger a call to Object.const_missing, as in MRI. Both the XYZ and the UndefinedConst are to be looked up "soft" without errors, autoload triggering, or calls to const_missing.

Actual Behavior

We appear to be calling const_missing for the LHS of a colon2, and compiling it as a normal constant lookup rather than a "soft" defined? lookup.

class Object
  def self.const_missing(*a)
    p a
  end
end
defined?(Foo::YYZ) # outputs "[:Foo]" on JRuby, nothing on MRI

This will need to be fixed.

@enebo @subbuss This could be the source of occasional constant lookup oddities and bugs we get reported, since we're trying to resolve things that aren't meant to be resolved (or earlier than intended to be resolved).

headius added a commit that referenced this issue May 18, 2016

@headius headius added this to the JRuby 9.1.2.0 milestone May 18, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 18, 2016

Member

This is even a more generic issue than even Colon{2,3}. I can see that for bare constants in test_defined.rb from MRI test suite that we are blowing up as we try to autoload a non-existent file for a constant (referenced from defined).

Member

enebo commented May 18, 2016

This is even a more generic issue than even Colon{2,3}. I can see that for bare constants in test_defined.rb from MRI test suite that we are blowing up as we try to autoload a non-existent file for a constant (referenced from defined).

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 18, 2016

Member

As a quick followup here is MRI test which breaks with a simple constant:

class TestAutoloadedNoload
  autoload :A, "a"
  def a?
    defined?(A)
  end
end

p TestAutoloadedNoload.new.a?

This is broken in all versions of JRuby. I am not actually sure these are the same problem but it appears we are fully resolving autoloads in defined? so it is not implausible the const_missing logic is the same code path.

Member

enebo commented May 18, 2016

As a quick followup here is MRI test which breaks with a simple constant:

class TestAutoloadedNoload
  autoload :A, "a"
  def a?
    defined?(A)
  end
end

p TestAutoloadedNoload.new.a?

This is broken in all versions of JRuby. I am not actually sure these are the same problem but it appears we are fully resolving autoloads in defined? so it is not implausible the const_missing logic is the same code path.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 18, 2016

Member

So in fact, both lexical and inheritance search instrs which are only used by defined? now have are performing autoload resolution. Believe it or not though these instrs are not the entire problem listed originally in this bug.

The bug here is that our defined? logic for colon2 will end up doing a 'build(leftNode)' which is the same as calling the constant outright. This should be buildGetDefinition(leftNode) but also with some minor changes since we need to check return value instead of catching error case it in a protected block.

Member

enebo commented May 18, 2016

So in fact, both lexical and inheritance search instrs which are only used by defined? now have are performing autoload resolution. Believe it or not though these instrs are not the entire problem listed originally in this bug.

The bug here is that our defined? logic for colon2 will end up doing a 'build(leftNode)' which is the same as calling the constant outright. This should be buildGetDefinition(leftNode) but also with some minor changes since we need to check return value instead of catching error case it in a protected block.

@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 23, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jun 14, 2016

Member

Follow up message to document the behavior a bit more. My previous message is not really right. defined? will autoload the lhs of a colon2 but not the rhs. So if I have:

a.rb

LOADED_ME=1
module FOO
  module A
    B = 1
  end
end

and defined2.rb:

module FOO
  autoload :A, 'a'
end

p defined?(FOO::A)
module FOO
  autoload :A, 'a'
end
module FOO
  autoload :A, 'a'
end

p defined?(FOO::A)
p defined?(LOADED_ME)
p defined?(FOO::A::B)
p defined?(LOADED_ME)

The first defined will have A as lhs and it will not autoload for LOADED_ME will not be defined, but once we add 'B' then A is lhs of a colon2 for B. Then we can see the autoload fire.

Member

enebo commented Jun 14, 2016

Follow up message to document the behavior a bit more. My previous message is not really right. defined? will autoload the lhs of a colon2 but not the rhs. So if I have:

a.rb

LOADED_ME=1
module FOO
  module A
    B = 1
  end
end

and defined2.rb:

module FOO
  autoload :A, 'a'
end

p defined?(FOO::A)
module FOO
  autoload :A, 'a'
end
module FOO
  autoload :A, 'a'
end

p defined?(FOO::A)
p defined?(LOADED_ME)
p defined?(FOO::A::B)
p defined?(LOADED_ME)

The first defined will have A as lhs and it will not autoload for LOADED_ME will not be defined, but once we add 'B' then A is lhs of a colon2 for B. Then we can see the autoload fire.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 12, 2016

Member

@enebo Isn't this fixed with your defined? const2 rework?

Member

headius commented Aug 12, 2016

@enebo Isn't this fixed with your defined? const2 rework?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 12, 2016

Member

@headius weirdly I still have a failing test case on this one. So some of it was fixed but it needs some more work.

Member

enebo commented Aug 12, 2016

@headius weirdly I still have a failing test case on this one. So some of it was fixed but it needs some more work.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 22, 2016

Member

I did solve part of this but I do not want to chance fixing the rest right before release.

Member

enebo commented Aug 22, 2016

I did solve part of this but I do not want to chance fixing the rest right before release.

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.15.0 Dec 2, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 2, 2017

Member

This appears to have been fixed as of at least 9.1.15. @enebo You did some work on nested constants, perhaps that was it?

Member

headius commented Dec 2, 2017

This appears to have been fixed as of at least 9.1.15. @enebo You did some work on nested constants, perhaps that was it?

@headius headius closed this Dec 2, 2017

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