Improve KindFormula.Error reporting (#1085)

* Improve KindFormula.Error reporting

* reduce duplication

core/src/main/scala/org/bykn/bosatsu/KindFormula.scala

@@ -34,15 +34,22 @@ object KindFormula {
   case object Type extends KindFormula
   case class Cons(arg: Arg, result: KindFormula) extends KindFormula
 
-  sealed abstract class Error
+  sealed abstract class Error {
+    def dt: DefinedType[Option[Kind.Arg]]
+  }
   object Error {
     case class Unsatisfiable(
+        dte: DefinedType[Either[KnownShape, Kind.Arg]],
         constraints: LongMap[NonEmptyList[Constraint]],
         existing: LongMap[Variance],
         unknowns: SortedSet[Long]
-    ) extends Error
+    ) extends Error {
+      def dt: DefinedType[Option[Kind.Arg]] = dte.map(_.toOption)
+    }
 
-    case class FromShapeError(shapeError: Shape.Error) extends Error
+    case class FromShapeError(shapeError: Shape.Error) extends Error {
+      def dt = shapeError.dt
+    }
   }
 
   sealed abstract class Sat
@@ -299,7 +306,7 @@ object KindFormula {
       // we have allocated all variance variables now, we can see how many
       varCount <- state.nextId
       topo = Impl.combineTopo(varCount, constraints)
-      maybeRes = Impl.go(constraints, dirs, topo).map { vars =>
+      maybeRes = Impl.go(dt, constraints, dirs, topo).map { vars =>
         dtFormula.map(Impl.unformula(_, vars))
       }
     } yield maybeRes).run.value
@@ -362,6 +369,7 @@ object KindFormula {
     // invariant: all subgraph values must be valid keys in the result
     // we can process the subgraph list in parallel. Those are all the indepentent next values
     def allSolutionChunk(
+        dt: DefinedType[Either[Shape.KnownShape, Kind.Arg]], 
         cons: Cons,
         existing: LongMap[Variance],
         directions: LongMap[Direction],
@@ -422,30 +430,32 @@ object KindFormula {
         NonEmptyLazyList.fromLazyList(validVariances) match {
           case Some(nel) => Validated.valid(nel)
           case None =>
-            Validated.invalidNec(Error.Unsatisfiable(cons, existing, subgraph))
+            Validated.invalidNec(Error.Unsatisfiable(dt, cons, existing, subgraph))
         }
       }
 
     def allSolutions(
+        dt: DefinedType[Either[Shape.KnownShape, Kind.Arg]], 
         cons: Cons,
         existing: LongMap[Variance],
         directions: LongMap[Direction],
         subgraph: NonEmptyList[SortedSet[Long]]
     ): EitherNec[Error, NonEmptyLazyList[LongMap[Variance]]] =
       subgraph
-        .traverse(allSolutionChunk(cons, existing, directions, _))
+        .traverse(allSolutionChunk(dt, cons, existing, directions, _))
         .map(KindFormula.mergeCrossProduct(_))
         .toEither
 
     def go[F[_]: Foldable](
+        dt: DefinedType[Either[Shape.KnownShape, Kind.Arg]], 
         cons: Cons,
         directions: LongMap[Direction],
         topo: F[NonEmptyList[SortedSet[Long]]]
     ): ValidatedNec[Error, LongMap[Variance]] =
       topo.foldM(NonEmptyLazyList(LongMap.empty[Variance])) {
         (sols, subgraph) =>
           // if there is at least one good solution, we can keep going
-          val next = sols.map(allSolutions(cons, _, directions, subgraph))
+          val next = sols.map(allSolutions(dt, cons, _, directions, subgraph))
           val nextPaths: LazyList[LongMap[Variance]] =
             next.toLazyList.flatMap {
               case Right(ll) => ll.toLazyList
@@ -863,7 +873,6 @@ object KindFormula {
             )
           } yield ()
         }
-
     }
   }
 

core/src/main/scala/org/bykn/bosatsu/PackageError.scala

@@ -534,90 +534,64 @@ object PackageError {
   case class KindInferenceError(pack: PackageName, kindError: KindFormula.Error, regions: Map[Type.Const.Defined, Region]) extends PackageError {
     def message(sourceMap: Map[PackageName, (LocationMap, String)], errColor: Colorize) = {
       val (lm, _) = sourceMap.getMapSrc(pack)
-      kindError match {
-        case KindFormula.Error.Unsatisfiable(_, _, _) => 
-           val prefix = sourceMap.headLine(pack, None)
-          (prefix + Doc.text(s": $kindError")).render(80)
+      val region = regions(kindError.dt.toTypeConst)
+      val ctx = lm.showRegion(region, 2, errColor)
+        .getOrElse(Doc.str(region)) // we should highlight the whole region
+      val prefix = sourceMap.headLine(pack, Some(region))
+      val message = kindError match {
+        case KindFormula.Error.Unsatisfiable(_, _, _, _) => 
+           // TODO: would be good to give a more precise problem, e.g. which
+           // type parameters are the problem.
+          Doc.text("could not solve for valid variances")
         case KindFormula.Error.FromShapeError(se) =>
           se match {
-            case Shape.UnificationError(dt, cons, left, right) =>
-              val region = regions(dt.toTypeConst)
-              val prefix = sourceMap.headLine(pack, Some(region))
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
-              (prefix + Doc.hardLine + Doc.text("shape error: expected ") + Shape.shapeDoc(left) + Doc.text(" and ") + Shape.shapeDoc(right) +
-                Doc.text(s" to match in the constructor ${cons.name.sourceCodeRepr}") + Doc.hardLine + Doc.hardLine +
-                ctx).render(80)
-            case Shape.ShapeMismatch(dt, cons, outer, tyApp, right) =>
+            case Shape.UnificationError(_, cons, left, right) =>
+              Doc.text("shape error: expected ") + Shape.shapeDoc(left) + Doc.text(" and ") + Shape.shapeDoc(right) +
+                Doc.text(s" to match in the constructor ${cons.name.sourceCodeRepr}") + Doc.hardLine
+            case Shape.ShapeMismatch(_, cons, outer, tyApp, right) =>
               val tmap = showTypes(pack, outer :: tyApp :: Nil)
-              val region = regions(dt.toTypeConst)
-              val prefix = sourceMap.headLine(pack, Some(region))
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
               val typeDoc =
                 if (outer != tyApp) (tmap(outer) + Doc.text(" at application ") + tmap(tyApp))
                 else tmap(outer)
 
-              (prefix + Doc.text(" shape error: expected ") + Shape.shapeDoc(right) + Doc.text(" -> ?") + Doc.text(" but found * ") +
+              Doc.text("shape error: expected ") + Shape.shapeDoc(right) + Doc.text(" -> ?") + Doc.text(" but found * ") +
                 Doc.text(s"in the constructor ${cons.name.sourceCodeRepr} inside type ") +
                 typeDoc + 
-                Doc.hardLine + Doc.hardLine +
-                ctx).render(80)
+                Doc.hardLine
             case Shape.FinishFailure(dt, left, right) =>
-              val region = regions(dt.toTypeConst)
               val tdoc = showTypes(pack, dt.toTypeTyConst :: Nil)(dt.toTypeTyConst)
-              val prefix = sourceMap.headLine(pack, Some(region))
-              val message = Doc.text("in type ") + tdoc + Doc.text(" could not unify shapes: ") + Shape.shapeDoc(left) + Doc.text(" and ") +
+              Doc.text("in type ") + tdoc + Doc.text(" could not unify shapes: ") + Shape.shapeDoc(left) + Doc.text(" and ") +
                 Shape.shapeDoc(right)
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
-              (prefix + Doc.hardLine + message + Doc.hardLine + ctx).render(80)
             case Shape.ShapeLoop(dt, tpe, _) =>
-              val region = regions(dt.toTypeConst)
               val tpe2 = tpe match {
                 case Left(ap) => ap
                 case Right(v) => Type.TyVar(v)
               }
               val tdocs = showTypes(pack, dt.toTypeTyConst :: tpe2 :: Nil)
 
-              val prefix = sourceMap.headLine(pack, Some(region))
-              val message = Doc.text("in type ") + tdocs(dt.toTypeTyConst) + Doc.text(" cyclic dependency encountered in ") +
+              Doc.text("in type ") + tdocs(dt.toTypeTyConst) + Doc.text(" cyclic dependency encountered in ") +
                 tdocs(tpe2)
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
-              (prefix + Doc.hardLine + message + Doc.hardLine + ctx).render(80)
             case Shape.UnboundVar(dt, cfn, v) =>
-              val region = regions(dt.toTypeConst)
               val tpe2 = Type.TyVar(v)
               val tdocs = showTypes(pack, dt.toTypeTyConst :: tpe2 :: Nil)
 
-              val prefix = sourceMap.headLine(pack, Some(region))
               val cfnMsg = if (dt.isStruct) Doc.empty else {
                 Doc.text(s" in constructor ${cfn.name.sourceCodeRepr} ")
               }
-              val message = Doc.text("in type ") + tdocs(dt.toTypeTyConst) +
+              Doc.text("in type ") + tdocs(dt.toTypeTyConst) +
                 Doc.text(" unbound type variable ") + tdocs(tpe2) + cfnMsg
-
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
-              (prefix + Doc.hardLine + message + Doc.hardLine + ctx).render(80)
             case Shape.UnknownConst(dt, cfn, c) =>
-              val region = regions(dt.toTypeConst)
               val tpe2 = Type.TyConst(c)
               val tdocs = showTypes(pack, dt.toTypeTyConst :: tpe2 :: Nil)
 
-              val prefix = sourceMap.headLine(pack, Some(region))
               val cfnMsg = if (dt.isStruct) Doc.empty else {
                 Doc.text(s" in constructor ${cfn.name.sourceCodeRepr} ")
               }
-              val message = Doc.text("in type ") + tdocs(dt.toTypeTyConst) +
+              Doc.text("in type ") + tdocs(dt.toTypeTyConst) +
                 Doc.text(" unknown type ") + tdocs(tpe2) + cfnMsg
-
-              val ctx = lm.showRegion(region, 2, errColor)
-                .getOrElse(Doc.str(region)) // we should highlight the whole region
-              (prefix + Doc.hardLine + message + Doc.hardLine + ctx).render(80)
           }
       }
+      (prefix + Doc.hardLine + message + Doc.hardLine + ctx).render(80)
     }
   }
 }

core/src/main/scala/org/bykn/bosatsu/Shape.scala

@@ -67,7 +67,10 @@ object Shape {
         Doc.text("kind(") + tdoc + Doc.char(')')
     }
 
-  sealed abstract class Error
+  sealed abstract class Error {
+    def dt: DefinedType[Option[Kind.Arg]]
+  }
+
   case class ShapeLoop(
       dt: DefinedType[Option[Kind.Arg]],
       bound: Either[rankn.Type.TyApply, rankn.Type.Var.Bound],

core/src/test/scala/org/bykn/bosatsu/EvaluationTest.scala

@@ -2806,7 +2806,8 @@ package Foo
 struct Foo[a: *](a: a[Int])
 """)) { case kie@PackageError.KindInferenceError(_, _, _) =>
       assert(kie.message(Map.empty, Colorize.None) ==
-        """in file: <unknown source>, package Foo shape error: expected * -> ? but found * in the constructor Foo inside type a[Bosatsu/Predef::Int]
+        """in file: <unknown source>, package Foo
+shape error: expected * -> ? but found * in the constructor Foo inside type a[Bosatsu/Predef::Int]
 
 Region(14,41)""")
       ()
@@ -3270,4 +3271,19 @@ res = z matches (1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
       ()
     }
   }
+
+  test("kind errors show the region of the type") {
+    val testCode = """
+package ErrorCheck
+
+struct Foo[a: -*](get: a)
+
+"""
+   evalFail(List(testCode)) { case kie@PackageError.KindInferenceError(_, _, _) =>
+      val message = kie.message(Map.empty, Colorize.None)
+      assert(message.contains("Region(21,46)"))
+      assert(testCode.substring(21, 46) == "struct Foo[a: -*](get: a)")
+      ()
+    }
+  }
 }