Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WX-1460] WDL 1.1 Struct Literal Parsing #7391

Merged
merged 11 commits into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: struct_literal
testFormat: workflowsuccess

files {
workflow: struct_literal/struct_literal.wdl
}

metadata {
workflowName: struct_literal
status: Succeeded
"outputs.struct_literal.out": 44
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
version development-1.1

struct Plant {
String color
Int id
}

struct Fungi {
File fungiFile
}


struct Animal {
Plant jacket
Fungi hat
}

task a {
input {
Plant in_plant_literal = Plant{color: "red", id: 44}
}

command {
echo "${in_plant_literal.id}"
}

output {
Animal out_animal = Animal{jacket: Plant{color: "green", id: 10}, hat: Fungi{fungiFile: stdout()}}
}

runtime {
docker: "ubuntu:latest"
}

meta {
volatile: true
}
}

task b {
input {
Animal in_animal
}

command {
cat ${in_animal.hat.fungiFile}
}

output {
Int out = read_int(stdout())
}

runtime {
docker: "ubuntu:latest"
}

meta {
volatile: true
}
}

workflow struct_literal {
call a
call b {input: in_animal=a.out_animal}
output {
Int out = b.out
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ object ExpressionElement {

final case class KvPair(key: String, value: ExpressionElement)
final case class ObjectLiteral(elements: Map[String, ExpressionElement]) extends ExpressionElement
final case class StructLiteral(structTypeName: String, elements: Map[String, ExpressionElement])
extends ExpressionElement
final case class ArrayLiteral(elements: Seq[ExpressionElement]) extends ExpressionElement
final case class MapLiteral(elements: Map[ExpressionElement, ExpressionElement]) extends ExpressionElement
final case class PairLiteral(left: ExpressionElement, right: ExpressionElement) extends ExpressionElement
Expand Down
1,944 changes: 983 additions & 961 deletions wdl/transforms/biscayne/src/main/java/wdl/biscayne/parser/WdlParser.java

Large diffs are not rendered by default.

13 changes: 12 additions & 1 deletion wdl/transforms/biscayne/src/main/resources/CHANGELOG.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
2023-03-16
Synced grammar from OpenWDL `development` version, which is actually development of 2.0. There is no 1.1 Hermes grammar, develop it here.
Changed version declaration to `development1_1`.
This disallows `version 1.1` workflows to run with incomplete support. Once development is finished, change to `1.1`.
This disallows `version 1.1` workflows to run with incomplete support. Once development is finished, change to `1.1`.

2024-02-28
When changing the grammar file, generate a new parser by:
- changing current working directory to cromwell/wdl/transforms/biscayne
- running: hermes generate src/main/resources/grammar.hgr \
--language=java \
--directory=src/main/java \
--name=wdl \
--java-package=wdl.biscayne.parser \
--java-use-apache-commons --java-imports=org.apache.commons.lang3.StringEscapeUtils \
--header
1 change: 1 addition & 0 deletions wdl/transforms/biscayne/src/main/resources/grammar.hgr
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ grammar {
(*:unary) $e = :not $e -> LogicalNot(expression=$1)
(-:unary) $e = :plus $e -> UnaryPlus(expression=$1)
(-:unary) $e = :dash $e -> UnaryNegation(expression=$1)
(*:left) $e = :identifier <=> :lbrace list($object_kv, :comma) :rbrace -> StructLiteral(name=$0, map=$2)
(*:left) $e = :identifier <=> :lparen list($e, :comma) :rparen -> FunctionCall(name=$0, params=$2)
(*:left) $e = $e <=> :lsquare $e :rsquare -> ArrayOrMapLookup(lhs=$0, rhs=$2)
(*:left) $e = $e <=> :dot :identifier -> MemberAccess(value=$0, member=$2)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,14 @@ object BiscayneExpressionValueConsumers {
// None literals consume no values:
Set.empty[UnlinkedConsumedValueHook]
}

implicit val structLiteralExpressionValueConsumer: ExpressionValueConsumer[StructLiteral] =
new ExpressionValueConsumer[StructLiteral] {
override def expressionConsumedValueHooks(a: StructLiteral)(implicit
expressionValueConsumer: ExpressionValueConsumer[ExpressionElement]
): Set[UnlinkedConsumedValueHook] =
a.elements.values
.flatMap(element => expressionValueConsumer.expressionConsumedValueHooks(element)(expressionValueConsumer))
.toSet
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ package object consumed {

case a: StringExpression => a.expressionConsumedValueHooks(expressionValueConsumer)
case a: ObjectLiteral => a.expressionConsumedValueHooks(expressionValueConsumer)
case a: StructLiteral => a.expressionConsumedValueHooks(expressionValueConsumer)
case a: PairLiteral => a.expressionConsumedValueHooks(expressionValueConsumer)
case a: ArrayLiteral => a.expressionConsumedValueHooks(expressionValueConsumer)
case a: MapLiteral => a.expressionConsumedValueHooks(expressionValueConsumer)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package wdl.transforms.biscayne.linking.expression.files

import cats.implicits.{catsSyntaxValidatedId, toTraverseOps}
import common.validation.ErrorOr.ErrorOr
import wdl.model.draft3.elements.ExpressionElement
import wdl.model.draft3.elements.ExpressionElement.{
AsMap,
AsPairs,
Expand All @@ -10,16 +13,20 @@
Quote,
Sep,
SQuote,
StructLiteral,
SubPosix,
Suffix,
Unzip
}
import wdl.model.draft3.graph.expression.FileEvaluator
import wdl.model.draft3.graph.expression.{FileEvaluator, ValueEvaluator}
import wdl.transforms.base.linking.expression.files.EngineFunctionEvaluators.{
threeParameterFunctionPassthroughFileEvaluator,
twoParameterFunctionPassthroughFileEvaluator
}
import wdl.transforms.base.linking.expression.files.EngineFunctionEvaluators.singleParameterPassthroughFileEvaluator
import wom.expression.IoFunctionSet
import wom.types.{WomCompositeType, WomType}
import wom.values.{WomFile, WomValue}

object BiscayneFileEvaluators {

Expand All @@ -39,4 +46,38 @@
implicit val maxFunctionEvaluator: FileEvaluator[Max] = twoParameterFunctionPassthroughFileEvaluator[Max]

implicit val unzipFunctionEvaluator: FileEvaluator[Unzip] = singleParameterPassthroughFileEvaluator

implicit val structLiteralEvaluator: FileEvaluator[StructLiteral] = new FileEvaluator[StructLiteral] {
override def predictFilesNeededToEvaluate(a: StructLiteral,
inputs: Map[String, WomValue],
ioFunctionSet: IoFunctionSet,
coerceTo: WomType
)(implicit
fileEvaluator: FileEvaluator[ExpressionElement],
valueEvaluator: ValueEvaluator[ExpressionElement]
): ErrorOr[Set[WomFile]] = {
def filesInObjectField(fieldAndWomTypeTuple: (String, WomType)): ErrorOr[Set[WomFile]] = {
val (field, womType) = fieldAndWomTypeTuple
a.elements.get(field) match {
case Some(fieldElement) =>
fileEvaluator.predictFilesNeededToEvaluate(fieldElement, inputs, ioFunctionSet, womType)(fileEvaluator,
valueEvaluator
)
case None => s"Invalid assignment to struct. Required field $field was not specified.".invalidNel

Check warning on line 66 in wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala

View check run for this annotation

Codecov / codecov/patch

wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala#L66

Added line #L66 was not covered by tests
}
}

coerceTo match {
case WomCompositeType(mapping, _) => mapping.toList.traverse(filesInObjectField).map(_.flatten.toSet)
case _ =>
a.elements.values.toList
.traverse(
fileEvaluator.evaluateFilesNeededToEvaluate(_, inputs, ioFunctionSet, coerceTo)(fileEvaluator,

Check warning on line 75 in wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala

View check run for this annotation

Codecov / codecov/patch

wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala#L73-L75

Added lines #L73 - L75 were not covered by tests
valueEvaluator
)
)
.map(_.toSet.flatten)

Check warning on line 79 in wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala

View check run for this annotation

Codecov / codecov/patch

wdl/transforms/biscayne/src/main/scala/wdl/transforms/biscayne/linking/expression/files/BiscayneFileEvaluators.scala#L79

Added line #L79 was not covered by tests
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import wom.expression.IoFunctionSet
import wom.types.WomType
import wom.values.{WomFile, WomValue}
import wdl.transforms.biscayne.linking.expression.files.BiscayneFileEvaluators._

package object files {

implicit val expressionFileEvaluator: FileEvaluator[ExpressionElement] = new FileEvaluator[ExpressionElement] {
Expand All @@ -37,6 +36,8 @@ package object files {
a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
case a: ObjectLiteral =>
a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
case a: StructLiteral =>
a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
case a: MapLiteral =>
a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
case a: ArrayLiteral =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,16 @@ object BiscayneTypeEvaluators {
case other => s"Cannot invoke 'unzip' on type '${other.stableName}'. Expected an array of pairs".invalidNel
}
}

implicit val structLiteralTypeEvaluator: TypeEvaluator[StructLiteral] = new TypeEvaluator[StructLiteral] {
override def evaluateType(a: StructLiteral, linkedValues: Map[UnlinkedConsumedValueHook, GeneratedValueHandle])(
implicit expressionTypeEvaluator: TypeEvaluator[ExpressionElement]
): 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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ package object types {
case a: StringLiteral => a.evaluateType(linkedValues)(typeEvaluator)
case a: StringExpression => a.evaluateType(linkedValues)(typeEvaluator)
case a: ObjectLiteral => a.evaluateType(linkedValues)(typeEvaluator)
case a: StructLiteral => a.evaluateType(linkedValues)(typeEvaluator)
case a: MapLiteral => a.evaluateType(linkedValues)(typeEvaluator)
case a: ArrayLiteral => a.evaluateType(linkedValues)(typeEvaluator)
case a: PairLiteral => a.evaluateType(linkedValues)(typeEvaluator)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import wdl.transforms.base.linking.expression.values.EngineFunctionEvaluators.{
}
import wom.expression.IoFunctionSet
import wom.types._
import wom.values.{WomArray, WomFloat, WomInteger, WomMap, WomOptionalValue, WomPair, WomString, WomValue}
import wom.values.{WomArray, WomFloat, WomInteger, WomMap, WomObject, WomOptionalValue, WomPair, WomString, WomValue}
import wom.types.coercion.defaults._

object BiscayneValueEvaluators {
Expand Down Expand Up @@ -351,4 +351,29 @@ object BiscayneValueEvaluators {
s"Invalid call of 'unzip' on parameter of type '${other.womType.stableName}' (expected Array[Pair[X, Y]])".invalidNel
}
}

implicit val structLiteralValueEvaluator: ValueEvaluator[StructLiteral] = new ValueEvaluator[StructLiteral] {
// This works fine, but is missing a feature from the WDL 1.1 spec: users are allowed to omit optional values from their struct literal.
// This requires some extra work to be done in a subsequent PR.
// Specifically, make the known struct definitions available to this function so we can populate k/v pairs with None appropriately.
override def evaluateValue(a: StructLiteral,
inputs: Map[String, WomValue],
ioFunctionSet: IoFunctionSet,
forCommandInstantiationOptions: Option[ForCommandInstantiationOptions]
)(implicit expressionValueEvaluator: ValueEvaluator[ExpressionElement]): ErrorOr[EvaluatedValue[_ <: WomValue]] = {

val evaluated: ErrorOr[List[(String, EvaluatedValue[_])]] = a.elements.toList traverse {
case (key: String, value: ExpressionElement) =>
expressionValueEvaluator
.evaluateValue(value, inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
.map(key -> _)
}

evaluated map { mapping =>
val value = mapping.map(entry => entry._1 -> entry._2.value).toMap
val sideEffectFiles = mapping.flatMap(entry => entry._2.sideEffectFiles)
EvaluatedValue(WomObject(value), sideEffectFiles)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ package object values {
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: NoneLiteralElement.type =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)

case a: StringLiteral =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: StringExpression =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: ObjectLiteral =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: StructLiteral =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: MapLiteral =>
a.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions)(expressionValueEvaluator)
case a: ArrayLiteral =>
Expand Down
39 changes: 39 additions & 0 deletions wdl/transforms/biscayne/src/test/cases/struct_literal.wdl
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
version development-1.1

struct Plant {
String color
Boolean tasty
}

struct Animal {
String name
Boolean? isGood
}

task test_struct_parsing {
input {
Plant standard_plant_input
Animal standard_animal_input
}

runtime {
docker: "ubuntu:latest"
}

command {
echo "all dogs are good"
}

output {
Plant standard_plant_forwarded = standard_plant_input
Animal standard_animal_forwarded = standard_animal_input
Plant plant_output_literal = Plant{color: "red", tasty: true}
}
}

workflow struct_literal {
call test_struct_parsing {
input: standard_plant_input = Plant{color: "green", tasty: true},
standard_animal_input = Animal{name: "mittens", isGood: false}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import wdl.transforms.biscayne.ast2wdlom._
import wdl.transforms.biscayne.parsing.WdlBiscayneSyntaxErrorFormatter
import wom.callable.MetaValueElement.MetaValueElementInteger
import wom.types.WomIntegerType
import wom.values.WomInteger
import wom.values.{WomBoolean, WomInteger}

import scala.jdk.CollectionConverters._

Expand Down Expand Up @@ -126,4 +126,13 @@ class Ast2WdlomSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers {
val expr = fromString[ExpressionElement](str, parser.parse_e)
expr shouldBeValid (Unzip(IdentifierLookup("some_array_of_pairs")))
}

it should "parse a struct literal" in {
val str = """Dog{breed: "fluffy", isGood: true}"""
val expr = fromString[ExpressionElement](str, parser.parse_e)
expr shouldBeValid (StructLiteral(
"Dog",
Map("breed" -> StringLiteral("fluffy"), "isGood" -> PrimitiveLiteralExpressionElement(WomBoolean(true)))
))
}
}
Loading
Loading