Handled the pickValue in the check_types function to avoid unnecessary warnings. (#397)

Author: LanderOtto

cwl_utils/parser/cwl_v1_2_utils.py

@@ -210,6 +210,7 @@ def check_all_types(
         extra_message = (
             "pickValue is %s" % sink.pickValue if sink.pickValue is not None else None
         )
+        sink_type = type_dict[sink.id]
         match sink:
             case cwl.WorkflowOutputParameter():
                 sourceName = "outputSource"
@@ -220,7 +221,7 @@ def check_all_types(
             case _:
                 continue
         if sourceField is not None:
-            if isinstance(sourceField, MutableSequence):
+            if isinstance(sourceField, MutableSequence) and len(sourceField) > 1:
                 linkMerge: str | None = sink.linkMerge or (
                     "merge_nested" if len(sourceField) > 1 else None
                 )
@@ -231,23 +232,34 @@ def check_all_types(
                     srcs_of_sink += [src_dict[parm_id]]
                     if (
                         _is_conditional_step(param_to_step, parm_id)
-                        and sink.pickValue is not None
+                        and sink.pickValue is None
                     ):
-                        validation["warning"].append(
-                            SrcSink(
-                                src_dict[parm_id],
-                                sink,
-                                linkMerge,
-                                message="Source is from conditional step, but pickValue is not used",
+                        src_typ = aslist(type_dict[src_dict[parm_id].id])
+                        if "null" not in src_typ:
+                            src_typ = ["null"] + cast(list[Any], src_typ)
+                        if (
+                            not isinstance(sink_type, MutableSequence)
+                            or "null" not in sink_type
+                        ):
+                            validation["warning"].append(
+                                SrcSink(
+                                    src_dict[parm_id],
+                                    sink,
+                                    linkMerge,
+                                    message="Source is from conditional step, but pickValue is not used",
+                                )
                             )
-                        )
+                        type_dict[src_dict[parm_id].id] = src_typ
                     if _is_all_output_method_loop_step(param_to_step, parm_id):
                         src_typ = type_dict[src_dict[parm_id].id]
                         type_dict[src_dict[parm_id].id] = cwl.ArraySchema(
                             items=src_typ, type_="array"
                         )
             else:
-                parm_id = cast(str, sourceField)
+                if isinstance(sourceField, MutableSequence):
+                    parm_id = cast(str, sourceField[0])
+                else:
+                    parm_id = cast(str, sourceField)
                 if parm_id not in src_dict:
                     raise SourceLine(sink, sourceName, ValidationException).makeError(
                         f"{sourceName} not found: {parm_id}"
@@ -289,7 +301,7 @@ def check_all_types(
             for src in srcs_of_sink:
                 check_result = check_types(
                     type_dict[cast(str, src.id)],
-                    type_dict[sink.id],
+                    sink_type,
                     linkMerge,
                     getattr(sink, "valueFrom", None),
                 )
@@ -313,19 +325,24 @@ def check_types(
     """
     if valueFrom is not None:
         return "pass"
-    if linkMerge is None:
-        if can_assign_src_to_sink(srctype, sinktype, strict=True):
-            return "pass"
-        if can_assign_src_to_sink(srctype, sinktype, strict=False):
-            return "warning"
-        return "exception"
-    if linkMerge == "merge_nested":
-        return check_types(
-            cwl.ArraySchema(items=srctype, type_="array"), sinktype, None, None
-        )
-    if linkMerge == "merge_flattened":
-        return check_types(merge_flatten_type(srctype), sinktype, None, None)
-    raise ValidationException(f"Invalid value {linkMerge} for linkMerge field.")
+    match linkMerge:
+        case None:
+            if can_assign_src_to_sink(srctype, sinktype, strict=True):
+                return "pass"
+            if can_assign_src_to_sink(srctype, sinktype, strict=False):
+                return "warning"
+            return "exception"
+        case "merge_nested":
+            return check_types(
+                cwl.ArraySchema(items=srctype, type_="array"),
+                sinktype,
+                None,
+                None,
+            )
+        case "merge_flattened":
+            return check_types(merge_flatten_type(srctype), sinktype, None, None)
+        case _:
+            raise ValidationException(f"Invalid value {linkMerge} for linkMerge field.")
 
 
 def content_limit_respected_read_bytes(f: IO[bytes]) -> bytes:

cwl_utils/testdata/checker_wf/no-warning-wf.cwl

@@ -0,0 +1,44 @@
+#!/usr/bin/env cwl-runner
+class: Workflow
+cwlVersion: v1.2
+requirements:
+  ScatterFeatureRequirement: {}
+  MultipleInputFeatureRequirement: {}
+  StepInputExpressionRequirement: {}
+inputs:
+  letters0:
+    type: [string, int]
+    default: "a0"
+  letters1:
+    type: string[]
+    default: ["a1", "b1"]
+  letters2:
+    type: [string, int]
+    default: "a2"
+  letters3:
+    type: string[]
+    default: ["a3", "b3"]
+  letters4:
+    type: string
+    default: "a4"
+  letters5:
+    type: string[]
+    default: ["a5", "b5", "c5"]
+  exec:
+    type: bool
+    default: False
+
+outputs:
+  single:
+    type: File?
+    outputSource:
+     - step1/txt
+
+steps:
+  - id: step1
+    run: echo.cwl
+    in:
+      exec: exec
+    out: [txt]
+    when: $(inputs.exec)
+

cwl_utils/testdata/checker_wf/warning-wf.cwl

@@ -0,0 +1,44 @@
+#!/usr/bin/env cwl-runner
+class: Workflow
+cwlVersion: v1.2
+requirements:
+  ScatterFeatureRequirement: {}
+  MultipleInputFeatureRequirement: {}
+  StepInputExpressionRequirement: {}
+inputs:
+  letters0:
+    type: [string, int]
+    default: "a0"
+  letters1:
+    type: string[]
+    default: ["a1", "b1"]
+  letters2:
+    type: [string, int]
+    default: "a2"
+  letters3:
+    type: string[]
+    default: ["a3", "b3"]
+  letters4:
+    type: string
+    default: "a4"
+  letters5:
+    type: string[]
+    default: ["a5", "b5", "c5"]
+  exec:
+    type: bool
+    default: False
+
+outputs:
+  single:
+    type: File
+    outputSource:
+     - step1/txt
+
+steps:
+  - id: step1
+    run: echo.cwl
+    in:
+      exec: exec
+    out: [txt]
+    when: $(inputs.exec)
+

cwl_utils/testdata/checker_wf/warning-wf2.cwl

@@ -0,0 +1,43 @@
+#!/usr/bin/env cwl-runner
+class: Workflow
+cwlVersion: v1.2
+requirements:
+  ScatterFeatureRequirement: {}
+  MultipleInputFeatureRequirement: {}
+  StepInputExpressionRequirement: {}
+inputs:
+  letters0:
+    type: [string, int]
+    default: "a0"
+  letters1:
+    type: string[]
+    default: ["a1", "b1"]
+  letters2:
+    type: [string, int]
+    default: "a2"
+  letters3:
+    type: string[]
+    default: ["a3", "b3"]
+  letters4:
+    type: string
+    default: "a4"
+  letters5:
+    type: string[]
+    default: ["a5", "b5", "c5"]
+  exec:
+    type: bool
+    default: False
+
+outputs:
+  single_a:
+    type: File
+    outputSource: step1/txt
+
+steps:
+  - id: step1
+    run: echo.cwl
+    in:
+      exec: exec
+    out: [txt]
+    when: $(inputs.exec)
+

cwl_utils/testdata/checker_wf/warning-wf3.cwl

@@ -0,0 +1,50 @@
+#!/usr/bin/env cwl-runner
+class: Workflow
+cwlVersion: v1.2
+requirements:
+  ScatterFeatureRequirement: {}
+  MultipleInputFeatureRequirement: {}
+  StepInputExpressionRequirement: {}
+inputs:
+  letters0:
+    type: [string, int]
+    default: "a0"
+  letters1:
+    type: string[]
+    default: ["a1", "b1"]
+  letters2:
+    type: [string, int]
+    default: "a2"
+  letters3:
+    type: string[]
+    default: ["a3", "b3"]
+  letters4:
+    type: string
+    default: "a4"
+  letters5:
+    type: string[]
+    default: ["a5", "b5", "c5"]
+  exec:
+    type: bool
+    default: False
+
+outputs:
+  pair:
+    type: File[]
+    outputSource:
+     - step1/txt
+     - step2/txt
+
+steps:
+  - id: step1
+    run: echo.cwl
+    in:
+      exec: exec
+    out: [txt]
+    when: $(inputs.exec)
+  - id: step2
+    run: echo.cwl
+    in: []
+    out: [txt]
+    when: $(inputs.exec)
+

cwl_utils/tests/test_parser_utils.py

@@ -1,11 +1,13 @@
 # SPDX-License-Identifier: Apache-2.0
 """Test the CWL parsers utility functions."""
+import logging
+import re
 import tempfile
 from collections.abc import MutableSequence
 from typing import cast
 
 import pytest
-from pytest import raises
+from pytest import raises, LogCaptureFixture
 from schema_salad.exceptions import ValidationException
 
 import cwl_utils.parser.cwl_v1_0
@@ -71,6 +73,39 @@ def test_static_checker_fail(cwlVersion: str) -> None:
         cwl_utils.parser.utils.static_checker(cwl_obj7)
 
 
+def test_static_checker_warning(caplog: LogCaptureFixture) -> None:
+    uri1 = get_path("testdata/checker_wf/warning-wf.cwl").as_uri()
+    cwl_obj1 = load_document_by_uri(uri1)
+    with caplog.at_level(level=logging.WARNING, logger="cwl_utils"):
+        cwl_utils.parser.utils.static_checker(cwl_obj1)
+    assert "Source is from conditional step and may produce `null`" in caplog.text
+    caplog.clear()
+
+    uri2 = get_path("testdata/checker_wf/no-warning-wf.cwl").as_uri()
+    cwl_obj2 = load_document_by_uri(uri2)
+    with caplog.at_level(level=logging.WARNING, logger="cwl_utils"):
+        cwl_utils.parser.utils.static_checker(cwl_obj2)
+    assert not caplog.text
+    caplog.clear()
+
+    uri3 = get_path("testdata/checker_wf/warning-wf2.cwl").as_uri()
+    cwl_obj3 = load_document_by_uri(uri3)
+    with caplog.at_level(level=logging.WARNING, logger="cwl_utils"):
+        cwl_utils.parser.utils.static_checker(cwl_obj3)
+    assert "Source is from conditional step and may produce `null`" in caplog.text
+    caplog.clear()
+
+    uri4 = get_path("testdata/checker_wf/warning-wf3.cwl").as_uri()
+    cwl_obj4 = load_document_by_uri(uri4)
+    with caplog.at_level(level=logging.WARNING, logger="cwl_utils"):
+        cwl_utils.parser.utils.static_checker(cwl_obj4)
+    assert re.search(
+        'with sink \'pair\' of type {"name":.* "items": "File", "type": "array"}',
+        caplog.text,
+    )
+    assert "Source is from conditional step, but pickValue is not used" in caplog.text
+
+
 @pytest.mark.parametrize("cwlVersion", ["v1_0", "v1_1", "v1_2"])
 def test_static_checker_success(cwlVersion: str) -> None:
     """Confirm that static type checker correctly validates workflows."""