-
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
Mark AppliedType cachedSuper valid Nowhere when using provisional args #20527
Conversation
test performance please |
performance test scheduled: 50 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/20527/ to see the changes. Benchmarks is based on merging with main (746b00b) |
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.
I'm not sure why it wasn't the case previously.
I think the thinking was because it's only type constructor that influences the shape of the supertype, if the type constructor is no longer provisional, the supertype is always the same. It might still be provisional if one of the arguments is provisional, but that's another question. I am not really sure where the hole in that reasoning is. The change seems to fix something, but i am not sure why. It would be good to have an explanation, why the problem happened and how changing isProvisional fixed it.
After some investigation, here is what I found: In the example of object Monad extends TypeClassCompanion1:
type Impl[T[_]] = MonadCommon { type This = [X] =>> T[X] }
implicit object ListMonad extends MonadCommon:
type This[+X] = List[X]
def flattened[T[_], A]($this: T[T[A]])(implicit ev: Monad.Impl[T]): T[A] = ???
flattened(List(List(1, 2, 3), List(4, 5)))(using ListMonad) the key observation is that the When checking |
If the changes in this PR are deemed sufficient to address the issue, I can definitely add a comment to |
That's a great answer. I think we should have a condensed version in the code for AppliedType#superType, with a link to the test that would otherwise fail. The changes look all good to me. |
Sounds good, I'll do that 👍 |
Note I also added a change unrelated to the doc in ee54814 |
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 a question
tests/pos/typeclass-encoding3b.scala
Outdated
When checking `ListMonad <:< functors.Monad.Impl[T]` | ||
we eventually get to the comparison `[X] =>> T[X] <:< [+X] =>> List[X]` | ||
because the `This` type member of `ListMonad` has a covariance annotation. | ||
This fails the variance conformance checks despite the fact that T has been instantiated to List, |
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.
I don't follow the comment. How does it failing the conformance check lead it to not erroring?
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.
Ah, I'd overlooked updating the tense
) Backports #20857 to the LTS branch. PR submitted by the release tooling.
I'm not sure why it wasn't the case previously.
It solves some stale caching issues I encounter when introducing more caching for match types in #20268, as well as the problem in
neg/typeclass-encoding3
apparently.There seems to be no significant performance impact from the reduced opportunities for caching.