Emit generic signatures for all trait fields#25780
Conversation
SolalPirelli
added
the
needs-squashing
| // since that information never existed. | ||
| // Thus, we first check if the symbol was specifically marked as having generic information, | ||
| if sym.is(MixedIn) then mixinPhase.asInstanceOf[Mixin].mixinGenericInfos.get(sym) match | ||
| mixinPhase.asInstanceOf[Mixin].mixinGenericInfos.get(sym) match |
There was a problem hiding this comment.
Adding MixedIn to everything that's mixed in causes so many tests to fail... and is not necessary here anyway
| private object PrivateObject | ||
| } | ||
|
|
||
| object Test extends T |
There was a problem hiding this comment.
regression test for some crashes while developing this
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
The approach looks good 👍 while module class bar$ is fixed, the class bar's generic signature is not yet fixed.
| classOf[bar.type].getMethods.sortBy(_.getName).filter(_.getName.contains("foo")).foreach(m => { | ||
| println(m) | ||
| println(m.toGenericString) | ||
| }) |
There was a problem hiding this comment.
How about Class.forName("bar") ? The generic signature for bar.foo() seems still a raw type based on javap (above is testing for bar$).
There was a problem hiding this comment.
Indeed... There's a whole separate way to create generic signatures for mirror classes. I've spent ~10h trying to fix this and gone nowhere because the interaction of erasure and mixins means we fundamentally have incorrect information once we reach the backend, so we end up in cases where descriptors and generic signatures diverge in ways that aren't correct, e.g., the gensig has String where the descriptor has Object.
I suggest we open an issue and accept this for now.
There was a problem hiding this comment.
Hmm, for Java interop, the relevant entry point is bar.foo() static forwarder, not bar$.foo(). If the generic signature for bar.foo() remains raw type, I think we should keep #24275 open for tracking the issue.
On the other hand, this PR seems fixing #6350 ? We might wanna add a test for #6350, merge this PR and close it, and keep #24275 open?
There was a problem hiding this comment.
(I've changed the description to say "part of #24275" instead of "fixes")
SolalPirelli
force-pushed
the
solal/trait-field-generic-signature
branch
from
abea633 to
7423013
Compare
| public void Bar.Foo$_setter_$foo_$eq(scala.Option) | ||
| public void Bar.Foo$_setter_$foo_$eq(scala.Option<java.lang.String>) | ||
| public scala.Option Bar.foo() | ||
| public scala.Option<java.lang.String> Bar.foo() |
Part of scala#24275 Fixes scala#6350 ## How much have you relied on LLM-based tools in this contribution? not ## How was the solution tested? issue reproducer as automated test
2 participants
Part of #24275
Fixes #6350
How much have you relied on LLM-based tools in this contribution?
not
How was the solution tested?
issue reproducer as automated test