andrea[m]1 has quit [*.net *.split]
andrea[m]1 has joined #jruby
headius has quit [*.net *.split]
nilsding has quit [*.net *.split]
nilsding has joined #jruby
headius has joined #jruby
razetime has joined #jruby
razetime has quit [Ping timeout: 268 seconds]
razetime has joined #jruby
<headius> good morning!
<nilsding> good afternoon ;)
razetime has quit [Ping timeout: 256 seconds]
razetime has joined #jruby
razetime has quit [Quit: - Chat comfortably. Anywhere.]
<headius> enebo: I went ahead and merged that PR
<enebo[m]> cool
<headius> I see that prepend constants thing and a syntax error still marked for 9.3.7
<enebo[m]> headius: There is a PR for prepend constants
<enebo[m]> the syntax error with regexp I just landed
<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 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 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
<headius> probably not
<headius> in CRuby
<headius> so those are the three commits leading up to resolv/registry moving to fiddle
<enebo[m]> ah I just looked at resolv and it adds some ipv6 but we are sooo close on that we should leave it
<enebo[m]> I guess registry.rb is the broken thing rtight?
<headius> yeah
<enebo[m]> The diff appears to only be fiddle vs win32/importer
<headius> yeah pretty much
<enebo[m]> < str.encode(WCHAR) + 0.chr.encode(WCHAR)
<enebo[m]> ---
<enebo[m]> > str.encode(WCHAR)
<enebo[m]> < str.encode(WCHAR) + 0.chr.encode(WCHAR)
<enebo[m]> ---
<enebo[m]> > str.encode(WCHAR)
<enebo[m]> That is funny
<enebo[m]> It should be \0\0
<headius> yeah but I think the encode handles that
<enebo[m]> The former is 9.3 and later is master
<enebo[m]> yeah it maybe is writing \0\0\0\0 in 9.3?
<headius> wchar should be 16 bit
<enebo[m]> oh wait
<enebo[m]> yeah
<headius> UCS-2 or something
<enebo[m]> so the str.encode with fiddle maybe is correct
<enebo[m]> but without it we have to manually add it
<headius> yeah I think so
<enebo[m]> but WCHAR is \0\0
<enebo[m]> so funny but probably just something fiddle handles
<enebo[m]> I can just copy 9.3 to master unless you plan on auditing this more
<headius> no if that makes it work go for it
<headius> the problem is just that our fiddle sucks which is nothing new
<enebo[m]> I need to also put importer back
<enebo[m]> ok I pushed it and I will build and see what happens on windows now
<enebo[m]> builds again and benoit's script runs
<headius> Ok
<enebo[m]> I will I guess close out the 200 issues opened on that
<enebo[m]> ah I guess it was 3 and not 200 and you closed one already. So just the single fiddle broken issue remains open.
<headius> yeah cool... so we just need someone to volunteer to make fiddle work better
byteit101[m] has quit [Ping timeout: 255 seconds]
byteit101[m] has joined #jruby
sagax has joined #jruby