Base multiversal equality on typeclass derivation #5843
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
odersky
force-pushed
the
derive-multiversal
branch
2 times, most recently
from
17b5611 to
a28a078
Compare
odersky
force-pushed
the
derive-multiversal
branch
2 times, most recently
from
a76f3cc to
3c27142
Compare
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
odersky
force-pushed
the
derive-multiversal
branch
from
fda184c to
dcd3f55
Compare
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/5843/ to see the changes. Benchmarks is based on merging with master (b44adf7) |
odersky
force-pushed
the
derive-multiversal
branch
from
daceb76 to
6422ef8
Compare
2 tasks
Treat Eq like any other derivable typeclass. This caused a bit of changes in fuzzing tests.
1. Don't apply the lifting rules when strictEquality is set.
2. Be more strict about null checking
3. Compensate for (2) by changing errorTerm in parser so that
it has an error type instead of Null.
4. Reorganize implementation to match new docs.
1. Don't apply the lifting rules when strictEquality is set.
2. Be more strict about null checking
3. Compensate for (2) by changing errorTerm in parser so that
it has an error type instead of Null.
4. Reorganize implementation to match new docs.
5. Handle Eql instances involving primitive types or Null directly
in the compiler (previously, we were missing some of them, see
pos/multivarsal.scala as a test)
6. Sharpen Eql instance of Proxy to only work for AnyRef arguments.
This avoids false negatives when comparing value types with null.
Some neg tests now return more errors since we do stricter checking that comparisons with null make sense. There's an unfortunate interaction with `errorTerm` in Parser which returns a `null` literal as a tree when the parser gets confused. That null literal now causes follow on errors in some cases. I believe the right thing to do would be to let `errorTerm` return a tree with an `ErrorType`. I tried that, and it would remove again all the added errors in this commit. Unfortunately, it breaks SignatureHelpTest in a way I cannot fix.
Sine it now uses typeclass derivation, it makes sense if it comes directly afterwards.
Eq is already used with a slightly different meaning as a unary typeclass, e.g. cats.Eq.
Avoid duplicating parameter names in derivations of n-ary type classes
odersky
force-pushed
the
derive-multiversal
branch
from
6422ef8 to
f791a52
Compare
OlivierBlanvillain
approved these changes
Feb 8, 2019
| @@ -67,7 +67,7 @@ import collection.mutable.ListBuffer | |||
| */ | |||
| object Denotations { | |||
|
|
|||
| implicit def eqDenotation: Eq[Denotation, Denotation] = Eq | |||
| implicit def eqDenotation: Eq[Denotation, Denotation] = Eq.derived | |||
There was a problem hiding this comment.
I would give it another name because here the instance is not really derived, bur rather declared or something.
| type `Eq[L, R]`, provided that neither `L` nor `R` have `Eq` instances | ||
| defined on them. | ||
| Even though `eqlAny` is not declared `implied`, the compiler will still | ||
| construct an `eqlAny` instance as answer to an implicit search for the |
There was a problem hiding this comment.
Is there also an alternative name for implicit search? Mixing the two terminologies here makes the sentence a bit confusing...
| - _lifting_ a type `S` means replacing all references to abstract types | ||
| in covariant positions of `S` by their upper bound, and to replacing | ||
| all refinement types in covariant positions of `S` by their parent. | ||
| - a type `T` has a _reflexive `Eql` instance_ if the implicit search for `Eql[T, T]` |
There was a problem hiding this comment.
Same comment (on implicit search)
library/src/scala/Eq.scala
Outdated
|
|
||
| // The next three definitions can go into the companion objects of classes |
There was a problem hiding this comment.
I agree, but then maybe then spec shouldn't mention them...
| @@ -63,8 +65,6 @@ sidebar: | |||
| url: docs/reference/contextual/relationship-implicits.html | |||
| - title: Other New Features | |||
| subsection: | |||
| - title: Multiversal Equality | |||
| url: docs/reference/other-new-features/multiversal-equality.html | |||
There was a problem hiding this comment.
I think this will break all links to the Multiversal Equality doc, we should keep the old url with a redirect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We still need to consider the idea of making equality an extension method.