-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle overloaded members when generating Java varargs bridges #13066
Conversation
* but that does not work, since `allOverriddenSymbols` gets confused because the | ||
* signatures of a Java varargs method and a Scala varargs override are not the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that allOverriddenSymbols
delegates to Denotation#matchingDenotation
which is missing the logic present in Denotation#matchesLoosely
to handle matching denotations across languages (java/scala2/scala3), I think this needs to be fixed but this is a good stop-gap meanwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today I learned something :)
@@ -85,7 +102,8 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase => | |||
private def isVarargsMethod(sym: Symbol)(using Context) = | |||
hasVarargsAnnotation(sym) || | |||
hasRepeatedParams(sym) && | |||
(sym.allOverriddenSymbols.exists(s => s.is(JavaDefined) || hasVarargsAnnotation(s))) | |||
overridesJava(sym) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version skipped the checks on overriden symbols when there were no repeated params (hence the parens). Is it intentional to change that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is missing a pair of parens
Fixes #13043