From 6033fce56c675c291d13f5b5d44adcb700263735 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 7 Nov 2020 14:21:27 +0100 Subject: [PATCH 1/4] Refactor some TypeComparer code - streamline comparisons of applied types - optimize sufficientEither to not try the second alternative if the first succeeds without narrowing the constraint. --- .../dotty/tools/dotc/core/TypeComparer.scala | 52 +++++++++++-------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 669fa51260d5..d79cc4f62cda 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -157,9 +157,12 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling /** Are we forbidden from recording GADT constraints? */ private var frozenGadt = false - private inline def inFrozenGadt[T](op: => T): T = { + private inline def inFrozenGadt[T](inline op: T): T = + inFrozenGadtIf(true)(op) + + private inline def inFrozenGadtIf[T](cond: Boolean)(inline op: T): T = { val savedFrozenGadt = frozenGadt - frozenGadt = true + frozenGadt = cond try op finally frozenGadt = savedFrozenGadt } @@ -967,7 +970,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling * corresponding arguments are subtypes relative to their variance (see `isSubArgs`). */ def isMatchingApply(tp1: Type): Boolean = tp1 match { - case AppliedType(tycon1, args1) => + case tp1 as AppliedType(tycon1, args1) => // We intentionally do not dealias `tycon1` or `tycon2` here. // `TypeApplications#appliedTo` already takes care of dealiasing type // constructors when this can be done without affecting type @@ -977,6 +980,10 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling // // Foo[?F, ?T] <:< Foo[[X] =>> (X, String), Int] // + // where + // + // type Foo[F[_], T] = ErasedFoo[F[T]] + // // Naturally, we'd like to infer: // // ?F := [X] => (X, String) @@ -1016,10 +1023,9 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling } val res = ( - tycon1sym == tycon2sym - && isSubPrefix(tycon1.prefix, tycon2.prefix) - || byGadtBounds(tycon1sym, tycon2, fromAbove = true) - || byGadtBounds(tycon2sym, tycon1, fromAbove = false) + tycon1sym == tycon2sym && isSubPrefix(tycon1.prefix, tycon2.prefix) + || byGadtBounds(tycon1sym, tycon2, fromAbove = true) + || byGadtBounds(tycon2sym, tycon1, fromAbove = false) ) && { // There are two cases in which we can assume injectivity. // First we check if either sym is a class. @@ -1031,11 +1037,11 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling // must be instantiated, making the two tycons equal val tyconIsInjective = (tycon1sym.isClass || tycon2sym.isClass) - && (if touchedGADTs then gadtIsInstantiated else true) - def checkSubArgs() = isSubArgs(args1, args2, tp1, tparams) - // we only record GADT constraints if *both* tycons are effectively injective - if (tyconIsInjective) checkSubArgs() - else inFrozenGadt { checkSubArgs() } + && (!touchedGADTs || gadtIsInstantiated) + + inFrozenGadtIf(!tyconIsInjective) { + isSubArgs(args1, args2, tp1, tparams) + } } if (res && touchedGADTs) GADTused = true res @@ -1544,19 +1550,19 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling * Method name comes from the notion that we are keeping a constraint which is sufficient to satisfy * one of subtyping relationships. */ - private def sufficientEither(op1: => Boolean, op2: => Boolean): Boolean = { + private def sufficientEither(op1: => Boolean, op2: => Boolean): Boolean = val preConstraint = constraint - op1 && { - val leftConstraint = constraint - constraint = preConstraint - if (!(op2 && subsumes(leftConstraint, constraint, preConstraint))) { - if (constr != noPrinter && !subsumes(constraint, leftConstraint, preConstraint)) - constr.println(i"CUT - prefer $leftConstraint over $constraint") - constraint = leftConstraint - } + if op1 then + if constraint ne preConstraint then + // check whether `op2` generates a weaker constraint than `op1` + val leftConstraint = constraint + constraint = preConstraint + if !(op2 && subsumes(leftConstraint, constraint, preConstraint)) then + if constr != noPrinter && !subsumes(constraint, leftConstraint, preConstraint) then + constr.println(i"CUT - prefer $leftConstraint over $constraint") + constraint = leftConstraint true - } || op2 - } + else op2 /** Returns true iff the result of evaluating either `op1` or `op2` is true, keeping the smaller constraint if any. * E.g., if From 5ae3655af8d6cffc43ea7ae2077faa1b4ac292cd Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 7 Nov 2020 15:51:24 +0100 Subject: [PATCH 2/4] Fix #10129: Fix comparison of applied type aliases Given type F[Xs] = ... F[As] <: F[Bs] Do we dealias `F` or check the arguments directly? It turns out that neither choice is correct in all instances. Checking the arguments directly is necessary for hk type inference, but may narrow the constraint too much. The solution is to go to an either that tries both options. --- .../dotty/tools/dotc/core/TypeComparer.scala | 53 +++++++++++++++++-- tests/pos/i10129.scala | 14 +++++ 2 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 tests/pos/i10129.scala diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index d79cc4f62cda..a4b6dd3c152f 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -971,7 +971,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling */ def isMatchingApply(tp1: Type): Boolean = tp1 match { case tp1 as AppliedType(tycon1, args1) => - // We intentionally do not dealias `tycon1` or `tycon2` here. + // We intentionally do not automatically dealias `tycon1` or `tycon2` here. // `TypeApplications#appliedTo` already takes care of dealiasing type // constructors when this can be done without affecting type // inference, doing it here would not only prevent code from compiling @@ -997,6 +997,25 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling // ?F := [X] =>> (Int, X) // // Which is not what we wanted! + // On the other hand, we are not allowed to stop at the present arguments either. + // An example is i10129.scala. Here, we have the following situation: + // + // type Lifted[A] = Err | A + // def point[O](o: O): Lifted[O] = o + // extension [O, U](o: Lifted[O]) def map(f: O => U): Lifted[U] = ??? + // point("a").map(_ => if true then 1 else error) + // + // This leads to the constraint Lifted[U] <: Lifted[Int]. If we just + // check the arguments this gives `U <: Int`. But this is wrong. Dealiasing + // `Lifted` gives `Err | U <: Err | Int`, hence it should be `U <: Err | Int`. + // + // So it's a conundrum. We need to check the immediate arguments for hk type inference, + // but this could narrow the constraint too much. The solution is to do an + // either with two alternatives when encountering an applied type alis. + // The first alternative checks the arguments, the second alternative checks the + // dealiased types. We pick the alternative that constrains least, or if there + // is no winner, the first one. We are forced to use a `sufficientEither` + // in the comparison, which is still not quite right. See the comment below. def loop(tycon1: Type, args1: List[Type]): Boolean = tycon1 match { case tycon1: TypeParamRef => (tycon1 == tycon2 || @@ -1039,9 +1058,35 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling (tycon1sym.isClass || tycon2sym.isClass) && (!touchedGADTs || gadtIsInstantiated) - inFrozenGadtIf(!tyconIsInjective) { - isSubArgs(args1, args2, tp1, tparams) - } + def checkArgs(): Boolean = + if tycon1sym == tycon2sym && tycon1sym.isAliasType then + // if types are simple forwarding aliases then try the dealiased types + tp1.superType match + case tp1a as AppliedType(_, args1a) if args1a.eqElements(args1) => + tp2.superType match + case tp2a as AppliedType(_, args2a) if args2a.eqElements(args2) => + return recur(tp1a, tp2a) + case _ => + case _ => + // Otherwise either compare arguments or compare dealiased types. + // Note: This test should be done with an `either`, but this fails + // for hk-alias-unification.scala The problem is not GADT boundschecking; + // that is taken care of by the outer inFrozenGadtIf test. The problem is + // that hk-alias-unification.scala relies on the right isSubArgs being performed + // when constraining the result type of a method. But there we are only allowed + // to use a necessaryEither, since it might be that an implicit conversion will + // be inserted on the result. So strictly speaking by going to a sufficientEither + // we overshoot and narrow the constraint unnecessarily. On the other hand, this + // is probably an extreme corner case. If we exclude implicit conversions, the + // problem goes away since in that case we would have a normal subtype test here, + // which demands a sufficientEither anyway. + sufficientEither( + isSubArgs(args1, args2, tp1, tparams), + recur(tp1.superType, tp2.superType)) + else + isSubArgs(args1, args2, tp1, tparams) + + inFrozenGadtIf(!tyconIsInjective)(checkArgs()) } if (res && touchedGADTs) GADTused = true res diff --git a/tests/pos/i10129.scala b/tests/pos/i10129.scala new file mode 100644 index 000000000000..e4c8dfde8894 --- /dev/null +++ b/tests/pos/i10129.scala @@ -0,0 +1,14 @@ +class Err + +type Lifted[A] = Err | A + +def point[O](o: O): Lifted[O] = o +extension [O, U](o: Lifted[O]) def map(f: O => U): Lifted[U] = ??? + +val error: Err = Err() + +def ok: Int | Err = + point("a").map(_ => if true then 1 else error) + +def fail: Lifted[Int] = + point("a").map(_ => if true then 1 else error) // error \ No newline at end of file From 965efd9481068c057638b8159f60392d96b35ee7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 8 Nov 2020 22:53:05 +0100 Subject: [PATCH 3/4] Drop dead code, and improve explanation --- .../dotty/tools/dotc/core/TypeComparer.scala | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index a4b6dd3c152f..f4b8274e7d94 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -1060,26 +1060,23 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling def checkArgs(): Boolean = if tycon1sym == tycon2sym && tycon1sym.isAliasType then - // if types are simple forwarding aliases then try the dealiased types - tp1.superType match - case tp1a as AppliedType(_, args1a) if args1a.eqElements(args1) => - tp2.superType match - case tp2a as AppliedType(_, args2a) if args2a.eqElements(args2) => - return recur(tp1a, tp2a) - case _ => - case _ => - // Otherwise either compare arguments or compare dealiased types. + // Either compare arguments or compare dealiased types. // Note: This test should be done with an `either`, but this fails // for hk-alias-unification.scala The problem is not GADT boundschecking; // that is taken care of by the outer inFrozenGadtIf test. The problem is // that hk-alias-unification.scala relies on the right isSubArgs being performed - // when constraining the result type of a method. But there we are only allowed - // to use a necessaryEither, since it might be that an implicit conversion will - // be inserted on the result. So strictly speaking by going to a sufficientEither - // we overshoot and narrow the constraint unnecessarily. On the other hand, this - // is probably an extreme corner case. If we exclude implicit conversions, the - // problem goes away since in that case we would have a normal subtype test here, - // which demands a sufficientEither anyway. + // when constraining the result type of a method. But since #8635 we are only allowed + // to use a necessaryEither, since we might otherwise constrain too much. + // So strictly speaking by going to a sufficientEither we overshoot and narrow + // the constraint unnecessarily. But unlike in the situation of #8365 with test cases + // and-inf.scala and or-inf.scala, we don't cut off solutions in principle by dealiasing. + // We "just" risk missing an opportunity to do a desired hk-type inference. This + // is hopefully a less frequent corner case. Where it goes wrong compared to the status + // before this fix: If isSubArgs succeeds and then the dealised test also succeeds + // while constraining strictly less than the isSubArgs, then the we used to pick the + // constraint after isSubArgs, but we now pick the constraint after the dealiased test. + // There's one test in shapeless/deriving that had to be disabled + // (modules/deriving/src/test/scala/shapeless3/deriving/deriving.scala, value v7 in the functor test). sufficientEither( isSubArgs(args1, args2, tp1, tparams), recur(tp1.superType, tp2.superType)) From 88dbba4fe3b576df10732708297c4e72c660689d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 12 Nov 2020 19:55:24 +0100 Subject: [PATCH 4/4] Try another variant --- .../dotty/tools/dotc/core/TypeComparer.scala | 70 ++++++++----------- 1 file changed, 29 insertions(+), 41 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index f4b8274e7d94..1a95397f0b79 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -997,7 +997,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling // ?F := [X] =>> (Int, X) // // Which is not what we wanted! - // On the other hand, we are not allowed to stop at the present arguments either. + // On the other hand, we are not allowed to always stop at the present arguments either. // An example is i10129.scala. Here, we have the following situation: // // type Lifted[A] = Err | A @@ -1010,12 +1010,12 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling // `Lifted` gives `Err | U <: Err | Int`, hence it should be `U <: Err | Int`. // // So it's a conundrum. We need to check the immediate arguments for hk type inference, - // but this could narrow the constraint too much. The solution is to do an - // either with two alternatives when encountering an applied type alis. - // The first alternative checks the arguments, the second alternative checks the - // dealiased types. We pick the alternative that constrains least, or if there - // is no winner, the first one. We are forced to use a `sufficientEither` - // in the comparison, which is still not quite right. See the comment below. + // but this could narrow the constraint too much. The solution is to also + // check the constraint arising from the dealiased subtype test + // in the case where isSubArgs adds a constraint. If that second constraint + // is weaker than the first, we keep it in place of the first. + // Note that if the isSubArgs test fails, we will proceed anyway by + // dealising by doing a compareLower. def loop(tycon1: Type, args1: List[Type]): Boolean = tycon1 match { case tycon1: TypeParamRef => (tycon1 == tycon2 || @@ -1058,32 +1058,14 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling (tycon1sym.isClass || tycon2sym.isClass) && (!touchedGADTs || gadtIsInstantiated) - def checkArgs(): Boolean = + inFrozenGadtIf(!tyconIsInjective) { if tycon1sym == tycon2sym && tycon1sym.isAliasType then - // Either compare arguments or compare dealiased types. - // Note: This test should be done with an `either`, but this fails - // for hk-alias-unification.scala The problem is not GADT boundschecking; - // that is taken care of by the outer inFrozenGadtIf test. The problem is - // that hk-alias-unification.scala relies on the right isSubArgs being performed - // when constraining the result type of a method. But since #8635 we are only allowed - // to use a necessaryEither, since we might otherwise constrain too much. - // So strictly speaking by going to a sufficientEither we overshoot and narrow - // the constraint unnecessarily. But unlike in the situation of #8365 with test cases - // and-inf.scala and or-inf.scala, we don't cut off solutions in principle by dealiasing. - // We "just" risk missing an opportunity to do a desired hk-type inference. This - // is hopefully a less frequent corner case. Where it goes wrong compared to the status - // before this fix: If isSubArgs succeeds and then the dealised test also succeeds - // while constraining strictly less than the isSubArgs, then the we used to pick the - // constraint after isSubArgs, but we now pick the constraint after the dealiased test. - // There's one test in shapeless/deriving that had to be disabled - // (modules/deriving/src/test/scala/shapeless3/deriving/deriving.scala, value v7 in the functor test). - sufficientEither( - isSubArgs(args1, args2, tp1, tparams), - recur(tp1.superType, tp2.superType)) + val preConstraint = constraint + isSubArgs(args1, args2, tp1, tparams) + && tryAlso(preConstraint, recur(tp1.superType, tp2.superType)) else isSubArgs(args1, args2, tp1, tparams) - - inFrozenGadtIf(!tyconIsInjective)(checkArgs()) + } } if (res && touchedGADTs) GADTused = true res @@ -1594,17 +1576,23 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling */ private def sufficientEither(op1: => Boolean, op2: => Boolean): Boolean = val preConstraint = constraint - if op1 then - if constraint ne preConstraint then - // check whether `op2` generates a weaker constraint than `op1` - val leftConstraint = constraint - constraint = preConstraint - if !(op2 && subsumes(leftConstraint, constraint, preConstraint)) then - if constr != noPrinter && !subsumes(constraint, leftConstraint, preConstraint) then - constr.println(i"CUT - prefer $leftConstraint over $constraint") - constraint = leftConstraint - true - else op2 + if op1 then tryAlso(preConstraint, op2) else op2 + + /** Check whether `op` generates a weaker constraint than the + * current constraint if we run it starting with `preConstraint`. + * If that's the case, replace the current constraint with the + * constraint generated by `op`. + */ + private def tryAlso(preConstraint: Constraint, op: => Boolean): true = + if constraint ne preConstraint then + // check whether `op2` generates a weaker constraint than `op1` + val leftConstraint = constraint + constraint = preConstraint + if !(op && subsumes(leftConstraint, constraint, preConstraint)) then + if constr != noPrinter && !subsumes(constraint, leftConstraint, preConstraint) then + constr.println(i"CUT - prefer $leftConstraint over $constraint") + constraint = leftConstraint + true /** Returns true iff the result of evaluating either `op1` or `op2` is true, keeping the smaller constraint if any. * E.g., if