Skip to content

Commit

Permalink
Fix OrderingConstraint#order forgetting constraints; fix avoidLambdaP…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
smarter committed Apr 29, 2022
1 parent f8e5203 commit a8641c5
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 12 deletions.
11 changes: 6 additions & 5 deletions compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
Original file line number Diff line number Diff line change
Expand Up @@ -627,11 +627,7 @@ trait ConstraintHandling {
case x =>
// Happens if param was already solved while processing earlier params of the same TypeLambda.
// See #4720.

// Should propagate bounds even when param has been solved.
// See #11682.
lower.forall(addOneBound(_, x, isUpper = true)) &&
upper.forall(addOneBound(_, x, isUpper = false))
true
}
}
}
Expand Down Expand Up @@ -677,6 +673,11 @@ trait ConstraintHandling {
case t @ TypeParamRef(tl: TypeLambda, n) if comparedTypeLambdas contains tl =>
val bounds = tl.paramInfos(n)
range(bounds.lo, bounds.hi)
case tl: TypeLambda =>
val saved = comparedTypeLambdas
comparedTypeLambdas -= tl
try mapOver(tl)
finally comparedTypeLambdas = saved
case _ =>
mapOver(t)
}
Expand Down
20 changes: 13 additions & 7 deletions compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala
Original file line number Diff line number Diff line change
Expand Up @@ -354,13 +354,18 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
* `<:<` relationships between parameters ("edges") but not bounds.
*/
def order(current: This, param1: TypeParamRef, param2: TypeParamRef, direction: UnificationDirection = NoUnification)(using Context): This =
if (param1 == param2 || current.isLess(param1, param2)) this
else {
assert(contains(param1), i"$param1")
assert(contains(param2), i"$param2")
// /!\ Careful here: we're adding constraints on `current`, not `this`, so
// think twice when using an instance method! We only need to pass `this` as
// the `prev` argument in methods on `ConstraintLens`.
// TODO: Refactor this code to take `prev` as a parameter and add
// constraints on `this` instead?
if param1 == param2 || current.isLess(param1, param2) then current
else
assert(current.contains(param1), i"$param1")
assert(current.contains(param2), i"$param2")
val unifying = direction != NoUnification
val newUpper = {
val up = exclusiveUpper(param2, param1)
val up = current.exclusiveUpper(param2, param1)
if unifying then
// Since param2 <:< param1 already holds now, filter out param1 to avoid adding
// duplicated orderings.
Expand All @@ -374,7 +379,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
param2 :: up
}
val newLower = {
val lower = exclusiveLower(param1, param2)
val lower = current.exclusiveLower(param1, param2)
if unifying then
// Similarly, filter out param2 from lowerly-ordered parameters
// to avoid duplicated orderings.
Expand All @@ -390,7 +395,8 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
val current1 = newLower.foldLeft(current)(upperLens.map(this, _, _, newUpper ::: _))
val current2 = newUpper.foldLeft(current1)(lowerLens.map(this, _, _, newLower ::: _))
current2
}
end if
end order

/** The list of parameters P such that, for a fresh type parameter Q:
*
Expand Down

0 comments on commit a8641c5

Please sign in to comment.