<enebo[m]>
I added you are a reviewer to this. I think it makes sense but it is a delicate area :)
<headius>
You reviewed that fix and it looks good?
<enebo[m]>
The regexp thing was that we cannot do arbitrary jumps in Java like C and a piece of replicated code was not
<enebo[m]>
which made evaluating it simpler (plus I only fixed the other place which had the right fix a few months ago)
<enebo[m]>
headius: I want you to look it over but it makes sense we would not store constants on the wrappers themselves and origin seems like a logical place
<headius>
Ok
<enebo[m]>
I don't know about how cref works for this stuff but I guess it would still be a different part of the code
<headius>
that prepend patch is surprisingly small
<headius>
is this behavior new in a recent Ruby?
<enebo[m]>
that I don't know
<enebo[m]>
but 9.4 also has similar failing issues on MRI test suite so this should fix those as well
<enebo[m]>
He did make a test so that is good but I don't know if I care enough to extract that test and run it on earlier rubys
<headius>
and it passes everything else
<headius>
it is just surprising that such a visible issue was not reported or fixed before
<enebo[m]>
yeah I wondered about that as well but I think perhaps the issue is that most constants are not from the same module you are including/prepending
<enebo[m]>
but "most" does not mean none so I also wondered about it
<enebo[m]>
with that said no constant should be put into the wrapper for the include/prepend so I do not see what we have as matching Ruby behavior
<enebo[m]>
hmm I am not even sure you can put a constant into the wrapper :)
<headius>
so you're saying the delegation makes more sense
<enebo[m]>
In my mind the wrapper is just a mechanism for us to get the real bits into an inheritance hierarchy
<enebo[m]>
If we want a constant from 'prepend Foo' we probably want to look for constants from Foo and not what we wrap Foo in
<enebo[m]>
I wanted you to review just to make sure my last sentence is really how this all works. I think so but I find myself fretting whenever I am in prepend code
<headius>
yeah the fix is so simple it scares me
<enebo[m]>
hahah yeah I was very impressed
<enebo[m]>
so impressed I was worried
<headius>
ok I see
<enebo[m]>
headius: what do you think of my thought process above though?
<headius>
the prepend causes it to lose track of the constants in the origin
<enebo[m]>
yeah it looks at the wrapper for the module and not the module (origin)
<headius>
your thought process makes sense and this may simply be a delegation to the origin we missed over time adding prepend logic
<enebo[m]>
nothing will be in the origina expect module/object
<headius>
it worked fine without prepend
<enebo[m]>
yeah that is true...no way this was an issue with include
<enebo[m]>
So how does include do it right?
<enebo[m]>
Maybe cref
<enebo[m]>
just happens to generally be lexically hit
<enebo[m]>
Hmm perhaps we need to take prepend example and see if it really does work with include
<headius>
that i what I am trying to understand
<enebo[m]>
I wonder if we just do the wrapper insert differently between the two wrapper
<enebo[m]>
I am checking
<enebo[m]>
hahah you know it is amusing to me that we have constantTableStore defined to go against origin already
<enebo[m]>
hahaha Dortch put that in so this goes way back
<enebo[m]>
shoot. I am going to make a test case for include now
<headius>
I am refreshing my memory about how including a prepended module works
<enebo[m]>
ok so first-level simple prepend or include finds this fine
<enebo[m]>
I would expect that
<enebo[m]>
oh you know I misread the actual scope of this. It is just in resolving colon2
<headius>
so include includes the methodLocation, I think that is the main issue
<headius>
it also fails if you try to look it up in the method body
<headius>
I tested that as well
<headius>
so I think this is the sequence... prepending a module B into module A causes the method location for A to now point at the wrapper for the prepend, which is above A in the hierarchy
<enebo[m]>
yeah the prepend makes a wrapper for the included module and we look there for the constant but it is not there since it is a new module
<headius>
so without then delegating back to origin (which is the actual A module object) for constant lookups it misses the constants defined below the prepend wrapper
<enebo[m]>
And the only way to see this behavior is to have a prepend happen first
<enebo[m]>
which it would
<enebo[m]>
but how many people do this combo?
<headius>
yeah because it changes methodLocation to a layer above the constants
<headius>
and we never look back at origin
<enebo[m]>
ok so why are you saying methodLocation?
<headius>
RubyClass wrapper = new IncludedModuleWrapper(getRuntime(), insertAbove.getSuperClass(), moduleToInclude, moduleToInclude.getMethodLocation());
<enebo[m]>
ok so origin
<headius>
the module A, after prepending, will have methodLocation = prepend wrapper
<enebo[m]>
origin in this case == method location
<enebo[m]>
we have methodLocation, origin, non-inluded class, and delegate :)
<headius>
origin and methodLocation are different
<enebo[m]>
oh yeah I see it is the second constructor
<enebo[m]>
well origin makes sense to me but only because now I don't know how these two terms actually differ here
<headius>
yeah this remains a problem
<headius>
and I think the CRuby origin is opposite our usage
<headius>
I think the patch is fine though... it delegates the constant lookup back to the base of the module's hierarchy where the constants are
<enebo[m]>
Ah I just looked up methodLocation that seems like the wrong place to put/get a constant
<enebo[m]>
If I prepend to two modules which are both included won't they see the same constants?
<enebo[m]>
Or I should say should'nt the included module be setting a constant in a place where anyone using that included module can still see it
<headius>
well I'm not sure about those store and remove methods... technically you should never be able to modify constants via an include wrapper
<enebo[m]>
methodLocation is only not the same if it happens in the case of a prepend
<headius>
iyeah
<headius>
yeah the prepend changes the virtual "root" of the module
<enebo[m]>
but we do not want to store anything there I don't think
<headius>
yeah but you don't
<headius>
constant A still points at the bottom
<enebo[m]>
module Foobar
<enebo[m]>
prepend PrependTest
<enebo[m]>
TESTCONSTANT = "bar"
<headius>
for method lookup purposes it goes to methodLocation if prepended
<enebo[m]>
any consumer of Foobar to see TESTCONSTANT
<enebo[m]>
yeah for method lookup totall7y
<headius>
hmm
<enebo[m]>
for constants we want Foobar to get that constant
<headius>
now I'm not clear if prepend constants should shadow the original module
<headius>
constant assignment should always end up in the module itself because we don't traverse methodLocation for that
<enebo[m]>
TESTCONSTANT should go on Foobar right? It is not like prepend changes where a constant assign goes
<enebo[m]>
yeah
<headius>
the problem is include using methodLocation as the thing to include, which is somewhere above Foobar
<enebo[m]>
I think this is why origin makes sense. It should just be Foobar here
<headius>
yeah ok
<enebo[m]>
hmm
<enebo[m]>
Ok so we know this solves the reported problem. Sicne it will just ask Foobar
<enebo[m]>
Does it still see any constants added to PrependTest though?
<headius>
yeah I am checking that
<enebo[m]>
I can see we do it currently
<headius>
yeah I am testing if the patch breaks that
<headius>
seems ok
<enebo[m]>
I am applying and trying this out as well since I might be trying a different thing
<enebo[m]>
I can see the case I added working before the patch
<headius>
yeah the case with a same constant in PrependTest works as you expect on 9.3.6 but without the constant in PrependTest it fails to find the one in Foobar
<enebo[m]>
Should a prepend constant shadowing an included one be seen first too?
<headius>
with the patch both work
<headius>
it seems to
<headius>
it is not suprising that it works in the old case because we just didn't care about Foobar constants after the prepend
<enebo[m]>
yeah it just put them somewhere we never looked
<enebo[m]>
ok I did ordering test as well and that is correct
<enebo[m]>
So here is the only thing which bugs me...
<enebo[m]>
why did we have remove and store already hitting origin and then just say "fuck it" and not add the fetch?
<headius>
yeah I am not sure
<enebo[m]>
Seems like fetch would have been the first one we considered
<headius>
like I say I don't know how you would even trigger those mutating methods to be called
<enebo[m]>
With that said I did a big fix to prepend a year or two ago
<headius>
constant assignment always goes to the actual class/module
<headius>
and you can't reference an include layer
<enebo[m]>
and a big focus was "origin" making sense.
<enebo[m]>
Although perhaps my definition is not MRIs :)
<enebo[m]>
We had a lot of combos of the other things being used and we consolidated what we were asking for stuff
<enebo[m]>
which did fix tests
<headius>
yeah
<enebo[m]>
headius: and yeah that was where I started out this convo this morning. It was obviuosly wrong to fetch from there
<enebo[m]>
it has to be the real module
<headius>
right so I think I am satisfied with this now
<enebo[m]>
but that PR makes too much sense :)
<enebo[m]>
HAHAHA we both are bothered at how sensible this PR is
<headius>
ruby is complicated
<enebo[m]>
ok we can merge this and then just do 9.3.8.0 in a couple of days if the other shoe drops
<enebo[m]>
but I cannot think of any issue with this PR now
<enebo[m]>
Since we are giving time for rsim we can just land and revert if we think of something
<enebo[m]>
lunch
<headius>
everything is merged and closed, not sure how long we should wait for rsim to verify
<headius>
I'm going to shift gears and see if I can get this loongarch stuff moving
<enebo[m]>
ok. I wonder if I should look at windows build on master
<enebo[m]>
or I guess networking on windows
<enebo[m]>
headius: you revert registry.rb?
<headius>
that is all on master and I have not fixed anything
<headius>
so no I have not reverted it to the old win32api version
<headius>
that would be a short term fix but roll back to an earlier registry.rb
<enebo[m]>
so we pull in default gem and we still have older code on 9.3 which uses ffi
<enebo[m]>
yeah
<headius>
I don't think any of these bits are coming from gems yet
<enebo[m]>
ah ok then not nearly as big of an issue other than any changes to functionality
<headius>
so we can change in local repo, mostly just rolling back that win32 update I think?
<enebo[m]>
I somewhat doubt a lot has changed in resolve.rb