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

Dotty style downwards comparisons for implicit search and overloading resolution #6037

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
@milessabin
Contributor

milessabin commented Aug 13, 2017

This is a backport of lampepfl/dotty@8954026 to scalac. As in the Dotty implementation the specificity comparison is as-if all contravariant positions in type constructors were covariant. The feature is enabled by specifying -Xsource:3.0.

Initially this broke test/files/run/t2030.scala in exactly the same way as in Dotty as reported by @smarter here. Since then the landing of the new collections has fixed this.

This fixes scala/bug#2509. It also aligns with the contravariant behaviour hoped for in scala/bug#7768, although it differs on the covariant behaviour, which is unchanged. I think that's a reasonable outcome and that scala/bug#7768 can be declared as fixed.

@scala-jenkins scala-jenkins added this to the 2.12.4 milestone Aug 13, 2017

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Aug 13, 2017

Contributor

Nice!

The new specificity behaviour is behind flag -Ydotty-specificity.

I think -Xsource:3.0 was proposed for this sort of things: scala/scala-dev#374

Contributor

smarter commented Aug 13, 2017

Nice!

The new specificity behaviour is behind flag -Ydotty-specificity.

I think -Xsource:3.0 was proposed for this sort of things: scala/scala-dev#374

@retronym

I'd also like to see how this would be described in the spec, assuming it were to become default behaviour. Sometimes these concepts are easier to describe in comments in the implenentation than they are in the more limited vocabulary of the spec.

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Aug 15, 2017

Contributor

Could you try making Ordering contravariant on top of this PR ? (cf http://github.com/scala/bug/issues/7179, this might require using @unsafeVariance to deal with Java superclasses)

Contributor

smarter commented Aug 15, 2017

Could you try making Ordering contravariant on top of this PR ? (cf http://github.com/scala/bug/issues/7179, this might require using @unsafeVariance to deal with Java superclasses)

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 22, 2017

Contributor

@smarter aside from the inevitable issues with max and min it looks fine.

Contributor

milessabin commented Aug 22, 2017

@smarter aside from the inevitable issues with max and min it looks fine.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 22, 2017

Contributor

Quick and dirty PoC of a contravariant (Partial)Ordering and Equiv here: milessabin@9ca6be8.

I made max and min extension methods of Ordering[T] to avoid the variance issue and didn't bother to handle the Float and Double specializations.

It compiles and partests successfully.

Contributor

milessabin commented Aug 22, 2017

Quick and dirty PoC of a contravariant (Partial)Ordering and Equiv here: milessabin@9ca6be8.

I made max and min extension methods of Ordering[T] to avoid the variance issue and didn't bother to handle the Float and Double specializations.

It compiles and partests successfully.

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Aug 22, 2017

Contributor

I made max and min extension methods of Ordering[T] to avoid the variance issue

Would this work?

def max[U <: T](x: U, y: U): U = if (gteq(x, y)) x else y
Contributor

smarter commented Aug 22, 2017

I made max and min extension methods of Ordering[T] to avoid the variance issue

Would this work?

def max[U <: T](x: U, y: U): U = if (gteq(x, y)) x else y
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 23, 2017

Contributor

Would this work?

Yes, but it'd be a bit more work. The nested Ops class interferes.

Contributor

milessabin commented Aug 23, 2017

Would this work?

Yes, but it'd be a bit more work. The nested Ops class interferes.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 25, 2017

Contributor

Latest commit fixes the problem that @retronym pointed out by normalizing the LHS to the type constructor of the RHS via baseType. My reading of the discussion motivating this change is that people's expectations are that specificity runs in the same direction as extension rather than subtyping, so the use of base types at the outer level seems appropriate.

Contributor

milessabin commented Aug 25, 2017

Latest commit fixes the problem that @retronym pointed out by normalizing the LHS to the type constructor of the RHS via baseType. My reading of the discussion motivating this change is that people's expectations are that specificity runs in the same direction as extension rather than subtyping, so the use of base types at the outer level seems appropriate.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 27, 2017

Member

I'd prefer to have this target 2.13, as it's about exploring dotty compatibility.

Member

adriaanm commented Sep 27, 2017

I'd prefer to have this target 2.13, as it's about exploring dotty compatibility.

@adriaanm adriaanm modified the milestones: 2.12.4, 2.13.0-M3 Sep 27, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Sep 28, 2017

Contributor

I'm fine with this going into 2.13.x. Would you like me to change -Ydotty-specificity to -Xsource:2.13?

The Dotty bug corresponding to the issue that @retronym raised has been fixed with same test case as included in this PR so we can be reasonably confident that the semantics match: lampepfl/dotty#2974.

Contributor

milessabin commented Sep 28, 2017

I'm fine with this going into 2.13.x. Would you like me to change -Ydotty-specificity to -Xsource:2.13?

The Dotty bug corresponding to the issue that @retronym raised has been fixed with same test case as included in this PR so we can be reasonably confident that the semantics match: lampepfl/dotty#2974.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 28, 2017

Member

Thanks, yes, let's standardize on -Xsource. Regarding the target of 2.13 v 3.0, we can tweak once it's clear what the impact on user code is.

Member

adriaanm commented Sep 28, 2017

Thanks, yes, let's standardize on -Xsource. Regarding the target of 2.13 v 3.0, we can tweak once it's clear what the impact on user code is.

@ctongfei

This comment has been minimized.

Show comment
Hide comment
@ctongfei

ctongfei Oct 8, 2017

This is great :-) Cats can make all these typeclasses contravariant now!

ctongfei commented Oct 8, 2017

This is great :-) Cats can make all these typeclasses contravariant now!

