[jvm] Fix SAM#12903
Merged
Merged
Conversation
…sition SAMs Revert the AbstractCast/Common/Gctx parts of a134fd8. That commit dropped the functional_interfaces_used hashtbl on the claim that the genjvm AST scan in collect_used_functional_interfaces "strictly subsumes" the AbstractCast writeback. It does not. AbstractCast.do_check_cast's SAM branch unifies eright.etype against the SAM signature and returns eright **unchanged** — no TCast wrapper. So when a function expression is passed where a Java SAM is expected, the SAM TInst never appears in the typed AST. The only place it exists is on the extern callee's signature, which the scan deliberately skips (`if not (has_class_flag c CExtern)`) to keep classpath noise out. For TCall sites this is masked: the callee TField has a TFun etype whose parameter types are visited, so the SAM TInst is reached transitively. But TNew has no callee subexpression — the constructor's parameter types live on c.cl_constructor.cf_type, which Type.iter never visits. A bound instance-method reference passed to a Java extern's constructor (e.g. `new TextToSpeech(this, onTtsInit)` against android.jar) therefore produces a closure that does not implement the SAM interface, hitting ClassCastException at runtime. Restore the field on Common.context + Gctx.t (shared in Common.clone), the AbstractCast writeback keyed by @:native path, and seed collect_used_functional_interfaces's `used` hashtbl from it before the AST scan runs.
…erence The existing cases either bind to a typed local (SAM TInst lands in the AST directly) or call a static method (TCall callee's TField has a TFun etype that exposes parameter types to the scan). Neither exercises the path that AbstractCast's functional_interfaces_used writeback is needed for: a bound instance-method reference passed to a Java extern's constructor, with the SAM type never named anywhere else in user code. Add CtorOnly + CtorSam to test.Listeners and a Holder class in Main.hx that does `new Listeners_CtorSam(onCtor, 42)`. Without the writeback the emitted closure does not implement CtorOnly and the JVM throws ClassCastException at runtime — same shape as RideAssist's `new TextToSpeech(this, onTtsInit)` crash against android.jar.
…e in AST Replace the functional_interfaces_used hashtbl plumbing (Common.context + Gctx.t + AbstractCast writeback + genjvm seeding) with a one-line change in AbstractCast: wrap the function expression in mk_cast — a TCast(eright, None) with the SAM type as the cast's etype. The SAM TInst now lives in the typed AST at every conversion site, so genjvm's existing collect_used_functional_interfaces scan finds it naturally via note_fi_in_type on the TCast's etype. No cross-module mutable state, no typing↔codegen back-channel, no @:native path-rewrite bridging. mk_cast is already the established pattern in this file — it's what the MultiType abstract path uses a few lines up.
Member
|
Is this actually a regression from #12901 though? In that case I'd be curious what exactly caused the behavioral change. |
Contributor
Author
Contributor
Author
|
That mkcast looks way better to me than the previous |
Member
|
I'm not sure because relying on the presence of a type of a cast expression seems a little brittle. It introduces the potential for the AST node to be optimized/mapped away which would then lead to subtle failures. The explicit lookup definitely looks more dorky, but it might also have been more robust. But I don't have particularly strong feelings in either direction. As long as the tests cover this I'm fine with the change as-is. |
Contributor
Author
|
Merging this, as current situation is bad (support has been added for SAM but it can crash at runtime). I'm using this feature a lot, I should notice early if such case can happen, and I would then go back to it and find a more robust solution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We cleaned up a bit too much #12901 ; I quickly found a runtime error after merging... 😅