Skip to content
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

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Jun 5, 2024

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.

@EugeneFlesselle
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Jun 5, 2024

performance test scheduled: 50 job(s) in queue, 1 running.

@EugeneFlesselle
Copy link
Contributor Author

test performance please: 59b0f3a c6fbe6f

@dottybot
Copy link
Member

dottybot commented Jun 5, 2024

performance test scheduled for 59b0f3a c6fbe6f: 50 job(s) in queue, 1 running.

@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review June 5, 2024 15:16
@dottybot
Copy link
Member

dottybot commented Jun 6, 2024

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/20527/ to see the changes.

Benchmarks is based on merging with main (746b00b)

@EugeneFlesselle EugeneFlesselle requested a review from odersky June 6, 2024 15:16
@EugeneFlesselle
Copy link
Contributor Author

The last commit only changes a test case, the previous commit (with the change in AppliedType) looks to be inline with main.
Screenshot 2024-06-06 at 17 30 48

Copy link
Contributor

@odersky odersky left a 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.

@EugeneFlesselle
Copy link
Contributor Author

After some investigation, here is what I found: TypeApplications#appliedTo can perform reductions that depend on the arguments, which are not structurally insignificant (although it is true the results are equal with respect to =:=).
In particular, it will perform eta-reduction if the de-aliased tycon is an HKTypeLambda and the dealiased.paramRefs == dealiasedArgs, but ignoring variance annotations.
This can have an impact on TypeComparer#compareTypeLambda as it requires the variances to conform.

In the example of pos/typeclass-encoding3b

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 This type member of ListMonad has a variance annotation.

When checking ListMonad <:< functors.Monad.Impl[T] we eventually get to the comparison [X] =>> T[X] <:< [+X] =>> List[X] which fails the variance conformance checks, despite the fact that T has been instantiated to List, since it has been substituted into the refinement (and cached) before its instantiation.
If we however recompute the superType after having found T := List, appliedTo now yields the same covariance annotation, which allows the type comparison to succeed.

@EugeneFlesselle
Copy link
Contributor Author

If the changes in this PR are deemed sufficient to address the issue, I can definitely add a comment to AppliedType#superType explaining why the change is needed.

@odersky
Copy link
Contributor

odersky commented Jun 6, 2024

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.

@EugeneFlesselle
Copy link
Contributor Author

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.

Sounds good, I'll do that 👍

@EugeneFlesselle
Copy link
Contributor Author

EugeneFlesselle commented Jun 6, 2024

Note I also added a change unrelated to the doc in ee54814

@EugeneFlesselle EugeneFlesselle requested a review from odersky June 6, 2024 20:35
Copy link
Member

@dwijnand dwijnand left a 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

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,
Copy link
Member

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?

Copy link
Contributor Author

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

@EugeneFlesselle EugeneFlesselle merged commit 04ada79 into scala:main Jun 7, 2024
19 checks passed
@EugeneFlesselle EugeneFlesselle deleted the cachedSuper branch June 7, 2024 23:12
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Jun 28, 2024
EugeneFlesselle added a commit that referenced this pull request Jul 1, 2024
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
WojciechMazur pushed a commit that referenced this pull request Jul 8, 2024
WojciechMazur added a commit that referenced this pull request Jul 9, 2024
)

Backports #20857 to the LTS branch.

PR submitted by the release tooling.
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.

5 participants