<enebo[m]>
Jason Lunn: we had an issue where some code paths would check twice and at other times it would not check at all
<enebo[m]>
I find this aesthetically unpleasing but for IRubyObject[] signatuers you need to add an explicit check like done in the various changes in that PR
<JasonLunn[m]>
This is a breaking change for existing code,
<JasonLunn[m]>
not just code that was built on 9.4.3
<enebo[m]>
yeah
<JasonLunn[m]>
a minor version doesn't seem like the ideal place to introduce that new behavior...
<enebo[m]>
this is a conversation headius and I will have but some times I think we get tunnel blind and consider our use of annos as the only thing
<enebo[m]>
more or less any arity can be passed to any native extension which means code assuming a number of args can crash
<enebo[m]>
so I get the gravity of the problem
<headius>
g'day
<headius>
looks like I broke something?
<JasonLunn[m]>
Just a little :-)
<headius>
Jason Lunn: so there were two reasons for the original change
<headius>
the first was to let the target method be at the top of the stack trace as in MRI... we have not matched that for years and it is confusing and has been reported repeatedly
<headius>
the other was due to frequent bug reports about calls to Java-based Ruby methods that accept IRubyObject[] and never check arity... even in JRuby they would blindly access the array assuming the Ruby method binding had done the check
<headius>
the latter is what broke this behavior; where the bindings would add arity-checking before, they do not now expecting that anything with variable arity should be doing its own checking
<headius>
I agree somewhat this is maybe a big change to introduce in a minor release but it was to fix some long-standing behavior differences from MRI
<headius>
it's also worth pointing out that MRI method bindings in C also require you to do your own arity checking, so this aligns with that behavior as well
<headius>
enebo: an idea: there could be an arity-checking field on the annotation that allows methods to opt out of automatic arity checking
<headius>
all the core JRuby methods would do it because they gained arity checks in the code, but any extensions that don't opt out would get automatic checking for varargs paths
<headius>
I'll put this info in the bug
<JasonLunn[m]>
I definitely understand why this needs to change long term
<JasonLunn[m]>
I'm just saying this introduces a problem felt not only by 3rd party extend devs like us,
<JasonLunn[m]>
but by end users who thought it would be safe to update from 9.4.2 to 9.4.3 without a headache
<headius>
so there was code depending on the error?
<JasonLunn[m]>
I like the opt out idea in the short term
<JasonLunn[m]>
and in 9.5 you could announce plans to phase it out
<headius>
I figured this would not be a likely breakage for any users because the code would have errored before and not run properly anyway
<JasonLunn[m]>
There was a code doing checking on the var args for the optional arguments,
<JasonLunn[m]>
but because it was also specifying required args,
<JasonLunn[m]>
it made the assumption that it was safe to access the low indices of the args array that were formerly guaranteed to be there
<headius>
so the code calling this method without sufficient arguments was itself broken, yes?
<JasonLunn[m]>
I mean, in our case, we caught it a unit as we were introducing 9.4.3 into CI
<headius>
so there was a test testing that this error got raised?
<JasonLunn[m]>
Yes - and a typed error at that,
<JasonLunn[m]>
so it got mad that it was no longer an ArgumentError,
<JasonLunn[m]>
but a Java index out of bounds error
<JasonLunn[m]>
I've patched our test,
<JasonLunn[m]>
and we're not currently being held up by it
<headius>
ok I understand how it failed for you then
<headius>
it should not affect normal usage since it would have been broken for code to trigger this arity error, but I agree we should mitigate this with the opt-out flag
<headius>
I need to go back through the JRuby methods with varargs and make sure they opt out now
<headius>
enebo: should this use true/false or should I introduce an annotation AUTO, MANUAL if there are other arity checks we want in the future?
<headius>
I'm leaning toward YAGNI and this is just for a transitional period until we expect users to check arity themselves
<enebo[m]>
headius: I think simpler the better
<enebo[m]>
but what other possibilities do we see?
<enebo[m]>
just blue sky do you see 'checkArity=org.heh.CustomValidator'?
<enebo[m]>
we can just embed a MH chain as text and compile it :)
<headius>
yeah that sort of thing did come to mind
<headius>
but also keyword args checking maybe
<headius>
I think that would be better with expanded @Keyword line annotations though
<headius>
@Keyword like annotations
<enebo[m]>
yeah I have wondered more about just using IRubyObject foo(ThreadCointext context, @KeywordOpt IRubyObject freeze) {}
<Davinci[m]>
<headius> "Davinci: Did you find a way to..." <- There is a form, avoiding high level methods and reproducing its code changing parser code size limit. It seems like garbage, and perhaps it is more fragile and less concise code, but it works for now. But I think we should not be forced to program an intricate solution for doing a so normal thing as loading an yaml file because its size is big. It breaks the less surprise principle.
<JasonLunn[m]>
<headius> "Jason Lunn: basic fix is here..." <- Is the long term fix for 3rd party devs to use the same Arity.checkArgumentCount() call that is being used internally now?
<headius>
Davinci: yeah I am not sure why the limit is so low by default; it could be much higher without risking a serious DOS
<headius>
Jason Lunn: yes, or just check the array size yourself and raise an appropriate error
<headius>
the utility methods we provide are probably easiest
<JasonLunn[m]>
Just wanted confirm that you intend for the utility methods to be part of the long term supported public API surface before I bake them into the gem
<headius>
yeah they aren't going anywhere
<headius>
enebo: blessed API ahem
<headius>
enebo Jason Lunn PR now has the opt-outs in place for all core varargs methods
<headius>
oops I missed some require + rest
<headius>
there fixed
<headius>
even added a simple test... feel free to expand it to other cases
<headius>
enebo: half day today but late last week I think I figured out the path to the rubygems API logic in mavengem
<headius>
it's a bit involved but I think I can hook it up to the newer API, and it won't scan all versions of a given gem anymore, just the specified version
<headius>
so that will be a nice reduction in noise during the full build