Add explicit fallback to array reduction; add error value (#485)

Changes the IR and compiler to define the empty array output of array
reduction operations in the frontend, allowing better type propagation
(especially disambiguating float 0 and range 0 for sums).

Changes the error handling to generate an error value instead of
crashing the compiler, allowing a partial compile instead of a unhelpful
stack trace.

In the compiler, moves the param errors into the param value, while
tracking overassigns separately because of the immutability of the param
value system.

Also tightens up what reduction operations are allowed in the array
subtypes.

Resolves #223

Author: ducky64

compiler/src/main/scala/edg/ExprBuilder.scala

@@ -49,9 +49,11 @@ object ExprBuilder {
       expr = expr.ValueExpr.Expr.Unary(expr.UnaryExpr(op = op, `val` = Some(vals)))
     )
 
-    def UnarySetOp(op: expr.UnarySetExpr.Op, vals: expr.ValueExpr): expr.ValueExpr = expr.ValueExpr(
-      expr = expr.ValueExpr.Expr.UnarySet(expr.UnarySetExpr(op = op, vals = Some(vals)))
-    )
+    def UnarySetOp(op: expr.UnarySetExpr.Op, vals: expr.ValueExpr, emptyValue: expr.ValueExpr): expr.ValueExpr =
+      expr.ValueExpr(
+        expr =
+          expr.ValueExpr.Expr.UnarySet(expr.UnarySetExpr(op = op, vals = Some(vals), emptyValue = Some(emptyValue)))
+      )
 
     def IfThenElse(cond: expr.ValueExpr, tru: expr.ValueExpr, fal: expr.ValueExpr): expr.ValueExpr = expr.ValueExpr(
       expr = expr.ValueExpr.Expr.IfThenElse(expr.IfThenElseExpr(cond = Some(cond), tru = Some(tru), fal = Some(fal)))
@@ -113,6 +115,9 @@ object ExprBuilder {
   }
 
   object Literal {
+    def Error(msg: Option[String]): lit.ValueLit =
+      lit.ValueLit(`type` = lit.ValueLit.Type.Error(lit.ErrorLit(msg.getOrElse(""))))
+
     def Floating(value: Double): lit.ValueLit = Floating(value.toFloat)
     def Floating(value: Float): lit.ValueLit = lit.ValueLit(`type` = lit.ValueLit.Type.Floating(lit.FloatLit(value)))
 

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

@@ -117,7 +117,7 @@ class AssignNamer() {
 }
 
 object Compiler {
-  final val kExpectedProtoVersion = 10
+  final val kExpectedProtoVersion = 11
 }
 
 /** Compiler for a particular design, with an associated library to elaborate references from.
@@ -919,7 +919,8 @@ class Compiler private (
                     path.asIndirect ++ portPostfix + IndirectStep.Allocated,
                     ValueExpr.UnarySetOp(
                       expr.UnarySetExpr.Op.FLATTEN,
-                      ValueExpr.Array(Seq(connectTerms) ++ arrayConnectTermss)
+                      ValueExpr.Array(Seq(connectTerms) ++ arrayConnectTermss),
+                      ValueExpr.Array(Seq())
                     ),
                     path,
                     ""
@@ -1235,7 +1236,8 @@ class Compiler private (
                   path.asIndirect ++ portPostfix + IndirectStep.Allocated,
                   ValueExpr.UnarySetOp(
                     expr.UnarySetExpr.Op.FLATTEN,
-                    ValueExpr.Array(Seq(connectTerms) ++ arrayConnectTermss)
+                    ValueExpr.Array(Seq(connectTerms) ++ arrayConnectTermss),
+                    ValueExpr.Array(Seq())
                   ),
                   path,
                   ""

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

@@ -51,25 +51,25 @@ class ConstProp() {
   // Undeclared parameters cannot have values set, but can be forced (though the value is not effective until declared)
   private val paramTypes = mutable.HashMap[IndirectDesignPath, Class[_ <: ExprValue]]()
 
+  // This tracks overassign errors and their sources, which otherwise breaks the immutability of the dependency graph
+  private val overassigns = mutable.HashMap[IndirectDesignPath, mutable.HashSet[IndirectDesignPath]]()
+
   private val connectedLink = DependencyGraph[ConnectedLinkRecord, ConnectedLinkPort]() // tracks the port -> link paths
 
   // Params that have a forced/override value, so they arent over-assigned.
   private val forcedParams = mutable.Set[IndirectDesignPath]()
 
-  // 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
-      && paramErrors.isEmpty)
+    require(
+      paramAssign.isEmpty && paramSource.isEmpty && paramTypes.isEmpty && forcedParams.isEmpty && overassigns.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)
-    paramErrors.addAll(that.paramErrors)
+    overassigns.addAll(that.overassigns)
   }
 
   //
@@ -116,14 +116,11 @@ class ConstProp() {
       readyList.foreach { constrTarget =>
         val assign = paramAssign(constrTarget)
         new ExprEvaluatePartial(getValue, assign.root).map(assign.value) match {
+          case ExprResult.Result(result @ ErrorValue(_)) =>
+            params.setValue(constrTarget, result, stop = true)
+            onParamSolved(constrTarget, result)
           case ExprResult.Result(result) =>
-            result match {
-              case result @ ErrorValue(_) =>
-                paramErrors.getOrElseUpdate(constrTarget, mutable.ListBuffer()).append(result)
-                params.clearReadyNode(constrTarget)
-              case result => params.setValue(constrTarget, result)
-            }
-
+            params.setValue(constrTarget, result)
             onParamSolved(constrTarget, result)
           case ExprResult.Missing(missing) => // account for CONNECTED_LINK prefix
             val missingCorrected = missing.map { path =>
@@ -211,10 +208,9 @@ class ConstProp() {
     } else {
       if (!forcedParams.contains(target)) {
         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")
-          )
+          val (prevRoot, prevConstr, _) = paramSource(target)
+          overassigns.getOrElseUpdate(target, mutable.HashSet()).add(root.asIndirect + constrName)
+          overassigns.getOrElseUpdate(target, mutable.HashSet()).add(prevRoot.asIndirect + prevConstr)
           return // first set "wins"
         }
         params.addNode(target, paramTypesDep)
@@ -264,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. Cannot return an ErrorValue
+    * are resolved yet by testing against None. Overassigns return their first value for consistency.
     */
   def getValue(param: IndirectDesignPath): Option[ExprValue] = {
     resolveConnectedLink(param) match {
@@ -291,10 +287,16 @@ class ConstProp() {
     * references.
     */
   def getUnsolved: Set[IndirectDesignPath] = {
-    paramTypes.keySet.toSet -- params.knownValueKeys -- paramErrors.keys
+    paramTypes.keySet.toSet -- params.knownValueKeys
+  }
+
+  private def getOverassignValues: Map[IndirectDesignPath, ErrorValue] = {
+    overassigns.map { case (target, sources) =>
+      target -> ErrorValue(Some(s"overassign from " + sources.map(_.toString).mkString(", ")))
+    }.toMap
   }
 
-  def getAllSolved: Map[IndirectDesignPath, ExprValue] = params.toMap
+  def getAllSolved: Map[IndirectDesignPath, ExprValue] = params.toMap ++ getOverassignValues
 
   def getAllConnections: Map[DesignPath, DesignPath] = connectedLink.toMap.collect {
     case (ConnectedLinkRecord.ConnectedLink(towardsBlockPort), ConnectedLinkPort(linkPath, linkPortSuffix))
@@ -303,8 +305,10 @@ class ConstProp() {
   }
 
   def getErrors: Seq[ExprError] = {
-    paramErrors.flatMap { case (target, errors) =>
-      errors.map(error => ExprError(target, error.msg))
-    }.toSeq
+    (params.toMap.collect { case (target, ErrorValue(Some(msg))) =>
+      ExprError(target, msg)
+    } ++ getOverassignValues.map {
+      case (target, error) => ExprError(target, error.msg.get)
+    }).toSeq
   }
 }

compiler/src/main/scala/edg/compiler/DesignRefsValidate.scala

@@ -34,8 +34,12 @@ object CollectExprRefs extends ValueExprMap[Seq[ref.LocalPath]] {
   override def mapUnary(unary: expr.UnaryExpr, `val`: Seq[ref.LocalPath]): Seq[ref.LocalPath] = {
     `val`
   }
-  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: Seq[ref.LocalPath]): Seq[ref.LocalPath] = {
-    vals
+  override def mapUnarySet(
+      unarySet: expr.UnarySetExpr,
+      vals: Seq[ref.LocalPath],
+      emptyValue: Seq[ref.LocalPath]
+  ): Seq[ref.LocalPath] = {
+    vals ++ emptyValue
   }
   override def mapArray(array: expr.ArrayExpr, vals: Seq[Seq[ref.LocalPath]]): Seq[ref.LocalPath] = {
     vals.flatten

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

@@ -59,7 +59,9 @@ object ExprEvaluate {
             val lower = contribMax * targetMin
             val upper = contribMin * targetMax
             if (lower > upper) {
-              ErrorValue(s"shrink_mult($lhs, $rhs) produces empty range, target $lhs tighter tol than contrib $rhs")
+              ErrorValue(
+                Some(s"shrink_mult($lhs, $rhs) produces empty range, target $lhs tighter tol than contrib $rhs")
+              )
             } else {
               RangeValue(lower, upper)
             }
@@ -293,8 +295,8 @@ object ExprEvaluate {
       case (Op.MIN, RangeValue(valMin, _)) => FloatValue(valMin)
       case (Op.MAX, RangeValue(_, valMax)) => FloatValue(valMax)
 
-      case (Op.MAX, RangeEmpty) => ErrorValue("max(RangeEmpty) is undefined")
-      case (Op.MIN, RangeEmpty) => ErrorValue("min(RangeEmpty) is undefined")
+      case (Op.MAX, RangeEmpty) => ErrorValue(Some("max(RangeEmpty) is undefined"))
+      case (Op.MIN, RangeEmpty) => ErrorValue(Some("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))
@@ -303,25 +305,23 @@ object ExprEvaluate {
     }
   }
 
-  def evalUnarySet(unarySet: expr.UnarySetExpr, vals: ExprValue): ExprValue = {
+  def evalUnarySet(unarySet: expr.UnarySetExpr, vals: ExprValue, emptyValue: ExprValue): ExprValue = {
     import expr.UnarySetExpr.Op
     (unarySet.op, vals) match {
+      case (_, ArrayValue.Empty(_)) => emptyValue
       // In this case we don't do numeric promotion
-      case (Op.SUM, ArrayValue.Empty(_)) => FloatValue(0) // TODO type needs to be dynamic
       case (Op.SUM, ArrayValue.ExtractFloat(vals)) => FloatValue(vals.sum)
       case (Op.SUM, ArrayValue.ExtractInt(vals)) => IntValue(vals.sum)
       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 ArrayValue.UnpackRange.RangeWithEmpty(_, _) => RangeEmpty
           case ArrayValue.UnpackRange.EmptyRange() => RangeEmpty
-          case ArrayValue.UnpackRange.EmptyArray() =>
-            RangeValue(0, 0) // unreachable in practice, superseded by float 0 case
+          case ArrayValue.UnpackRange.EmptyArray() => // empty array case handled above
+            throw new AssertionError("compiler internal error, shouldn't happen")
         }
 
-      case (Op.ALL_TRUE, ArrayValue.Empty(_)) => BooleanValue(true)
       case (Op.ALL_TRUE, ArrayValue.ExtractBoolean(vals)) => BooleanValue(vals.forall(_ == true))
-      case (Op.ANY_TRUE, ArrayValue.Empty(_)) => BooleanValue(false)
       case (Op.ANY_TRUE, ArrayValue.ExtractBoolean(vals)) => BooleanValue(vals.contains(true))
 
       // TODO better support for empty arrays?
@@ -335,11 +335,10 @@ object ExprEvaluate {
       case (Op.MINIMUM, ArrayValue.ExtractFloat(vals)) => FloatValue(vals.min)
       case (Op.MINIMUM, ArrayValue.ExtractInt(vals)) => IntValue(vals.min)
 
-      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 {
-          ErrorValue(f"set_extract($vals) with non-equal values")
+          ErrorValue(Some(f"set_extract($vals) with non-equal values"))
         }
 
       // Any empty value means the expression result is empty
@@ -349,21 +348,20 @@ object ExprEvaluate {
             if (maxMin <= minMax) {
               RangeValue(maxMin, minMax)
             } else { // does not intersect, null set
-              ErrorValue(f"intersection($extracted) produces empty set")
+              ErrorValue(Some(f"intersection($extracted) produces empty set"))
             }
           case ArrayValue.UnpackRange.RangeWithEmpty(_, _) => RangeEmpty
           case ArrayValue.UnpackRange.EmptyRange() => RangeEmpty
-          // The implicit initial value of intersect is the full range
-          case ArrayValue.UnpackRange.EmptyArray() => RangeValue(Float.NegativeInfinity, Float.PositiveInfinity)
-          case _ => ErrorValue(f"intersection($vals) is undefined")
+          case _ => ErrorValue(Some(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 ArrayValue.UnpackRange.EmptyRange() => RangeEmpty
-          case ArrayValue.UnpackRange.EmptyArray() => RangeEmpty // TODO: should this be an error?
+          case ArrayValue.UnpackRange.EmptyArray() => // empty array case handled above
+            throw new AssertionError("compiler internal error, shouldn't happen")
         }
       case (Op.HULL, ArrayValue.ExtractFloat(vals)) => RangeValue(vals.min, vals.max)
 
@@ -408,7 +406,7 @@ object ExprEvaluate {
     case (FloatPromotable(lhs), FloatPromotable(rhs)) => if (lhs <= rhs) {
         RangeValue(lhs, rhs)
       } else {
-        ErrorValue(s"range($minimum, $maximum) is malformed, $minimum > $maximum")
+        ErrorValue(Some(s"range($minimum, $maximum) is malformed, $minimum > $maximum"))
       }
     case _ => throw new ExprEvaluateException(s"Unknown range operands types $minimum $maximum from $range")
   }
@@ -442,8 +440,8 @@ class ExprEvaluate(refs: ConstProp, root: DesignPath) extends ValueExprMap[ExprV
   override def mapUnary(unary: expr.UnaryExpr, `val`: ExprValue): ExprValue =
     ExprEvaluate.evalUnary(unary, `val`)
 
-  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: ExprValue): ExprValue =
-    ExprEvaluate.evalUnarySet(unarySet, vals)
+  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: ExprValue, emptyValue: ExprValue): ExprValue =
+    ExprEvaluate.evalUnarySet(unarySet, vals, emptyValue)
 
   override def mapArray(array: expr.ArrayExpr, vals: Seq[ExprValue]): ExprValue =
     ExprEvaluate.evalArray(array, vals)

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

@@ -70,10 +70,14 @@ class ExprEvaluatePartial(refResolver: IndirectDesignPath => Option[ExprValue],
     case ExprResult.Result(resVal) => ExprResult.Result(ExprEvaluate.evalUnary(unary, resVal))
   }
 
-  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: ExprResult): ExprResult = vals match {
-    case vals @ ExprResult.Missing(_) => vals
-    case ExprResult.Result(vals) => ExprResult.Result(ExprEvaluate.evalUnarySet(unarySet, vals))
-  }
+  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: ExprResult, emptyValue: ExprResult): ExprResult =
+    (vals, emptyValue) match {
+      case (ExprResult.Missing(vals), ExprResult.Missing(emptyValue)) => ExprResult.Missing(vals ++ emptyValue)
+      case (vals @ ExprResult.Missing(_), _) => vals
+      case (_, emptyValue @ ExprResult.Missing(_)) => emptyValue
+      case (ExprResult.Result(vals), ExprResult.Result(emptyValue)) =>
+        ExprResult.Result(ExprEvaluate.evalUnarySet(unarySet, vals, emptyValue))
+    }
 
   override def mapArray(array: expr.ArrayExpr, vals: Seq[ExprResult]): ExprResult = {
     val missing = vals.collect {

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

@@ -30,6 +30,7 @@ class ExprToString() extends ValueExprMap[String] {
       val arrayElts = array.elts.map(mapLiteral)
       s"[${arrayElts.mkString(", ")}]"
     case lit.ValueLit.Type.Struct(_) => "unsupported struct"
+    case lit.ValueLit.Type.Error(literal) => s"error(${literal.message})"
     case lit.ValueLit.Type.Empty => "(empty)"
   }
 
@@ -143,7 +144,7 @@ class ExprToString() extends ValueExprMap[String] {
     }
   }
 
-  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: String): String = unarySet.op match {
+  override def mapUnarySet(unarySet: expr.UnarySetExpr, vals: String, emptyValue: String): String = unarySet.op match {
     case UnarySetExprOp(op) => s"$op(${vals})"
     case op => s"unknown[$op](${vals})"
   }

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

@@ -25,6 +25,10 @@ object ExprValue {
   }
 
   def fromValueLit(literal: lit.ValueLit): ExprValue = literal.`type` match {
+    case lit.ValueLit.Type.Error(literal) => ErrorValue(literal.message match {
+        case "" => None
+        case msg => Some(msg)
+      })
     case lit.ValueLit.Type.Floating(literal) => FloatValue(literal.`val`.toFloat)
     case lit.ValueLit.Type.Integer(literal) => IntValue(literal.`val`)
     case lit.ValueLit.Type.Boolean(literal) => BooleanValue(literal.`val`)
@@ -47,6 +51,11 @@ object ExprValue {
   }
 }
 
+case class ErrorValue(msg: Option[String]) extends ExprValue {
+  def toLit: lit.ValueLit = Literal.Error(msg)
+  def toStringValue: String = s"error(${msg.getOrElse("")})"
+}
+
 // These should be consistent with what is in init.proto
 object FloatPromotable {
   def unapply(floatPromotable: FloatPromotable): Option[Float] = {
@@ -207,9 +216,3 @@ 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/compiler/ValueExprMap.scala

@@ -43,8 +43,8 @@ trait ValueExprMap[OutputType] {
     throw new NotImplementedError(s"Undefined mapBinarySet for $binarySet")
   def mapUnary(unary: expr.UnaryExpr, `val`: OutputType): OutputType =
     throw new NotImplementedError(s"Undefined mapUnary for $unary")
-  def mapUnarySet(unarySet: expr.UnarySetExpr, vals: OutputType): OutputType =
-    throw new NotImplementedError(s"Undefined mapBinarySet for $unarySet")
+  def mapUnarySet(unarySet: expr.UnarySetExpr, vals: OutputType, emptyValue: OutputType): OutputType =
+    throw new NotImplementedError(s"Undefined mapUnarySet for $unarySet")
   def mapArray(array: expr.ArrayExpr, vals: Seq[OutputType]): OutputType =
     throw new NotImplementedError(s"Undefined mapArray for $array")
   def mapStruct(struct: expr.StructExpr, vals: Map[String, OutputType]): OutputType =
@@ -117,7 +117,7 @@ trait ValueExprMap[OutputType] {
     mapUnary(unary, map(unary.`val`.get))
   }
   def wrapUnarySet(unarySet: expr.UnarySetExpr): OutputType = {
-    mapUnarySet(unarySet, map(unarySet.vals.get))
+    mapUnarySet(unarySet, map(unarySet.vals.get), map(unarySet.emptyValue.get))
   }
   def wrapArray(array: expr.ArrayExpr): OutputType = {
     mapArray(array, array.vals.map(value => map(value)))