tighten optional-to-String coercions (#600)

Disallow implicit coercion of File? Directory? Boolean? Float? Int to (non-optional) String, which should not have been permitted. #596

previously subject to deprecation warning (#602)

WDL/Expr.py

@@ -704,7 +704,7 @@ def _infer_type(self, type_env: Env.Bindings[Type.Base]) -> Type.Base:
 
         # typecheck members vs struct declaration
         try:
-            object_type.check(struct_type)
+            object_type.check(struct_type, self._check_quant)
         except TypeError as exn:
             raise Error.StaticTypeMismatch(
                 self, struct_type, object_type, exn.args[0] if exn.args else ""

WDL/Lint.py

@@ -1183,85 +1183,3 @@ def expr(self, obj: Expr.Base) -> Any:
             and _find_doc(obj).effective_wdl_version not in ("draft-2", "1.0")
         ):
             self.add(obj, "replace 'object' with specific struct type [WDL >= 1.1]", obj.pos)
-
-        # The remainder of this class body deals with issue 596, future tightening of typechecking
-        # implicit coercions from optional non-String expressions where non-optional String is
-        # expected
-
-        if isinstance(obj, Expr.Apply):
-            if obj.function_name == "_add":
-                assert len(obj.arguments) == 2
-                arg0ty = obj.arguments[0].type
-                arg1ty = obj.arguments[1].type
-                if (isinstance(arg0ty, Type.String) or isinstance(arg1ty, Type.String)) and (
-                    _issue596_coercion(Type.String(), arg0ty)
-                    or _issue596_coercion(Type.String(), arg1ty)
-                ):
-                    self.add(
-                        obj,
-                        "future miniwdl version will disallow optional "
-                        + str(arg0ty if isinstance(arg1ty, Type.String) else arg1ty)
-                        + " operand in string concatenation; use select_first() coercion",
-                        obj.pos,
-                    )
-            else:
-                F = getattr(
-                    StdLib.TaskOutputs(_find_doc(obj).effective_wdl_version), obj.function_name
-                )
-                if isinstance(F, StdLib.StaticFunction):
-                    for i in range(min(len(F.argument_types), len(obj.arguments))):
-                        F_i = F.argument_types[i]
-                        arg_i = obj.arguments[i].type
-                        if _issue596_coercion(F_i, arg_i):
-                            msg = (
-                                f"future miniwdl version will disallow optional {arg_i}"
-                                f" for non-optional String argument of {obj.function_name}()"
-                                + "; use select_first() coercion"
-                            )
-                            self.add(obj, msg, obj.arguments[i].pos)
-
-        if (
-            isinstance(obj, Expr.Struct)
-            and isinstance(obj.type, Type.StructInstance)
-            and obj.type.members
-        ):
-            for field, field_expr in obj.members.items():
-                if _issue596_coercion(obj.type.members[field], field_expr.type):
-                    msg = (
-                        f"future miniwdl version will disallow optional expression within {field_expr.type} for"
-                        f" non-optional String within member {obj.type.members[field]} {field}"
-                        "; use select_first() or select_all() coercion"
-                    )
-                    self.add(obj, msg, field_expr.pos)
-
-    def decl(self, obj: Tree.Decl) -> Any:
-        if obj.expr and _issue596_coercion(obj.type, obj.expr.type):
-            self.add(
-                obj,
-                f"future miniwdl version will disallow optional expression within {obj.expr.type} for non-optional"
-                f" String within {obj.expr.type} {obj.name}; use select_first() or select_all() coercion",
-            )
-
-    def call(self, obj: Tree.Call) -> Any:
-        for name, inp_expr in obj.inputs.items():
-            decl = _find_input_decl(obj, name)
-            # treat input with default as optional, with or without the ? type quantifier
-            decltype = decl.type.copy(optional=True) if decl.expr else decl.type
-            if _issue596_coercion(decltype, inp_expr.type):
-                self.add(
-                    obj,
-                    f"future miniwdl version will disallow optional expression within {inp_expr.type} for non-optional"
-                    f" String within input {decltype} {decl.name}; use select_first() or select_all() coercion",
-                )
-
-
-def _issue596_coercion(to_type, from_type):
-    return _compound_coercion(
-        to_type,
-        from_type,
-        Type.String,
-        predicates=lambda to_type2, from_type2: isinstance(to_type2, Type.String)
-        and not to_type2.optional
-        and not isinstance(from_type2, Type.String)
-        and from_type2.optional,
-    )

WDL/StdLib.py

@@ -579,7 +579,7 @@ def infer_type(self, expr: "Expr.Apply") -> Type.Base:
         if t2 is None:
             # neither operand is a string; defer to _ArithmeticOperator
             return super().infer_type(expr)
-        if not t2.coerces(Type.String(optional=not expr._check_quant)):
+        if not t2.coerces(Type.String(), check_quant=expr._check_quant):
             raise Error.IncompatibleOperand(
                 expr,
                 "Cannot add/concatenate {} and {}".format(

WDL/Type.py

@@ -151,7 +151,7 @@ def __init__(self, optional: bool = False) -> None:
     def check(self, rhs: Base, check_quant: bool = True) -> None:
         """"""
         if isinstance(rhs, String):
-            return
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
 
@@ -162,7 +162,7 @@ def __init__(self, optional: bool = False) -> None:
     def check(self, rhs: Base, check_quant: bool = True) -> None:
         """"""
         if isinstance(rhs, String):
-            return
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
 
@@ -175,7 +175,7 @@ def check(self, rhs: Base, check_quant: bool = True) -> None:
         if isinstance(rhs, Float):
             return self._check_optional(rhs, check_quant)
         if isinstance(rhs, String):
-            return
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
 
@@ -186,7 +186,7 @@ def __init__(self, optional: bool = False) -> None:
     def check(self, rhs: Base, check_quant: bool = True) -> None:
         """"""
         if isinstance(rhs, String):
-            return
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
 
@@ -197,7 +197,7 @@ def __init__(self, optional: bool = False) -> None:
     def check(self, rhs: Base, check_quant: bool = True) -> None:
         """"""
         if isinstance(rhs, String):
-            return
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
 
@@ -261,7 +261,9 @@ def check(self, rhs: Base, check_quant: bool = True) -> None:
             self.item_type.check(rhs.item_type, check_quant)
             return self._check_optional(rhs, check_quant)
         if isinstance(rhs, String):
-            return None if self.item_type is None else self.item_type.check(String())
+            if self.item_type is not None:
+                self.item_type.check(String())
+            return self._check_optional(rhs, check_quant)
         super().check(rhs, check_quant)
 
     def copy(self, optional: Optional[bool] = None, nonempty: Optional[bool] = None) -> Base:
@@ -486,7 +488,7 @@ def check(self, rhs: Base, check_quant: bool = True) -> None:
             # map value type.
             String().check(rhs.item_type[0])
             for vt in self.members.values():
-                vt.check(rhs.item_type[1])
+                vt.check(rhs.item_type[1], check_quant=check_quant)
             return
         if isinstance(rhs, (Any, Object)):
             # Don't worry about Object coercion because we expect a further coercion to

tests/test_3corpi.py

@@ -224,7 +224,6 @@ class gatk4_somatic_snvs_indels(unittest.TestCase):
         "UnusedCall": 1,
         "StringCoercion": 4,
         "MissingVersion": 6,
-        "Deprecated": 1,
     },
     check_quant=False,
 )
@@ -399,7 +398,7 @@ class dxWDL(unittest.TestCase):
         "_suppressions": 8,
         "UnusedImport": 4,
         "NameCollision": 27,
-        "StringCoercion": 8,
+        "StringCoercion": 7,
         "FileCoercion": 3,
         "NonemptyCoercion": 1,
         "UnnecessaryQuantifier": 5,
@@ -409,11 +408,10 @@ class dxWDL(unittest.TestCase):
         "MissingVersion": 7,
         "UnboundDeclaration": 1,
         "UnverifiedStruct": 3,
-        "Deprecated": 10,
-        "OptionalCoercion": 1,
+        "Deprecated": 2,
         "UnexpectedRuntimeValue": 1,
     },
-    blocklist=["check_quant", "incomplete_call"],
+    blocklist=["check_quant", "incomplete_call", "issue596"],
 )
 class Contrived(unittest.TestCase):
     pass
