add keyword arg support to callMethod directly#9403
Conversation
|
I like this and I also like the PR only pushes this into embedding API itself. We have a long running task to remove callInfo and to make kwargs more first class and not based on some out-of-band field set per thread. My only thing I wonder about is that Map<String, Object> would somehow be nicer if it could be String or RubySymbol for people who already know they are directly interacting with Ruby from an embedded context. Not sure I am recommending another set of overloads here or whether that case is solved by our larger effort to rewrite kwargs but it seems like a reasonable use case (also it probably could be added as a later PR). |
|
Yes, this seems like a reasonable addition. @enebo To your point, we could use Map<CharSequence, Object> since RubySymbol and RubyString also implement CharSequence. Dunno if it's worth it or not. I'll review. |
headius
left a comment
There was a problem hiding this comment.
Generally on the right track. I think it's worth discussing whether it's safer/cleaner to just add new method names for the kwargs calls rather than more complicated overloads that might be ambiguous in some cases.
|
|
||
| // --- Keyword argument support --- | ||
|
|
||
| public Object callMethod(Object receiver, String methodName, Map<String, Object> kwargs) { |
There was a problem hiding this comment.
I have some vague concerns about this signature and the Object... signature above co-existing. There's some cases that might end up ambiguous in existing code (even though these are a little weird)
Object[] ary = ...
Map<String, Object> map = ...
callMethod(recv, name, ary, map); # ambiguous callSo I'm wondering if it might be better to give these methods different names to ensure it's clear you're calling with explicit keywords. We could potentially even keep the Object... for positional args:
callKeywordMethod(Object receiver, String methodName, Map<String, Object> kwargs, Object... posArgs)There was a problem hiding this comment.
After mulling this over a bit, I'm in agreement that a new name would be better to avoid confusion and properly disambiguate when Object or Object.. are involved in the callMethod sig. That does mean we'd likely want to (re-) implement similar arg permutations of the preexisting API but with keyword args (hopefully with an additional goal of reducing ambiguity.)
Personally I'm not a huge fan of Object and 'Object...' in user targeting API signatures as it leads to disambiguation problems like this one. The worst culprit for ambiguity in this particular case is likely the generics form where the last arg is the Class Type token as you point out. Those can be interpreted as one of the Object... elements, since java.lang.Class also extends Object iirc. I'm honestly not sure why it compiles and runs now that you point it out. It's certainly not intuitive regarding which one "wins" (at least to me, and I wrote the new APIs 😅)
Maybe we just start with List for ordered arguments and a Map<CharSequence, Object> for keyword args, ending with the return Type for for a T like:
T callMethodWithKeywords(Object receiver, String methodName, List orderedArgs, Map<String, Object> keywordArgs, Class type)
If folks find it useful we may want to add more permutations in later iterations.
Random thought: keeping the "callMethod" method name prefix would probably be more auto complete friendly / discoverable to users familiar with the existing APIs than callKeywordMethod, but I defer to you on naming here.
There was a problem hiding this comment.
An alternative to the ... Vararg support we could provide 1..N arg permutation override methods and have the last arg always be the type for consistency across the new API signature-wise. N can be sufficiently small to cover 95pct use and outliers just use the array arg version which the N permutation APIs would support >N outlier cases. Like N=5 or so:
N=1
T callMethodWithKeywords(Object receiver, String methodName, RubyObject ro1, Map<CharSequence, Object> keywordArgs, Class type)
...
N=5
T callMethodWithKeywords(Object receiver, String methodName, RubyObject ro1, RubyObject ro2, RubyObject ro3, RubyObject ro4, RubyObject ro5, Map<CharSequence, Object> keywordArgs, Class type)
N>5 user just use the list one:
T callMethodWithKeywords(Object receiver, String methodName, List orderedArgs, Map<CharSequence, Object> keywordArgs, Class type)
There was a problem hiding this comment.
An alternative with a much larger deviation from the current API would be to support a first class scripting Parameters class that could be constructed with a fluent API or similar...
| finalArgs = new IRubyObject[rubyArgs.length + 1]; | ||
| System.arraycopy(rubyArgs, 0, finalArgs, 0, rubyArgs.length); | ||
| finalArgs[rubyArgs.length] = kwargsHash; | ||
| context.callInfo = ThreadContext.CALL_KEYWORD; |
There was a problem hiding this comment.
I think we should also defensively clear context.callInfo on the way out so it doesn't get stuck if we call a method that behaves badly. Perhaps also save existing callInfo and restore it, but I don't think we should go too far in trying to preserve callInfo for Java embedding API consumers.
There was a problem hiding this comment.
Yeah probably in a finally block. I can try and get to this sometime this week
started a discussion here: #9402, copying verbatim here for reference:
I've been using JRuby to wrap some pre-existing Gems to be called by Java using the ScriptingContainer interface provided by JRuby 10.x, however, whenever a Gem uses keyword arguments there doesn't seem to be a clean way to force a Map to be treated as the keyword arguments for that Ruby API. I see theres a ThreadContext.CALL_KEYWORD flag I can use, but it seems more an internal implementation detail than something that should be exposed to scripting users. Afaict this works:
What I'm proposing is adding further callMethod method overloads to include a last argument of Map<String, Object> or similar that will become those keyword arguments (convert strings to symbols automatically, call convertJavaToUsableRubyObject() on values, etc. My guess on why this hasn't come up before is that any scripting use cases predated Ruby's 3.2 order argument changes around auto-conversion of the last hash into keyword args. Since 3.2+ that behavior is disallowed so some other mechanism needs to exist going forward. I think what I propose makes sense here, but i could be missing something important.
This is my attempt at implementing it