Merge pull request #3194 from broadinstitute/cjl_nested_ifs_broken_30…

…_hotfix Reuse ogins between if condition and the inner block [30 hotfix edition]
broadinstitute · Jan 25, 2018 · 16f3632 · 16f3632
2 parents 0152bc3 + b8b1989
commit 16f3632
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 21 deletions.
diff --git a/wdl/src/main/scala/wdl/If.scala b/wdl/src/main/scala/wdl/If.scala
@@ -39,13 +39,6 @@ object If {
     * @param preserveIndexForOuterLookups When we're evaluating the condition boolean, should we preserve scatter index if we have to use the outerLookup?
     */
   def womConditionalNode(ifBlock: If, localLookup: Map[String, GraphNodePort.OutputPort], outerLookup: Map[String, OutputPort], preserveIndexForOuterLookups: Boolean): ErrorOr[ConditionalNodeWithNewNodes] = {
-    val ifConditionExpression = WdlWomExpression(ifBlock.condition, ifBlock)
-    val ifConditionGraphInputExpressionValidation = WdlWomExpression.toAnonymousExpressionNode(
-      WomIdentifier("conditional"), ifConditionExpression, localLookup, outerLookup, preserveIndexForOuterLookups, ifBlock, PlainAnonymousExpressionNode.apply)
-    val ifConditionTypeValidation = ifConditionExpression.evaluateType((localLookup ++ outerLookup).map { case (k, v) => k -> v.womType }) flatMap {
-      case WomBooleanType => Valid(())
-      case other => s"An if block must be given a boolean expression but instead got '${ifBlock.condition.toWomString}' (a ${other.toDisplayString})".invalidNel
-    }
 
     /**
       * Why? Imagine that we're building three nested levels of a innerGraph.
@@ -69,6 +62,14 @@ object If {
     }
     val possiblyNeededNestedOginPorts: Map[String, OutputPort] = possiblyNeededNestedOgins map { case (name: String, ogin: OuterGraphInputNode) => name -> ogin.singleOutputPort }
 
+    val ifConditionExpression = WdlWomExpression(ifBlock.condition, ifBlock)
+    val ifConditionGraphInputExpressionValidation = WdlWomExpression.toAnonymousExpressionNode(
+      WomIdentifier("conditional"), ifConditionExpression, localLookup ++ possiblyNeededNestedOginPorts, Map.empty, preserveIndexForOuterLookups, ifBlock, PlainAnonymousExpressionNode.apply)
+    val ifConditionTypeValidation = ifConditionExpression.evaluateType((localLookup ++ outerLookup).map { case (k, v) => k -> v.womType }) flatMap {
+      case coerceable if WomBooleanType.isCoerceableFrom(coerceable) => Valid(())
+      case other => s"An if block must be given a boolean expression but instead got '${ifBlock.condition.toWomString}' (a ${other.toDisplayString})".invalidNel
+    }
+
     val innerGraphValidation: ErrorOr[Graph] = WdlGraphNode.buildWomGraph(
       ifBlock,
       Set.empty,

diff --git a/wdl/src/main/scala/wdl/Scatter.scala b/wdl/src/main/scala/wdl/Scatter.scala
@@ -40,18 +40,7 @@ object Scatter {
     * @param preserveIndexForOuterLookups When we're evaluating the scatter collection, should we preserve scatter index when we have to use the outerLookup?
     */
   def womScatterNode(scatter: Scatter, localLookup: Map[String, GraphNodePort.OutputPort], outerLookup: Map[String, OutputPort], preserveIndexForOuterLookups: Boolean): ErrorOr[ScatterNodeWithNewNodes] = {
-    // Convert the scatter collection WdlExpression to a WdlWomExpression 
-    val scatterCollectionExpression = WdlWomExpression(scatter.collection, scatter)
-    // Generate an ExpressionNode from the WdlWomExpression
-    val scatterCollectionExpressionNode =
-      WdlWomExpression.toAnonymousExpressionNode(WomIdentifier(scatter.item), scatterCollectionExpression, localLookup, outerLookup, preserveIndexForOuterLookups, scatter, PlainAnonymousExpressionNode.apply)
-    // Validate the collection evaluates to a traversable type
-    val scatterItemTypeValidation = scatterCollectionExpression.evaluateType((localLookup ++ outerLookup).map { case (k, v) => k -> v.womType }) flatMap {
-      case WomArrayType(itemType) => Valid(itemType) // Covers maps because this is a custom unapply (see WdlArrayType)
-      case other => s"Cannot scatter over a non-traversable type ${other.toDisplayString}".invalidNel
-    }
-
-    /**
+    /*
       * Why? Imagine that we're building three nested levels of a innerGraph.
       * - Say we're building the middle layer.
       * - We have a set of OutputPorts in the outer layer that we can make OGINs to if we need them.
@@ -68,6 +57,17 @@ object Scatter {
     }
     val possiblyNeededNestedOginPorts: Map[String, OutputPort] = possiblyNeededNestedOgins map { case (name: String, ogin: OuterGraphInputNode) => name -> ogin.singleOutputPort }
 
+    // Convert the scatter collection WdlExpression to a WdlWomExpression
+    val scatterCollectionExpression = WdlWomExpression(scatter.collection, scatter)
+    // Generate an ExpressionNode from the WdlWomExpression
+    val scatterCollectionExpressionNode =
+      WdlWomExpression.toAnonymousExpressionNode(WomIdentifier(scatter.item), scatterCollectionExpression, localLookup ++ possiblyNeededNestedOginPorts, Map.empty, preserveIndexForOuterLookups, scatter, PlainAnonymousExpressionNode.apply)
+    // Validate the collection evaluates to a traversable type
+    val scatterItemTypeValidation = scatterCollectionExpression.evaluateType((localLookup ++ outerLookup).map { case (k, v) => k -> v.womType }) flatMap {
+      case WomArrayType(itemType) => Valid(itemType) // Covers maps because this is a custom unapply (see WdlArrayType)
+      case other => s"Cannot scatter over a non-traversable type ${other.toDisplayString}".invalidNel
+    }
+
     for {
       itemType <- scatterItemTypeValidation
       expressionNode <- scatterCollectionExpressionNode

diff --git a/wdl/src/test/scala/wom/WdlNestedConditionalWomSpec.scala b/wdl/src/test/scala/wom/WdlNestedConditionalWomSpec.scala
@@ -27,7 +27,8 @@ class WdlNestedConditionalWomSpec extends FlatSpec with Matchers {
     ("same lookups in multiple if expressions", sameLookupsInMultipleIfExpressions),
     ("same lookups in multiple scatter expressions", sameLookupsInMultipleScatterExpressions),
     ("same lookups in if and scatter expressions", sameLookupsInIfAndScatterExpressions),
-    ("same lookups in scatters and elsewhere", sameLookupsInScattersAndElsewhere)
+    ("same lookups in scatters and elsewhere", sameLookupsInScattersAndElsewhere),
+    ("same lookups in if condition, scatter collection, and task calls", sameLookupsInConditionsAndInnerTaskCalls)
   )
 
 
@@ -310,4 +311,38 @@ object WdlNestedConditionalWomSpec {
       |   }
       | }
     """.stripMargin
+
+  val sameLookupsInConditionsAndInnerTaskCalls =
+    """workflow Test {
+      |  String? testString
+      |  Array[String] testStrings = [""]
+      |
+      |  if(true) {
+      |    if (defined(testString)) {
+      |      call testTask as tt {
+      |        input:
+      |          testString = testString
+      |      }
+      |    }
+      |  }
+      |
+      |  if(true) {
+      |    scatter(ts in testStrings) {
+      |      call testTask as tt2 {
+      |        input:
+      |          testString = testStrings[0]
+      |      }
+      |    }
+      |  }
+      |}
+      |
+      |task testTask {
+      |    String? testString
+      |    command {
+      |      echo "Hello world"
+      |    }
+      |    runtime {
+      |      docker: "ubuntu"
+      |    }
+      |}""".stripMargin
 }
diff --git a/wom/src/main/scala/wom/graph/ConditionalNode.scala b/wom/src/main/scala/wom/graph/ConditionalNode.scala
@@ -27,7 +27,8 @@ object ConditionalNode  {
 
   final case class ConditionalNodeWithNewNodes(node: ConditionalNode) extends GeneratedNodeAndNewNodes {
     override val newInputs = node.innerGraph.externalInputNodes
-    override val usedOuterGraphInputNodes = (node.conditionExpression.upstream.filterByType[OuterGraphInputNode]: Set[OuterGraphInputNode]) ++
+    override val usedOuterGraphInputNodes =
+      (node.conditionExpression.upstream.filterByType[OuterGraphInputNode]: Set[OuterGraphInputNode]) ++
         (node.innerGraph.outerGraphInputNodes.map(_.linkToOuterGraphNode).filterByType[OuterGraphInputNode]: Set[OuterGraphInputNode])
 
     override val newExpressions = Set(node.conditionExpression)