<enebo[m]>
headius: that is fixed on master right?
<enebo[m]>
or mitigated
<headius[m]>
it is worked around on master I believe
<headius[m]>
yeah
<headius[m]>
I thought it should be fixed on 9.2 though too
<enebo[m]>
just spending a couple of minutes triaging kw splatting issue involving blocks
<headius[m]>
ahhh right
<headius[m]>
the fix is on 9.2 but opt-in because it changes classloader closing
<enebo[m]>
We will not be landing a fix for this today :) But I will see what jumps out. I believe this will come out to blockbody arg munging and not be a kwargs thing at all
<headius[m]>
ok
<enebo[m]>
ah yeah I already forgot
<enebo[m]>
I can see if I switch from block.call to an fcall it works fine and the prep for the splats are identical setting up the site
<enebo[m]>
So we are passing the same thing so .call is very likely doing something
<enebo[m]>
which as we know is a disaster to debug
<enebo[m]>
It has gotten a lot better though over time so perhaps it will just be obvious. Too scary for a release this week though
<enebo[m]>
HAHAHAH
<enebo[m]>
If I change from .call to just a raw yield it works fine
<headius[m]>
enebo: I think that Dir.each is basically done
<headius[m]>
I am just verifying that fixes I already merged resolve all reported cases
<headius[m]>
and ran into something I regressed while fixing it
<enebo[m]>
ok cool. that was the weird encoded files stuff
<headius[m]>
yeah
<headius[m]>
the fix was to just make all of those methods use the same logic for getitng encoding kwarg
<headius[m]>
but then I botched part of that, fixing now
<headius[m]>
and adding specs since this case is apparently not tested anywhere
<headius[m]>
oh heck
<headius[m]>
enebo: I just pulled and got those damn graal tags back again
<enebo[m]>
hmm from my PR somehow?
<enebo[m]>
like foreign repo merge gets all the tags again
<headius[m]>
could be
<headius[m]>
tags are like a social disease
<enebo[m]>
It is pretty funny to see all the ping-ponging we do to calcuate arity value for a block call
<enebo[m]>
It may even happen more than once
<headius[m]>
I would not be surprised at all
<headius[m]>
so want to get fixed arity paths plumbed through there better
<enebo[m]>
ok we prune is there is a single arityValue of 1 but no restkwargs
<enebo[m]>
this is obviously wrong
<enebo[m]>
we have arity of 1 and one kwarg
<enebo[m]>
no kwrest
<enebo[m]>
I suspect this logic should be !kwargs_at_all
<enebo[m]>
if this conditional is matched then we strip off the first argument
<enebo[m]>
err only use the first one
<enebo[m]>
as I said I am not comfortable committing today on this for .15 reegardless of how obvious it seems but this may end up being fairly obvious
<enebo[m]>
ok it passes. Trying to see if test suites run locally
<enebo[m]>
I am trying to understand what would break. This will have call (not callDirect which skips this massaging) when any kwargs are present not strip them off of call. So I guess if I passed in kwargs to a block which does not declare any I think we wuold see two arguments and not one
<enebo[m]>
oh wait no...signature is for the block
<enebo[m]>
so we are stripping incoming block args for all blocks which say they use kwargs but there is a single positional argument before the kwargs
<enebo[m]>
stripping incoming kwarg args
<headius[m]>
reading up
<headius[m]>
ok
<headius[m]>
sounds like you are still finding the path
<enebo[m]>
proc { |b:| p a, b }.call(:a, {b: :b})
<enebo[m]>
No I think I have it outright. this will get listed as arityValue 1 and has no restkwargs
<enebo[m]>
so we go "whelp...let's just pass args[0] in
<enebo[m]>
but this is arityValue of 1 and is haskwargs
<enebo[m]>
because of that we cannot just use the first value we have to pass the whole thing in
<enebo[m]>
This is somewhat counter intuitive though if you look at the code
<enebo[m]>
procs allow you to pass as much garbage as you want
<enebo[m]>
but last value is where kwargs exist
<enebo[m]>
but it looks weird but I believe I totally understand this
<enebo[m]>
args[1] or args[length-1] will always have the valid kwargs in this case so in the presence of kwargs we have to pass the whole thing in
<enebo[m]>
Or I suppose only pass the last argument
<headius[m]>
9.2 fail is the deploy still being broken
<headius[m]>
though sometimes it works
<headius[m]>
thanks Travis
<enebo[m]>
oh heh ok well that will be something to figure out so we can actually release
<enebo[m]>
whoa what the hell bug was I looking at
<enebo[m]>
ah 6540
<enebo[m]>
weirdly there seems to be 2 block kwarg bugs and 2 ripper bugs. I was accessing the wrong one each time I flip between browser tabs
<headius[m]>
dir thing is retargeted back to .15 and closed
<headius[m]>
constants don't fail it appears... if the constant is reassigned we will rebind fully
<headius[m]>
the threshold is usually for chaining PIC like
<enebo[m]>
My PR I think explains the logic of at a minimum why that code is broken as it is written
<headius[m]>
yeah it makes sense to me
<enebo[m]>
So while I said we shouldn't even if it is obvious I want to talk through it
<enebo[m]>
This was clearly nuking kwargs data from calls
<enebo[m]>
but only if arityValue is 1
<headius[m]>
yeah so your modification only drops kwargs if the method is arity 1 and has no kwargs
<enebo[m]>
ok so literally if we have requiredKwargs article value is 1
<enebo[m]>
arity
<enebo[m]>
article must be some weird muscle memory
<enebo[m]>
so if arity is 1 and there are kwargs then there can be no other args
<headius[m]>
where before it dropped if arity=1 but no rest kwargs
<headius[m]>
which basically lost kwargs then
<enebo[m]>
we do not substract values
<enebo[m]>
yeah
<enebo[m]>
so |**a| more or less
<enebo[m]>
hmm well maybe not that specific signature
<enebo[m]>
I guess the breakage from a change like this would be that we want to ignore kwargs if there is another argument
<enebo[m]>
I think the answe to that is no but the only confusion is looking at arityValue() in Signature
<enebo[m]>
we have an in param of kwargs and of requiredKwargs
<enebo[m]>
we use the later for articleValue
<enebo[m]>
hahaha my mind
<headius[m]>
yeah it seems like the right fix
<enebo[m]>
I am just double leery
<headius[m]>
so it would drop kwargs if you had {|a, b:| and we lose b
<headius[m]>
or {|b:| is it?
<enebo[m]>
well |b:| would be arity 1 because it is a single required kwarg
<enebo[m]>
any required and arityValue gets 1 right off the bat
<enebo[m]>
and we do not subtract them
<headius[m]>
ok
<enebo[m]>
the presence of optional kwargs or restkwargs will be negative value UNLESS required kwargs exist
<headius[m]>
are there no specs for this case?
<enebo[m]>
so those are not in play at all
<headius[m]>
just wondering whether we can fill in some blanks to make you feel better about this
<enebo[m]>
good question. I guess I will see if anything is tagged
<enebo[m]>
I mostly wanted to get the fix out to look at but I will see if I can find something
<enebo[m]>
Seems like they should exist so if they don't I will make a commit adding something
<headius[m]>
re site rebinding... invocation is the other place we failover and it has very different more complex logic because it is tracking target types
<headius[m]>
so it only increments for new types anyway
<enebo[m]>
so endless singletons could do it
<headius[m]>
it uses a more structured site tracker... I will do something similar for yields on master to make this less prone to bugs
<headius[m]>
endless singletons would quickly fill up the max polymorphism count and then it just fails forever
<enebo[m]>
ah ok so it is protected in two ways?
<headius[m]>
it also tracks complete clears, which are from the cached type being changed after
<headius[m]>
yeah two ways: unique types and total flushes
<headius[m]>
6 total types are allowed to chain, or 1000 complete flushes before it stops attempting to fast path
<headius[m]>
configurable, which is another thing to be added to the yield logic (hardcoded to two blocks right now)
<enebo[m]>
ok no tests or specs I can see
<enebo[m]>
funny to see how many tests we run whenever I run MRI test suite
<enebo[m]>
we have 14 tags in proc but only fail 5 now
<headius[m]>
yield fix is in
<headius[m]>
I will file an issue for master to improve this logic
<enebo[m]>
At another it is just extra work in the case of none hot path code
<headius[m]>
fun day on freenode apparently
<enebo[m]>
CGML2 stands for what :)
<headius[m]>
yeah I am wondering
<enebo[m]>
compact geographical markup language seems most likely
michael_mbp has joined #jruby
<headius[m]>
enebo: tenderlove may not be able to get a psych release out until after 9.2.15.0 so don't let that issue hold us up
<headius[m]>
users can just upgrade psych over the default one so it is not a big deal (and this was just a one-off report of a very minor behavioral difference anyway)
<enebo[m]>
ok yeah it is not a huge issue
<enebo[m]>
I mean it would always be great if we can get it all in but we have a solution which will make it in
<enebo[m]>
just not this week
<headius[m]>
yeah and really snakeyaml just came out yesterday so it is just poor timing
<headius[m]>
the issue is still marked for 9.2.15.0 but you can retarget whenever you decide to close the door
<enebo[m]>
move it to .16
<headius[m]>
ok
<enebo[m]>
I will likely merge the block kwargs thing in the morning
<enebo[m]>
I will maybe wake up and realize something but right now I cannot think of what would break
<headius[m]>
yeah me neither
<enebo[m]>
It pretty much has to be arity 1 AND accept kwargs
<enebo[m]>
So basically that is what it fixes by not doing it
<headius[m]>
yeah
<headius[m]>
and it simply doesn't work without this
<headius[m]>
so it is net positive there
<headius[m]>
we need to unify this logic in the 3.0 work
<enebo[m]>
yeah if there was a way to have kwargs AND another arg which somehow was still 1 then I would be worried
<enebo[m]>
but the req kwargs is the 1
<headius[m]>
it will likely be simpler and less ambiguous and we can look at plumbing kwargs through a fast path then
<enebo[m]>
and if it is optional kwtgs or kwrest the arity value is negative
<enebo[m]>
If we had another type for this we could get rid of this processing as well
<enebo[m]>
It would just be detected as essentially direct and just pass in args[0] thought something directly
<enebo[m]>
IRDirectBlockBody :)
<enebo[m]>
something
<headius[m]>
any issues with optional kwargs?
<headius[m]>
|b:1|
<enebo[m]>
well there could be but not from this since optional kwargs would be -2 for that I think
<headius[m]>
that would be arity 0 or -1 I guess
<headius[m]>
yeah
<headius[m]>
|a, b:1| would be negative arity also I guess
<enebo[m]>
Today's investigation does tell me we can probably make direct invoke another type to kill some checks AND we can probably cache arityValue assuming we actually need it without that second time
<enebo[m]>
1 for a and *-1 - 1 for b
<enebo[m]>
hmm for b:1 is -1 and a, b:1 is -2?
<enebo[m]>
regardless opt args make it all negative
<headius[m]>
yeah so I can't think of a way to have arity 1 with kwargs other than what you have
<enebo[m]>
yeah it is what I think too but I am more keen right after I wake up and I may realize something else
<enebo[m]>
I will at least re-review it
<headius[m]>
ok
<headius[m]>
I guess we are all set otherwise
<enebo[m]>
So I am done for today. In the morning I will plan on figuring out why deploys fail if they fail for me locally
<headius[m]>
oh you will verify tomorrow... should I do a JNR pass and update everything?
<headius[m]>
I have some work time left today
<enebo[m]>
I don't think so unless there is something significant which we will benefit and not create a significant risk
<headius[m]>
hmmm
<enebo[m]>
At least if we want to do something where there is a chance we should get out of the habit of not doing it so close to release
<headius[m]>
these do not have the Java 9 fixes
<headius[m]>
the ones we currently ship
<headius[m]>
so they are like 6 months behind
<enebo[m]>
I don't even remember what those are
<headius[m]>
module names mostly
<enebo[m]>
oh so people on 9+ with full module support can work
<headius[m]>
yeah
<enebo[m]>
I mean at one level that would be nice but only if there are not like 5 other fixes changing sockets or whatnot
<headius[m]>
there were some issues with modules and the native jffi stub not loading in some restricted envs
<headius[m]>
I will try to review changes since these
<headius[m]>
will push a PR in any case and we can merge before or after
<enebo[m]>
ok
<enebo[m]>
We could also consider a .16 in a few weeks too
<headius[m]>
we always do 😀
<enebo[m]>
but I guess figure out how risky they are
<headius[m]>
ok maybe most of these actually are current
<headius[m]>
enebo: ok nevermind, I guess they are all up to date
<headius[m]>
there are some fixes since then that have not been released in JNR projects yet though
<enebo[m]>
cool
<headius[m]>
I will do a jnr release spin this evening and a PR for 15 to use those versions
<headius[m]>
but it is a much smaller increment now
<headius[m]>
enebo: actually scratch that too... someone pinged me this week about helping out with some JNR issues so I will work with them to figure out a release target