@milessabin milessabin changed the base branch from 2.12.x to 2.13.x Oct 17, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Oct 17, 2017

Contributor

Rebased against 2.13.x ...

@adriaanm where does this leave -Xsource:2.13 ... should I just make this unconditional? Or do we need a -Xsource:dotty? Or something else?

Contributor

milessabin commented Oct 17, 2017

Rebased against 2.13.x ...

@adriaanm where does this leave -Xsource:2.13 ... should I just make this unconditional? Or do we need a -Xsource:dotty? Or something else?

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Oct 17, 2017

Contributor

I would vote for making this unconditional so that it can influence the design of the collection-strawman

Contributor

smarter commented Oct 17, 2017

I would vote for making this unconditional so that it can influence the design of the collection-strawman

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Oct 18, 2017

Contributor

I would be very happy to make this unconditional ... @adriaanm?

Contributor

milessabin commented Oct 18, 2017

I would be very happy to make this unconditional ... @adriaanm?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 23, 2017

Member

More work is needed before we can get to deciding whether this can go in and which flag (if any) it should be gated by. In short, 1) a correct implementation and 2) a detailed explanation of its pros & cons, including how it affects existing code bases and future ones (the new collections).

First, the code has evolved in dotty since the commit you've ported: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1138. Also, your port of the original commit is not obviously correct to me, and it didn't carry over the nice comment that accompanies the dotty implementation. Can you explain your implementation strategy and why it's different from dotty's?

The higher-level concern is that this illustrates this is evolving in dotty. Are we ready to call the latest approach ready for production? We also need to evaluate how it affects existing code bases. If this helps the new collections design -- great! That's a good motivator and test bed, but it's not a sufficient condition.

Member

adriaanm commented Oct 23, 2017

More work is needed before we can get to deciding whether this can go in and which flag (if any) it should be gated by. In short, 1) a correct implementation and 2) a detailed explanation of its pros & cons, including how it affects existing code bases and future ones (the new collections).

First, the code has evolved in dotty since the commit you've ported: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1138. Also, your port of the original commit is not obviously correct to me, and it didn't carry over the nice comment that accompanies the dotty implementation. Can you explain your implementation strategy and why it's different from dotty's?

The higher-level concern is that this illustrates this is evolving in dotty. Are we ready to call the latest approach ready for production? We also need to evaluate how it affects existing code bases. If this helps the new collections design -- great! That's a good motivator and test bed, but it's not a sufficient condition.

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Oct 24, 2017

Contributor

