Add nullable JS API parameter support #1756
Conversation
|
I would like to run this idea with you before I fix the module (it doesn't export Kotlin dependencies, so the clients will need to explicitly add those) and add unit tests. |
drubanovich-soti
force-pushed
the
nullable-parameters
branch
from
24ac32e to
7b00220
Compare
|
Thanks for working on this! This is an interesting idea, and it'd be great to have Rhino work better with Kotlin. However, right now Rhino has no dependencies, and I think it's important that we keep it that way. This PR makes Rhino depend on a Kotlin package. I don't know how intrusive that is (like how many nested dependencies it brings in) but I'm still very reluctant to make a core library like this something that Rhino requires. Is there another way to do this, like making the dependency optional and searching for it via reflection, or adding a "rhino-kotlin" module that's loaded via ServiceLoader, or incorporating the part of the Kotlin library that Rhino needs in to the project directly? |
|
Sure Greg, if you like the idea, I will work on a proper implementation. |
|
One idea might be to add a new module, like "rhino-kotlin", that Kotlin
users can add... Just one of many possible ideas.
I'm hoping that we can release a "1.8.0" soon and this might not make it in
that release, but we can get to it soon after!
…On Sat, Dec 14, 2024 at 7:57 AM drubanovich-soti ***@***.***> wrote:
Sure Greg, if you like the idea, I will work on a proper implementation.
I'll try your ServiceLoader suggestion with Kotlin-specific implementation
in a new "rhino-kotlin" module.
—
Reply to this email directly, view it on GitHub
<#1756 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I27Y4BMEMIS4HEERPFT2FRIO5AVCNFSM6AAAAABTSNARF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBTGE3DKNBUHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
drubanovich-soti
force-pushed
the
nullable-parameters
branch
from
7b00220 to
d365134
Compare
|
I finally got some time to work on it. I tested it with my project and it works fine. |
|
I think that this approach will work -- having a separate "rhino-kotlin" module sounds good to me. Of course, I really think we need to find a way to build some test, even if it only runs as part of our CI, to actually exercise this code with Kotlin. Also, I don't necessarily understand why there is a new module called "rhino-reflection." Would this work if we only added the new "rhino-kotlin" module? |
drubanovich-soti
force-pushed
the
nullable-parameters
branch
from
d365134 to
d703bd0
Compare
|
I've added Kotlin tests in the third commit, note that it brings in several Kotlin dependencies though.
I can make NullabilityDetector interface to be a part of rhino. A separate "rhino-reflection" interface module gives more flexibility to clients though. E.g. clients who don't want to use Kotlin but still want to create JS functions with nullable primitive parameters can write their own NullabilityDetector implementation which relies on their custom Nullable annotations. P.S. After I wrote that I realized both "flexibility advantages" could be also achieved without a separate rhino-reflection module. The only minor advantage of rhino-reflection is that rhino-kotlin (and any custom extension) have more focused dependencies. I will work on merging rhino-reflection into rhino. |
|
I think that we should move the contents of "rhino-reflection" into the main "rhino" package. It's a very small amount of code and I don't think we lose anything in terms of flexibility by having it be part of the main module. Then we can still provide the behavior that the new behavior only is triggered when the "rhino-kotlin" package is present. |
drubanovich-soti
force-pushed
the
nullable-parameters
branch
3 times, most recently
from
170ab77 to
44e102b
Compare
|
Hi Greg, I added the tests and addressed the comments. |
| } | ||
|
|
||
| public NullabilityDetector getImpl() { | ||
| Iterator<NullabilityDetector> iterator = loader.iterator(); |
There was a problem hiding this comment.
I (very recently) added ScriptRuntime.loadOneServiceImplementation to handle this case and throw an error if there are duplicates (to avoid inconsistency if the classpath is weird). Can you look at using that? It's package-public but could easily be public.
|
|
||
| @Test | ||
| public void checkArgumentConversion() { | ||
| Context context = new Context(); |
There was a problem hiding this comment.
That context constructor is deprecated with a good reason. Take a look at the Javadoc for it. You can either use "Context.enter()" in a "try-with-resources" block, or better yet you can use Utils.runWithAllModes, which will test your stuff in both interpreted and compiled mode.
This change allows to define JS APIs which receive nullable primitive type parameters, by overriding NullabilityDetector interface. E.g. clients can introduce Nullable-like annotation which they can read at runtime, thus enabling passing null to JS function with string parameter (by default such function receives a string "null") One of the following commits will introduce KotlinNullabilityDetector, which will rely on parsing Kotlin metadata and will allow a better inter-operation of Rhino with Kotlin.
KotlinNullabilityDetector relies on Kotlin Metadata to detect parameter nullability. The limitation of this detector is that it cannot distinguish between overloads with the same number of parameters. To avoid false positives, KotlinNullabilityDetector returns array of 'false' values for methods or constructors with such overloads.
drubanovich-soti
force-pushed
the
nullable-parameters
branch
from
44e102b to
7f7c970
Compare
|
Back for the review |
|
This looks good to me, and thanks for doing all this work! |
This change allows to define JS APIs which receive nullable primitive type parameters. E.g. before the change it was not possible to pass null to JS function with string parameter, instead the function received a string "null".
As standard Java doesn't have nullable types, this feature currently can only be used if the files are written in Kotlin. Kotlin metadata library is used to detect nullable parameter types.