Skip to content

Fix desugaring of context bounds in extensions #11623

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

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Conversation

rjolly
Copy link
Contributor

@rjolly rjolly commented Mar 5, 2021

Fixes #11586
Unfixes #11583

@neko-kai
Copy link
Contributor

neko-kai commented Mar 5, 2021

I guess the change should avoid breaking #11583 to have a chance...

@rjolly rjolly marked this pull request as draft March 8, 2021 10:08
@rjolly
Copy link
Contributor Author

rjolly commented Mar 8, 2021

@neko-kai see my comment in #11584

@rjolly rjolly force-pushed the master branch 2 times, most recently from d5fbf96 to e462689 Compare March 8, 2021 16:44
@smarter smarter requested a review from bishabosha March 8, 2021 16:49
@rjolly rjolly force-pushed the master branch 4 times, most recently from 45c2226 to 0524d25 Compare March 8, 2021 20:54
@rjolly rjolly marked this pull request as ready for review March 8, 2021 22:47
@rjolly rjolly requested a review from bishabosha March 10, 2021 11:00
@bishabosha
Copy link
Member

bishabosha commented Mar 11, 2021

I find the main issue here is that this change results in a worse search for eligible extensions (from the same source) than the status quo.
If the compiler never improved search for extension methods, (where there is a context parameter after the extended parameter), would it be worth all the manual boilerplate to replicate the previous desugaring and get the old behaviour?

@neko-kai
Copy link
Contributor

@bishabosha I think the desugaring is not at fault here since you're going to get the same type inference behavior if you were to write using Functor[F] instead of : Functor.

Previous desugaring would merely mask the issue by making context bounds infer "better" by making them more than just a short-hand which they are usually, then cause surprising change in behavior if a context bound was replaced with an explicit using/implicit clause.

@bishabosha
Copy link
Member

@bishabosha I think the desugaring is not at fault here since you're going to get the same type inference behavior if you were to write using Functor[F] instead of : Functor.

Previous desugaring would merely mask the issue by making context bounds infer "better" by making them more than just a short-hand which they are usually, then cause surprising change in behavior if a context bound was replaced with an explicit using/implicit clause.

Sorry, I should perhaps rephrase my original comment, I am in agreement in principle with the new proposed desugaring. However I am concerned that perhaps it should not go forward right now, purely because an extension such as

extension [F[_], A](f: F[A])(using Functor[F]) def ...

can not be found as easily by the compiler (therefore leading to more boilerplate to put all using clauses at the end of extension methods, a.k.a - how context bounds desugar before this PR).

@neko-kai
Copy link
Contributor

However I am concerned that perhaps it should not go forward right now, purely because an extension such as can not be found as easily by the compiler (therefore leading to more boilerplate to put all using clauses at the end of extension methods, a.k.a - how context bounds desugar before this PR).

Yeah, but you'd lose that property immediately if you wrote an explicit using instead of a context bound anyway. I think ripping the band-aid early, while there are still very few users that would be impacted is better than waiting until there are even further impediments to this, such as binary compatibility, that would cause a temporary change 'for convenience' to be stuck in the language forever and become another embarassing inconsistency that has to be explained over and over to anyone who encounters it...

@rjolly rjolly force-pushed the master branch 3 times, most recently from 00d9fa6 to 8ce882f Compare March 13, 2021 18:30
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

LGTM

@bishabosha bishabosha merged commit 823206c into scala:master Mar 15, 2021
@bishabosha
Copy link
Member

@rjolly this PR was reverted after merging because it was decided to wait to fix issues like #11713 first

@rjolly
Copy link
Contributor Author

rjolly commented Mar 16, 2021

Yes, I saw that, thanks

@griggt
Copy link
Contributor

griggt commented Jul 10, 2022

Should this be revisited now that #11713 is fixed?

@bishabosha
Copy link
Member

would that be a scala 4 change?

@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context bound desugared at the wrong place in extension method
5 participants