Skip to content

Fix OrderingConstraint#order forgetting constraints; fix avoidLambdaParams #15077

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
May 2, 2022

Conversation

smarter
Copy link
Member

@smarter smarter commented Apr 29, 2022

order takes current as input and returns a constraint set that subsumes both
current and param1 <: param2, but it's an instance method because it relies
on this to determine if current is used linearly such that we can reuse its
backing arrays instead of copying them. However, the implementation of order
mistakenly returned this and called methods on this instead of current.
This lead to issues like #11682 but that was compensated by logic inserted
in ConstraintHandling#addToConstraint which we can now remove.

Fixing this also required fixing an unrelated issue in avoidLambdaParams to
prevent a regression in tests/pos/i9676.scala: we shouldn't avoid a lambda param
under its own binder even if it is in comparedTypeLambdas, the sequence of
operations where this happens is:

[A] =>> List[A] <:< [A] =>> G[A]
  // comparedTypeLambdas ++= ([A] =>> List[A], [A] =>> G[A])
  List[A] <:< G[A]
    [A] =>> List[A] <:< G
      // previously, avoidLambdaParams([A] =>> List[A]) = [A] =>> List[Any],
      // now it leaves the type lambda alone.

We end up checking [A] =>> List[A] <:< G instead of just List <:< G because
of ensureLambdaSub in compareAppliedTypeParamRef. I'm not sure if this is
actually needed, but I decided to not disturb that code too much for now.

…arams

`order` takes `current` as input and returns a constraint set that subsumes both
`current` and `param1 <: param2`, but it's an instance method because it relies
on `this` to determine if `current` is used linearly such that we can reuse its
backing arrays instead of copying them. However, the implementation of `order`
mistakenly returned `this` and called methods on `this` instead of `current`.
This lead to issues like scala#11682 but that was compensated by logic inserted
in ConstraintHandling#addToConstraint which we can now remove.

Fixing this also required fixing an unrelated issue in avoidLambdaParams to
prevent a regression in tests/pos/i9676.scala: we shouldn't avoid a lambda param
under its own binder even if it is in `comparedTypeLambdas`, the sequence of
operations where this happens is:

[A] =>> List[A] <:< [A] =>> G[A]
  // comparedTypeLambdas ++= ([A] =>> List[A], [A] =>> G[A])
  List[A] <:< G[A]
    [A] =>> List[A] <:< G
      // previously, avoidLambdaParams([A] =>> List[A]) = [A] =>> List[Any],
      // now it leaves the type lambda alone.

We end up checking `[A] =>> List[A] <:< G` instead of just `List <:< G` because
of `ensureLambdaSub` in `compareAppliedTypeParamRef`. I'm not sure if this is
actually needed, but I decided to not disturb that code too much for now.
@smarter smarter requested a review from odersky April 29, 2022 15:28
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.

Nice catches!

@odersky odersky merged commit 019caf8 into scala:main May 2, 2022
@odersky odersky deleted the fix-constraint-order branch May 2, 2022 10:28
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.

2 participants