The higher-level concern is that this illustrates this is evolving in dotty.

The only change to the behavior of dotty here was a fix for lampepfl/dotty#2974 and Miles updated his PR to match the agreed-upon behavior. No other changes in this area are planned, though of course we need to see how well this works in practice. I think getting this PR in scalac and seeing what happens on the community build would be very helpful for that.

Contributor

smarter commented Oct 24, 2017

The higher-level concern is that this illustrates this is evolving in dotty.

The only change to the behavior of dotty here was a fix for lampepfl/dotty#2974 and Miles updated his PR to match the agreed-upon behavior. No other changes in this area are planned, though of course we need to see how well this works in practice. I think getting this PR in scalac and seeing what happens on the community build would be very helpful for that.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 25, 2017

Member

Ok, thanks for the background. I'm all for experimentation here, and alignment with dotty, but this is a tricky part of the language and I would like to see a deep dive in how this is implemented and how it will impact existing code.

I still don't understand how the code I linked to and the code in this PR relate. What does FunctionOf do in dotty, and why doesn't it appear here -- is it a cheaper way to flip variance? It would be nice to avoid the symbol cloning (cloning without substitution makes me suspicious, without looking too deeply). Should we do the comparison before or after existential abstraction?

Member

adriaanm commented Oct 25, 2017

Ok, thanks for the background. I'm all for experimentation here, and alignment with dotty, but this is a tricky part of the language and I would like to see a deep dive in how this is implemented and how it will impact existing code.

I still don't understand how the code I linked to and the code in this PR relate. What does FunctionOf do in dotty, and why doesn't it appear here -- is it a cheaper way to flip variance? It would be nice to avoid the symbol cloning (cloning without substitution makes me suspicious, without looking too deeply). Should we do the comparison before or after existential abstraction?

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Oct 25, 2017

Contributor

I still don't understand how the code I linked to and the code in this PR relate. What does FunctionOf do in dotty, and why doesn't it appear here -- is it a cheaper way to flip variance?

Yeah basically you replace all type arguments Foo that appear in contravariant positions by a function type Foo => () to flip their variance, it's kind of a hack :).

Contributor

smarter commented Oct 25, 2017

I still don't understand how the code I linked to and the code in this PR relate. What does FunctionOf do in dotty, and why doesn't it appear here -- is it a cheaper way to flip variance?

Yeah basically you replace all type arguments Foo that appear in contravariant positions by a function type Foo => () to flip their variance, it's kind of a hack :).

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Oct 26, 2017

Contributor

The impact will be similar, though not quite as significant, as partial unification ... people have been working around the old specificity behaviour wrt to contravariant type classes in a variety of ad hoc and more or less unsatisfactory ways for years. This change in Dotty and Scala gives people the behaviour they expected all along and makes the workarounds unnecessary.

Existing code will break, as was the case with partial unification, but I think that's a price worth paying, and I'm fairly confident that most people working with contravariant type classes would agree.

Contributor

milessabin commented Oct 26, 2017

The impact will be similar, though not quite as significant, as partial unification ... people have been working around the old specificity behaviour wrt to contravariant type classes in a variety of ad hoc and more or less unsatisfactory ways for years. This change in Dotty and Scala gives people the behaviour they expected all along and makes the workarounds unnecessary.

Existing code will break, as was the case with partial unification, but I think that's a price worth paying, and I'm fairly confident that most people working with contravariant type classes would agree.

@adriaanm adriaanm removed this from the 2.13.0-M3 milestone Dec 13, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Apr 4, 2018

Contributor

@adriaanm can we move forward with this now?

Contributor

milessabin commented Apr 4, 2018

@adriaanm can we move forward with this now?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 17, 2018

Member

We're quizzing Martin on the current status of the spec & implementation of this feature in Dotty. Once we clarify that, I'll update you here. In any case, this PR will need some work to address my review: it also needs to port the comment from dotty, and I would rather see an implementation that's closer to dotty (using an internal FlipVariance marker trait, instead of cloning)

Member

adriaanm commented Apr 17, 2018

