Tweak tparam unification to work with lambda cleanup #22031
Conversation
There was a problem hiding this comment.
I can reproduce the problem with the change in this PR if I change the receiver of map2 to be a tuple of type parameters (K1, K2) instead of a single parameter K:
trait Ops[F[_], A]:
def map0[B](f0: A => B): F[B] = ???
trait Functor1[G[_]]
trait Functor2[H[_]]:
extension [C1, C2](hc: H[(C1, C2)])
def map2[D](f1: (C1, C2) => D): H[D]
trait Ref[I[_], +E]
final class Cov[+F]
class Test:
given [J[_]](using J: Functor1[J]): Functor2[J] with
extension [K1, K2](jk: J[(K1, K2)])
def map2[L](f2: (K1, K2) => L): J[L] = ???
def t1[
M[_[t]],
N[_],
](using N: Functor1[N]): Unit =
val x3: Ops[N, M[[t] =>> Ref[N, t]]] = ???
val x2: N[(M[N], M[[t] =>> Ref[N, t]])] = x3
.map0 { refs => (???, refs) }
.map2 { case (not, refs) => (???, refs) }The root cause of the regression is that the removal of type variables in https://github.com/scala/scala3/pull/21466/files#diff-73257963d8a6d79cd4878ae4ff164e33c36629f77821886ffc7cea2e2e38af39R78 does not check that the removed type variables are not in use somewhere. I spent some time looking into it but couldn't find a robust way to do so (maybe a version of Inferencing#interpolateTypeVars that doesn't require a tree argument could be used on pt in normalizedCompatible to do this clean-up?), so I think for now it'd be best to just revert #21466
dwijnand
force-pushed
the
i21981-reg-drop-lambda
branch
3 times, most recently
from
29d89ce to
18e6a95
Compare
| if aboveOK then | ||
| tvar.instantiate(fromBelow = true) |
There was a problem hiding this comment.
Isn't it the other way around? From the documentation of Constraint#dependsOn:
@param `co` If true, test whether the constraint would change if the variable is made larger
aboveOK negates the result of dependsOn(..., co = true), so in this branch, the constraint should not change when the variable is made larger, so we should instantiate with fromBelow = false.
I notice that when I make this change, tests/pos/i21981.scala breaks, but it only works by chance here: both belowOK and aboveOK return true (in fact, in all of the added tests, belowOK == aboveOK is always true), it also breaks if I just change the order of the two if branches in this code.
So unfortunately I don't think that we can rely on dependsOn by itself, we'd need more of the logic of interpolateTypeVars to take into account the type of pt I think.
| if aboveOK then | ||
| if belowOK then | ||
| tvar.instantiate(fromBelow = true) | ||
| else if belowOK then | ||
| else if aboveOK then | ||
| tvar.instantiate(fromBelow = false) |
There was a problem hiding this comment.
To clarify what I meant in my previous comment, the code after this commit is:
if belowOK then
tvar.instantiate(fromBelow = true)
else if aboveOK then
tvar.instantiate(fromBelow = false)but if I swap the order of the branches:
if aboveOK then
tvar.instantiate(fromBelow = false)
else if belowOK then
tvar.instantiate(fromBelow = true)then tests/pos/i21981.scala starts failing, so that must mean that aboveOK being true is not a good enough condition to ensure that instantiating a type variable is safe.
dwijnand
force-pushed
the
i21981-reg-drop-lambda
branch
from
31b91f3 to
4c9f92a
Compare
| tvar.instantiate(fromBelow = false) // any direction | ||
| // Filter out any tvar that instantiating would further constrain the current constraint | ||
| // Similar to filterByDeps in interpolateTypeVars. | ||
| // Also, filter out any tvar that is the instantiation another tvar |
There was a problem hiding this comment.
typo:
| // Also, filter out any tvar that is the instantiation another tvar | |
| // Also, filter out any tvar that is the instantiation of another tvar |
| val excluded = ctx.typerState.ownedVars.filter(!_.isInstantiated) | ||
| var isInst = false | ||
| ctx.typerState.constraint.foreachTypeVar: tvar1 => | ||
| isInst ||= !excluded.contains(tvar1) && tvar1.instanceOpt == tvar |
There was a problem hiding this comment.
If I understand the logic right, then !excluded.contains(tvar1) is equivalent to tvar1.isInstantiated and therefore not needed.
| // (that we're not also trying to instantiate) | ||
| // For example, in tests/pos/i21981.scala | ||
| // when testing the compatibility of `.map2[?K]` on receiver `map0[?B]` | ||
| // the tvars for B and K unified, instantiating `B := K`, |
There was a problem hiding this comment.
The check tvar1.instanceOpt == tvar accounts for that, but it doesn't account for B := Foo[K]. Here's a test case with B := Contra[K]:
case class Inv[T](x: T)
class Contra[-ContraParam](x: ContraParam)
trait Ops[F[_], A]:
def map0[B](f0: A => Contra[B]): F[B] = ???
trait Functor1[G[_]]
trait Functor2[H[_]]
trait Ref[I[_], +E]
class Test:
given [J[_]](using J: Functor1[J]): Functor2[J] with
extension [K](jk: J[Contra[K]])
def map2[L](f2: K => L): J[L] = ???
def t1[
M[_[t]],
N[_],
](using N: Functor1[N]): Unit =
val x3: Ops[N, M[[t] =>> Ref[N, t]]] = ???
val x2: N[(M[N], M[[t] =>> Ref[N, t]])] = x3
.map0 { refs => Contra[Contra[(Nothing, M[[t] =>> Ref[N, t]])]](???) }
.map2 { case (not, refs) => (???, refs) }Now this could probably be fixed by checking tvar.occursIn(tvar1.instanceOpt) but at this point I'd started getting worried about time complexity: we're already inside two nested loops over all type variables and now we're traversing a type (in fact this is the traversal that dependsOn avoids for uninstantiated type variables).
Fixes #21981