From a0e14abf572eb8b08c41ea651699cac011bdc13e Mon Sep 17 00:00:00 2001 From: "Michael R. Crusoe" Date: Thu, 27 Jan 2022 12:19:09 +0100 Subject: [PATCH 1/3] Error reports include line and column number, if available Fixes #129 --- wdl2cwl/errors.py | 81 ++++++++++++++++++++++++++ wdl2cwl/main.py | 119 ++++++++++++++++++++++++-------------- wdl2cwl/tests/test_cwl.py | 8 +-- 3 files changed, 161 insertions(+), 47 deletions(-) create mode 100644 wdl2cwl/errors.py diff --git a/wdl2cwl/errors.py b/wdl2cwl/errors.py new file mode 100644 index 00000000..76a4ee26 --- /dev/null +++ b/wdl2cwl/errors.py @@ -0,0 +1,81 @@ +"""ContextManager to help with error reporting in miniWDL parsed files.""" + +import re +from types import TracebackType +from typing import Any, Callable, Optional, Tuple, Type, cast + +import WDL +from WDL._error_util import SourcePosition + +# Inspired by https://github.com/common-workflow-language/schema_salad/blob/661fb0fa8c745ed70253dda93bd12002007f6b33/schema_salad/sourceline.py#L232 + + +lineno_re = re.compile("^(.*?:[0-9]+:[0-9]+: )(( *)(.*))") + + +class WDLSourceLine: + """Contextmanager wrap exceptions with WDL source file locations.""" + + def __init__( + self, + item: Any, + raise_type: Callable[[str], Any] = str, + ): + """Which item and exception type to raise.""" + self.item = item + self.raise_type = raise_type + + def __enter__(self) -> "WDLSourceLine": + """Enter the context.""" + return self + + def __exit__( + self, + exc_type: Optional[Type[BaseException]], + exc_value: Optional[BaseException], + exc_tb: Optional[TracebackType], + ) -> None: + """ + Process the exit from the context. + + If there was an exception, wrap it in the raise_type. + """ + if not exc_value: + return + raise self.makeError(str(exc_value)) from exc_value + + def file(self) -> Optional[str]: + """Test if the item has file information attached.""" + if hasattr(self.item, "pos"): + pos: SourcePosition = cast(SourcePosition, self.item.pos) + return pos.uri + return None + + def makeLead(self) -> str: + """Caculate the error message prefix.""" + file = self.file() + if file: + pos = cast(SourcePosition, self.item.pos) + return f"{file}:{pos.line}:{pos.column}:" + return "" + + def makeError(self, msg: str) -> Any: + """Add the source info to the msg and instantiate the raise_type with it.""" + if not isinstance( + self.item, + ( + WDL.Error.SourceNode, + WDL.Tree.SourceComment, + WDL.Tree.DocImport, + WDL.Type.Base, + ), + ): + return self.raise_type(msg) + errs = [] + lead = self.makeLead() + for m in msg.splitlines(): + if bool(lineno_re.match(m)): + errs.append(m) + else: + errs.append(f"{lead}{m}") + return self.raise_type("\n".join(errs)) diff --git a/wdl2cwl/main.py b/wdl2cwl/main.py index 47a39756..8b84b9ce 100644 --- a/wdl2cwl/main.py +++ b/wdl2cwl/main.py @@ -1,19 +1,19 @@ """Main entrypoint for WDL2CWL.""" +import argparse import os import re -from typing import List, Union, Optional, Any, Set, Dict -import WDL -import cwl_utils.parser.cwl_v1_2 as cwl -import regex # type: ignore - -import textwrap import sys -import argparse - +import textwrap +from typing import Any, Dict, List, Optional, Set, Union +import cwl_utils.parser.cwl_v1_2 as cwl +import regex # type: ignore +import WDL from ruamel.yaml import scalarstring from ruamel.yaml.main import YAML +from wdl2cwl.errors import WDLSourceLine + valid_js_identifier = regex.compile( r"^(?!(?:do|if|in|for|let|new|try|var|case|else|enum|eval|null|this|true|" r"void|with|break|catch|class|const|false|super|throw|while|yield|delete|export|" @@ -28,6 +28,10 @@ # eval is not on the official list of reserved words, but it is a built-in function +class ConversionException(Exception): + """Error during conversion.""" + + def convert(doc: str) -> Dict[str, Any]: """Convert a WDL workflow, reading the file, into a CWL workflow Python object.""" wdl_path = os.path.relpath(doc) @@ -58,7 +62,9 @@ def load_wdl_objects( """Load a WDL SourceNode obj and returns either a Task or a Workflow.""" if isinstance(obj, WDL.Tree.Task): return self.load_wdl_task(obj) - raise Exception(f"Unimplemented type: {type(obj)}: {obj}") + raise WDLSourceLine(obj, ConversionException).makeError( + f"Unimplemented type: {type(obj)}: {obj}" + ) def load_wdl_task(self, obj: WDL.Tree.Task) -> cwl.CommandLineTool: """Load task and convert to CWL.""" @@ -153,7 +159,8 @@ def get_outdir_requirement( ) -> int: """Produce the memory requirement for the output directory from WDL runtime disks.""" # This is yet to be implemented. After Feature Parity. - return int(outdir.literal.value) * 1024 # type: ignore + with WDLSourceLine(outdir, ConversionException): + return int(outdir.literal.value) * 1024 # type: ignore def get_input(self, input_name: str) -> str: """Produce a consise, valid CWL expr/param reference lookup string for a given input name.""" @@ -165,11 +172,12 @@ def get_memory_requirement( self, memory_runtime: Union[WDL.Expr.Ident, WDL.Expr.Get, WDL.Expr.String] ) -> Union[str, float]: """Translate WDL Runtime Memory requirement to CWL Resource Requirement.""" - if isinstance(memory_runtime, WDL.Expr.String): - ram_min_literal = self.get_memory_literal(memory_runtime) - return ram_min_literal - ram_min = self.get_expr_name(memory_runtime.expr) # type: ignore - return self.get_ram_min_js(ram_min, "") + with WDLSourceLine(memory_runtime, ConversionException): + if isinstance(memory_runtime, WDL.Expr.String): + ram_min_literal = self.get_memory_literal(memory_runtime) + return ram_min_literal + ram_min = self.get_expr_name(memory_runtime.expr) # type: ignore + return self.get_ram_min_js(ram_min, "") def get_memory_literal(self, memory_runtime: WDL.Expr.String) -> float: """Get the literal value for memory requirement with type WDL.Expr.String.""" @@ -257,7 +265,9 @@ def get_expr(self, wdl_expr: Any) -> str: ): return self.get_literal_name(wdl_expr) else: - raise Exception(f"The expression '{wdl_expr}' is not handled yet.") + raise WDLSourceLine(wdl_expr, ConversionException).makeError( + f"The expression '{wdl_expr}' is not handled yet." + ) def get_literal_name( self, @@ -270,12 +280,16 @@ def get_literal_name( ) -> str: """Translate WDL Boolean, Int or Float Expression.""" if expr is None or not hasattr(expr, "parent"): - raise Exception(f"{type(expr)} has no attribute 'parent'") + raise WDLSourceLine(expr, ConversionException).makeError( + f"{type(expr)} has no attribute 'parent'" + ) # if the literal expr is used inside WDL.Expr.Apply # the literal value is what's needed if isinstance(expr.parent, WDL.Expr.Apply): # type: ignore return expr.literal.value # type: ignore - raise Exception(f"The parent expression for {expr} is not WDL.Expr.Apply") + raise WDLSourceLine(expr, ConversionException).makeError( + f"The parent expression for {expr} is not WDL.Expr.Apply" + ) def get_expr_string(self, wdl_expr_string: WDL.Expr.String) -> str: """Translate WDL String Expressions.""" @@ -321,7 +335,9 @@ def get_expr_apply(self, wdl_apply_expr: WDL.Expr.Apply) -> str: # type: ignore function_name = wdl_apply_expr.function_name arguments = wdl_apply_expr.arguments if not arguments: - raise Exception(f"The '{wdl_apply_expr}' expression has no arguments.") + raise WDLSourceLine(wdl_apply_expr, ConversionException).makeError( + f"The '{wdl_apply_expr}' expression has no arguments." + ) treat_as_optional = wdl_apply_expr.type.optional if function_name == "_add": left_operand, right_operand = arguments @@ -339,12 +355,13 @@ def get_expr_apply(self, wdl_apply_expr: WDL.Expr.Apply) -> str: # type: ignore if len(arguments) == 1: only_operand = arguments[0] is_file = isinstance(only_operand.type, WDL.Type.File) - only_operand = self.get_expr_name(only_operand.expr) # type: ignore - return ( - f"{only_operand}.basename" - if is_file - else f"{only_operand}.split('/').reverse()[0]" - ) + with WDLSourceLine(only_operand, ConversionException): + only_operand = self.get_expr_name(only_operand.expr) # type: ignore + return ( + f"{only_operand}.basename" + if is_file + else f"{only_operand}.split('/').reverse()[0]" + ) elif len(arguments) == 2: operand, suffix = arguments is_file = isinstance(operand.type, WDL.Type.File) @@ -372,12 +389,13 @@ def get_expr_apply(self, wdl_apply_expr: WDL.Expr.Apply) -> str: # type: ignore arg_name_with_file_check = self.get_expr_name_with_is_file_check( arg_name.expr # type: ignore ) - arg_value = arg_value.literal.value # type: ignore - return ( - f'{just_arg_name} === null ? "" : "{arg_value}" + {arg_name_with_file_check}' - if treat_as_optional - else f"{arg_value} $({arg_name_with_file_check})" - ) + with WDLSourceLine(arg_value, ConversionException): + arg_value = arg_value.literal.value # type: ignore + return ( + f'{just_arg_name} === null ? "" : "{arg_value}" + {arg_name_with_file_check}' + if treat_as_optional + else f"{arg_value} $({arg_name_with_file_check})" + ) elif function_name == "sub": wdl_apply, arg_string, arg_sub = arguments wdl_apply = self.get_expr(wdl_apply) # type: ignore @@ -417,7 +435,9 @@ def get_expr_apply(self, wdl_apply_expr: WDL.Expr.Apply) -> str: # type: ignore return glob else: - raise ValueError(f"Function name '{function_name}' not yet handled.") + raise WDLSourceLine(wdl_apply_expr, ConversionException).makeError( + f"Function name '{function_name}' not yet handled." + ) def get_expr_get(self, wdl_get_expr: WDL.Expr.Get) -> str: """Translate WDL Get Expressions.""" @@ -428,7 +448,9 @@ def get_expr_get(self, wdl_get_expr: WDL.Expr.Get) -> str: and wdl_get_expr.expr ): return self.get_expr_ident(wdl_get_expr.expr) - raise Exception(f"Get expressions with {member} are not yet handled.") + raise WDLSourceLine(wdl_get_expr, ConversionException).makeError( + f"Get expressions with {member} are not yet handled." + ) def get_expr_ident(self, wdl_ident_expr: WDL.Expr.Ident) -> str: """Translate WDL Ident Expressions.""" @@ -484,12 +506,14 @@ def get_cwl_docker_requirements( if dockerpull_expr is None or not isinstance( dockerpull_expr, WDL.Expr.Ident ): - raise Exception( + raise WDLSourceLine(wdl_docker, ConversionException).makeError( f"Unsupported type: {type(dockerpull_expr)}: {dockerpull_expr}" ) dockerpull_referee = dockerpull_expr.referee if dockerpull_referee is None: - raise Exception(f"Unsupported type: {type(dockerpull_referee)}") + raise WDLSourceLine(wdl_docker, ConversionException).makeError( + f"Unsupported type: {type(dockerpull_referee)}" + ) dockerpull = dockerpull_referee.expr.literal.value return cwl.DockerRequirement(dockerPull=dockerpull) @@ -514,7 +538,9 @@ def translate_wdl_placeholder(self, wdl_placeholder: WDL.Expr.Placeholder) -> st cwl_command_str = "" expr = wdl_placeholder.expr if expr is None: - raise Exception(f"Placeholder '{wdl_placeholder}' has no expr.") + raise WDLSourceLine(wdl_placeholder, ConversionException).makeError( + f"Placeholder '{wdl_placeholder}' has no expr." + ) placeholder_expr = self.get_expr(expr) options = wdl_placeholder.options if options: @@ -554,11 +580,11 @@ def translate_wdl_placeholder(self, wdl_placeholder: WDL.Expr.Placeholder) -> st + '"))' ) else: - raise Exception( + raise WDLSourceLine(wdl_placeholder, ConversionException).makeError( f"{wdl_placeholder} with expr of type {expr.type} is not yet handled" ) else: - raise Exception( + raise WDLSourceLine(wdl_placeholder, ConversionException).makeError( f"Placeholders with options {options} are not yet handled." ) else: @@ -584,14 +610,18 @@ def translate_wdl_placeholder(self, wdl_placeholder: WDL.Expr.Placeholder) -> st def get_expr_name(self, wdl_expr: WDL.Expr.Ident) -> str: """Extract name from WDL expr.""" if wdl_expr is None or not hasattr(wdl_expr, "name"): - raise Exception(f"{type(wdl_expr)} has not attribute 'name'") + raise WDLSourceLine(wdl_expr, ConversionException).makeError( + f"{type(wdl_expr)} has not attribute 'name'" + ) expr_name = self.get_input(wdl_expr.name) return expr_name def get_expr_name_with_is_file_check(self, wdl_expr: WDL.Expr.Ident) -> str: """Extract name from WDL expr and check if it's a file path.""" if wdl_expr is None or not hasattr(wdl_expr, "name"): - raise Exception(f"{type(wdl_expr)} has not attribute 'name'") + raise WDLSourceLine(wdl_expr, ConversionException).makeError( + f"{type(wdl_expr)} has not attribute 'name'" + ) expr_name = self.get_input(wdl_expr.name) is_file = isinstance(wdl_expr.type, WDL.Type.File) return expr_name if not is_file else f"{expr_name}.path" @@ -658,7 +688,10 @@ def get_cwl_type(self, input_type: WDL.Tree.Decl) -> str: elif isinstance(input_type, WDL.Type.Float): type_of = "float" else: - raise Exception(f"Input of type {input_type} is not yet handled.") + print(type(input_type)) + raise WDLSourceLine(input_type, ConversionException).makeError( + f"Input of type {input_type} is not yet handled." + ) return type_of def get_cwl_outputs( @@ -680,7 +713,9 @@ def get_cwl_outputs( type_of = self.get_cwl_type(wdl_output.type) # type: ignore if not wdl_output.expr: - raise ValueError("Missing expression") + raise WDLSourceLine(wdl_output, ConversionException).makeError( + "Missing expression" + ) if ( isinstance(wdl_output.expr, WDL.Expr.Apply) diff --git a/wdl2cwl/tests/test_cwl.py b/wdl2cwl/tests/test_cwl.py index b48b2760..559fbc26 100644 --- a/wdl2cwl/tests/test_cwl.py +++ b/wdl2cwl/tests/test_cwl.py @@ -1,11 +1,11 @@ """Tests for miniwdl.""" import os.path -from tempfile import NamedTemporaryFile from pathlib import Path +from tempfile import NamedTemporaryFile + import pytest -from ..main import main -from ..main import Converter +from ..main import Converter, main def get_file(path: str) -> str: @@ -87,8 +87,6 @@ def test_wdl_stdout(capsys) -> None: # type: ignore class TestObject: """Test object for creating WDL.Expr.String.""" - pass - testdata = [ ("20 MB", 19.073486328125), From 1bee7510911725c74e8b347fac4ac59d573d1fff Mon Sep 17 00:00:00 2001 From: "Michael R. Crusoe" Date: Thu, 27 Jan 2022 12:51:21 +0100 Subject: [PATCH 2/3] add test --- wdl2cwl/errors.py | 2 +- wdl2cwl/tests/test_wdlsourceline.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 wdl2cwl/tests/test_wdlsourceline.py diff --git a/wdl2cwl/errors.py b/wdl2cwl/errors.py index 76a4ee26..99d8f424 100644 --- a/wdl2cwl/errors.py +++ b/wdl2cwl/errors.py @@ -2,7 +2,7 @@ import re from types import TracebackType -from typing import Any, Callable, Optional, Tuple, Type, cast +from typing import Any, Callable, Optional, Type, cast import WDL from WDL._error_util import SourcePosition diff --git a/wdl2cwl/tests/test_wdlsourceline.py b/wdl2cwl/tests/test_wdlsourceline.py new file mode 100644 index 00000000..a2bf336a --- /dev/null +++ b/wdl2cwl/tests/test_wdlsourceline.py @@ -0,0 +1,21 @@ +"""Tests for WDLSourceLine.""" + +import re + +import pytest +import WDL + +from ..main import ConversionException, Converter +from .test_cwl import get_file + + +def test_wdlsourceline() -> None: + """Basic test of WDLSourceLine.""" + doc_tree = WDL.load(get_file("wdl_files/minCores.wdl")) + with pytest.raises( + ConversionException, + match=re.escape( + "minCores.wdl:1:1:Unimplemented type: " + ), + ): + Converter().load_wdl_objects(doc_tree) # type: ignore From 786d2a4bb42f393fd35e1fbd6f9c3e7ee1099633 Mon Sep 17 00:00:00 2001 From: "Michael R. Crusoe" Date: Thu, 27 Jan 2022 14:37:01 +0100 Subject: [PATCH 3/3] more tests & simplify --- wdl2cwl/errors.py | 16 +++------------ wdl2cwl/tests/test_wdlsourceline.py | 32 ++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/wdl2cwl/errors.py b/wdl2cwl/errors.py index 99d8f424..ff43961b 100644 --- a/wdl2cwl/errors.py +++ b/wdl2cwl/errors.py @@ -44,20 +44,10 @@ def __exit__( return raise self.makeError(str(exc_value)) from exc_value - def file(self) -> Optional[str]: - """Test if the item has file information attached.""" - if hasattr(self.item, "pos"): - pos: SourcePosition = cast(SourcePosition, self.item.pos) - return pos.uri - return None - def makeLead(self) -> str: """Caculate the error message prefix.""" - file = self.file() - if file: - pos = cast(SourcePosition, self.item.pos) - return f"{file}:{pos.line}:{pos.column}:" - return "" + pos: SourcePosition = cast(SourcePosition, self.item.pos) + return f"{pos.uri}:{pos.line}:{pos.column}:" def makeError(self, msg: str) -> Any: """Add the source info to the msg and instantiate the raise_type with it.""" @@ -77,5 +67,5 @@ def makeError(self, msg: str) -> Any: if bool(lineno_re.match(m)): errs.append(m) else: - errs.append(f"{lead}{m}") + errs.append(f"{lead} {m}") return self.raise_type("\n".join(errs)) diff --git a/wdl2cwl/tests/test_wdlsourceline.py b/wdl2cwl/tests/test_wdlsourceline.py index a2bf336a..e9e1cf2c 100644 --- a/wdl2cwl/tests/test_wdlsourceline.py +++ b/wdl2cwl/tests/test_wdlsourceline.py @@ -5,6 +5,7 @@ import pytest import WDL +from ..errors import WDLSourceLine from ..main import ConversionException, Converter from .test_cwl import get_file @@ -15,7 +16,36 @@ def test_wdlsourceline() -> None: with pytest.raises( ConversionException, match=re.escape( - "minCores.wdl:1:1:Unimplemented type: " + "minCores.wdl:1:1: Unimplemented type: " ), ): Converter().load_wdl_objects(doc_tree) # type: ignore + + +def test_wdlsourceline_non_wdl() -> None: + """Using non-miniWDL objects with WDLSourceLine.""" + message = WDLSourceLine({"non-WDL object"}).makeError("Something happened") + + assert message == "Something happened" + + +def test_wdlsourceline_non_wdl_context_manager() -> None: + """Test non-miniWDL objects with WDLSourceLine as a context manager.""" + with pytest.raises(ConversionException, match="Something went wrong"): + with WDLSourceLine({"non-WDL object"}, ConversionException) as test: + assert test is not None + raise Exception("Something went wrong") + assert True + + +def test_nested_wdlsourceline() -> None: + """Nested test of WDLSourceLine.""" + doc_tree = WDL.load(get_file("wdl_files/minCores.wdl")) + with pytest.raises( + ConversionException, + match=re.escape( + "minCores.wdl:1:1: Unimplemented type: " + ), + ): + with WDLSourceLine(doc_tree.tasks[0], ConversionException): + Converter().load_wdl_objects(doc_tree) # type: ignore