[WX-1531] Struct Literal Type Checking

wdl/model/draft3/src/main/scala/wdl/model/draft3/graph/expression/TypeEvaluator.scala

@@ -8,7 +8,10 @@ import wom.types.WomType
 
 @typeclass
 trait TypeEvaluator[A] {
-  def evaluateType(a: A, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+  def evaluateType(a: A,
+                   linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                   typeAliases: Map[String, WomType]
+  )(implicit
     expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
   ): ErrorOr[WomType]
 }

wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/types/BiscayneTypeEvaluators.scala

@@ -1,5 +1,6 @@
 package wdl.transforms.biscayne.linking.expression.types
 
+import cats.data.Validated.{Invalid, Valid}
 import cats.implicits.{catsSyntaxTuple2Semigroupal, catsSyntaxTuple3Semigroupal}
 import cats.syntax.validated._
 import common.validation.ErrorOr._
@@ -12,7 +13,10 @@ import wom.types._
 
 object BiscayneTypeEvaluators {
   implicit val keysFunctionEvaluator: TypeEvaluator[Keys] = new TypeEvaluator[Keys] {
-    override def evaluateType(a: Keys, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Keys,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomMapType(WomAnyType, WomAnyType)) flatMap {
@@ -22,7 +26,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val asMapFunctionEvaluator: TypeEvaluator[AsMap] = new TypeEvaluator[AsMap] {
-    override def evaluateType(a: AsMap, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: AsMap,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomArrayType(WomPairType(WomAnyType, WomAnyType))) flatMap {
@@ -34,7 +41,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val asPairsFunctionEvaluator: TypeEvaluator[AsPairs] = new TypeEvaluator[AsPairs] {
-    override def evaluateType(a: AsPairs, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: AsPairs,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomMapType(WomAnyType, WomAnyType)) flatMap {
@@ -44,8 +54,11 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val collectByKeyFunctionEvaluator: TypeEvaluator[CollectByKey] = new TypeEvaluator[CollectByKey] {
-    override def evaluateType(a: CollectByKey, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(
-      implicit expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
+    override def evaluateType(a: CollectByKey,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
+      expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomArrayType(WomPairType(WomAnyType, WomAnyType))) flatMap {
         case WomArrayType(WomPairType(x: WomPrimitiveType, y)) => WomMapType(x, WomArrayType(y)).validNel
@@ -67,29 +80,38 @@ object BiscayneTypeEvaluators {
     }
 
   implicit val minFunctionEvaluator: TypeEvaluator[Min] = new TypeEvaluator[Min] {
-    override def evaluateType(a: Min, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Min,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] = {
-      val type1 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues)
-      val type2 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues)
+      val type1 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues, typeAliases)
+      val type2 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues, typeAliases)
 
       (type1, type2) flatMapN resultTypeOfIntVsFloat("min")
     }
   }
 
   implicit val maxFunctionEvaluator: TypeEvaluator[Max] = new TypeEvaluator[Max] {
-    override def evaluateType(a: Max, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Max,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] = {
-      val type1 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues)
-      val type2 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues)
+      val type1 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues, typeAliases)
+      val type2 = expressionTypeEvaluator.evaluateType(a.arg1, linkedValues, typeAliases)
 
       (type1, type2) flatMapN resultTypeOfIntVsFloat("max")
     }
   }
 
   implicit val sepFunctionEvaluator: TypeEvaluator[Sep] = new TypeEvaluator[Sep] {
-    override def evaluateType(a: Sep, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Sep,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.arg2, linkedValues, WomArrayType(WomAnyType)) flatMap {
@@ -103,7 +125,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val subPosixFunctionEvaluator: TypeEvaluator[SubPosix] = new TypeEvaluator[SubPosix] {
-    override def evaluateType(a: SubPosix, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: SubPosix,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       (validateParamType(a.input, linkedValues, WomSingleFileType),
@@ -113,7 +138,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val suffixFunctionEvaluator: TypeEvaluator[Suffix] = new TypeEvaluator[Suffix] {
-    override def evaluateType(a: Suffix, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Suffix,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       (validateParamType(a.suffix, linkedValues, WomStringType),
@@ -122,7 +150,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val quoteFunctionEvaluator: TypeEvaluator[Quote] = new TypeEvaluator[Quote] {
-    override def evaluateType(a: Quote, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Quote,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomArrayType(WomAnyType)) flatMap {
@@ -136,7 +167,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val sQuoteFunctionEvaluator: TypeEvaluator[SQuote] = new TypeEvaluator[SQuote] {
-    override def evaluateType(a: SQuote, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: SQuote,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomArrayType(WomAnyType)) flatMap {
@@ -150,7 +184,10 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val unzipFunctionEvaluator: TypeEvaluator[Unzip] = new TypeEvaluator[Unzip] {
-    override def evaluateType(a: Unzip, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(implicit
+    override def evaluateType(a: Unzip,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
       expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
     ): ErrorOr[WomType] =
       validateParamType(a.param, linkedValues, WomArrayType(WomPairType(WomAnyType, WomAnyType))) flatMap {
@@ -161,14 +198,101 @@ object BiscayneTypeEvaluators {
   }
 
   implicit val structLiteralTypeEvaluator: TypeEvaluator[StructLiteral] = new TypeEvaluator[StructLiteral] {
-    override def evaluateType(a: StructLiteral, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(
-      implicit expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
+
+    // Is the evaluated type allowed to be assigned to the expectedType?
+    def areTypesAssignable(evaluatedType: WomType, expectedType: WomType): Boolean =
+      // NB: This check is a little looser than we'd like it to be.
+      // For example, String is coercible to Int (Int i = "1" is OK)
+      // It's not until we actually evaluate the value of the string that we can know if that coercion succeeded or not. (Int i = "orange" will fail)
+      // We don't know whether the user has provided "1" or "orange" at this stage.
+      // This is OK as-is because the value evaluators do the coercing and throw meaningful errors if the coercion fails.
+      expectedType.isCoerceableFrom(evaluatedType)
+
+    // Helper method to check something (maybe) found in the struct literal to something (maybe) found in the struct definition.
+    def checkIfMemberIsValid(typeName: String,
+                             memberName: String,
+                             evaluatedType: Option[WomType],
+                             expectedType: Option[WomType]
     ): ErrorOr[WomType] =
-      // This works fine, but is not yet a strong enough type check for the WDL 1.1 spec
-      // (i.e. users are able to instantiate struct literals with k/v pairs that aren't actually in the struct definition, without an error being thrown.)
-      // We want to add extra validation here, and return a WomCompositeType that matches the struct definition of everything is OK.
-      // Note that users are allowed to omit optional k/v pairs in their literal.
-      // This requires some extra work to be done in a subsequent PR.
-      WomObjectType.validNel
+      evaluatedType match {
+        case Some(evaluated) =>
+          expectedType match {
+            case Some(expected) =>
+              if (areTypesAssignable(evaluated, expected)) evaluated.validNel
+              else
+                s"$typeName.$memberName expected to be ${expected.friendlyName}. Found ${evaluated.friendlyName}.".invalidNel
+            case None => s"Type $typeName does not have a member called $memberName.".invalidNel
+          }
+        case None => s"Error evaluating the type of ${typeName}.${memberName}.".invalidNel
+      }
+
+    // For each member in the literal, check that it exists in the struct definition and is the expected type.
+    def checkMembersAgainstDefinition(a: StructLiteral,
+                                      structDefinition: WomCompositeType,
+                                      linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                                      typeAliases: Map[String, WomType]
+    ): ErrorOr[WomCompositeType] = {
+      val checkedMembers: Map[String, ErrorOr[WomType]] = a.elements.map { case (memberKey, memberExpressionElement) =>
+        val evaluatedType =
+          expressionTypeEvaluator.evaluateType(memberExpressionElement, linkedValues, typeAliases).toOption
+        val expectedType = structDefinition.typeMap.get(memberKey)
+        (memberKey, checkIfMemberIsValid(a.structTypeName, memberKey, evaluatedType, expectedType))
+      }
+
+      val (errors, validatedTypes) = checkedMembers.partition { case (_, errorOr) =>
+        errorOr match {
+          case Invalid(_) => true
+          case Valid(_) => false
+        }
+      }
+
+      errors.headOption match {
+        case Some((_, Invalid(_))) =>
+          errors.collect { case (_, Invalid(e)) => e.toList.mkString }.toList.mkString(",").invalidNel
+        case _ =>
+          val types = validatedTypes.collect { case (key, Valid(v)) => (key, v) }
+          WomCompositeType(types, Some(a.structTypeName)).validNel
+      }
+    }
+
+    // For every member in the definition, if that member isn't optional, confirm that it is also in the struct literal.
+    def checkForMissingMembers(foundMembers: Map[String, WomType],
+                               structDefinition: WomCompositeType
+    ): Option[String] = {
+      val errors: Iterable[String] = structDefinition.typeMap flatMap { case (memberName, memberType) =>
+        memberType match {
+          case WomOptionalType(_) => None
+          case _ =>
+            if (!foundMembers.contains(memberName)) Some(s"Expected member ${memberName} not found. ")
+            else None
+        }
+      }
+      errors match {
+        case Nil => None
+        case _ => Some(errors.mkString)
+      }
+    }
+    override def evaluateType(a: StructLiteral,
+                              linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle],
+                              typeAliases: Map[String, WomType]
+    )(implicit
+      expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
+    ): ErrorOr[WomType] = {
+      val structDefinition = typeAliases.get(a.structTypeName)
+      structDefinition match {
+        case Some(definition) =>
+          definition match {
+            case compositeType: WomCompositeType =>
+              checkMembersAgainstDefinition(a, compositeType, linkedValues, typeAliases).flatMap { foundMembers =>
+                checkForMissingMembers(foundMembers.typeMap, compositeType) match {
+                  case Some(error) => error.invalidNel
+                  case _ => compositeType.validNel
+                }
+              }
+            case _ => s"Unexpected error while parsing ${a.structTypeName}".invalidNel
+          }
+        case None => s"Could not find Struct Definition for type ${a.structTypeName}".invalidNel
+      }
+    }
   }
 }