refactor(workflows): let the evaluator report its own leaves (#4274)

src/specify_cli/workflows/expressions.py

@@ -9,6 +9,7 @@
 
 import json
 import re
+from contextvars import ContextVar
 from typing import Any
 
 
@@ -134,6 +135,15 @@ def _filter_from_json(value: Any) -> Any:
 _EXPR_PATTERN = re.compile(r"\{\{(.+?)\}\}")
 
 
+# The one definition of an indexed path segment. _resolve_dot_path matches
+# against it, and the condition gate below reuses it rather than describing the
+# same shape a second time, so widening what indexing accepts cannot leave the
+# evaluator and the gate disagreeing.
+_INDEXED_SEGMENT = re.compile(r"^([\w-]+)\[(\d+)\]$")
+
+_PLAIN_SEGMENT = re.compile(r"^[\w-]+$")
+
+
 def _resolve_dot_path(obj: Any, path: str) -> Any:
     """Resolve a dotted path like ``steps.specify.output.file`` against *obj*.
 
@@ -143,7 +153,7 @@ def _resolve_dot_path(obj: Any, path: str) -> Any:
     current = obj
     for part in parts:
         # Handle list indexing: name[0]
-        idx_match = re.match(r"^([\w-]+)\[(\d+)\]$", part)
+        idx_match = _INDEXED_SEGMENT.match(part)
         if idx_match:
             key, idx = idx_match.group(1), int(idx_match.group(2))
             if isinstance(current, dict):
@@ -479,6 +489,11 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An
 # evaluator will actually split on.
 _COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ")
 
+# Set only while `_collect_leaves` probes an expression; None everywhere else, so
+# a normal evaluation costs one `.get()`. A ContextVar rather than a module global
+# so concurrent probes cannot append into each other's list.
+_leaf_sink: ContextVar[list[str] | None] = ContextVar("_leaf_sink", default=None)
+
 
 def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
     """Evaluate a simple expression against the namespace.
@@ -627,7 +642,12 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
         ]
         return items
 
-    # Variable reference (dot-path)
+    # Variable reference (dot-path). This is the one place a substring stops being
+    # grammar and becomes a name to resolve, so it is where a probe can learn what
+    # the evaluator will actually look up. Literals have all returned above.
+    sink = _leaf_sink.get()
+    if sink is not None:
+        sink.append(expr)
     return _resolve_dot_path(namespace, expr)
 
 
@@ -1047,8 +1067,9 @@ def _has_incomplete_operand(text: str) -> bool:
 # None, so a correction built on one turns a truthy condition false.
 _NAMESPACE_ROOTS = ("inputs", "steps", "item", "fan_in", "context")
 
-# Exactly what _resolve_dot_path accepts: a name, optionally one numeric index.
-_PATH_SEGMENT = re.compile(r"^[\w-]+(\[\d+\])?$")
+def _is_path_segment(segment: str) -> bool:
+    """Whether _resolve_dot_path can walk *segment*: a name, or a name it indexes."""
+    return bool(_PLAIN_SEGMENT.match(segment) or _INDEXED_SEGMENT.match(segment))
 
 
 class _ProbeNamespace(dict):
@@ -1097,123 +1118,51 @@ def _evaluator_rejects(text: str) -> str | None:
 
 
 
-def _looks_numeric(text: str) -> bool:
-    """Mirror the evaluator's numeric literal test exactly.
+def _collect_leaves(text: str) -> list[str]:
+    """Every substring *text* hands to the evaluator as a name to resolve.
 
