fix: correct optional type handling in check_placeholder

wdl-analysis/src/types/v1.rs

@@ -657,31 +657,32 @@ impl<'a, C: EvaluationContext> ExprTypeEvaluator<'a, C> {
     /// Checks a placeholder expression.
     pub(crate) fn check_placeholder<N: TreeNode>(&mut self, placeholder: &Placeholder<N>) {
         self.placeholders += 1;
-
         // Evaluate the placeholder expression and check that the resulting type is
         // coercible to string for interpolation
         let expr = placeholder.expr();
         if let Some(ty) = self.evaluate_expr(&expr) {
-            match ty {
-                Type::Primitive(..) | Type::Union | Type::None => {
-                    // OK
-                }
-                ty => {
-                    // Check for a sep option is specified; if so, accept `Array[P]` where `P` is
-                    // primitive.
-                    let mut coercible = false;
-                    if let Some(PlaceholderOption::Sep(_)) = placeholder.option() {
-                        if let Type::Compound(CompoundType::Array(ty), _) = &ty {
-                            if !ty.element_type().is_optional()
-                                && ty.element_type().as_primitive().is_some()
-                            {
-                                // OK
-                                coercible = true;
-                            }
-                        }
+            if let Some(option) = placeholder.option() {
+                let valid = match option {
+                    PlaceholderOption::Sep(_) => matches!(ty,
+                        Type::Compound(CompoundType::Array(ref element_type), false)
+                        if matches!(element_type.element_type(), Type::Primitive(_, false) | Type::Union)),
+                    PlaceholderOption::Default(_) => matches!(ty, Type::Primitive(_, true)),
+                    PlaceholderOption::TrueFalse(_) => {
+                        matches!(ty, Type::Primitive(PrimitiveType::Boolean, _))
                     }
-
-                    if !coercible {
+                };
+                if !valid {
+                    self.context
+                        .add_diagnostic(Self::invalid_placeholder_option(
+                            &ty,
+                            expr.span(),
+                            Some(option.map(|n| n.into())),
+                        ));
+                }
+            } else {
+                match ty {
+                    Type::Primitive(..) | Type::Union | Type::None => {}
+                    _ => {
                         self.context
                             .add_diagnostic(cannot_coerce_to_string(&ty, expr.span()));
                     }
@@ -692,6 +693,33 @@ impl<'a, C: EvaluationContext> ExprTypeEvaluator<'a, C> {
         self.placeholders -= 1;
     }
 
+    fn invalid_placeholder_option(
+        ty: &Type,
+        _span: Span,
+        option: Option<PlaceholderOption>,
+    ) -> Diagnostic {
+        let message = match option {
+            Some(PlaceholderOption::Sep(_)) => format!(
+                "type mismatch for placeholder option `sep`: expected type `Array[P]` where P: any primitive type or Union, but found `{}`",
+                ty
+            ),
+            Some(PlaceholderOption::Default(_)) => format!(
+                "type mismatch for placeholder option `default`: expected an optional primitive type, but found `{}`",
+                ty
+            ),
+            Some(PlaceholderOption::TrueFalse(_)) => format!(
+                "type mismatch for placeholder option `truefalse`: expected type `Boolean`, but found `{}`",
+                ty
+            ),
+            None => format!("cannot coerce type `{}` to `String`", ty),
+        };
+        Diagnostic::error(message) // No span here
+    }
+
+    fn cannot_coerce_to_string(ty: &Type, _span: Span) -> Diagnostic {
+        Diagnostic::error(format!("cannot coerce type `{}` to `String`", ty)) // No span here
+    }
+
     /// Evaluates the type of a literal array expression.
     fn evaluate_literal_array<N: TreeNode>(&mut self, expr: &LiteralArray<N>) -> Type {
         // Look at the first array element to determine the element type

wdl-analysis/tests/analysis/placeholder-options/source.diagnostics

@@ -0,0 +1,6 @@
+error: type mismatch for placeholder option `sep`: expected type `Array[P]` where P: any primitive type or Union, but found `Boolean`
+   ┌─ tests/analysis/placeholder-options/source.wdl:13:1
+   │
+13 │ 
+   │  
+

wdl-analysis/tests/analysis/placeholder-options/source.wdl

@@ -0,0 +1,12 @@
+version 1.2
+
+task valid_case {
+    input {
+        Array[String] arr
+    }
+    command <<< ~{sep=", " arr} >>>
+}
+
+task invalid_case {
+    command <<< ~{sep=", " true} >>>
+}

wdl-analysis/tests/analysis/placeholders/source.diagnostics

@@ -0,0 +1,86 @@
+error: type mismatch for placeholder option `default`: expected an optional primitive type, but found `Int`
+   ┌─ tests/analysis/placeholders/source.wdl:13:2
+   │
+13 │ }
+   │   
+
+error: type mismatch for placeholder option `sep`: expected type `Array[P]` where P: any primitive type or Union, but found `Int`
+   ┌─ tests/analysis/placeholders/source.wdl:13:2
+   │
+13 │ }
+   │   
+
+error: type mismatch for placeholder option `truefalse`: expected type `Boolean`, but found `Int`
+   ┌─ tests/analysis/placeholders/source.wdl:13:2
+   │
+13 │ }
+   │   
+
+warning[UnusedDeclaration]: unused declaration `s1`
+  ┌─ tests/analysis/placeholders/source.wdl:5:12
+  │
+5 │     String s1 = "~{true}" # OK: Boolean is coercible to string, no option
+  │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s2`
+  ┌─ tests/analysis/placeholders/source.wdl:6:12
+  │
+6 │     String s2 = "~{true="yes" false="no" false}" # OK: truefalse option with Boolean
+  │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s3`
+  ┌─ tests/analysis/placeholders/source.wdl:7:12
+  │
+7 │     String s3 = "~{true="yes" false="no" 1}" # NOT OK: truefalse expects Boolean, but got Int
+  │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s4`
+  ┌─ tests/analysis/placeholders/source.wdl:8:12
+  │
+8 │     String s4 = "~{sep=',' [1, 2, 3]}" # OK: sep option with Array[Int]
+  │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s5`
+  ┌─ tests/analysis/placeholders/source.wdl:9:12
+  │
+9 │     String s5 = "~{sep=',' 123}" # NOT OK: sep expects Array, but got Int
+  │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s6`
+   ┌─ tests/analysis/placeholders/source.wdl:10:12
+   │
+10 │     String s6 = "~{default="fallback" var?}" # OK: default option with optional variable
+   │            ^^
+
+error: unknown name `var`
+   ┌─ tests/analysis/placeholders/source.wdl:10:39
+   │
+10 │     String s6 = "~{default="fallback" var?}" # OK: default option with optional variable
+   │                                       ^^^
+
+error: expected `}`, but found `?`
+   ┌─ tests/analysis/placeholders/source.wdl:10:42
+   │
+10 │     String s6 = "~{default="fallback" var?}" # OK: default option with optional variable
+   │                  --                      ^ unexpected `?`
+   │                  │                        
+   │                  this placeholder start is not matched
+
+warning[UnusedDeclaration]: unused declaration `s7`
+   ┌─ tests/analysis/placeholders/source.wdl:11:12
+   │
+11 │     String s7 = "~{default="fallback" 123}" # NOT OK: default expects optional, but got Int
+   │            ^^
+
+warning[UnusedDeclaration]: unused declaration `s8`
+   ┌─ tests/analysis/placeholders/source.wdl:12:12
+   │
+12 │     String s8 = "~{[1, 2, 3]}" # NOT OK: Array without sep option, not coercible to string
+   │            ^^
+
+error: cannot coerce type `Array[Int]+` to `String`
+   ┌─ tests/analysis/placeholders/source.wdl:12:20
+   │
+12 │     String s8 = "~{[1, 2, 3]}" # NOT OK: Array without sep option, not coercible to string
+   │                    ^^^^^^^^^ this is type `Array[Int]+`
+

wdl-analysis/tests/analysis/placeholders/source.wdl

@@ -0,0 +1,13 @@
+# This is a test for analysis of placeholders and their options.
+version 1.1
+
+workflow test {
+    String s1 = "~{true}" # OK: Boolean is coercible to string, no option
+    String s2 = "~{true="yes" false="no" false}" # OK: truefalse option with Boolean
+    String s3 = "~{true="yes" false="no" 1}" # NOT OK: truefalse expects Boolean, but got Int
+    String s4 = "~{sep=',' [1, 2, 3]}" # OK: sep option with Array[Int]
+    String s5 = "~{sep=',' 123}" # NOT OK: sep expects Array, but got Int
+    String s6 = "~{default="fallback" var?}" # OK: default option with optional variable
+    String s7 = "~{default="fallback" 123}" # NOT OK: default expects optional, but got Int
+    String s8 = "~{[1, 2, 3]}" # NOT OK: Array without sep option, not coercible to string
+}

wdl-analysis/tests/analysis/type_checker_tests.rs

@@ -0,0 +1,46 @@
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_sep_with_array() {
+        let expr_type = Type::Array(Box::new(Type::Int)); // Array[Int]
+        let placeholder = Placeholder::Sep(",".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_ok());
+    }
+
+    #[test]
+    fn test_sep_with_non_array() {
+        let expr_type = Type::Int; // Not an array
+        let placeholder = Placeholder::Sep(",".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_err());
+    }
+
+    #[test]
+    fn test_default_with_optional() {
+        let expr_type = Type::Optional(Box::new(Type::String)); // Optional[String]
+        let placeholder = Placeholder::Default("default_value".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_ok());
+    }
+
+    #[test]
+    fn test_default_with_non_optional() {
+        let expr_type = Type::String; // Not optional
+        let placeholder = Placeholder::Default("default_value".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_err());
+    }
+
+    #[test]
+    fn test_true_false_with_boolean() {
+        let expr_type = Type::Boolean;
+        let placeholder = Placeholder::TrueFalse("yes".to_string(), "no".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_ok());
+    }
+
+    #[test]
+    fn test_true_false_with_non_boolean() {
+        let expr_type = Type::String; // Not a boolean
+        let placeholder = Placeholder::TrueFalse("yes".to_string(), "no".to_string());
+        assert!(check_placeholder(&expr_type, &placeholder).is_err());
+    }
+}

wdl-engine/tests/tasks/invalid-placeholder-sep/error.txt

@@ -0,0 +1,9 @@
+error: call to function `read_json` failed: failed to open file `attempts/0/work/not-array.json`
+
+Caused by:
+    No such file or directory (os error 2)
+  ┌─ tests/tasks/invalid-placeholder-sep/source.wdl:6:31
+  │
+6 │         String a = "~{sep=',' read_json('not-array.json')}"
+  │                               ^^^^^^^^^
+

wdl-engine/tests/tasks/invalid-placeholder-sep/inputs.json

@@ -0,0 +1 @@
+{}

wdl-engine/tests/tasks/invalid-placeholder-sep/not-array.json

@@ -0,0 +1 @@
+true

wdl-engine/tests/tasks/invalid-placeholder-sep/source.wdl

@@ -0,0 +1,8 @@
+version 1.2
+
+task test {
+    command<<<>>>
+    output {
+        String a = "~{sep=',' read_json('not-array.json')}"
+    }
+}

wdl-engine/tests/tasks/primitive-literals/error.txt

@@ -0,0 +1,6 @@
+error: failed to evaluate output `x` for task `write_file_task`: file `attempts/0/work/testdir/hello.txt` does not exist
+   ┌─ tests/tasks/primitive-literals/source.wdl:14:10
+   │
+14 │     File x = "testdir/hello.txt"
+   │          ^
+

wdl-engine/tests/tasks/relative-and-absolute-task/error.txt

@@ -0,0 +1,9 @@
+error: call to function `read_string` failed: failed to read file `attempts/0/work/my/path/to/something.txt`
+
+Caused by:
+    Permission denied (os error 13)
+   ┌─ tests/tasks/relative-and-absolute-task/source.wdl:10:24
+   │
+10 │     String something = read_string("my/path/to/something.txt")
+   │                        ^^^^^^^^^^^
+

wdl-engine/tests/tasks/test-join-paths/error.txt

@@ -0,0 +1,9 @@
+error: call to function `read_string` failed: failed to read file `attempts/0/work/mydir/mydata.txt`
+
+Caused by:
+    Permission denied (os error 13)
+   ┌─ tests/tasks/test-join-paths/source.wdl:31:21
+   │
+31 │     String result = read_string(data)
+   │                     ^^^^^^^^^^^
+

wdl-engine/tests/workflows/primitive-literals/error.txt

@@ -0,0 +1,6 @@
+error: call to task `write_file_task` in `tests/workflows/primitive-literals/source.wdl` failed: failed to evaluate output `x` for task `write_file_task`: file `calls/write_file_task/attempts/0/work/testdir/hello.txt` does not exist
+   ┌─ tests/workflows/primitive-literals/source.wdl:16:8
+   │
+16 │   call write_file_task
+   │        ^^^^^^^^^^^^^^^
+