@@ -427,7 +425,7 @@ class Contrived(unittest.TestCase):
         "NameCollision": 43,
         "StringCoercion": 13,
         "FileCoercion": 5,
-        "OptionalCoercion": 4,
+        "OptionalCoercion": 10,
         "NonemptyCoercion": 2,
         "UnnecessaryQuantifier": 9,
         "UnusedDeclaration": 9,
@@ -437,7 +435,7 @@ class Contrived(unittest.TestCase):
         "MissingVersion": 11,
         "UnboundDeclaration": 1,
         "UnverifiedStruct": 3,
-        "Deprecated": 11,
+        "Deprecated": 3,
         "UnexpectedRuntimeValue": 1,
     },
     check_quant=False,
@@ -453,16 +451,17 @@ class Contrived2(unittest.TestCase):
         # these use the pattern 'input { Type? x = default }' and need check_quant=False
         "mergecounts",
         "somaticseq",
+        "bamstats",
+        "biopet",
+        "sampleconfig",
+        "seqstat",
     ],
     expected_lint={
-        "OptionalCoercion": 9,
-        "UnusedDeclaration": 18,
+        "OptionalCoercion": 2,
+        "UnusedDeclaration": 15,
         "NonemptyCoercion": 1,
-        "NameCollision": 1,
         "SelectArray": 1,
-        "UnverifiedStruct": 1,
         "UnnecessaryQuantifier": 8,
-        "Deprecated": 7,
     },
 )
 class BioWDLTasks(unittest.TestCase):