-    `_evaluate_simple_expression` only calls `float()` when a `.` is present and
-    `int()` otherwise, so `1e3` is not a number to it -- it falls through to a path
-    lookup and resolves to None. A bare `float()` here accepted `1e3` and the
-    correction turned a truthy condition false.
-    """
-    try:
-        if "." in text:
-            float(text)
-        else:
-            int(text)
-    except (ValueError, TypeError):
-        return False
-    return True
+    Runs the same probe ``_evaluator_rejects`` uses, with the leaf sink armed.
+    Literals never reach the dot-path resolution, and operands, filter arguments
+    and list elements all do -- ``_evaluate_simple_expression`` evaluates both
+    sides of ``or``/``and`` eagerly rather than short-circuiting, so a leaf is
+    recorded whatever the other side is worth.
 
-
-def _is_literal(text: str) -> bool:
-    """Mirror the evaluator's literal tests exactly.
-
-    The string case is the opening quote's *matching close being the final
-    character*, not first/last-character equality: `'a' 'b'` passes the latter but
-    is two literals to the evaluator, which falls through to a path lookup.
+    A probe run can still raise on its own placeholder values, which is what
+    ``_evaluator_rejects`` sorts out. The leaves seen before that point are real
+    -- the evaluator reached them -- so they are kept rather than discarded:
+    ``inputs.tags | join(bogus)`` records ``bogus`` and only then trips over the
+    placeholder handed to ``join``.
     """
-    if text[:1] in ("'", '"') and text.find(text[0], 1) == len(text) - 1:
-        return True
-    return text.lower() in ("true", "false", "none", "null") or _looks_numeric(text)
-
+    leaves: list[str] = []
+    token = _leaf_sink.set(leaves)
+    try:
+        _evaluate_simple_expression(
+            text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS}
+        )
+    except Exception:  # noqa: BLE001 - probe values, reported by _evaluator_rejects
+        pass
+    finally:
+        _leaf_sink.reset(token)
+    return leaves
 
-def _unresolvable_term(text: str) -> str | None:
-    """The first operand in *text* the evaluator cannot resolve, or ``None``.
 
-    Walks operands the way ``_evaluate_simple_expression`` does -- filters, then
-    ``or``/``and``/``not``, then comparisons -- and checks each leaf. A leaf must be
-    a literal or a dotted path rooted in ``_NAMESPACE_ROOTS``.
+def _unresolvable_leaf(leaf: str) -> str | None:
+    """Why the evaluator cannot resolve the name *leaf*, or ``None``.
 
-    Enumerating broken shapes is what made this take several rounds: each new gate
-    only knew the shapes named so far. ``inputs.a === inputs.b`` split cleanly on
-    ``==`` and looked complete, while the evaluator read ``= inputs.b`` as a path
-    and resolved it to ``None``; ``bogus == 'x'`` passed for the same reason one
-    level up. Recursing to the leaves covers both without naming either.
+    Namespace knowledge only. Everything about where operands live now comes from
+    the evaluator itself, so nothing here restates the grammar.
     """
