<headius> We do warn about the flag but maybe we could be more explicit in suggesting it be added to a global configuration
<headius> I thought about doing the configuration as a list of packages and modules to open but that seemed too specific and eventually someone would need to configure it differently
<headius> What do we need the opens flags for exactly?
<headius> The other answer of course is finding legal ways to do what we need to do or enhancing JRuby to have access to things that should have access to without extra config
<byteit101[m]> sometimes it's the reverse, like javafx reflectively building stuff. but inheritance/protected methods and fxml loading are the two ones I know of off the top of my head (run jrubyfx:master/samples/* with zulu jdk+fx to see the ones that fail, notably contrib/fxmlloader)
<byteit101[m]> contrib/concurrenty_demos/service is another one that fails (protected method)
<byteit101[m]> Both of those generate no warnings/information about missing flags
<byteit101[m]> headius: basic jrubyfx opens are -J--add-opens=javafx.graphics/com.sun.javafx.application=org.jruby.dist -J--add-opens=javafx.graphics/javafx.stage=org.jruby.dist
<byteit101[m]> the full list is like 20 or so for each package and namespace
<byteit101[m]> *module
<headius> In retrospect I guess this is a downside of us no longer binding protected methods that can't be opened
<headius> We could reverse that and warn if the method has not been opened, or potentially fix the base problem and calculate whether the call is being made from within a subclass
<headius> The latter is obviously the trickier bit because we only pass the caller object so far through the call path
<headius> I am not in the office at the moment but it would be good to know what percentage of these would be fixed if dealt with protected methods better
<headius> This is the sort of project we could look at fixing for JRuby X
<byteit101[m]> generated code should work, I think, for accessing protected methods
<headius> We can also use invoke dynamic to do it, there's a way to call a method given a particular calling class
<headius> Since we know we will have a subclass if we intend to call protected methods, we just need to get a special invoke dynamic lookup object that lets us access those methods
<headius> Generated works too but a lot of these calls will still end up going through dynamic plumbing
<headius> And we don't necessarily want to generate stubs for every possible protective method because then we're basically just opening those methods up
<headius> Anyway, we have options
<kares[m]> Have a PR for review: https://github.com/jruby/jruby/pull/7164
<kares[m]> a follow up of the JavaClass cleanup that happened in 9.3,
<kares[m]> NOT PLANNING TO MERGING THIS TO 9.3 only master - this served as a confirmation that all tests are green ...
<kares[m]> all the commits pick cleanly to master - so once approved I will close and push to master
<enebo[m]> kares this is not in your PR but can I ask you do check something for me? RubyClass.isReifiable passes in an out param but in looking at this I don't see how the outparam is ever different than the boolean return
<enebo[m]> byteit101: I think your original version of this method did have this distinction but it went away
<kares[m]> I've seen isReifiable(boolean[]) still assigning to the param, so I left it as is ... haven't checked further
<enebo[m]> kares: I noticed original commit did seem to have both possibly have different results but now I cannot see how it is possible. You could throw it into your PR since that code is newish (only in one point release)
<enebo[m]> unless I am wrong 👀
<kares[m]> that part I only touched lightly - mostly renamed some fields ... it's not really the gist of the PR
<enebo[m]> yeah you don't have to but I noticed it as part of looking at your PR and if we can get consensus it is not needed it could be filed under cleanup (like renaming fields in said code which was unrelated to your PR)
<enebo[m]> I can also do it later too though
<kares[m]> do not have the ide open now as this PR has been finished a day or two back but I can revisit later, once merging
<enebo[m]> ok it is not that important but when I saw it I wondered why we needed an out param
<kares[m]> yeah same and I started following the code but got a bit lost there
<kares[m]> how's it going btw?
<enebo[m]> I thought perhaps it have multiple versions of that method but nope. Then I noticed original version could have different values
<enebo[m]> Well I went deep down a rabbit hole on generating ripper from the parser using a dsl language in MRIs parser
<enebo[m]> I think I know what I will do to make it past that point but to get to that point was a heinous amount of effort
<enebo[m]> I can now generate the parser but I got to a point where defs need to stash state and restore it later but all the types in it are basically IRubyObject
<enebo[m]> If it pans out the work for updated ripper will be pretty small though
<kares[m]> ooh what ripper and parser will have the same backend?
<enebo[m]> they sort of do already but they are manual copies of each other with different java types
<kares[m]> that sounds amazing ...
<enebo[m]> ripper lexer is ruby lexer with some slightly different logic in a few places
<enebo[m]> That will still be true for now (copies of the same lexer) but it is getting smaller using a common class
<kares[m]> yeah that's what I meant - recall some differences and taking a different path on a place or two
<kares[m]> well except for the long ERRRRRRRRRRRRR line 😉
<enebo[m]> There are some conceptual ugliness too :)
<kares[m]> reads nicely
<enebo[m]> These erbish lines with ripper: is a special DSL but some of the code is actually C calls which the dsl will also do its best
<enebo[m]> In theory once I get done I could submit something back to MRI to make this less of C calls and more some common names. The main issue for C is they are not doing variadic stuff so you see number of params as a first parameter for some calls or a different name rb_ary_new3
<enebo[m]> since they are also running a dsl in Ruby they could translate 3-arg new_array to rb_ary_new3 but then they have the more complicated dsl code than currently so I am not sure they will think this is an improvement
<byteit101[m]> enebo: re: RubyClass.isReifiable out param if the class is a non-ruby class it should be true, while if it's a ruby-derived class it is false. lines 1264-1268
<byteit101[m]> kares: the out param is just because I wanted to avoid making a struct class so I could return multiple values (isReifiable in general, is our parent java-based or ruby-based?). If you want to clean that up go to use a struct class, feel free
<enebo[m]> byteit101: Read the code and tell me how the return result can be different
<enebo[m]> byteit101: When you first landed it I could see how that would be but I cannot now
<enebo[m]> byteit101: byt I could be mistaken
<enebo[m]> Kwarg issue of the day
<enebo[m]> We mark last argument for kwargs with flags keyword_argument, keyword_rest_argument.
<enebo[m]> During argument processing in the method called we look at these flags
<enebo[m]> We must reset these flags (set back to false) when we are done processing
<byteit101[m]> enebo: (true, false) when superClass.getRealClass() == RubyBasicObject.class, (true, true) when superClass.getRealClass() == ArrayList.class
<enebo[m]> ok I see it now
<enebo[m]> byteit101: thanks for pointing that out. I stared at that and my mind should have figured that out but didn't
<enebo[m]> kares: ignore my comment about not needing out param ^^
<enebo[m]> Right now this kwargs logic is failing to do this reset in some places
<enebo[m]> parameter binding in a method is just instrs in our IR and by the end of that processing we want to reset this flag info before executing rest of method (although I think this may only be neccesary for kwrest)
<enebo[m]> optional values to parameter values means executing calls (well potentially other things too) but that means we could raise an exception
<enebo[m]> what would be ideal would be try { param_binding_instrs } finally { reset } but we do not handle this section as special
<headius> kares: looks good from your description, I will have a deeper look to review
<headius> enebo: stringio and strscan contribs have all finally consented to license so I don't think there's anything left on my end to release those
<enebo[m]> headius: great!
<headius> I'm going to start looking into PRs to have openssl -java gem just depend on jruby-openssl
<headius> I don't think anything in Rails depends on openssl gem yet but this is going to happen sooner or later
<enebo[m]> I am wrestling with kwargs but it is productive but not figure out fully
<enebo[m]> two but sentence
<headius> check under the collar
<enebo[m]> It is in the collar for sure
<enebo[m]> I am going to knock off but I am going to do a writeup on the changes to kwargs in IR
<enebo[m]> kwargs = recv_kwargs_arg will be an instruction for kwargs accepting methods before check_arity
<enebo[m]> If kwargs are not present it will basically be undef as a value
<enebo[m]> check_arity will receive this as an operand
<enebo[m]> all recv_* will also recieve this
<enebo[m]> Before this was all implicit which caused us to constantly check last argument
<enebo[m]> So we indirect through a temp but we eliminate that
<enebo[m]> but it more directly solves a 3+ issue in the current IR...we can unmark any kwargs flag on that argument when we call recv_kwargs_arg
<enebo[m]> This should eliminate us ever leaking those flags out past the beginning of the method
<enebo[m]> The interesting problem of kwargs in 3 is they still are permissive of things you may not expect like: def m(a); end; m(**{a: 1})
<enebo[m]> With the new separation you would half expect this to error but it instead erases kwrest and treats it as an ordinary hash
<enebo[m]> this is why all recv instrs will need to receive this since it might really be an ordinary hash even though it was actually passed in as a kwrest
<headius> That sounds good
<headius> Then methods will have a kwargs descriptor and I can look at making fast path Indy calls
<lopex[m]> I thought it was a writeup already
<lopex[m]> does that insanity also affect pattern matching ?