@@ -478,7 +477,6 @@ class BioWDLTasks(unittest.TestCase):
         "NameCollision": 1,
         "UnverifiedStruct": 1,
         "UnnecessaryQuantifier": 13,
-        "Deprecated": 7,
     },
     check_quant=False,
 )
@@ -495,7 +493,6 @@ class BioWDLAligning(unittest.TestCase):
         "NameCollision": 1,
         "UnverifiedStruct": 1,
         "UnnecessaryQuantifier": 9,
-        "Deprecated": 7,
     },
     check_quant=False,
 )
@@ -512,7 +509,6 @@ class BioWDLExpressionQuantification(unittest.TestCase):
         "NonemptyCoercion": 37,
         "SelectArray": 5,
         "UnnecessaryQuantifier": 3,
-        "Deprecated": 8,
     },
     check_quant=False,
 )

tests/test_7runner.py

@@ -925,6 +925,34 @@ def test_spec_select_all_wdl(self):
         self.assertEqual(outp["fivethree"], [5, 3])
         self.assertEqual(outp["is_true"], True)
 
+    def test_issue596(self):
+        self._run("""
+        task reference_prepare {
+            input {
+                # You need to define either this...
+                File? reference_fa_file
+
+                # Or both of these.
+                File?   reference_zipped_directory
+                String? reference_fa_filename_in_zipped_directory
+            }
+
+            # get the basename of the reference
+            String? basename_reference = basename(reference_zipped_directory)
+
+            command <<<
+                set -eux -o pipefail
+
+                if [[ ! "~{reference_zipped_directory}" = "" ]]
+                then
+                    cp ~{reference_zipped_directory} .
+                    unzip ~{basename_reference}
+                fi
+
+                # do other things here
+            >>>
+        }""", {}, expected_exception=WDL.Error.StaticTypeMismatch)
+
     def test_issue614(self):
         wdl = r"""
         version 1.0