Skip to content

Use the real module for Kernel autoload#6749

Merged
headius merged 4 commits intojruby:masterfrom
headius:kernel_autoload_realclass
Jul 12, 2021
Merged

Use the real module for Kernel autoload#6749
headius merged 4 commits intojruby:masterfrom
headius:kernel_autoload_realclass

Conversation

@headius
Copy link
Member

@headius headius commented Jul 8, 2021

This logic needs to walk up to the nearest non-singleton, non-
include wrapper to find the "real" module, since constants do not
live on those other "non-real" module types.

Fixes #6708

This logic needs to walk up to the nearest non-singleton, non-
include wrapper to find the "real" module, since constants do not
live on those other "non-real" module types.

Fixes jruby#6708
@headius headius added this to the JRuby 9.3.0.0 milestone Jul 8, 2021
The frame class is frequently pointing at the include wrapper for
a given module, in order to indicate where super should start
from. However with the "real module" logic this causes the target
of the autoload to not be on that included module, but on the
superclas of the include wrapper. This led to breakage in the
Kernel#autoload specs, since they require that Kernel.autoload and
Kernel#autoload define the constant on the included module.
@headius headius mentioned this pull request Jul 8, 2021
We do not have a concept of cbase in our codebase, but the cbase
in CRuby is used as the target for class variables and other
class-y things that can be done from within a method body. The
logic in IRRuntimeHelpers.getModuleFromScope seems to
find the appropriate location.

We do not do class variable targeting exactly right (e.g. in
cloned modules) so this logic is not quite matching CRuby, but it
does pass the original case in jruby#6708.

This still fails a regression test from "JRUBY-6570" but it
appears that the original fix for that issue did not really fix
the problem. It merely made the expected case pass by always using
Object as the target, which is clearly not correct.

In order to start using the language of CRuby I have introduced
a new method getCurrentClassBase that should eventually be
equivalent to CRuby's vm_get_cbase.
@headius headius force-pushed the kernel_autoload_realclass branch from ffff32f to 06e13b6 Compare July 12, 2021 17:02
headius added a commit to headius/spec that referenced this pull request Jul 12, 2021
This behavior is peculiar but can be traced back at least as far
as 1.9.3 (with 1.8.7 exhibiting different behavior). The target
for a bare 'autoload' call in a method in a Class.new block should
be the new class. This differs from constant lookup, which will
skip the new class and use whatever "concrete" class scope is
above it.

This behavior was reported broken in JRuby many years ago and
never completely fixed. The spec provided here happened to work
until a recent fix for autoload scoping broke it (see
jruby/jruby#6749).

There are various problems with this behavior, the most obvious
being that the autoload (used to define a lazy constant) does not
go to the same place that constant lookup happens, and so this
code likely does not do what anyone would want it to do.
Nevertheless, this is current behavior of Ruby.
This spec defines current behavior of CRuby, but that behavior is
rather questionable due to unexpectedly different targeting of
autoload versus constant definition. We believe this regression
spec was never truly fixed in JRuby and merely happened to pass,
due to a naive fix that was still incorrect behavior but close
enough to make the given test pass. In order to move forward with
the fixes for jruby#6708 we are moving this spec into rubyspec via
ruby/spec#839, so we can discuss whether this is really correct
behavior or not.
@headius headius merged commit d737be3 into jruby:master Jul 12, 2021
@headius headius deleted the kernel_autoload_realclass branch July 12, 2021 18:52
eregon pushed a commit to ruby/spec that referenced this pull request Jul 29, 2021
This behavior is peculiar but can be traced back at least as far
as 1.9.3 (with 1.8.7 exhibiting different behavior). The target
for a bare 'autoload' call in a method in a Class.new block should
be the new class. This differs from constant lookup, which will
skip the new class and use whatever "concrete" class scope is
above it.

This behavior was reported broken in JRuby many years ago and
never completely fixed. The spec provided here happened to work
until a recent fix for autoload scoping broke it (see
jruby/jruby#6749).

There are various problems with this behavior, the most obvious
being that the autoload (used to define a lazy constant) does not
go to the same place that constant lookup happens, and so this
code likely does not do what anyone would want it to do.
Nevertheless, this is current behavior of Ruby.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Autoload regression

1 participant