-    stripped = text.strip()
-    if not stripped:
-        return "an operand is empty"
-
-    if _find_top_level(stripped, "|") != -1:
-        segments = _split_top_level(stripped, "|")
-        reason = _unresolvable_term(segments[0])
-        if reason is not None:
-            return reason
-        # A filter argument is an ordinary operand to `_apply_filter`, which
-        # evaluates it with `_evaluate_simple_expression` like any other. Skipping
-        # it let `inputs.tags | join(bogus)` be offered as paste-ready: `bogus` is
-        # no namespace root, resolves to None, and the wrapped form then raises
-        # `join: expected a string separator, got NoneType`. Parse with the same
-        # pattern `_apply_filter` uses, so a form this does not recognize is left
-        # to the evaluator probe rather than guessed at here.
-        for segment in segments[1:]:
-            match = re.fullmatch(r"(\w+)\((.+)\)", segment.strip())
-            if match is None:
-                continue
-            reason = _unresolvable_term(match.group(2))
-            if reason is not None:
-                return reason
-        return None
-
-    for op in (" or ", " and "):
-        idx = _find_top_level(stripped, op)
-        if idx != -1:
-            return _unresolvable_term(stripped[:idx]) or _unresolvable_term(
-                stripped[idx + len(op):]
-            )
-
-    if stripped.startswith("not "):
-        return _unresolvable_term(stripped[4:])
-
-    for op in _COMPARISON_OPERATORS:
-        idx = _find_top_level(stripped, op)
-        if idx != -1:
-            return _unresolvable_term(stripped[:idx]) or _unresolvable_term(
-                stripped[idx + len(op):]
-            )
-
-    if _is_literal(stripped):
-        return None
-
-    # A list literal is a term the evaluator understands, and it recurses into the
-    # elements rather than resolving the brackets as a name. Not mirroring that
-    # denied the correction to `inputs.tag in ['x', 'y']` -- a condition wrapping
-    # repairs completely -- while reporting the list as an unresolvable name. The
-    # empty-segment skip matches `_evaluate_simple_expression`, which drops them so
-    # `[1, 2,]` is `[1, 2]` rather than `[1, 2, None]`.
-    if stripped.startswith("[") and stripped.endswith("]"):
-        inner = stripped[1:-1].strip()
-        if not inner:
-            return None
-        for element in _split_top_level_commas(inner):
-            if not element.strip():
-                continue
-            reason = _unresolvable_term(element)
-            if reason is not None:
-                return reason
-        return None
-
-    segments = _split_top_level(stripped, ".")
-    if not _PATH_SEGMENT.match(segments[0].strip()):
-        return f"{stripped!r} is not a name the evaluator can resolve"
+    segments = _split_top_level(leaf, ".")
+    if not _is_path_segment(segments[0].strip()):
+        return f"{leaf!r} is not a name the evaluator can resolve"
     # `item` is the only root that is not always a mapping: `StepContext.item` is
     # `Any` and a fan-out assigns the item value itself, so when that value is a
     # list `_resolve_dot_path` indexes it and `item[0] == 'x'` resolves. Every
     # other root comes back from `_build_namespace` as a mapping, and the index
     # branch returns None for those however it is written -- so the index is
     # stripped for `item` alone rather than for roots in general.
     root = segments[0].strip()
-    indexed_root = re.fullmatch(r"([\w-]+)\[\d+\]", root)
+    indexed_root = _INDEXED_SEGMENT.match(root)
     if indexed_root is not None and indexed_root.group(1) == "item":
         root = indexed_root.group(1)
     if root not in _NAMESPACE_ROOTS:
@@ -1222,11 +1171,38 @@ def _unresolvable_term(text: str) -> str | None:
             f"({', '.join(_NAMESPACE_ROOTS)})"
         )
     for segment in segments[1:]:
-        if not _PATH_SEGMENT.match(segment.strip()):
+        if not _is_path_segment(segment.strip()):
             return f"{segment.strip()!r} is not a valid path segment"
     return None
 
 
+def _unresolvable_term(text: str) -> str | None:
+    """The first name in *text* the evaluator cannot resolve, or ``None``.
+
+    Asks the evaluator which names it will look up, then applies the namespace
+    rules to those. The previous implementation derived the names itself by
+    re-walking the grammar -- filters, then ``or``/``and``/``not``, then
+    comparisons, then list literals -- and had to be kept in step with
+    ``_evaluate_simple_expression`` by hand.
+
+    That is what made this take several rounds: each round fixed one shape the
+    walk disagreed about (``inputs.a === inputs.b``, ``bogus == 'x'``, list
+    elements, filter arguments, a newline before ``and``) and nothing stopped the
+    next one. Reading the leaves off the evaluator removes the class rather than
+    another instance of it: the two cannot disagree about where the operands are
+    when only one of them decides.
+    """
+    stripped = text.strip()
+    if not stripped:
+        return "an operand is empty"
+
+    for leaf in _collect_leaves(stripped):
+        reason = _unresolvable_leaf(leaf)
+        if reason is not None:
+            return reason
+    return None
+
+
 def _wrapping_would_not_repair(core: str) -> str | None:
     """Why wrapping *core* in ``{{ }}`` would not yield the expression intended.