Non-fatal shrink-multiply operation in compiler (#403)

Adds general infrastructure for non-fatal expr errors in the compiler,
folding the over-assign errors into this mechanism. Also bumps expected
proto version.

Some error semantics changed:
- max/min(RangeEmpty) now returns a nonfatal error (instead of NaN)
- unpack_range(empty) now returns a nonfatal error (instead of
RangeEmpty)
- set_extract(empty | different) now returns a nonfatal error (instead
of a fatal compiler exception)
- intersection(non-intersecting range) now returns a nonfatal error
(instead of range-empty) - is this the right answer?

Infrastructural changes:
- Change expr eval ref arg to a refResolver instead of the whole
constProp object
- ConstProp has a clearReadyNode that removes a node from ready without
setting a value / propagating it, to handle these non-propagating error
values
- Add errors-should-be-empty tests on some unit tests

TODO:
- [x] remove error type from IR
- [x] add overassign cause to overassign error

Author: ducky64

compiler/src/main/scala/edg/compiler/Compiler.scala

@@ -117,7 +117,7 @@ class AssignNamer() {
 }
 
 object Compiler {
-  final val kExpectedProtoVersion = 6
+  final val kExpectedProtoVersion = 7
 }
 
 /** Compiler for a particular design, with an associated library to elaborate references from.
@@ -1695,7 +1695,7 @@ class Compiler private (
   }
 
   def evaluateExpr(root: DesignPath, value: expr.ValueExpr): ExprResult = {
-    new ExprEvaluatePartial(constProp, root).map(value)
+    new ExprEvaluatePartial(constProp.getValue, root).map(value)
   }
 
   // Primarily used for unit tests, TODO clean up this API?

compiler/src/main/scala/edg/compiler/CompilerError.scala

@@ -104,16 +104,15 @@ object CompilerError {
     }
   }
 
-  case class OverAssign(target: IndirectDesignPath, causes: Seq[OverAssignCause]) extends CompilerError {
-    override def toString: String = s"Overassign to $target:\n" +
-      s"${causes.map(x => s"- $x").mkString("\n")}"
+  case class ExprError(target: IndirectDesignPath, msg: String) extends CompilerError {
+    override def toString: String = s"Expr error at $target: $msg"
 
     override def toIr: edgcompiler.ErrorRecord = {
       edgcompiler.ErrorRecord(
         path = Some(target.toLocalPath),
-        kind = "Overassign",
+        kind = "Expr",
         name = "",
-        details = causes.map(_.toString).mkString(", ")
+        details = msg
       )
     }
   }

compiler/src/main/scala/edg/compiler/ConstProp.scala

@@ -1,17 +1,16 @@
 package edg.compiler
 
-import scala.collection.mutable
-import scala.collection.Set
+import edg.ExprBuilder
+import edg.compiler.CompilerError.ExprError
+import edg.util.DependencyGraph
+import edg.wir._
 import edgir.expr.expr
 import edgir.init.init
-import edg.wir._
-import edg.util.{DependencyGraph, MutableBiMap}
-import edg.ExprBuilder
 import edgir.ref.ref.LocalPath
 
-case class AssignRecord(target: IndirectDesignPath, root: DesignPath, value: expr.ValueExpr)
+import scala.collection.{Set, mutable}
 
-case class OverassignRecord(assigns: mutable.Set[(DesignPath, String, expr.ValueExpr)] = mutable.Set())
+case class AssignRecord(target: IndirectDesignPath, root: DesignPath, value: expr.ValueExpr)
 
 sealed trait ConnectedLinkRecord // a record in the connected link directed graph
 object ConnectedLinkRecord {
@@ -53,21 +52,20 @@ class ConstProp() {
   // Params that have a forced/override value, so they arent over-assigned.
   private val forcedParams = mutable.Set[IndirectDesignPath]()
 
-  // Overassigns, for error tracking
-  // This only tracks overassigns that were discarded, not including assigns that took effect.
-  // Additional analysis is needed to get the full set of conflicting assigns.
-  private val discardOverassigns = mutable.HashMap[IndirectDesignPath, OverassignRecord]()
+  // Errors that were generated during the process of resolving parameters, including overassigns
+  // A value may or may not exist (and may or not have been propagated) in the param dependency graph
+  private val paramErrors = mutable.HashMap[IndirectDesignPath, mutable.ListBuffer[ErrorValue]]()
 
   def initFrom(that: ConstProp): Unit = {
     require(paramAssign.isEmpty && paramSource.isEmpty && paramTypes.isEmpty && forcedParams.isEmpty
-      && discardOverassigns.isEmpty)
+      && paramErrors.isEmpty)
     paramAssign.addAll(that.paramAssign)
     paramSource.addAll(that.paramSource)
     params.initFrom(that.params)
     paramTypes.addAll(that.paramTypes)
     connectedLink.initFrom(that.connectedLink)
     forcedParams.addAll(that.forcedParams)
-    discardOverassigns.addAll(that.discardOverassigns)
+    paramErrors.addAll(that.paramErrors)
   }
 
   //
@@ -120,9 +118,15 @@ class ConstProp() {
       }
       readyList.foreach { constrTarget =>
         val assign = paramAssign(constrTarget)
-        new ExprEvaluatePartial(this, assign.root).map(assign.value) match {
+        new ExprEvaluatePartial(getValue, assign.root).map(assign.value) match {
           case ExprResult.Result(result) =>
-            params.setValue(constrTarget, result)
+            result match {
+              case result @ ErrorValue(_) =>
+                paramErrors.getOrElseUpdate(constrTarget, mutable.ListBuffer()).append(result)
+                params.clearReadyNode(constrTarget)
+              case result => params.setValue(constrTarget, result)
+            }
+
             onParamSolved(constrTarget, result)
           case ExprResult.Missing(missing) => // account for CONNECTED_LINK prefix
             val missingCorrected = missing.map { path =>
@@ -202,9 +206,11 @@ class ConstProp() {
       params.addNode(target, Seq(), overwrite = true) // forced can overwrite other records
     } else {
       if (!forcedParams.contains(target)) {
-        if (params.nodeDefinedAt(target)) {
-          val record = discardOverassigns.getOrElseUpdate(target, OverassignRecord())
-          record.assigns.add(paramSourceRecord)
+        if (params.nodeDefinedAt(target)) { // TODO add propagated assign
+          val (prevRoot, prevConstr, _) = paramSource.get(target).getOrElse("?", "?", "")
+          paramErrors.getOrElseUpdate(target, mutable.ListBuffer()).append(
+            ErrorValue(s"over-assign from $root.$constrName, prev assigned from $prevRoot.$prevConstr")
+          )
           return // first set "wins"
         }
         params.addNode(target, Seq())
@@ -254,7 +260,7 @@ class ConstProp() {
   }
 
   /** Returns the value of a parameter, or None if it does not have a value (yet?). Can be used to check if parameters
-    * are resolved yet by testing against None.
+    * are resolved yet by testing against None. Cannot return an ErrorValue
     */
   def getValue(param: IndirectDesignPath): Option[ExprValue] = {
     resolveConnectedLink(param) match {
@@ -264,7 +270,6 @@ class ConstProp() {
 
   }
   def getValue(param: DesignPath): Option[ExprValue] = {
-    // TODO should this be an implicit conversion?
     getValue(param.asIndirect)
   }
 
@@ -282,20 +287,14 @@ class ConstProp() {
     * references.
     */
   def getUnsolved: Set[IndirectDesignPath] = {
-    paramTypes.keySet.toSet -- params.knownValueKeys
+    paramTypes.keySet.toSet -- params.knownValueKeys -- paramErrors.keys
   }
 
   def getAllSolved: Map[IndirectDesignPath, ExprValue] = params.toMap
 
-  def getErrors: Seq[CompilerError] = {
-    discardOverassigns.map { case (target, record) =>
-      val propagatedAssign = paramSource.get(target).map { case (root, constrName, value) =>
-        CompilerError.OverAssignCause.Assign(target, root, constrName, value)
-      }.toSeq
-      val discardedAssigns = record.assigns.map { case (root, constrName, value) =>
-        CompilerError.OverAssignCause.Assign(target, root, constrName, value)
-      }
-      CompilerError.OverAssign(target, propagatedAssign ++ discardedAssigns)
+  def getErrors: Seq[ExprError] = {
+    paramErrors.flatMap { case (target, errors) =>
+      errors.map(error => ExprError(target, error.msg))
     }.toSeq
   }
 }

compiler/src/main/scala/edg/compiler/ExprEvaluate.scala

@@ -60,8 +60,8 @@ object ExprEvaluate {
           case (RangeValue(targetMin, targetMax), RangeValue(contribMin, contribMax)) =>
             val lower = contribMax * targetMin
             val upper = contribMin * targetMax
-            if (lower > upper) { // TODO this should store a nonfatal error result instead of crashing on-the-spot
-              throw new ExprEvaluateException(s"Empty range result in $lhs ${binary.op} $rhs from $binary")
+            if (lower > upper) {
+              ErrorValue(s"shrink_mult($lhs, $rhs) produces empty range, target $lhs tighter tol than contrib $rhs")
             } else {
               RangeValue(lower, upper)
             }
@@ -285,10 +285,8 @@ object ExprEvaluate {
       case (Op.MIN, RangeValue(valMin, _)) => FloatValue(valMin)
       case (Op.MAX, RangeValue(_, valMax)) => FloatValue(valMax)
 
-      // TODO can we have stricter semantics to avoid min(RangeEmpty) and max(RangeEmpty)?
-      // This just NaNs out so at least it propagates
-      case (Op.MAX, RangeEmpty) => FloatValue(Float.NaN)
-      case (Op.MIN, RangeEmpty) => FloatValue(Float.NaN)
+      case (Op.MAX, RangeEmpty) => ErrorValue("max(RangeEmpty) is undefined")
+      case (Op.MIN, RangeEmpty) => ErrorValue("min(RangeEmpty) is undefined")
 
       case (Op.CENTER, RangeValue(valMin, valMax)) => FloatValue((valMin + valMax) / 2)
       case (Op.WIDTH, RangeValue(valMin, valMax)) => FloatValue(math.abs(valMax - valMin))
@@ -307,7 +305,7 @@ object ExprEvaluate {
       case (Op.SUM, ArrayValue.ExtractBoolean(vals)) => IntValue(vals.count(_ == true))
       case (Op.SUM, ArrayValue.UnpackRange(extracted)) => extracted match {
           case ArrayValue.UnpackRange.FullRange(valMins, valMaxs) => RangeValue(valMins.sum, valMaxs.sum)
-          case _ => RangeEmpty // TODO how should sum behave on empty ranges?
+          case _ => ErrorValue("unpack_range(empty) is undefined")
         }
 
       case (Op.ALL_TRUE, ArrayValue.Empty(_)) => BooleanValue(true)
@@ -326,13 +324,11 @@ object ExprEvaluate {
       case (Op.MINIMUM, ArrayValue.ExtractFloat(vals)) => FloatValue(vals.min)
       case (Op.MINIMUM, ArrayValue.ExtractInt(vals)) => IntValue(vals.min)
 
-      // TODO this should be a user-level assertion instead of a compiler error
-      case (Op.SET_EXTRACT, ArrayValue.Empty(_)) =>
-        throw new ExprEvaluateException(s"SetExtract with empty values from $unarySet")
+      case (Op.SET_EXTRACT, ArrayValue.Empty(_)) => ErrorValue("set_extract(empty) is undefined")
       case (Op.SET_EXTRACT, ArrayValue(vals)) => if (vals.forall(_ == vals.head)) {
           vals.head
         } else {
-          throw new ExprEvaluateException(s"SetExtract with non-equal values $vals from $unarySet")
+          ErrorValue(f"set_extract($vals) with non-equal values")
         }
 
       // Any empty value means the expression result is empty
@@ -342,19 +338,20 @@ object ExprEvaluate {
             if (maxMin <= minMax) {
               RangeValue(maxMin, minMax)
             } else { // does not intersect, null set
-              RangeEmpty
+              ErrorValue(f"intersection($extracted) produces empty set")
             }
           // The implicit initial value of intersect is the full range
           // TODO are these good semantics?
           case ArrayValue.UnpackRange.EmptyArray() => RangeValue(Float.NegativeInfinity, Float.PositiveInfinity)
-          case _ => RangeEmpty
+          case _ => ErrorValue(f"intersection($vals) is undefined")
         }
 
       // Any value is used (empty effectively discarded)
       case (Op.HULL, ArrayValue.UnpackRange(extracted)) => extracted match {
           case ArrayValue.UnpackRange.FullRange(valMins, valMaxs) => RangeValue(valMins.min, valMaxs.max)
           case ArrayValue.UnpackRange.RangeWithEmpty(valMins, valMaxs) => RangeValue(valMins.min, valMaxs.max)
-          case _ => RangeEmpty
+          case ArrayValue.UnpackRange.EmptyArray() => RangeEmpty // TODO: should this be an error?
+          case ArrayValue.UnpackRange.EmptyRange() => RangeEmpty
         }
       case (Op.HULL, ArrayValue.ExtractFloat(vals)) => RangeValue(vals.min, vals.max)
 
@@ -397,7 +394,7 @@ object ExprEvaluate {
     case (FloatPromotable(lhs), FloatPromotable(rhs)) => if (lhs <= rhs) {
         RangeValue(lhs, rhs)
       } else {
-        throw new ExprEvaluateException(s"Range with min $minimum not <= max $maximum from $range")
+        ErrorValue(s"range($minimum, $maximum) is malformed, $minimum > $maximum")
       }
     case _ => throw new ExprEvaluateException(s"Unknown range operands types $minimum $maximum from $range")
   }

compiler/src/main/scala/edg/compiler/ExprEvaluatePartial.scala

@@ -16,7 +16,8 @@ object ExprResult {
   * missing set may increase as more parameters are solved, because some expressions have dynamic references. For
   * example, if-then-else additionally depends on either true or false subexprs once the condition is known.
   */
-class ExprEvaluatePartial(refs: ConstProp, root: DesignPath) extends ValueExprMap[ExprResult] {
+class ExprEvaluatePartial(refResolver: IndirectDesignPath => Option[ExprValue], root: DesignPath)
+    extends ValueExprMap[ExprResult] {
   override def mapLiteral(literal: lit.ValueLit): ExprResult =
     ExprResult.Result(ExprEvaluate.evalLiteral(literal))
 
@@ -126,11 +127,11 @@ class ExprEvaluatePartial(refs: ConstProp, root: DesignPath) extends ValueExprMa
     val container = mapExtract.getContainer.expr.ref.getOrElse( // TODO restrict allowed types in proto
       throw new ExprEvaluateException(s"Non-ref container type in mapExtract $mapExtract"))
     val containerPath = root ++ container
-    refs.getValue(containerPath.asIndirect + IndirectStep.Elements) match {
+    refResolver(containerPath.asIndirect + IndirectStep.Elements) match {
       case Some(paramValue) =>
         val eltsVals = ArrayValue.ExtractText(paramValue).map { elt =>
           val eltPath = containerPath.asIndirect + elt ++ mapExtract.getPath
-          refs.getValue(eltPath) match {
+          refResolver(eltPath) match {
             case Some(value) => ExprResult.Result(value)
             case None => ExprResult.Missing(Set(eltPath))
           }
@@ -151,7 +152,7 @@ class ExprEvaluatePartial(refs: ConstProp, root: DesignPath) extends ValueExprMa
 
   override def mapRef(path: ref.LocalPath): ExprResult = {
     val refPath = root.asIndirect ++ path
-    refs.getValue(refPath) match {
+    refResolver(refPath) match {
       case Some(value) => ExprResult.Result(value)
       case None => ExprResult.Missing(Set(refPath))
     }

compiler/src/main/scala/edg/compiler/ExprToString.scala

@@ -29,7 +29,7 @@ class ExprToString() extends ValueExprMap[String] {
     case lit.ValueLit.Type.Array(array) =>
       val arrayElts = array.elts.map(mapLiteral)
       s"[${arrayElts.mkString(", ")}]"
-    case lit.ValueLit.Type.Struct(value) => "unsupported struct"
+    case lit.ValueLit.Type.Struct(_) => "unsupported struct"
     case lit.ValueLit.Type.Empty => "(empty)"
   }
 

compiler/src/main/scala/edg/compiler/ExprValue.scala

@@ -20,7 +20,8 @@ object ExprValue {
     case init.ValInit.Val.Text(_) => classOf[TextValue]
     case init.ValInit.Val.Range(_) => classOf[RangeType]
     case init.ValInit.Val.Array(_) => classOf[ArrayValue[Nothing]]
-    case _ => throw new IllegalArgumentException(s"Unknown valinit $initObj")
+    case init.ValInit.Val.Empty | init.ValInit.Val.Set(_) | init.ValInit.Val.Struct(_) =>
+      throw new IllegalArgumentException(s"unsupported valInit $initObj")
   }
 
   def fromValueLit(literal: lit.ValueLit): ExprValue = literal.`type` match {
@@ -41,7 +42,8 @@ object ExprValue {
       }
     case lit.ValueLit.Type.Array(arrayLiteral) =>
       ArrayValue(arrayLiteral.elts.map { lit => fromValueLit(lit) })
-    case _ => throw new IllegalArgumentException(s"Unknown literal $literal")
+    case lit.ValueLit.Type.Empty | lit.ValueLit.Type.Struct(_) =>
+      throw new IllegalArgumentException(s"unsupported literal $literal")
   }
 }
 
@@ -205,3 +207,9 @@ case class ArrayValue[T <: ExprValue](values: Seq[T]) extends ExprValue {
     s"[$valuesString]"
   }
 }
+
+// a special value not in the IR that indicates and error
+case class ErrorValue(msg: String) extends ExprValue {
+  override def toLit: lit.ValueLit = throw new IllegalArgumentException("cannot convert error value to literal")
+  override def toStringValue: String = msg
+}

compiler/src/main/scala/edg/util/DependencyGraph.scala

@@ -62,7 +62,17 @@ class DependencyGraph[KeyType, ValueType] {
   // Node must exist, or this will exception out
   def nodeMissing(node: KeyType): Set[KeyType] = deps(node).toSet
 
+  // Clears a node from ready without setting a value in the graph.
+  // Useful to stop propagation at some point, but without crashing.
+  // The node may be marked ready again by other sources.
+  def clearReadyNode(node: KeyType): Unit = {
+    require(ready.contains(node), s"attempt to clear ready node $node that is not ready")
+    ready -= node
+  }
+
   // Sets the value of a node. May not overwrite values.
+  // If stop is true, propagation will not continue:
+  // while the node will have a value, dependents will not be marked as ready
   def setValue(node: KeyType, value: ValueType): Unit = {
     require(!values.isDefinedAt(node), s"redefinition of $node (prior value ${values(node)}, new value $value)")
     deps.put(node, mutable.Set())

compiler/src/test/scala/edg/compiler/ConstPropArrayTest.scala

@@ -5,6 +5,7 @@ import org.scalatest.flatspec.AnyFlatSpec
 import matchers.should.Matchers._
 import edg.wir.{DesignPath, IndirectDesignPath, IndirectStep}
 import edg.ExprBuilder._
+import edg.compiler.CompilerError.ExprError
 
 class ConstPropArrayTest extends AnyFlatSpec {
   behavior.of("ConstProp with array paths")
@@ -89,12 +90,12 @@ class ConstPropArrayTest extends AnyFlatSpec {
     val constProp = new ConstProp()
     addArray(constProp, Seq(), 4, { i => ValueExpr.Literal(i + 2) }, Seq("ports"), Seq("param"))
     constProp.addDeclaration(DesignPath() + "reduce", ValInit.Integer)
-    an[ExprEvaluateException] should be thrownBy {
-      constProp.addAssignExpr(
-        IndirectDesignPath() + "reduce",
-        ValueExpr.UnarySetOp(Op.SET_EXTRACT, ValueExpr.MapExtract(Ref("ports"), "param"))
-      )
-    }
+    constProp.addAssignExpr(
+      IndirectDesignPath() + "reduce",
+      ValueExpr.UnarySetOp(Op.SET_EXTRACT, ValueExpr.MapExtract(Ref("ports"), "param"))
+    )
+    constProp.getValue(IndirectDesignPath() + "reduce") should equal(None)
+    constProp.getErrors should not be empty
   }
 
   it should "SetExtract for same values" in {