<enebo[m]>
so Bar ends up with its own space for cvars
<headius>
and we do not do that right currently
<enebo[m]>
yeah but I think this is the same cbasei ssue
<headius>
DefineInstMethod passed both my cases but failed others
<headius>
I mean using the same logic to find a target class
<enebo[m]>
Assuming I am correct and both use cbase
<headius>
findInstanceMethodContainer
<enebo[m]>
but the fact jeremyevans said it brought up this incompatibility
<enebo[m]>
or that cbase is not always cref
<headius>
I think we need to consider that this regression spec was never really fixed
<headius>
we modified the logic to always use Object as the target which allowed it to pass
<headius>
but that is not correct either of course
<enebo[m]>
yeah that is probably true we just happened to pass a single case
<enebo[m]>
and not really since we could see it was using Object
<headius>
such a spec does not exist anywhere and using autoload in this way would be problematic because the very constant lookups you want to autoload would not see it
<headius>
so I am on the fence about spending more time trying to resolve this
<enebo[m]>
It also begs a big question of why you would want this to work
<enebo[m]>
but I think the larger issue is if you clone a type you want a new place for constants
<headius>
perhaps I should just make a "getClassBase" utility method that both class vars and this use and eventually we can fix that to do the right thing
<enebo[m]>
which is my tangent here but I think the bigger issue
<headius>
yeah there are many missing piece for cloned modules in JRuby
<enebo[m]>
if you autoload onto the wrong thing is obviously bad but if we fixed this use case it would autoload twice and why would you want that
<headius>
using the same logic as GetClassVarContainerModule passes the original autoload case and fails the regression spec in the same way, but at least it is sharing logic with class vars
<headius>
and passes specs that using instance method target failed
<enebo[m]>
So I think that test is bogus in showing the main issue which is that we need to show the constants are really defined on the instance of Class.new
<headius>
yes
<headius>
it does not test the right thing and just happened to pass with the old logic
<headius>
this is for class var in method body in Class.new body
<enebo[m]>
yeah I am looking at classVarContainer in IRBuilder right now
<enebo[m]>
this does not consider Class.new as a scope but treats this as purely lexical
<headius>
I am moving forward with using the same logic under a new name "getCurrentClassBase" which will call the same logic for class vars
<headius>
if we fix our "cbase" at some point then this other case will work as well as cloned class var contexts
<enebo[m]>
We record the static scope for cvar and this feels partially wrong since we probably walk up to top lexical scope in your original autoload test
<headius>
the original test does do the autoload at top scope
<headius>
the regression example does it within Class.new
<headius>
right it should be and yes it does go up to Object
<enebo[m]>
and IRBuilder walks static scopes which is not right
<headius>
which is why it is nil, because Time is already defined at that level
<headius>
autoload just noops when the const is define
<enebo[m]>
a 99% solution is to look for {Module,Class}.new perhaps in build but that is not a total solution but could make a make a more dynamic lookup
<enebo[m]>
Otheriwse constants a site optimized for more live work probably will have no perf affect on them
<enebo[m]>
Do we callsite cache cvars?
<enebo[m]>
Not starting at the wrong place is only part of the solution but this builder logic needs to change
<enebo[m]>
The maintenance burder of .new { } sorts of issues is surprisingly high :)
<headius>
no we don't optimize class vars in any way
<headius>
edipo.federle mentioned this the other day since TR did finally optimize class vars
<enebo[m]>
headius: so your new method (I am looking at getModuleFromScope in IRRuntime) should start from its current scope and go down in this helper (or whatever name it gets)
<headius>
I want to as well but not until we are sure we are doing them right (and TR may not be right either)
<enebo[m]>
optimizing cvars then maybe will be more of a visible thing since we will do some more walking every access
<headius>
yeah my new method just calls getModuleFromScope against context.getCurrentStaticScope
<headius>
which feels roughly equivalent to vm_getcbase
<enebo[m]>
does Class.new make a live static scope?
<enebo[m]>
in the block form
<headius>
I think it does via module_eval/yieldUnder
<headius>
it is not a cref scope so constants are not defined there but it is a cbase scope
<headius>
🙄
<headius>
I will file an issue with CRuby to deprecate this Kernel#autoload behavior in any case because nobody would want this
<headius>
autoload should always go to the same place constants will be retrieved from
<headius>
in this case, it does not
<enebo[m]>
ok so I guess maybe getclassvarcontainermodule is just always starting at current and we don't try and calculate the jump off point in that instruction
<enebo[m]>
s/current/current static scope+module/
<enebo[m]>
but that means constants and class vars will do a first time walk on callsite cache but with no cache then it will happen every access until a cache is made
<enebo[m]>
I think in both cases once the location is found it will never change locations (realistically)
<headius>
if we were cloning the actual method body I would agree
<headius>
but we don't do that right now either
<headius>
in any case, are we in agreement that failing this regression test is not reason enough to fail this fix?
<headius>
I do not think we can fix this until the class variable base logic works properly
<enebo[m]>
I think we already know we were broken before and after just a little differently
<headius>
the "big" change in this PR is that it no longer uses the "super" class from frame to target autoload
<enebo[m]>
I am not sure if the wallpaper which made the regression pass somehow makes people magically work in other cases
<headius>
which fixes the real case from #6708 but exposes the fact that we never really fixed the regression case
<headius>
yeah I do not know either
<headius>
how about this... I will delete our regression spec but move it into a rubyspec for discussion
<headius>
that will likely lead to a deprecation of this behavior since it is so unexpected
<enebo[m]>
I think we should really consider fixing this but I know it is complicated because we need to introduce something for the new home
<enebo[m]>
well yeah for autoload I don't care
<enebo[m]>
I am more thinking about any constants
<headius>
I can't run rbx but I would wager they failed this too
<enebo[m]>
So any constants defined within these sorts of metaprogrammed types will just parent above it
<enebo[m]>
So if you end up doing n types at lets say a module scope and different types define the same constant one will overwrite the other
<enebo[m]>
(or cvar)
<enebo[m]>
I don't think that is a common thing either
<headius>
yeah
<headius>
Ruby is a reverse democracy
<headius>
the minority view usually wins because nobody wants to break anyone's code
<headius>
so we have weird stuff like this and class variables lingering forever
<headius>
yeah the autoload in this regression spec behaves like self.class.const_set
<headius>
what is curious to me is that this changed in 1.9
<headius>
that may have been unintentional during the changeover to yarv
<enebo[m]>
So this confirms to me we create a new static scope based evalBelow (or whatever it is called). If we changed lookup logic from what we are doing now in builder and had a helper walk I think we would work for cvars too?
<headius>
1.8.7 behaves like we do now
<enebo[m]>
with the cost of walking per access
<headius>
yeah maybe
<headius>
but ideally we wouldn't have to walk, which is how it would behave if autoload just used cref
<enebo[m]>
well I am ignoring autoload itself just talking about solving what I think may also ultimately ending up solving the 'include MyModule.clone'
<enebo[m]>
I think more is needed on the clone since I doubt we duplicate a scope for clone
<enebo[m]>
oh funny
<enebo[m]>
headius: in initCopy in module we syncClassVariables (which dup calls which maybe somehow we have a path from clone?) and it so a putAll(other.getClassVariablesForRead())
<enebo[m]>
Will the cvar snippet I used earlier get fixed if we just clone() on each CVar from that list?
<enebo[m]>
err ignore that...heh I will check but this dup logic I bet is not getting called by clone since this looks to be doing the right thing
<enebo[m]>
nope..I may step debug this. It seems like we have logic to duplicate new cvars in this case
<headius>
yeah I am not sure what the correct behavior for the cloning case is
<enebo[m]>
I also removed the lexical pre-calc on IRBuilder
<enebo[m]>
well I have seen at least one library use 'include Foo.clone' and in that case they expect the cvars in both to be their own storage
<headius>
9.3 back down to 38 open items
<headius>
I am moving on to the handle_interrupt issue to make sure that is 100%