Merge pull request #4842 from oliver-sanders/jinja2-error-context

jinja2: improve error context information

CHANGES.md

@@ -35,6 +35,9 @@ Third Release Candidate for Cylc 8 suitable for acceptance testing.
 
 ### Enhancements
 
+[#4842](https://github.com/cylc/cylc-flow/pull/4842) -
+Improve Jinja2 error reporting when the error is behind an `{% include`.
+
 [#4828](https://github.com/cylc/cylc-flow/pull/4828) - scan CLI: corrupt
 workflow contact files should result in a warning, not a crash.
 

cylc/flow/parsec/empysupport.py

@@ -21,12 +21,30 @@
 from io import StringIO
 import em
 import os
+import typing as t
 
 from cylc.flow.parsec.exceptions import EmPyError
 
 
-def empyprocess(flines, dir_, template_vars=None):
-    """Pass configure file through EmPy processor."""
+def empyprocess(
+    _fpath: str,
+    flines: t.List[str],
+    dir_: str,
+    template_vars: t.Dict[str, t.Any] = None,
+) -> t.List[str]:
+    """Pass configure file through EmPy processor.
+
+    Args:
+        _fpath:
+            The path to the root template file (i.e. the flow.cylc file)
+        flines:
+            List of template lines to process.
+        dir_:
+            The path to the configuration directory.
+        template_vars:
+            Dictionary of template variables.
+
+    """
 
     cwd = os.getcwd()
 
@@ -38,7 +56,10 @@ def empyprocess(flines, dir_, template_vars=None):
         interpreter.file(ftempl, '<template>', template_vars)
     except Exception as exc:
         lineno = interpreter.contexts[-1].identify()[1]
-        raise EmPyError(str(exc), lines=flines[max(lineno - 4, 0): lineno])
+        raise EmPyError(
+            str(exc),
+            lines={'<template>': flines[max(lineno - 4, 0): lineno]},
+        )
     finally:
         interpreter.shutdown()
         xworkflow = xtempl.getvalue()

cylc/flow/parsec/exceptions.py

@@ -17,6 +17,7 @@
 from copy import copy
 import os
 import textwrap
+import typing as t
 
 from cylc.flow.parsec.util import itemstr
 
@@ -68,10 +69,40 @@ def __str__(self):
 
 
 class FileParseError(ParsecError):
-    """Error raised when attempting to read in the config file(s)."""
-
-    def __init__(self, reason, index=None, line=None, lines=None,
-                 err_type=None, fpath=None, help_lines=None):
+    """Error raised when attempting to read in the config file(s).
+
+    Args:
+        reason:
+            Description of error.
+        err_type:
+            Classification of error (e.g. Jinja2Error).
+        help_lines:
+            Additional info to include in the exception.
+
+    Args (ways of providing exception context - TODO rationalise this!):
+        lines:
+            (preferred) Dictionary in the format
+            {filename: [context_line, ..., error_line]}
+        index:
+            The line number of the error in the config (counting from the
+            shebang line *not* the first line).
+        line:
+            The line of the error in the config.
+        fpath:
+            The path to the file containing the error.
+
+    """
+
+    def __init__(
+        self,
+        reason: str,
+        index: t.Optional[int] = None,
+        line: t.Optional[str] = None,
+        lines: t.Optional[t.Dict[str, t.List[str]]] = None,
+        err_type: t.Optional[str] = None,
+        fpath: t.Optional[str] = None,
+        help_lines: t.Optional[t.Iterable[str]] = None,
+    ):
         self.reason = reason
         self.line_num = index + 1 if index is not None else None
         self.line = line
@@ -80,7 +111,7 @@ def __init__(self, reason, index=None, line=None, lines=None,
         self.fpath = fpath
         self.help_lines = help_lines or []
 
-    def __str__(self):
+    def __str__(self) -> str:
         msg = ''
         msg += self.reason
 
@@ -94,10 +125,11 @@ def __str__(self):
         if self.line:
             msg += ":\n   " + self.line.strip()
         if self.lines:
-            msg += "\nContext lines:\n" + "\n".join(self.lines)
-            msg += "\t<--"
-            if self.err_type:
-                msg += ' %s' % self.err_type
+            for filename, lines in self.lines.items():
+                msg += f'\nFile {filename}\n  ' + '\n  '.join(lines)
+                msg += "\t<--"
+                if self.err_type:
+                    msg += ' %s' % self.err_type
         help_lines = list(self.help_lines)
         if self.line_num:
             # TODO - make 'view' function independent of cylc:
@@ -116,11 +148,27 @@ class EmPyError(FileParseError):
 
 
 class Jinja2Error(FileParseError):
-    """Wrapper class for Jinja2 exceptions."""
-
-    def __init__(self, exception, lines=None, filename=None):
+    """Wrapper class for Jinja2 exceptions.
+
+    Args:
+        exception:
+            The exception being re-raised
+        lines:
+            Dictionary in the format
+            {filename: [context_line, ..., error_line]}
+        filename:
+            Alternative to "lines" where less detail is available.
+
+    """
+
+    def __init__(
+        self,
+        exception: Exception,
+        lines: t.Optional[t.Dict[str, t.List[str]]] = None,
+        filename: t.Optional[str] = None,
+    ):
         # extract the first sentence of exception
-        msg = str(exception)
+        msg: str = str(exception)
         try:
             msg, tail = msg.split('. ', 1)
         except ValueError:

cylc/flow/parsec/fileparse.py

@@ -31,11 +31,10 @@
 """
 
 import os
+from pathlib import Path
 import re
 import sys
-
-from pathlib import Path
-from typing import Any, Dict, List, Optional
+import typing as t
 
 from cylc.flow import __version__, iter_entry_points
 from cylc.flow import LOG
@@ -305,9 +304,9 @@ def process_plugins(fpath, opts):
 
 
 def merge_template_vars(
-    native_tvars: Dict[str, Any],
-    plugin_result: Dict[str, Any]
-) -> Dict[str, Any]:
+    native_tvars: t.Dict[str, t.Any],
+    plugin_result: t.Dict[str, t.Any]
+) -> t.Dict[str, t.Any]:
     """Manage the merger of Cylc Native and Plugin template variables.
 
     Args:
@@ -348,7 +347,12 @@ def merge_template_vars(
         return native_tvars
 
 
-def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
+def read_and_proc(
+    fpath: str,
+    template_vars: t.Optional[t.Dict[str, t.Any]] = None,
+    viewcfg: t.Any = None,
+    opts: t.Any = None,
+) -> t.List[str]:
     """
     Read a cylc parsec config file (at fpath), inline any include files,
     process with Jinja2, and concatenate continuation lines.
@@ -424,7 +428,7 @@ def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
                 raise ParsecError('EmPy Python package must be installed '
                                   'to process file: ' + fpath)
             flines = empyprocess(
-                flines, fdir, template_vars
+                fpath, flines, fdir, template_vars
             )
 
     # process with Jinja2
@@ -444,7 +448,7 @@ def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
                 raise ParsecError('Jinja2 Python package must be installed '
                                   'to process file: ' + fpath)
             flines = jinja2process(
-                flines, fdir, template_vars
+                fpath, flines, fdir, template_vars
             )
 
     # concatenate continuation lines
@@ -456,8 +460,8 @@ def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
 
 
 def hashbang_and_plugin_templating_clash(
-    templating: str, flines: List[str]
-) -> Optional[str]:
+    templating: str, flines: t.List[str]
+) -> t.Optional[str]:
     """Return file's hashbang/shebang, but raise TemplateVarLanguageClash
     if plugin-set template engine and hashbang do not match.
 
@@ -484,7 +488,7 @@ def hashbang_and_plugin_templating_clash(
                 ...
             cylc.flow.parsec.exceptions.TemplateVarLanguageClash: ...
     """
-    hashbang: Optional[str] = None
+    hashbang: t.Optional[str] = None
     # Get hashbang if possible:
     if flines:
         match = re.match(r'^#!(\S+)', flines[0])
@@ -502,7 +506,12 @@ def hashbang_and_plugin_templating_clash(
     return hashbang
 
 
-def parse(fpath, output_fname=None, template_vars=None, opts=None):
+def parse(
+    fpath: str,
+    output_fname: t.Optional[str] = None,
+    template_vars: t.Optional[t.Dict[str, t.Any]] = None,
+    opts: t.Any = None,
+) -> OrderedDictWithDefaults:
     """Parse file items line-by-line into a corresponding nested dict."""
 
     # read and process the file (jinja2, include-files, line continuation)
@@ -514,7 +523,7 @@ def parse(fpath, output_fname=None, template_vars=None, opts=None):
 
     nesting_level = 0
     config = OrderedDictWithDefaults()
-    parents = []
+    parents: t.List[str] = []
 
     maxline = len(flines) - 1
     index = -1

cylc/flow/parsec/jinja2support.py

@@ -19,13 +19,14 @@
 """
 
 from contextlib import suppress
+from glob import glob
 import importlib
 import os
 import pkgutil
 import re
 import sys
 import traceback
-from glob import glob
+import typing as t
 
 from jinja2 import (
     BaseLoader,
@@ -39,7 +40,9 @@
 from cylc.flow import LOG
 from cylc.flow.parsec.exceptions import Jinja2Error
 
-TRACEBACK_LINENO = re.compile(r'(\s+)?File "<template>", line (\d+)')
+TRACEBACK_LINENO = re.compile(
+    r'\s+?File "(?P<file>.*)", line (?P<line>\d+), in .*template'
+)
 CONTEXT_LINES = 3
 
 
@@ -189,22 +192,65 @@ def jinja2environment(dir_=None):
     return env
 
 
-def get_error_location():
-    """Extract template line number from end of traceback.
+def get_error_lines(
+    base_template_file: str,
+    template_lines: t.List[str],
+) -> t.Dict[str, t.List[str]]:
+    """Extract exception lines from Jinja2 tracebacks.
 
     Returns:
-        int: The line number or None if not found.
+        {filename: [exception_line, ...]}
+
+        There may be multiple entries due to {% include %} statements.
 
     """
+    ret = {}
     for line in reversed(traceback.format_exc().splitlines()):
         match = TRACEBACK_LINENO.match(line)
+        lines = None
         if match:
-            return int(match.groups()[1])
-    return None
+            filename = match.groupdict()['file']
+            lineno = int(match.groupdict()['line'])
+            start_line = max(lineno - CONTEXT_LINES, 0)
+            if filename == '<template>':
+                filename = base_template_file
+                lineno += 1  # shebang line ignored by jinja2
+                lines = template_lines[start_line:lineno]
+            else:
+                with suppress(FileNotFoundError):  # noqa: SIM117
+                    with open(filename, 'r') as jinja2_file:
+                        jinja2_file.seek(start_line, 0)
+                        lines = [
+                            # use splitlines to remove the newline char at the
+                            # end of the line
+                            jinja2_file.readline().splitlines()[0]
+                            for _ in range(CONTEXT_LINES)
+                        ]
+            if lines:
+                ret[filename] = lines
+
+    return ret
+
+
+def jinja2process(
+    fpath: str,
+    flines: t.List[str],
+    dir_: str,
+    template_vars: t.Dict[str, t.Any] = None,
+) -> t.List[str]:
+    """Pass configure file through Jinja2 processor.
+
+    Args:
+        fpath:
+            The path to the root template file (i.e. the flow.cylc file)
+        flines:
+            List of template lines to process.
+        dir_:
+            The path to the configuration directory.
+        template_vars:
+            Dictionary of template variables.
 
-
-def jinja2process(flines, dir_, template_vars=None):
-    """Pass configure file through Jinja2 processor."""
+    """
     # Load file lines into a template, excluding '#!jinja2' so that
     # '#!cylc-x.y.z' rises to the top. Callers should handle jinja2
     # TemplateSyntaxerror and TemplateError.
@@ -240,18 +286,17 @@ def jinja2process(flines, dir_, template_vars=None):
                 lines = []
                 for _ in range(CONTEXT_LINES):
                     lines.append(include_file.readline().splitlines()[0])
-        if lines:
-            # extract context lines from source lines
-            lines = lines[max(exc.lineno - CONTEXT_LINES, 0):exc.lineno]
 
-        raise Jinja2Error(exc, lines=lines, filename=filename)
+        raise Jinja2Error(
+            exc,
+            lines=get_error_lines(fpath, flines),
+            filename=filename
+        )
     except Exception as exc:
-        lineno = get_error_location()
-        lines = None
-        if lineno:
-            lineno += 1  # shebang line ignored by jinja2
-            lines = flines[max(lineno - CONTEXT_LINES, 0):lineno]
-        raise Jinja2Error(exc, lines=lines)
+        raise Jinja2Error(
+            exc,
+            lines=get_error_lines(fpath, flines),
+        )
 
     flow_config = []
     for line in lines: