Make alignArgsInAnd safe and turn it on by default

Turned out hmaps.scala requires the arg alignment to compile.
So we have our first counterexample that we cannot drop this
hack. Now it is made safe in the sense that no constraints get
lost anymore.

5 files changed, 30 insertions, 28 deletions

diff --git a/compiler/src/dotty/tools/dotc/config/Config.scala b/compiler/src/dotty/tools/dotc/config/Config.scala
index 900e5669f..dc56ad8b8 100644
--- a/compiler/src/dotty/tools/dotc/config/Config.scala
+++ b/compiler/src/dotty/tools/dotc/config/Config.scala
@@ -75,20 +75,14 @@ object Config {
   /** If this flag is set, take the fast path when comparing same-named type-aliases and types */
   final val fastPathForRefinedSubtype = true
 
-  /** If this flag is set, and we compute `T1 { X = S1 }` & `T2 { X = S2 }`,
-   *  try to align the refinements by computing `S1 =:= S2` (which might instantiate type parameters).
-   *  This rule is contentious because it cuts the constraint set. Also, it is
-   *  currently unsound because `&` gets called in computations on a constraint
-   *  itself. If the `=:=` test generates a new constraint, that constraint is then
-   *  out of sync with with the constraint on which the computation is performed.
-   *  The constraint resulting from `=:=` ends up to be thrown away whereas
-   *  its result is used, which is unsound. So if we want to turn this flag on
-   *  permanently instead of just for debugging, we have to refactor occurrences
-   *  of `&` in `OrderedConstraint` so that they take the `=:=` result into account.
+  /** If this flag is set, and we compute `T1 { X = S1 }` & `T2 { X = S2 }` as a new
+   *  upper bound of a constrained parameter, try to align the refinements by computing
+   *  `S1 =:= S2` (which might instantiate type parameters).
+   *  This rule is contentious because it cuts the constraint set.
    *
    *  For more info, see the comment in `TypeComparer#distributeAnd`.
    */
-  final val alignArgsInAnd = false
+  final val alignArgsInAnd = true
 
   /** If this flag is set, higher-kinded applications are checked for validity
    */
diff --git a/compiler/src/dotty/tools/dotc/core/Constraint.scala b/compiler/src/dotty/tools/dotc/core/Constraint.scala
index c99b748b7..50136a26c 100644
--- a/compiler/src/dotty/tools/dotc/core/Constraint.scala
+++ b/compiler/src/dotty/tools/dotc/core/Constraint.scala
@@ -111,12 +111,6 @@ abstract class Constraint extends Showable {
    */
   def replace(param: PolyParam, tp: Type)(implicit ctx: Context): This
 
-  /** Narrow one of the bounds of type parameter `param`
-   *  If `isUpper` is true, ensure that `param <: `bound`, otherwise ensure
-   *  that `param >: bound`.
-   */
-  def narrowBound(param: PolyParam, bound: Type, isUpper: Boolean)(implicit ctx: Context): This
-
   /** Is entry associated with `pt` removable? This is the case if
    *  all type parameters of the entry are associated with type variables
    *  which have their `inst` fields set.
diff --git a/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala b/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
index 3aa20f15b..89861f6db 100644
--- a/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
+++ b/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
@@ -44,6 +44,13 @@ trait ConstraintHandling {
     try op finally alwaysFluid = saved
   }
 
+  /** If set, align arguments `S1`, `S2`when taking the glb
+   *  `T1 { X = S1 } & T2 { X = S2 }` of a constraint upper bound for some type parameter.
+   *  Aligning means computing `S1 =:= S2` which may change the current constraint.
+   *  See note in TypeComparer#distributeAnd.
+   */
+  protected var homogenizeArgs = false
+
   /** We are currently comparing polytypes. Used as a flag for
    *  optimization: when `false`, no need to do an expensive `pruneLambdaParams`
    */
@@ -64,7 +71,8 @@ trait ConstraintHandling {
       }
       if (Config.checkConstraintsSeparated)
         assert(!occursIn(bound), s"$param occurs in $bound")
-      val c1 = constraint.narrowBound(param, bound, isUpper)
+      val newBound = narrowedBound(param, bound, isUpper)
+      val c1 = constraint.updateEntry(param, newBound)
       (c1 eq constraint) || {
         constraint = c1
         val TypeBounds(lo, hi) = constraint.entry(param)
@@ -72,6 +80,20 @@ trait ConstraintHandling {
       }
     }
 
+  /** Narrow one of the bounds of type parameter `param`
+   *  If `isUpper` is true, ensure that `param <: `bound`, otherwise ensure
+   *  that `param >: bound`.
+   */
+  def narrowedBound(param: PolyParam, bound: Type, isUpper: Boolean)(implicit ctx: Context): TypeBounds = {
+    val oldBounds @ TypeBounds(lo, hi) = constraint.nonParamBounds(param)
+    val saved = homogenizeArgs
+    homogenizeArgs = Config.alignArgsInAnd
+    try
+      if (isUpper) oldBounds.derivedTypeBounds(lo, hi & bound)
+      else oldBounds.derivedTypeBounds(lo | bound, hi)
+    finally homogenizeArgs = saved
+  }
+
   protected def addUpperBound(param: PolyParam, bound: Type): Boolean = {
     def description = i"constraint $param <: $bound to\n$constraint"
     if (bound.isRef(defn.NothingClass) && ctx.typerState.isGlobalCommittable) {
diff --git a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala b/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala
index 72c7a8e51..61dd5a445 100644
--- a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala
+++ b/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala
@@ -354,14 +354,6 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
     updateEntry(p1, p1Bounds).replace(p2, p1)
   }
 
-  def narrowBound(param: PolyParam, bound: Type, isUpper: Boolean)(implicit ctx: Context): This = {
-    val oldBounds @ TypeBounds(lo, hi) = nonParamBounds(param)
-    val newBounds =
-      if (isUpper) oldBounds.derivedTypeBounds(lo, hi & bound)
-      else oldBounds.derivedTypeBounds(lo | bound, hi)
-    updateEntry(param, newBounds)
-  }
-
 // ---------- Removals ------------------------------------------------------------
 
   /** A new constraint which is derived from this constraint by removing
diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
index 744112280..6042cc12c 100644
--- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
+++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
@@ -1315,7 +1315,7 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
           // Given two refinements `T1 { X = S1 }` and `T2 { X = S2 }` rwrite to
           // `T1 & T2 { X B }` where `B` is the conjunction of the bounds of `X` in `T1` and `T2`.
           //
-          // However, if `Config.alignArgsInAnd` is set, and both aliases `X = Si` are
+          // However, if `homogenizeArgs` is set, and both aliases `X = Si` are
           // nonvariant, and `S1 =:= S2` (possibly by instantiating type parameters),
           // rewrite instead to `T1 & T2 { X = S1 }`. This rule is contentious because
           // it cuts the constraint set. On the other hand, without it we would replace
@@ -1329,7 +1329,7 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
             case tp: TypeAlias => tp.variance == 0
             case _ => false
           }
-          if (Config.alignArgsInAnd &&
+          if (homogenizeArgs &&
               isNonvariantAlias(rinfo1) && isNonvariantAlias(rinfo2))
             isSameType(rinfo1, rinfo2)