-
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
Detect case where two alternatives are the same after widening ExprTypes #18787
Conversation
In implicit or extension method search we might get two alternatives that are different but that point to the same singleton type after widening ExprTypes. In that case we can arbitrarily pick one of them instead of declaring an ambiguity. Fixes scala#18768
TY for the fix 🙏 |
@benhutchison You are very welcome! The good minimization helped a lot. |
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.
LGTM, just some performance questions. Shall we run benchmarks seeing as we're adding the usage of =:=
and widening to what was originally just int and boolean comparison?
if (winsType2) 0 else 1 | ||
if winsType1 && winsType2 | ||
&& alt1.widenExpr =:= alt2.widenExpr | ||
&& alt1.widenExpr.isStable |
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.
Check is isStable
first, for performance?
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.
Yes, good idea.
I think with the swap in conditions you suggested there are no worries about performance anymore. Merging |
In implicit or extension method search we might get two alternatives that are different but that point to the same singleton type after widening ExprTypes. In that case we can arbitrarily pick one of them instead of declaring an ambiguity.
Fixes #18768