We're quizzing Martin on the current status of the spec & implementation of this feature in Dotty. Once we clarify that, I'll update you here. In any case, this PR will need some work to address my review: it also needs to port the comment from dotty, and I would rather see an implementation that's closer to dotty (using an internal FlipVariance marker trait, instead of cloning)

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Apr 17, 2018

Contributor

Sounds good, though I don't understand what you mean by "port the comment from Dotty"?

Contributor

milessabin commented Apr 17, 2018

Sounds good, though I don't understand what you mean by "port the comment from Dotty"?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 18, 2018

Member

The comment is here: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1137-L1163. We ended up discovering some inconsistencies, which are being addressed here: lampepfl/dotty#4329

Member

adriaanm commented Apr 18, 2018

The comment is here: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1137-L1163. We ended up discovering some inconsistencies, which are being addressed here: lampepfl/dotty#4329

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jun 14, 2018

Member

Conflicts with changes in src/library/scala/collection/immutable/TreeSet.scala, so this needs another rebase..

Member

dwijnand commented Jun 14, 2018

Conflicts with changes in src/library/scala/collection/immutable/TreeSet.scala, so this needs another rebase..

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Jul 1, 2018

Member

Since we now have a process to discuss changes that narrow the gap between Scala 2 and Scala 3, I suggest we follow this process with this pull request as well. Let's have this change on the agenda of one of the next SIP meetings?

Member

xeno-by commented Jul 1, 2018

Since we now have a process to discuss changes that narrow the gap between Scala 2 and Scala 3, I suggest we follow this process with this pull request as well. Let's have this change on the agenda of one of the next SIP meetings?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jul 26, 2018

Contributor

Rebased, fixed tests broken by 2.13.x deprecations.

More importantly, changed to match the current Dotty semantics which are now to flip variances throughout the type, not just at the top level.

@smarter, I'd welcome your feedback on this.

@adriaanm do you want this behind an -Xsource flag? In which case, what version?

Contributor

milessabin commented Jul 26, 2018

Rebased, fixed tests broken by 2.13.x deprecations.

More importantly, changed to match the current Dotty semantics which are now to flip variances throughout the type, not just at the top level.

@smarter, I'd welcome your feedback on this.

@adriaanm do you want this behind an -Xsource flag? In which case, what version?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jul 26, 2018

Contributor

Hey ho ... this should be OK after squashing ...

Contributor

milessabin commented Jul 26, 2018

Hey ho ... this should be OK after squashing ...

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jul 26, 2018

Contributor

Squashed.

Contributor

milessabin commented Jul 26, 2018

Squashed.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 3, 2018

Member

My current position is that this should go under -Xsource:3.0. It can then be discussed in a SIP meeting on whether we want this on by default in 2.14. I still think this is a bit of a hack, and the interaction with type inference needs to be fleshed out. See e.g. lampepfl/dotty#4465.

It's too late for this to influence the collections, so I don't think it's urgent from a 2.13 release planning perspective.

Member

adriaanm commented Aug 3, 2018

My current position is that this should go under -Xsource:3.0. It can then be discussed in a SIP meeting on whether we want this on by default in 2.14. I still think this is a bit of a hack, and the interaction with type inference needs to be fleshed out. See e.g. lampepfl/dotty#4465.

It's too late for this to influence the collections, so I don't think it's urgent from a 2.13 release planning perspective.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 3, 2018

Contributor

If you're happy to merge it with the -Xsource:3.0 logic then I should be able to find time for it next week.

Contributor

milessabin commented Aug 3, 2018

If you're happy to merge it with the -Xsource:3.0 logic then I should be able to find time for it next week.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 3, 2018

Member
Member

adriaanm commented Aug 3, 2018

Downwards comparisons for implicit search and overloading resolution
This is a backport of the following Dotty change to scalac,

lampepfl/dotty@8954026

As in the Dotty implementation the specificity comparison which is used
for overload resolution and implicit selection is now performed as-if
all contravariant positions in type constructors were covariant.

Fixes scala/bug#2509. Fixes scala/bug#7768.
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 6, 2018

Contributor

