Skip to content

Only generate generic signature if it differs from descriptor#25880

Open
SolalPirelli wants to merge 2 commits intoscala:mainfrom
dotty-staging:solal/gensig-if-needed
Open

Only generate generic signature if it differs from descriptor#25880
SolalPirelli wants to merge 2 commits intoscala:mainfrom
dotty-staging:solal/gensig-if-needed

Conversation

@SolalPirelli
Copy link
Copy Markdown
Contributor

Fixes #10837

Perhaps someone can tell me why this is a bad idea.

How much have you relied on LLM-based tools in this contribution?

Not at all

How was the solution tested?

Manual tests because writing automated tests is impractical, described below (in detail)

I ran scalac on the repro from the issue and looked at the output of javap -v on both A and A$. I don't know how to check for the existence of a signature in a non-generic method programmatically without importing ASM in the test which seems too heavyweight.

val gensig = getGenericSignatureHelper(sym, moduleClass, memberTpe).orNull
if gensig == descriptor then null
else gensig
else null
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not pretty, but this method belongs in the lowest circle of Hell anyway and should disappear if I ever have the courage to clean up mirror generic signature generation.

@SolalPirelli SolalPirelli requested a review from lrytz April 21, 2026 07:53
@lrytz
Copy link
Copy Markdown
Member

lrytz commented Apr 21, 2026

In Scala 2 this is handled by needsJavaSig returning false. Probably that should also be false in Scala 3 for the varargs forwarder?

@SolalPirelli
Copy link
Copy Markdown
Contributor Author

I could've added another special case but I didn't get why we even have this "needs generic signature" test, there are probably other such cases of methods that shouldn't have a generic signature but do right now, it's hard to test, and all tests pass with this change.

Is there some drawback to the "always generate unless it's been turned off or it's unneeded" approach that I'm not aware of?

@lrytz
Copy link
Copy Markdown
Member

lrytz commented Apr 21, 2026

I could've added another special case

But it shouldn't need a special case; needsJavaSig should be false for (Array[String]): String

I didn't get why we even have this "needs generic signature" test

I don't know either... maybe git history would reveal something.

From the top of my head what you say makes sense, we don't need a signature that duplicates the descriptor. But it feels a bit like patching things up given there's the needsJavaSig check already.

@SolalPirelli
Copy link
Copy Markdown
Contributor Author

But it shouldn't need a special case; needsJavaSig should be false for (Array[String]): String

The reason it's currently true isn't the type itself but the fact that method is considered a bridge, and for some reason we generate generic signatures for all such methods. (I think it might also have the artifact flag... I don't remember exactly since I looked at it)

As to why we check these flags, it dates back to the introduction of GenASM (i.e., it was in the commit that added that file), so I guess we'll never know 😅

@SolalPirelli SolalPirelli added the needs-squashing PR whose commits should be squashed by the author or via the "Squash and Merge" button label Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-squashing PR whose commits should be squashed by the author or via the "Squash and Merge" button

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Needless (incorrect?) Signature added to Java varargs methods

2 participants