@adriaanm done. Merge when you're happy :-)

I've taken out the spec change for now because having the language conditionalized on the language version was a bit awkward. We should deal with that as part of the SIP for the feature.

Contributor

milessabin commented Aug 6, 2018

@adriaanm done. Merge when you're happy :-)

I've taken out the spec change for now because having the language conditionalized on the language version was a bit awkward. We should deal with that as part of the SIP for the feature.

@adriaanm adriaanm merged commit cdd6f80 into scala:2.13.x Aug 6, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
cla @milessabin signed the Scala CLA. Thanks!
Details
validate-main [3537] SUCCESS. Took 34 min.
Details
@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Aug 8, 2018

Member

test/files/run/t7768.scala is failing on Windows. at e.g. https://scala-ci.typesafe.com/view/scala-2.13.x/job/scala-2.13.x-integrate-windows/658/consoleFull we see:

 Inv[A] Inv[B] Inv[C] Inv[D] Inv[E]
-Con[A] Con[A] Con[C] Con[C] Con[E]
-Cov[E] Cov[E] Cov[E] Cov[E] Cov[E]
+java.lang.NoClassDefFoundError: Con
...
+Caused by: java.lang.ClassNotFoundException: Con
...

@milessabin any idea why that might be...?

Member

SethTisue commented Aug 8, 2018

test/files/run/t7768.scala is failing on Windows. at e.g. https://scala-ci.typesafe.com/view/scala-2.13.x/job/scala-2.13.x-integrate-windows/658/consoleFull we see:

 Inv[A] Inv[B] Inv[C] Inv[D] Inv[E]
-Con[A] Con[A] Con[C] Con[C] Con[E]
-Cov[E] Cov[E] Cov[E] Cov[E] Cov[E]
+java.lang.NoClassDefFoundError: Con
...
+Caused by: java.lang.ClassNotFoundException: Con
...

@milessabin any idea why that might be...?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 8, 2018

Member

🤣 Con is a reserved file name on windows 😱

Member

adriaanm commented Aug 8, 2018

🤣 Con is a reserved file name on windows 😱

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Aug 8, 2018

Member

oh LOL, it must be because con is a reserved name on Windows filesystems?

Member

SethTisue commented Aug 8, 2018

oh LOL, it must be because con is a reserved name on Windows filesystems?

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Aug 8, 2018

Member

jinx

Member

SethTisue commented Aug 8, 2018

jinx

SethTisue added a commit to SethTisue/scala that referenced this pull request Aug 8, 2018

fix t7668 test case on Windows
CON you call something CON on Windows? No, you CONnot.

references scala#6037
@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Aug 8, 2018

Member

this leaves me puzzled how Shapeless is able to call something Aux, which is also a dirty word in Redmond.

but, a glance at the source shows that Shapeless's Aux is a type alias, and therefore doesn't produce a .class file. This seems in keeping with our suspicions that @milessabin doesn't even exist at runtime.

PR with fix: #7024

Member

SethTisue commented Aug 8, 2018

this leaves me puzzled how Shapeless is able to call something Aux, which is also a dirty word in Redmond.

but, a glance at the source shows that Shapeless's Aux is a type alias, and therefore doesn't produce a .class file. This seems in keeping with our suspicions that @milessabin doesn't even exist at runtime.

PR with fix: #7024

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 8, 2018

Contributor

Oh. My. Goodness.

Contributor

milessabin commented Aug 8, 2018

Oh. My. Goodness.

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Aug 8, 2018

Narrator: There comes a moment in everyone's life when they must choose what kind of a person they're going to be. A moment where steel is tempered in the unyielding fires of decision. A moment where all the churning demons of ten thousand unanswered slights rage as one and demand retribution.

In that moment, our hero knew: he wasn't a Windows user.

djspiewak commented Aug 8, 2018

Narrator: There comes a moment in everyone's life when they must choose what kind of a person they're going to be. A moment where steel is tempered in the unyielding fires of decision. A moment where all the churning demons of ten thousand unanswered slights rage as one and demand retribution.

In that moment, our hero knew: he wasn't a Windows user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment