From b059b21461df0f52d47be0df6d294f8530a6c577 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 11:10:49 -0700 Subject: [PATCH 01/14] Parse attribute access This has still a critical flaw when parsing attributes of the same name from two different objects. I'm going to commit as-is and then see if Claude will work with this more like human review process. Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/compiler/statements.py | 57 +++- src/flowrep/compiler/sugar.py | 63 ++++ src/flowrep/parsers/attribute_parser.py | 112 +++++++ src/flowrep/parsers/case_helpers.py | 2 + src/flowrep/parsers/parser_helpers.py | 25 +- src/flowrep/parsers/symbol_scope.py | 13 +- src/flowrep/parsers/workflow_parser.py | 102 ++++++- tests/flowrep_static/library.py | 18 +- .../parsers/test_parsing_attribute_access.py | 86 ++++++ .../parsers/test_parsing_reassignment.py | 12 +- tests/unit/compiler/test_compiler.py | 235 ++++++++++++++- tests/unit/parsers/test_attribute_parser.py | 183 ++++++++++++ tests/unit/parsers/test_symbol_scope.py | 36 +++ tests/unit/parsers/test_workflow_parser.py | 275 +++++++++++++++++- 14 files changed, 1194 insertions(+), 25 deletions(-) create mode 100644 src/flowrep/compiler/sugar.py create mode 100644 src/flowrep/parsers/attribute_parser.py create mode 100644 tests/integration/parsers/test_parsing_attribute_access.py create mode 100644 tests/unit/parsers/test_attribute_parser.py diff --git a/src/flowrep/compiler/statements.py b/src/flowrep/compiler/statements.py index af7fcf85..39c1c676 100644 --- a/src/flowrep/compiler/statements.py +++ b/src/flowrep/compiler/statements.py @@ -7,7 +7,7 @@ from pyiron_snippets import versions from flowrep import base_models, edge_models, subgraph_validation -from flowrep.compiler import flow_control, function +from flowrep.compiler import flow_control, function, sugar from flowrep.prospective import ( atomic_recipe, constant_recipe, @@ -252,6 +252,52 @@ def resolve(source) -> str: ) required_by_handle[handle] = name + def _obj_source( + label: str, + ) -> edge_models.SourceHandle | edge_models.InputSource | None: + target = edge_models.TargetHandle(node=label, port=sugar.OBJ_PORT) + return recipe.input_edges.get(target) or recipe.edges.get(target) + + def _obj_is_symbolic(label: str) -> bool: + """True if the getattr's object resolves to a symbol we can write '.' after. + + An object fed by an *inlined* constant would sugar to nonsense (``5 .a``), so + such a node keeps the plain-call emission. The parser cannot produce one. + """ + source = _obj_source(label) + if source is None: + return False + if isinstance(source, edge_models.InputSource): + return True + peer = recipe.nodes[source.node] + if isinstance(peer, constant_recipe.ConstantRecipe): + return source.node in materialized_constants + return True + + # A recognised getattr node is emitted as attribute syntax -- `sym = obj.attr` + # -- always as its own statement, never inlined into a consumer's argument list. + # + # Inlining would be prettier (`f(dc.a)`, `dc.a.val`) but it is not safe. The + # parser injects a getattr node together with a `ConstantRecipe` peer carrying the + # attribute name, and constant labels come from one shared `constant_N` counter + # over the whole workflow. Inlining moves a getattr's creation to its consumer's + # statement, which reorders that name-constant relative to every *other* constant + # in the workflow (e.g. a literal call argument) and permutes the counter. The + # recipe would still execute identically, but it would not re-parse to an equal + # one. Emitting one statement per getattr, in topological order, reproduces the + # parser's injection order exactly, so the round trip is exact. + # + # Materialising also gives the efficiency guarantee for free: one node, one + # statement, one symbol -- a getattr consumed by several nodes stays a single + # node on re-parse rather than being duplicated into each consumer. + sugared: dict[str, str] = { + label: attr_name + for label, node in recipe.nodes.items() + if sugar.is_std_getattr(node) + and (attr_name := sugar.attribute_name(label, recipe)) is not None + and _obj_is_symbolic(label) + } + for label in _topological_nodes(recipe): node = recipe.nodes[label] if isinstance(node, constant_recipe.ConstantRecipe): @@ -265,6 +311,15 @@ def resolve(source) -> str: lines.append(f"{name} = {repr(node.constant)}") continue + if label in sugared: + name = required_by_handle.get((label, sugar.ATTR_PORT)) or alloc.fresh( + _output_name_suggestion(label, sugar.ATTR_PORT, 1) + ) + produced[(label, sugar.ATTR_PORT)] = name + obj = resolve(_obj_source(label)) + lines.append(f"{name} = {obj}.{sugared[label]}") + continue + call_path = node_call_path(node, label, emitter, alloc) if call_path is not None: lhs_syms = _allocate_outputs( diff --git a/src/flowrep/compiler/sugar.py b/src/flowrep/compiler/sugar.py new file mode 100644 index 00000000..f472074f --- /dev/null +++ b/src/flowrep/compiler/sugar.py @@ -0,0 +1,63 @@ +from __future__ import annotations + +import keyword +from typing import Any, cast + +from flowrep import edge_models +from flowrep.prospective import ( + atomic_recipe, + constant_recipe, + std, + union_types, + workflow_recipe, +) + +OBJ_PORT = "obj" +NAME_PORT = "name" +ATTR_PORT = "attr" + +# `LabeledRecipe.recipe` is a discriminated union; the cast narrows it for mypy. +_GETATTR_FQN = cast( + atomic_recipe.AtomicRecipe, std.getattr_.recipe +).reference.info.fully_qualified_name + + +def is_std_getattr(node: union_types.RecipeDiscrimination) -> bool: + """True if *node* is the standard-library attribute-access recipe. + + Matched on the referenced function's fully qualified name rather than on + ``VersionInfo`` equality, so a recipe serialised under a different flowrep + version is still recognised. + """ + return ( + isinstance(node, atomic_recipe.AtomicRecipe) + and node.reference.info.fully_qualified_name == _GETATTR_FQN + and node.inputs == [OBJ_PORT, NAME_PORT] + and node.outputs == [ATTR_PORT] + ) + + +def is_attribute_syntax(value: Any) -> bool: + """True if *value* can be written after a ``.`` in Python source. + + Deliberately laxer than :func:`base_models.is_valid_label`, which also + excludes the reserved port names ``inputs``/``outputs``. Those are perfectly + legal attributes, and ``dc.inputs`` must compile back to ``dc.inputs``. + """ + return ( + isinstance(value, str) and value.isidentifier() and not keyword.iskeyword(value) + ) + + +def attribute_name(label: str, recipe: workflow_recipe.WorkflowRecipe) -> str | None: + """The identifier fed to *label*'s ``name`` port by a constant peer, else None.""" + source = recipe.edges.get(edge_models.TargetHandle(node=label, port=NAME_PORT)) + if source is None: + return None + peer = recipe.nodes.get(source.node) + if not isinstance(peer, constant_recipe.ConstantRecipe): + return None + constant = peer.constant + if isinstance(constant, str) and is_attribute_syntax(constant): + return constant + return None diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py new file mode 100644 index 00000000..667a1463 --- /dev/null +++ b/src/flowrep/parsers/attribute_parser.py @@ -0,0 +1,112 @@ +from __future__ import annotations + +import ast + +from flowrep import edge_models +from flowrep.parsers import constant_parser, label_helpers, symbol_scope +from flowrep.prospective import std, union_types + +_OBJ_PORT = "obj" +_NAME_PORT = "name" +_ATTR_PORT = "attr" + + +def chain_root(node: ast.expr) -> ast.Name | None: + """The innermost ``ast.Name`` of a pure Attribute/Name chain, else ``None``.""" + while isinstance(node, ast.Attribute): + node = node.value + return node if isinstance(node, ast.Name) else None + + +def is_data_attribute(node: ast.expr, symbol_map: symbol_scope.SymbolScope) -> bool: + """True if *node* is attribute access rooted at a symbol known to the graph. + + The symbol map deliberately shadows the object scope: a workflow input named + ``os`` makes ``os.path`` a data attribute access on that input, not a module + lookup, exactly as it would be at runtime. + """ + if not isinstance(node, ast.Attribute): + return False + root = chain_root(node) + return root is not None and root.id in symbol_map + + +def attribute_name(node: ast.Attribute) -> str: + """The outermost attribute name, e.g. ``dc.a.val`` -> ``"val"``.""" + return node.attr + + +def inject_attribute_chain( + node: ast.expr, + symbol_map: symbol_scope.SymbolScope, + nodes: union_types.Recipes, +) -> edge_models.SourceHandle: + """Add one ``std.getattr_`` node (plus its name constant) per link of *node*. + + Walks the chain root-outward so that node insertion order -- and therefore + labels and the enclosing scope's input ordering -- are a deterministic + function of the source. Returns the handle of the outermost link's ``attr`` + output. + + *node* must be a data attribute chain (see :func:`is_data_attribute`); callers + are expected to gate on that before calling. + """ + links: list[ast.Attribute] = [] + current: ast.expr = node + while isinstance(current, ast.Attribute): + links.append(current) + current = current.value + links.reverse() + + root = chain_root(node) + if root is None: # pragma: no cover - callers gate on is_data_attribute + raise TypeError(f"Not an attribute chain rooted at a symbol: {ast.dump(node)}") + if root.id in symbol_map.all_accumulators: + raise ValueError( + f"Cannot take an attribute of accumulator '{root.id}'. Accumulators are " + f"name-tracked list declarations, not graph data; append to it and take " + f"attributes of the resulting collection outside the loop instead." + ) + + handle: edge_models.SourceHandle | None = None + for link in links: + label = label_helpers.unique_suffix(f"getattr_{link.attr}", nodes) + nodes[label] = std.getattr_.recipe + if handle is None: + symbol_map.consume(root.id, label, _OBJ_PORT) + else: + symbol_map.consume_source(handle, label, _OBJ_PORT) + constant_parser.inject_constant(nodes, symbol_map, link.attr, label, _NAME_PORT) + handle = edge_models.SourceHandle(node=label, port=_ATTR_PORT) + assert handle is not None # links is non-empty: node is an ast.Attribute + return handle + + +def hoist_call_arguments( + call: ast.Call, + symbol_map: symbol_scope.SymbolScope, + nodes: union_types.Recipes, +) -> dict[ast.expr, edge_models.SourceHandle]: + """Inject every data-attribute argument of *call* before the call's own node. + + Hoisting is what makes ``f(x0, comp.val)`` and ``v = comp.val; f(x0, v)`` parse + to identical recipes: the getattr nodes are created (and their sources consumed) + ahead of the call in both forms. + """ + hoisted: dict[ast.expr, edge_models.SourceHandle] = {} + arguments = list(call.args) + [kw.value for kw in call.keywords] + for argument in arguments: + if is_data_attribute(argument, symbol_map): + hoisted[argument] = inject_attribute_chain(argument, symbol_map, nodes) + return hoisted + + +def reject_method_call(call: ast.Call, symbol_map: symbol_scope.SymbolScope) -> None: + """Raise if *call* invokes an attribute of a known symbol (``dc.method(x)``).""" + if is_data_attribute(call.func, symbol_map): + chain = ast.unparse(call.func) + raise ValueError( + f"Workflow python definitions cannot call methods on workflow data: " + f"'{chain}(...)'. Bind the attribute to a symbol and call it via a " + f"node, or wrap the method call in an @atomic function." + ) diff --git a/src/flowrep/parsers/case_helpers.py b/src/flowrep/parsers/case_helpers.py index 3fef73c7..c79b3658 100644 --- a/src/flowrep/parsers/case_helpers.py +++ b/src/flowrep/parsers/case_helpers.py @@ -8,6 +8,7 @@ from flowrep import edge_models, subgraph_validation from flowrep.parsers import ( atomic_parser, + attribute_parser, object_scope, parser_helpers, parser_protocol, @@ -39,6 +40,7 @@ def parse_case( raise ValueError( "Test conditions must be a function call, but got " f"{type(test).__name__}" ) + attribute_parser.reject_method_call(test, symbol_map) condition = atomic_parser.get_labeled_recipe(test, set(), scope, info_factory) if len(condition.recipe.outputs) != 1: diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index ef91c0e3..df574d12 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -9,7 +9,12 @@ from typing import Any, TypeVar, cast from flowrep import base_models, edge_models -from flowrep.parsers import constant_parser, label_helpers, symbol_scope +from flowrep.parsers import ( + attribute_parser, + constant_parser, + label_helpers, + symbol_scope, +) from flowrep.prospective import constant_recipe, helper_models, union_types @@ -158,6 +163,7 @@ def consume_call_arguments( *, condition_bindings: dict[str, constant_recipe.ConstantRecipe] | None = None, reserved_ports: set[str] | None = None, + hoisted: dict[ast.expr, edge_models.SourceHandle] | None = None, ) -> None: """Record all argument->port consumptions for a node-creating call. @@ -171,13 +177,30 @@ def consume_call_arguments( constant peer inside it, so the peer lives one level up. *reserved_ports* carries synthetic port names already allocated for this flow-control chain so that multiple literals -- across an if/elif chain -- get distinct, deterministic ports. + + Data-attribute arguments are injected ahead of the call by + ``attribute_parser.hoist_call_arguments`` and arrive here in *hoisted*, mapping + the argument's AST node to the ``SourceHandle`` of its outermost getattr node. In + condition mode there is no room for a peer getattr node inside a flow-control + condition, so a data-attribute argument is rejected instead. """ reserved = set() if reserved_ports is None else reserved_ports + already_hoisted = {} if hoisted is None else hoisted def _consume(arg_node: ast.expr, consumer_port: str) -> None: + if (handle := already_hoisted.get(arg_node)) is not None: + scope.consume_source(handle, child.label, consumer_port) + return if isinstance(arg_node, ast.Name): scope.consume(arg_node.id, child.label, consumer_port) return + if attribute_parser.is_data_attribute(arg_node, scope): + raise ValueError( + f"Attribute access on workflow data is not supported in flow-control " + f"condition arguments; for input '{consumer_port}' of condition node " + f"'{child.label}' found '{ast.unparse(arg_node)}'. Bind it to a " + f"symbol before the flow control and pass that symbol instead." + ) is_literal, value = constant_parser.try_parse_constant(arg_node) if not is_literal: raise TypeError( diff --git a/src/flowrep/parsers/symbol_scope.py b/src/flowrep/parsers/symbol_scope.py index f93920fe..ffbbfd2a 100644 --- a/src/flowrep/parsers/symbol_scope.py +++ b/src/flowrep/parsers/symbol_scope.py @@ -266,10 +266,21 @@ def consume_input_source( def produce(self, output_port: str, symbol: str | None = None) -> None: """Record that `output_port` is sourced from `symbol`.""" produced_symbol = output_port if symbol is None else symbol + self.produce_source(output_port, self[produced_symbol]) + + def produce_source( + self, + output_port: str, + source: edge_models.SourceHandle | edge_models.InputSource, + ) -> None: + """Record a production whose source is a fixed ``SourceHandle`` (e.g. an + injected getattr node), bypassing symbol lookup so no synthetic symbol + enters ``_sources`` and risks colliding with a user symbol. The + production twin of :meth:`consume_source`.""" if any(p.output_port == output_port for p in self._productions): raise ValueError(f"Output port '{output_port}' already produced.") self._productions.append( - SymbolProduction(output_port=output_port, source=self[produced_symbol]) + SymbolProduction(output_port=output_port, source=source) ) def produce_symbols(self, symbols: list[str]) -> None: diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index 29dd3924..f46d7e5a 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -12,6 +12,7 @@ from flowrep import base_models, edge_models from flowrep.parsers import ( atomic_parser, + attribute_parser, constant_parser, for_parser, if_parser, @@ -271,6 +272,10 @@ def _handle_assign(self, body: ast.Assign | ast.AnnAssign): rhs = body.value if isinstance(rhs, ast.Call): + attribute_parser.reject_method_call(rhs, self.symbol_map) + hoisted = attribute_parser.hoist_call_arguments( + rhs, self.symbol_map, self.nodes + ) child = atomic_parser.get_labeled_recipe( rhs, self.nodes.keys(), @@ -279,7 +284,7 @@ def _handle_assign(self, body: ast.Assign | ast.AnnAssign): ) self.nodes[child.label] = child.recipe parser_helpers.consume_call_arguments( - self.symbol_map, rhs, child, self.nodes + self.symbol_map, rhs, child, self.nodes, hoisted=hoisted ) self.symbol_map.register(new_symbols, child) elif isinstance(rhs, ast.List) and len(rhs.elts) == 0: @@ -289,6 +294,23 @@ def _handle_assign(self, body: ast.Assign | ast.AnnAssign): f"got {new_symbols}" ) self.symbol_map.register_accumulator(new_symbols[0]) + elif rhs is not None and attribute_parser.is_data_attribute( + rhs, self.symbol_map + ): + if len(new_symbols) != 1: + raise ValueError( + f"Attribute-access assignment must target exactly one symbol, " + f"got {new_symbols}" + ) + handle = attribute_parser.inject_attribute_chain( + rhs, self.symbol_map, self.nodes + ) + self.symbol_map.register( + new_symbols, + helper_models.LabeledRecipe( + label=handle.node, recipe=self.nodes[handle.node] + ), + ) elif rhs is not None and (parsed := constant_parser.try_parse_constant(rhs))[0]: if len(new_symbols) != 1: raise ValueError( @@ -316,8 +338,9 @@ def _handle_assign(self, body: ast.Assign | ast.AnnAssign): else: raise ValueError( f"Workflow python definitions can only interpret assignments with " - f"a call or empty list on the right-hand-side, but ast found " - f"{type(rhs)}" + f"a call, empty list, literal constant, symbol alias, or attribute " + f"access rooted at a known workflow symbol on the right-hand-side, " + f"but ast found {type(rhs)}" ) def _digest_flow_control( @@ -417,7 +440,16 @@ def _handle_appending_to_accumulator(self, append_call: ast.Call) -> None: used_accumulator = cast( ast.Name, cast(ast.Attribute, append_call.func).value ).id - appended_symbol = cast(ast.Name, append_call.args[0]).id + appended = append_call.args[0] + if attribute_parser.is_data_attribute(appended, self.symbol_map): + handle = attribute_parser.inject_attribute_chain( + appended, self.symbol_map, self.nodes + ) + port = attribute_parser.attribute_name(cast(ast.Attribute, appended)) + self.symbol_map.use_accumulator(used_accumulator, port) + self.symbol_map.produce_source(port, handle) + return + appended_symbol = cast(ast.Name, appended).id self.symbol_map.use_accumulator(used_accumulator, appended_symbol) appended_source = self.symbol_map[appended_symbol] if isinstance(appended_source, edge_models.SourceHandle): @@ -471,37 +503,75 @@ def handle_return( func: types.FunctionType, output_labels: Collection[str], ) -> None: - returned_symbols = parser_helpers.resolve_symbols_to_strings(body.value) + elements = _return_elements(body.value) + port_name_candidates = [ + _return_port_candidate(elt, self.symbol_map) for elt in elements + ] base_models.validate_unique( - returned_symbols, + port_name_candidates, message=f"Workflow python definitions must have unique returns, but " - f"got duplicates in: {returned_symbols}", + f"got duplicates in: {port_name_candidates}", ) annotated_returns = label_helpers.get_annotated_output_labels(func) scraped_labels = label_helpers.merge_labels( first_choice=annotated_returns, - fallback=returned_symbols, + fallback=port_name_candidates, message_prefix="Annotation labels and returned symbols mis-match. ", ) - if output_labels and len(output_labels) != len(returned_symbols): + if output_labels and len(output_labels) != len(elements): raise ValueError( f"When output_labels are specified ({output_labels}), workflow " f"python definitions have a matching number of returned symbols " - f"({returned_symbols})." + f"({port_name_candidates})." ) final_ports = list(output_labels) if output_labels else scraped_labels - for symbol, port in zip(returned_symbols, final_ports, strict=True): - if symbol not in self.symbol_map: - raise ValueError( - f"Return symbol '{symbol}' is not defined. " - f"Available: {list(self.symbol_map)}" + for elt, port in zip(elements, final_ports, strict=True): + if isinstance(elt, ast.Name): + if elt.id not in self.symbol_map: + raise ValueError( + f"Return symbol '{elt.id}' is not defined. " + f"Available: {list(self.symbol_map)}" + ) + self.symbol_map.produce(port, elt.id) + else: + handle = attribute_parser.inject_attribute_chain( + elt, self.symbol_map, self.nodes ) + self.symbol_map.produce_source(port, handle) + + +def _return_elements(node: ast.expr | None) -> list[ast.expr]: + """The returned expressions: a single expression, or a tuple's elements.""" + if node is None: + raise TypeError("Workflow python definitions must return a value.") + return list(node.elts) if isinstance(node, ast.Tuple) else [node] + - self.symbol_map.produce(port, symbol) +def _return_port_candidate(elt: ast.expr, symbol_map: symbol_scope.SymbolScope) -> str: + """The output-port name candidate for one returned element. + + A returned symbol contributes its own name; a data attribute chain contributes + its final attribute name. Anything else raises ``TypeError``. + + Deliberately does *not* delegate to + :func:`parser_helpers.resolve_symbols_to_strings` here: that helper also + accepts a bare ``ast.Tuple`` of names (it is built for the top-level + ``body.value``), which would silently succeed on a nested-tuple return + element -- e.g. ``return (a, b), c`` -- instead of raising. + """ + if isinstance(elt, ast.Name): + return elt.id + if attribute_parser.is_data_attribute(elt, symbol_map): + return attribute_parser.attribute_name(cast(ast.Attribute, elt)) + raise TypeError( + f"Expected each returned element to be a symbol or an attribute access " + f"rooted at a known workflow symbol, but could not parse this from " + f"{type(elt)}." + ) def is_append_call(node: ast.expr | ast.Expr) -> bool: diff --git a/tests/flowrep_static/library.py b/tests/flowrep_static/library.py index 2dd7babd..5e6ad214 100644 --- a/tests/flowrep_static/library.py +++ b/tests/flowrep_static/library.py @@ -2,7 +2,7 @@ import typing -from flowrep.parsers import atomic_parser, workflow_parser +from flowrep.parsers import atomic_parser, dataclass_parser, workflow_parser def undecorated_identity(x): @@ -149,3 +149,19 @@ def make_list(seed) -> typing.Annotated[list, {"label": "data"}]: @workflow_parser.workflow def macro_identity(x): return x + + +# test_attribute_parser / test_workflow_parser / test_compiler / integration + + +class ComplexData: + """A plain (non-recipe) payload class, reached via attribute access.""" + + def __init__(self, val: int = 0): + self.val = val + + +@dataclass_parser.dataclass +class MyDataclass: + a: ComplexData + x: int = 1 diff --git a/tests/integration/parsers/test_parsing_attribute_access.py b/tests/integration/parsers/test_parsing_attribute_access.py new file mode 100644 index 00000000..66bb4b12 --- /dev/null +++ b/tests/integration/parsers/test_parsing_attribute_access.py @@ -0,0 +1,86 @@ +import unittest + +from flowrep import edge_models, wfms +from flowrep.compiler import source +from flowrep.parsers import workflow_parser + +from flowrep_static import library, makers + + +@workflow_parser.workflow +def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + my_val = dc.a.val + repeated = library.my_mul(dc.x, my_val) + return repeated + + +class TestAttributeAccessEndToEnd(unittest.TestCase): + def test_parses_expected_graph(self): + recipe = wf.flowrep_recipe + + self.assertEqual( + set(recipe.nodes), + { + "MyDataclass_0", + "getattr_a_0", + "getattr_val_0", + "getattr_x_0", + "my_mul_0", + "constant_0", + "constant_1", + "constant_2", + }, + ) + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="getattr_a_0", port="obj")], + edge_models.SourceHandle(node="MyDataclass_0", port="instance"), + ) + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="getattr_val_0", port="obj")], + edge_models.SourceHandle(node="getattr_a_0", port="attr"), + ) + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="getattr_x_0", port="obj")], + edge_models.SourceHandle(node="MyDataclass_0", port="instance"), + ) + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="my_mul_0", port="a")], + edge_models.SourceHandle(node="getattr_x_0", port="attr"), + ) + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="my_mul_0", port="b")], + edge_models.SourceHandle(node="getattr_val_0", port="attr"), + ) + + def test_executes_via_run_recipe(self): + result = wfms.run_recipe( + wf.flowrep_recipe, x0=3, comp=library.ComplexData(val=7) + ) + expected = wf(3, library.ComplexData(val=7)) + self.assertEqual(result.output_ports["repeated"].value, expected) + self.assertEqual(expected, 21) + + def test_round_trips_through_source(self): + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + fn(3, library.ComplexData(val=7)), wf(3, library.ComplexData(val=7)) + ) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_compiled_source_is_sugared(self): + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + # Attribute syntax, one statement per access -- never a call to the + # underlying std wrapper, and never inlined into a consumer. + self.assertRegex(rendered.source, r"\n\s*(\w+) = \w+\.a\n\s*\w+ = \1\.val\n") + self.assertRegex(rendered.source, r"\n\s*\w+ = \w+\.x\n") + self.assertNotIn("_getattr_wrapper", rendered.source) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/parsers/test_parsing_reassignment.py b/tests/integration/parsers/test_parsing_reassignment.py index 42753ed0..ad0d0617 100644 --- a/tests/integration/parsers/test_parsing_reassignment.py +++ b/tests/integration/parsers/test_parsing_reassignment.py @@ -103,13 +103,19 @@ def macro(x): with self.assertRaises(ValueError): _parse(macro) - def test_attribute_rhs_still_raises(self): + def test_attribute_rhs_on_known_symbol_now_injects_getattr(self): + """ + Attribute access rooted at a known workflow symbol is parsed as an + injected ``std.getattr_`` node (see ``flowrep.parsers.attribute_parser``). + """ + def macro(x): y = x.real return y - with self.assertRaises(ValueError): - _parse(macro) + recipe = _parse(macro) + self.assertIn("getattr_real_0", recipe.nodes) + self.assertEqual(recipe.outputs, ["y"]) def test_alias_to_undefined_symbol_raises(self): def macro(x): diff --git a/tests/unit/compiler/test_compiler.py b/tests/unit/compiler/test_compiler.py index ee45a285..2b04f3f9 100644 --- a/tests/unit/compiler/test_compiler.py +++ b/tests/unit/compiler/test_compiler.py @@ -12,7 +12,7 @@ from pyiron_snippets import versions from flowrep import base_models, edge_models, wfms -from flowrep.compiler import flow_control, function, source, statements +from flowrep.compiler import flow_control, function, source, statements, sugar from flowrep.parsers import atomic_parser, workflow_parser from flowrep.prospective import ( atomic_recipe, @@ -20,6 +20,7 @@ for_recipe, helper_models, if_recipe, + std, try_recipe, while_recipe, workflow_recipe, @@ -2275,5 +2276,237 @@ def wf(m): self._round_trip(wf, 0.2) +class TestAttributeSugar(unittest.TestCase): + def test_single_access_as_call_argument(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + r = library.identity(dc.x) + return r + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + self.assertIn(".x", rendered.source) + self.assertNotIn("_getattr_wrapper", rendered.source) + fn = rendered.build() + self.assertEqual(fn(3, library.ComplexData(val=7)), 3) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_chain(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + v = dc.a.val + return v + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + # Each link of the chain is its own statement: a getattr node is never + # inlined into a consumer, because that would move its name-constant + # relative to the workflow's other constants and permute the shared + # `constant_N` counter on re-parse. + self.assertRegex(rendered.source, r"\n\s*(\w+) = \w+\.a\n\s*\w+ = \1\.val\n") + self.assertNotIn(".a.val", rendered.source) + fn = rendered.build() + self.assertEqual(fn(3, library.ComplexData(val=7)), 7) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_access_interleaved_with_a_literal_round_trips(self): + """Regression: a getattr's name-constant must not be reordered. + + Constant labels come from one shared `constant_N` counter over the whole + workflow. Inlining `dc.a` into the `identity` call would defer its name + constant past the literal `3`, swapping `constant_0` and `constant_1` on + re-parse -- functionally identical, structurally different. + """ + + def wf(comp: library.ComplexData, n: int): + dc = library.MyDataclass(comp, n) + v = dc.a + w = library.increment(3) + r = library.identity(v) + return r, w + + free = makers.reference_free(wf) + fn = source._workflow2python(free).build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_access_feeding_output_materializes_and_round_trips(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + return dc.a + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + self.assertRegex(rendered.source, r"\n\s*\w+ = \w+\.a\n") + return_lines = [ + ln for ln in rendered.source.splitlines() if ln.strip().startswith("return") + ] + self.assertEqual(len(return_lines), 1) + self.assertNotIn(".", return_lines[0]) + fn = rendered.build() + result = fn(3, library.ComplexData(val=7)) + self.assertEqual(result.val, 7) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_fan_out_materializes_exactly_once(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + v = dc.x + p = library.identity(v) + q = library.negate(v) + return p, q + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + assignment_lines = [ + ln + for ln in rendered.source.splitlines() + if re.fullmatch(r"\w+ = \w+\.x", ln.strip()) + ] + self.assertEqual(len(assignment_lines), 1) + fn = rendered.build() + self.assertEqual(fn(3, library.ComplexData(val=1)), (3, -3)) + rebuilt = fn.flowrep_recipe + getattr_nodes = [n for n in rebuilt.nodes.values() if sugar.is_std_getattr(n)] + self.assertEqual(len(getattr_nodes), 1) + self.assertEqual(makers.dump_no_refs(rebuilt), makers.dump_no_refs(free)) + + def test_access_appended_to_accumulator_pins_body_symbol(self): + def wf(items: list): + xs = [] + for item in items: + dc = library.MyDataclass(item, 1) + xs.append(dc.a) + return xs + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + self.assertRegex(rendered.source, r"\ba = \w+\.a\b") + self.assertIn("xs.append(a)", rendered.source) + fn = rendered.build() + result = fn([library.ComplexData(val=1), library.ComplexData(val=2)]) + self.assertEqual([r.val for r in result], [1, 2]) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_access_directly_on_workflow_input(self): + def wf(comp: library.ComplexData): + return comp.val + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + self.assertIn(".val", rendered.source) + fn = rendered.build() + self.assertEqual(fn(library.ComplexData(val=9)), 9) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_getattr_with_unwired_obj_is_not_sugared(self): + TH, SH, OT = ( + edge_models.TargetHandle, + edge_models.SourceHandle, + edge_models.OutputTarget, + ) + # WorkflowRecipe validates that every required input has a source, so an + # unwired `obj` (no default on _getattr_wrapper) cannot be constructed + # directly. Build a valid recipe first, then strip the edge via + # model_copy, which -- like the analogous shape in test_sugar.py -- skips + # validators and produces a shape only hand-construction can reach. + wf = workflow_recipe.WorkflowRecipe( + inputs=[], + outputs=["attr"], + nodes={ + "getattr_0": std.getattr_.recipe, + "constant_obj": constant_recipe.ConstantRecipe(constant="hi"), + "constant_0": constant_recipe.ConstantRecipe(constant="val"), + }, + input_edges={}, + edges={ + TH(node="getattr_0", port="obj"): SH( + node="constant_obj", port="constant" + ), + TH(node="getattr_0", port="name"): SH( + node="constant_0", port="constant" + ), + }, + output_edges={OT(port="attr"): SH(node="getattr_0", port="attr")}, + ) + stripped_edges = { + target: sh + for target, sh in wf.edges.items() + if not (target.node == "getattr_0" and target.port == "obj") + } + wf = wf.model_copy(update={"edges": stripped_edges}) + with self.assertRaises(ValueError): + source._workflow2python(wf, function_name="unwired_obj") + + def test_getattr_obj_fed_by_inlined_constant_is_not_sugared(self): + TH, SH, OT = ( + edge_models.TargetHandle, + edge_models.SourceHandle, + edge_models.OutputTarget, + ) + wf = workflow_recipe.WorkflowRecipe( + inputs=[], + outputs=["attr"], + nodes={ + "constant_obj": constant_recipe.ConstantRecipe(constant=3), + "constant_0": constant_recipe.ConstantRecipe(constant="real"), + "getattr_0": std.getattr_.recipe, + }, + input_edges={}, + edges={ + TH(node="getattr_0", port="obj"): SH( + node="constant_obj", port="constant" + ), + TH(node="getattr_0", port="name"): SH( + node="constant_0", port="constant" + ), + }, + output_edges={OT(port="attr"): SH(node="getattr_0", port="attr")}, + ) + rendered = source._workflow2python(wf, function_name="const_obj_getattr") + self.assertIn("_getattr_wrapper", rendered.source) + fn = rendered.build() + self.assertEqual(fn(), 3) + + def test_hand_built_non_sugarable_getattr_emits_call_and_executes(self): + TH, IS, SH, OT = ( + edge_models.TargetHandle, + edge_models.InputSource, + edge_models.SourceHandle, + edge_models.OutputTarget, + ) + wf = workflow_recipe.WorkflowRecipe( + inputs=["obj_in", "name_in"], + outputs=["attr"], + nodes={ + "identity_0": library.identity.flowrep_recipe, + "getattr_0": std.getattr_.recipe, + }, + input_edges={ + TH(node="identity_0", port="x"): IS(port="name_in"), + TH(node="getattr_0", port="obj"): IS(port="obj_in"), + }, + edges={ + TH(node="getattr_0", port="name"): SH(node="identity_0", port="x"), + }, + output_edges={OT(port="attr"): SH(node="getattr_0", port="attr")}, + ) + rendered = source._workflow2python(wf, function_name="nonsugar") + self.assertIn("_getattr_wrapper", rendered.source) + fn = rendered.build() + self.assertEqual(fn(library.ComplexData(val=42), "val"), 42) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_attribute_parser.py b/tests/unit/parsers/test_attribute_parser.py new file mode 100644 index 00000000..e0962872 --- /dev/null +++ b/tests/unit/parsers/test_attribute_parser.py @@ -0,0 +1,183 @@ +import ast +import unittest + +from flowrep import edge_models +from flowrep.parsers import attribute_parser, symbol_scope +from flowrep.prospective import constant_recipe, std + + +def _make_input(port: str) -> edge_models.InputSource: + return edge_models.InputSource(port=port) + + +def _make_source(node: str, port: str) -> edge_models.SourceHandle: + return edge_models.SourceHandle(node=node, port=port) + + +def _expr(src: str) -> ast.expr: + stmt = ast.parse(src).body[0] + assert isinstance(stmt, ast.Expr) + return stmt.value + + +class TestIsDataAttribute(unittest.TestCase): + def test_true_for_single_access(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + self.assertTrue(attribute_parser.is_data_attribute(_expr("dc.a"), scope)) + + def test_true_for_chain(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + self.assertTrue(attribute_parser.is_data_attribute(_expr("dc.a.val"), scope)) + + def test_false_for_bare_name(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + self.assertFalse(attribute_parser.is_data_attribute(_expr("dc"), scope)) + + def test_false_when_root_not_a_symbol(self): + scope = symbol_scope.SymbolScope({}) + self.assertFalse(attribute_parser.is_data_attribute(_expr("np.pi"), scope)) + + def test_false_when_chain_contains_call(self): + scope = symbol_scope.SymbolScope({"f": _make_source("f_0", "output_0")}) + self.assertFalse(attribute_parser.is_data_attribute(_expr("f(x).a"), scope)) + + def test_false_when_chain_contains_subscript(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + self.assertFalse(attribute_parser.is_data_attribute(_expr("dc[0].a"), scope)) + + def test_symbol_scope_shadows_object_scope(self): + """`os.path` is a data attribute access when `os` is a bound symbol.""" + scope = symbol_scope.SymbolScope({"os": _make_input("os")}) + self.assertTrue(attribute_parser.is_data_attribute(_expr("os.path"), scope)) + + +class TestAttributeName(unittest.TestCase): + def test_outermost_link(self): + node = _expr("dc.a.val") + assert isinstance(node, ast.Attribute) + self.assertEqual(attribute_parser.attribute_name(node), "val") + + def test_single_link(self): + node = _expr("dc.a") + assert isinstance(node, ast.Attribute) + self.assertEqual(attribute_parser.attribute_name(node), "a") + + +class TestInjectAttributeChain(unittest.TestCase): + def test_single_link_adds_getattr_and_constant(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + nodes = {} + node = _expr("dc.a") + assert isinstance(node, ast.Attribute) + handle = attribute_parser.inject_attribute_chain(node, scope, nodes) + + self.assertEqual(set(nodes), {"getattr_a_0", "constant_0"}) + self.assertIs(nodes["getattr_a_0"], std.getattr_.recipe) + self.assertIsInstance(nodes["constant_0"], constant_recipe.ConstantRecipe) + self.assertEqual(nodes["constant_0"].constant, "a") + self.assertEqual( + scope.edges, + { + edge_models.TargetHandle(node="getattr_a_0", port="obj"): _make_source( + "MyDataclass_0", "instance" + ), + edge_models.TargetHandle(node="getattr_a_0", port="name"): _make_source( + "constant_0", "constant" + ), + }, + ) + self.assertEqual(handle, _make_source("getattr_a_0", "attr")) + + def test_chain_of_two_links(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + nodes = {} + node = _expr("dc.a.val") + assert isinstance(node, ast.Attribute) + handle = attribute_parser.inject_attribute_chain(node, scope, nodes) + + self.assertEqual( + list(nodes), ["getattr_a_0", "constant_0", "getattr_val_0", "constant_1"] + ) + self.assertEqual( + scope.edges[edge_models.TargetHandle(node="getattr_val_0", port="obj")], + _make_source("getattr_a_0", "attr"), + ) + self.assertEqual(handle, _make_source("getattr_val_0", "attr")) + + def test_root_is_workflow_input_creates_input_edge(self): + scope = symbol_scope.SymbolScope({"comp": _make_input("comp")}) + nodes = {} + node = _expr("comp.val") + assert isinstance(node, ast.Attribute) + attribute_parser.inject_attribute_chain(node, scope, nodes) + + self.assertEqual( + scope.input_edges, + { + edge_models.TargetHandle(node="getattr_val_0", port="obj"): _make_input( + "comp" + ) + }, + ) + + def test_label_collision_increments_suffix(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + nodes = {} + node1 = _expr("dc.a") + node2 = _expr("dc.a") + assert isinstance(node1, ast.Attribute) and isinstance(node2, ast.Attribute) + attribute_parser.inject_attribute_chain(node1, scope, nodes) + handle2 = attribute_parser.inject_attribute_chain(node2, scope, nodes) + self.assertEqual(handle2.node, "getattr_a_1") + + def test_accumulator_root_raises(self): + scope = symbol_scope.SymbolScope({}, available_accumulators={"acc"}) + node = _expr("acc.a") + assert isinstance(node, ast.Attribute) + with self.assertRaises(ValueError) as ctx: + attribute_parser.inject_attribute_chain(node, scope, {}) + self.assertIn("accumulator", str(ctx.exception).lower()) + + +class TestRejectMethodCall(unittest.TestCase): + def test_raises_for_method_call_on_symbol(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + call = _expr("dc.method(x)") + assert isinstance(call, ast.Call) + with self.assertRaises(ValueError) as ctx: + attribute_parser.reject_method_call(call, scope) + self.assertIn("method", str(ctx.exception).lower()) + + def test_noop_for_module_call(self): + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + call = _expr("library.my_add(x)") + assert isinstance(call, ast.Call) + attribute_parser.reject_method_call(call, scope) # does not raise + + def test_noop_for_plain_call(self): + scope = symbol_scope.SymbolScope({}) + call = _expr("f(x)") + assert isinstance(call, ast.Call) + attribute_parser.reject_method_call(call, scope) # does not raise + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/parsers/test_symbol_scope.py b/tests/unit/parsers/test_symbol_scope.py index 04feb620..a0443a23 100644 --- a/tests/unit/parsers/test_symbol_scope.py +++ b/tests/unit/parsers/test_symbol_scope.py @@ -131,6 +131,42 @@ def test_produce_symbols_helper(self): ) +class TestProduceSource(unittest.TestCase): + def test_records_handle_verbatim(self): + scope = SymbolScope({}) + handle = _make_source("getattr_a_0", "attr") + scope.produce_source("attr", handle) + self.assertEqual( + scope.output_edges, {edge_models.OutputTarget(port="attr"): handle} + ) + + def test_does_not_add_symbol_to_scope(self): + scope = SymbolScope({}) + scope.produce_source("attr", _make_source("getattr_a_0", "attr")) + self.assertNotIn("attr", scope) + self.assertEqual(len(scope), 0) + + def test_duplicate_port_raises(self): + scope = SymbolScope({}) + scope.produce_source("attr", _make_source("getattr_a_0", "attr")) + with self.assertRaises(ValueError) as ctx: + scope.produce_source("attr", _make_source("getattr_a_1", "attr")) + self.assertIn("already produced", str(ctx.exception)) + + def test_interoperates_with_produce(self): + scope = SymbolScope({"a": _make_input("a")}) + scope.produce("a") + scope.produce_source("b", _make_source("getattr_a_0", "attr")) + self.assertEqual(scope.outputs, ["a", "b"]) + self.assertEqual( + scope.output_edges, + { + edge_models.OutputTarget(port="a"): _make_input("a"), + edge_models.OutputTarget(port="b"): _make_source("getattr_a_0", "attr"), + }, + ) + + class TestSymbolScopeFork(unittest.TestCase): def test_fork_with_remap(self): scope = SymbolScope({"xs": _make_input("xs"), "const": _make_input("const")}) diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index 782dba91..4270ce1a 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -17,7 +17,7 @@ ) from flowrep.prospective import atomic_recipe, constant_recipe, workflow_recipe -from flowrep_static import library +from flowrep_static import library, makers def add(x: float = 2.0, y: float = 1) -> float: @@ -1235,5 +1235,278 @@ def test_literal_assigned_to_multiple_targets_raises(self): self.assertIn("exactly one symbol", str(ctx.exception)) +class TestAttributeAccess(unittest.TestCase): + def test_single_access(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + y = dc.x + r = library.my_add(y, y) + return r + + node = workflow_parser.parse_workflow(wf) + self.assertIn("getattr_x_0", node.nodes) + constants = [ + n + for n in node.nodes.values() + if isinstance(n, constant_recipe.ConstantRecipe) + ] + self.assertTrue(any(c.constant == "x" for c in constants)) + self.assertEqual( + node.edges[edge_models.TargetHandle(node="getattr_x_0", port="obj")], + edge_models.SourceHandle(node="MyDataclass_0", port="instance"), + ) + + def test_chain(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + v = dc.a.val + return v + + node = workflow_parser.parse_workflow(wf) + self.assertIn("getattr_a_0", node.nodes) + self.assertIn("getattr_val_0", node.nodes) + self.assertEqual( + node.edges[edge_models.TargetHandle(node="getattr_val_0", port="obj")], + edge_models.SourceHandle(node="getattr_a_0", port="attr"), + ) + + def test_call_argument_each_access_is_its_own_node(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + r = library.my_add(dc.x, dc.x) + return r + + node = workflow_parser.parse_workflow(wf) + self.assertIn("getattr_x_0", node.nodes) + self.assertIn("getattr_x_1", node.nodes) + + def test_hoisting_invariant_call_argument_form(self): + def hoisted(x0: int, comp: library.ComplexData): + v = comp.val + y = library.my_add(x0, v) + return y + + def inlined(x0: int, comp: library.ComplexData): + y = library.my_add(x0, comp.val) + return y + + hoisted_recipe = workflow_parser.parse_workflow(hoisted) + inlined_recipe = workflow_parser.parse_workflow(inlined) + self.assertEqual( + makers.dump_no_refs(hoisted_recipe), makers.dump_no_refs(inlined_recipe) + ) + + def test_hoisting_invariant_inside_for_loop(self): + def hoisted(x0: int, comp: library.ComplexData, items: list): + acc = [] + for item in items: + v = comp.val + y = library.my_add(item, v) + acc.append(y) + return acc + + def inlined(x0: int, comp: library.ComplexData, items: list): + acc = [] + for item in items: + y = library.my_add(item, comp.val) + acc.append(y) + return acc + + hoisted_recipe = workflow_parser.parse_workflow(hoisted) + inlined_recipe = workflow_parser.parse_workflow(inlined) + self.assertEqual( + makers.dump_no_refs(hoisted_recipe), makers.dump_no_refs(inlined_recipe) + ) + + def test_root_is_workflow_input_creates_input_edge(self): + def wf(comp: library.ComplexData): + v = comp.val + return v + + node = workflow_parser.parse_workflow(wf) + self.assertEqual( + node.input_edges[ + edge_models.TargetHandle(node="getattr_val_0", port="obj") + ], + edge_models.InputSource(port="comp"), + ) + + def test_method_call_assignment_raises(self): + def wf(comp: library.ComplexData): + dc = library.MyDataclass(comp, 1) + y = dc.method(comp) + return y + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("method", str(ctx.exception).lower()) + + def test_method_call_in_if_condition_raises(self): + def wf(comp: library.ComplexData): + dc = library.MyDataclass(comp, 1) + if dc.method(comp): # noqa: SIM108 + y = comp + else: + y = comp + return y + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("method", str(ctx.exception).lower()) + + def test_method_call_in_while_condition_raises(self): + def wf(comp: library.ComplexData): + dc = library.MyDataclass(comp, 1) + while dc.method(comp): + comp = comp + return comp + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("method", str(ctx.exception).lower()) + + def test_attribute_chain_in_if_condition_argument_raises(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + if library.is_positive(dc.x): # noqa: SIM108 + y = x0 + else: + y = x0 + return y + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("bind", str(ctx.exception).lower()) + + def test_attribute_chain_in_while_condition_argument_raises(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + while library.is_positive(dc.x): + x0 = x0 + return x0 + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("bind", str(ctx.exception).lower()) + + def test_multi_target_attribute_assignment_raises(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + p, q = dc.a # noqa: F841 + return p + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("exactly one symbol", str(ctx.exception)) + + def test_attribute_assignment_target_raises(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + dc.a = comp + return dc + + with self.assertRaises(TypeError): + workflow_parser.parse_workflow(wf) + + def test_unknown_root_still_raises(self): + def wf(x0: int): + y = numpy.pi # noqa: F821 -- never executed, only parsed + return y + + with self.assertRaises(ValueError): + workflow_parser.parse_workflow(wf) + + def test_return_single_access(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + return dc.a + + node = workflow_parser.parse_workflow(wf) + self.assertEqual(node.outputs, ["a"]) + self.assertEqual( + node.output_edges[edge_models.OutputTarget(port="a")], + edge_models.SourceHandle(node="getattr_a_0", port="attr"), + ) + + def test_return_chain(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + return dc.a.val + + node = workflow_parser.parse_workflow(wf) + self.assertEqual(node.outputs, ["val"]) + + def test_return_access_with_explicit_output_label(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + return dc.a + + node = workflow_parser.parse_workflow(wf, "thing") + self.assertEqual(node.outputs, ["thing"]) + + def test_return_symbol_and_access_in_order(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + other_symbol = x0 + return dc.a, other_symbol + + node = workflow_parser.parse_workflow(wf) + self.assertEqual(node.outputs, ["a", "other_symbol"]) + + def test_return_duplicate_attribute_port_raises(self): + def wf(x0: int, comp: library.ComplexData, comp2: library.ComplexData): + dc = library.MyDataclass(comp, x0) + dc2 = library.MyDataclass(comp2, x0) + return dc.a, dc2.a + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("unique", str(ctx.exception).lower()) + + def test_accumulator_append_of_access(self): + def wf(items: list): + xs = [] + for item in items: + dc = library.MyDataclass(item, 1) + xs.append(dc.a) + return xs + + node = workflow_parser.parse_workflow(wf) + for_node = node.nodes["for_each_0"] + self.assertEqual(for_node.outputs, ["xs"]) + body = for_node.body_node.recipe + self.assertEqual(body.outputs, ["a"]) + self.assertIn("getattr_a_0", body.nodes) + + def test_return_no_value_raises(self): + def wf(x0: int): + return + + with self.assertRaises(TypeError): + workflow_parser.parse_workflow(wf) + + def test_return_non_name_non_attribute_element_raises(self): + def wf(x0: int): + return x0 + 1 + + with self.assertRaises(TypeError): + workflow_parser.parse_workflow(wf) + + def test_accumulator_append_of_access_duplicate_port_raises(self): + def wf(items: list, others: list): + xs = [] + ys = [] + for item, other in zip(items, others, strict=True): + dc = library.MyDataclass(item, 1) + dc2 = library.MyDataclass(other, 1) + xs.append(dc.a) + ys.append(dc2.a) + return xs, ys + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("already produced", str(ctx.exception)) + + if __name__ == "__main__": unittest.main() From a76de63ed825cc309be5e6197b6451ed357f48a1 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 12:33:59 -0700 Subject: [PATCH 02/14] Only append known symbols to accumulators Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/attribute_parser.py | 21 +++++++ src/flowrep/parsers/workflow_parser.py | 17 +++--- tests/unit/compiler/test_compiler.py | 7 ++- tests/unit/parsers/test_attribute_parser.py | 34 +++++++++++ tests/unit/parsers/test_workflow_parser.py | 65 ++++++++++++++------- 5 files changed, 112 insertions(+), 32 deletions(-) diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py index 667a1463..41f9c1ef 100644 --- a/src/flowrep/parsers/attribute_parser.py +++ b/src/flowrep/parsers/attribute_parser.py @@ -101,6 +101,27 @@ def hoist_call_arguments( return hoisted +def reject_unbound_attribute( + node: ast.expr, symbol_map: symbol_scope.SymbolScope, context: str +) -> None: + """Raise if *node* is an attribute chain used where a port name must be invented. + + Flowrep names a workflow output port, a flow-control input port, and a for-body + output port after the *symbol* supplying them. An attribute chain has no symbol, + so there is no honest name to give the port. Rather than invent one the user + cannot predict from reading their own source, require the binding. + """ + if not is_data_attribute(node, symbol_map): + return + chain = ast.unparse(node) + raise ValueError( + f"Attribute access must be bound to a symbol before it can be {context}: " + f"got {chain!r}. The port name is taken from the symbol, and {chain!r} has " + f"no symbol to take it from. Bind it first -- e.g. `my_value = {chain}` -- " + f"and use `my_value` instead." + ) + + def reject_method_call(call: ast.Call, symbol_map: symbol_scope.SymbolScope) -> None: """Raise if *call* invokes an attribute of a known symbol (``dc.method(x)``).""" if is_data_attribute(call.func, symbol_map): diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index f46d7e5a..7fbda6dd 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -441,15 +441,16 @@ def _handle_appending_to_accumulator(self, append_call: ast.Call) -> None: ast.Name, cast(ast.Attribute, append_call.func).value ).id appended = append_call.args[0] - if attribute_parser.is_data_attribute(appended, self.symbol_map): - handle = attribute_parser.inject_attribute_chain( - appended, self.symbol_map, self.nodes + attribute_parser.reject_unbound_attribute( + appended, self.symbol_map, "appended to an accumulator" + ) + if not isinstance(appended, ast.Name): + raise TypeError( + f"Workflow python definitions can only append a symbol to an " + f"accumulator, but '{used_accumulator}.append(...)' got " + f"{type(appended).__name__}. Bind the value to a symbol first." ) - port = attribute_parser.attribute_name(cast(ast.Attribute, appended)) - self.symbol_map.use_accumulator(used_accumulator, port) - self.symbol_map.produce_source(port, handle) - return - appended_symbol = cast(ast.Name, appended).id + appended_symbol = appended.id self.symbol_map.use_accumulator(used_accumulator, appended_symbol) appended_source = self.symbol_map[appended_symbol] if isinstance(appended_source, edge_models.SourceHandle): diff --git a/tests/unit/compiler/test_compiler.py b/tests/unit/compiler/test_compiler.py index 2b04f3f9..ea57153e 100644 --- a/tests/unit/compiler/test_compiler.py +++ b/tests/unit/compiler/test_compiler.py @@ -2383,13 +2383,14 @@ def wf(items: list): xs = [] for item in items: dc = library.MyDataclass(item, 1) - xs.append(dc.a) + inner = dc.a + xs.append(inner) return xs free = makers.reference_free(wf) rendered = source._workflow2python(free) - self.assertRegex(rendered.source, r"\ba = \w+\.a\b") - self.assertIn("xs.append(a)", rendered.source) + self.assertRegex(rendered.source, r"\binner = \w+\.a\b") + self.assertIn("xs.append(inner)", rendered.source) fn = rendered.build() result = fn([library.ComplexData(val=1), library.ComplexData(val=2)]) self.assertEqual([r.val for r in result], [1, 2]) diff --git a/tests/unit/parsers/test_attribute_parser.py b/tests/unit/parsers/test_attribute_parser.py index e0962872..c2268ade 100644 --- a/tests/unit/parsers/test_attribute_parser.py +++ b/tests/unit/parsers/test_attribute_parser.py @@ -179,5 +179,39 @@ def test_noop_for_plain_call(self): attribute_parser.reject_method_call(call, scope) # does not raise +class TestRejectUnboundAttribute(unittest.TestCase): + def _scope(self) -> symbol_scope.SymbolScope: + return symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + + def test_raises_for_chain_on_known_symbol(self): + with self.assertRaises(ValueError) as ctx: + attribute_parser.reject_unbound_attribute( + _expr("dc.a"), self._scope(), "appended to an accumulator" + ) + message = str(ctx.exception) + self.assertIn("appended to an accumulator", message) + self.assertIn("dc.a", message) + self.assertIn("bind", message.lower()) + + def test_raises_for_chain_of_chains(self): + with self.assertRaises(ValueError) as ctx: + attribute_parser.reject_unbound_attribute( + _expr("dc.a.val"), self._scope(), "returned from a workflow" + ) + self.assertIn("dc.a.val", str(ctx.exception)) + + def test_noop_for_bare_symbol(self): + attribute_parser.reject_unbound_attribute( + _expr("dc"), self._scope(), "returned from a workflow" + ) + + def test_noop_for_chain_on_unknown_root(self): + attribute_parser.reject_unbound_attribute( + _expr("numpy.pi"), self._scope(), "returned from a workflow" + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index 4270ce1a..5fc6e19e 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1463,21 +1463,6 @@ def wf(x0: int, comp: library.ComplexData, comp2: library.ComplexData): workflow_parser.parse_workflow(wf) self.assertIn("unique", str(ctx.exception).lower()) - def test_accumulator_append_of_access(self): - def wf(items: list): - xs = [] - for item in items: - dc = library.MyDataclass(item, 1) - xs.append(dc.a) - return xs - - node = workflow_parser.parse_workflow(wf) - for_node = node.nodes["for_each_0"] - self.assertEqual(for_node.outputs, ["xs"]) - body = for_node.body_node.recipe - self.assertEqual(body.outputs, ["a"]) - self.assertIn("getattr_a_0", body.nodes) - def test_return_no_value_raises(self): def wf(x0: int): return @@ -1492,20 +1477,58 @@ def wf(x0: int): with self.assertRaises(TypeError): workflow_parser.parse_workflow(wf) - def test_accumulator_append_of_access_duplicate_port_raises(self): + def test_accumulator_append_of_access_raises(self): + def wf(items: list): + xs = [] + for item in items: + dc = library.MyDataclass(item, 1) + xs.append(dc.a) + return xs + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("bind", str(ctx.exception).lower()) + + def test_accumulator_append_of_non_symbol_raises(self): + def wf(items: list): + xs = [] + for item in items: + xs.append(library.my_add(item, 1)) + return xs + + with self.assertRaises(TypeError): + workflow_parser.parse_workflow(wf) + + def test_bound_accesses_of_same_attribute_coexist(self): def wf(items: list, others: list): xs = [] ys = [] for item, other in zip(items, others, strict=True): dc = library.MyDataclass(item, 1) dc2 = library.MyDataclass(other, 1) - xs.append(dc.a) - ys.append(dc2.a) + first = dc.a + second = dc2.a + xs.append(first) + ys.append(second) return xs, ys - with self.assertRaises(ValueError) as ctx: - workflow_parser.parse_workflow(wf) - self.assertIn("already produced", str(ctx.exception)) + node = workflow_parser.parse_workflow(wf) + for_node = node.nodes["for_each_0"] + self.assertEqual(for_node.outputs, ["xs", "ys"]) + body = for_node.body_node.recipe + # Two accesses to the same attribute of two different objects are two + # distinct nodes feeding two distinctly-named ports. This is the defect. + self.assertIn("getattr_a_0", body.nodes) + self.assertIn("getattr_a_1", body.nodes) + self.assertEqual(body.outputs, ["first", "second"]) + self.assertEqual( + body.output_edges[edge_models.OutputTarget(port="first")], + edge_models.SourceHandle(node="getattr_a_0", port="attr"), + ) + self.assertEqual( + body.output_edges[edge_models.OutputTarget(port="second")], + edge_models.SourceHandle(node="getattr_a_1", port="attr"), + ) if __name__ == "__main__": From 9ee645c123260088468cb20630716ee443834ecf Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 12:56:56 -0700 Subject: [PATCH 03/14] Only accept symbols as workflow returns Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/symbol_scope.py | 13 +--- src/flowrep/parsers/workflow_parser.py | 71 +++++++--------------- tests/unit/compiler/test_compiler.py | 6 +- tests/unit/parsers/test_symbol_scope.py | 36 ----------- tests/unit/parsers/test_workflow_parser.py | 49 ++++++--------- 5 files changed, 43 insertions(+), 132 deletions(-) diff --git a/src/flowrep/parsers/symbol_scope.py b/src/flowrep/parsers/symbol_scope.py index ffbbfd2a..f93920fe 100644 --- a/src/flowrep/parsers/symbol_scope.py +++ b/src/flowrep/parsers/symbol_scope.py @@ -266,21 +266,10 @@ def consume_input_source( def produce(self, output_port: str, symbol: str | None = None) -> None: """Record that `output_port` is sourced from `symbol`.""" produced_symbol = output_port if symbol is None else symbol - self.produce_source(output_port, self[produced_symbol]) - - def produce_source( - self, - output_port: str, - source: edge_models.SourceHandle | edge_models.InputSource, - ) -> None: - """Record a production whose source is a fixed ``SourceHandle`` (e.g. an - injected getattr node), bypassing symbol lookup so no synthetic symbol - enters ``_sources`` and risks colliding with a user symbol. The - production twin of :meth:`consume_source`.""" if any(p.output_port == output_port for p in self._productions): raise ValueError(f"Output port '{output_port}' already produced.") self._productions.append( - SymbolProduction(output_port=output_port, source=source) + SymbolProduction(output_port=output_port, source=self[produced_symbol]) ) def produce_symbols(self, symbols: list[str]) -> None: diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index 7fbda6dd..9dea1134 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -504,75 +504,46 @@ def handle_return( func: types.FunctionType, output_labels: Collection[str], ) -> None: - elements = _return_elements(body.value) - port_name_candidates = [ - _return_port_candidate(elt, self.symbol_map) for elt in elements - ] + if body.value is not None: + elements = ( + body.value.elts if isinstance(body.value, ast.Tuple) else [body.value] + ) + for element in elements: + attribute_parser.reject_unbound_attribute( + element, self.symbol_map, "returned from a workflow" + ) + + returned_symbols = parser_helpers.resolve_symbols_to_strings(body.value) base_models.validate_unique( - port_name_candidates, + returned_symbols, message=f"Workflow python definitions must have unique returns, but " - f"got duplicates in: {port_name_candidates}", + f"got duplicates in: {returned_symbols}", ) annotated_returns = label_helpers.get_annotated_output_labels(func) scraped_labels = label_helpers.merge_labels( first_choice=annotated_returns, - fallback=port_name_candidates, + fallback=returned_symbols, message_prefix="Annotation labels and returned symbols mis-match. ", ) - if output_labels and len(output_labels) != len(elements): + if output_labels and len(output_labels) != len(returned_symbols): raise ValueError( f"When output_labels are specified ({output_labels}), workflow " f"python definitions have a matching number of returned symbols " - f"({port_name_candidates})." + f"({returned_symbols})." ) final_ports = list(output_labels) if output_labels else scraped_labels - for elt, port in zip(elements, final_ports, strict=True): - if isinstance(elt, ast.Name): - if elt.id not in self.symbol_map: - raise ValueError( - f"Return symbol '{elt.id}' is not defined. " - f"Available: {list(self.symbol_map)}" - ) - self.symbol_map.produce(port, elt.id) - else: - handle = attribute_parser.inject_attribute_chain( - elt, self.symbol_map, self.nodes + for symbol, port in zip(returned_symbols, final_ports, strict=True): + if symbol not in self.symbol_map: + raise ValueError( + f"Return symbol '{symbol}' is not defined. " + f"Available: {list(self.symbol_map)}" ) - self.symbol_map.produce_source(port, handle) - -def _return_elements(node: ast.expr | None) -> list[ast.expr]: - """The returned expressions: a single expression, or a tuple's elements.""" - if node is None: - raise TypeError("Workflow python definitions must return a value.") - return list(node.elts) if isinstance(node, ast.Tuple) else [node] - - -def _return_port_candidate(elt: ast.expr, symbol_map: symbol_scope.SymbolScope) -> str: - """The output-port name candidate for one returned element. - - A returned symbol contributes its own name; a data attribute chain contributes - its final attribute name. Anything else raises ``TypeError``. - - Deliberately does *not* delegate to - :func:`parser_helpers.resolve_symbols_to_strings` here: that helper also - accepts a bare ``ast.Tuple`` of names (it is built for the top-level - ``body.value``), which would silently succeed on a nested-tuple return - element -- e.g. ``return (a, b), c`` -- instead of raising. - """ - if isinstance(elt, ast.Name): - return elt.id - if attribute_parser.is_data_attribute(elt, symbol_map): - return attribute_parser.attribute_name(cast(ast.Attribute, elt)) - raise TypeError( - f"Expected each returned element to be a symbol or an attribute access " - f"rooted at a known workflow symbol, but could not parse this from " - f"{type(elt)}." - ) + self.symbol_map.produce(port, symbol) def is_append_call(node: ast.expr | ast.Expr) -> bool: diff --git a/tests/unit/compiler/test_compiler.py b/tests/unit/compiler/test_compiler.py index ea57153e..04b565be 100644 --- a/tests/unit/compiler/test_compiler.py +++ b/tests/unit/compiler/test_compiler.py @@ -2338,7 +2338,8 @@ def wf(comp: library.ComplexData, n: int): def test_access_feeding_output_materializes_and_round_trips(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) - return dc.a + v = dc.a + return v free = makers.reference_free(wf) rendered = source._workflow2python(free) @@ -2400,7 +2401,8 @@ def wf(items: list): def test_access_directly_on_workflow_input(self): def wf(comp: library.ComplexData): - return comp.val + v = comp.val + return v free = makers.reference_free(wf) rendered = source._workflow2python(free) diff --git a/tests/unit/parsers/test_symbol_scope.py b/tests/unit/parsers/test_symbol_scope.py index a0443a23..04feb620 100644 --- a/tests/unit/parsers/test_symbol_scope.py +++ b/tests/unit/parsers/test_symbol_scope.py @@ -131,42 +131,6 @@ def test_produce_symbols_helper(self): ) -class TestProduceSource(unittest.TestCase): - def test_records_handle_verbatim(self): - scope = SymbolScope({}) - handle = _make_source("getattr_a_0", "attr") - scope.produce_source("attr", handle) - self.assertEqual( - scope.output_edges, {edge_models.OutputTarget(port="attr"): handle} - ) - - def test_does_not_add_symbol_to_scope(self): - scope = SymbolScope({}) - scope.produce_source("attr", _make_source("getattr_a_0", "attr")) - self.assertNotIn("attr", scope) - self.assertEqual(len(scope), 0) - - def test_duplicate_port_raises(self): - scope = SymbolScope({}) - scope.produce_source("attr", _make_source("getattr_a_0", "attr")) - with self.assertRaises(ValueError) as ctx: - scope.produce_source("attr", _make_source("getattr_a_1", "attr")) - self.assertIn("already produced", str(ctx.exception)) - - def test_interoperates_with_produce(self): - scope = SymbolScope({"a": _make_input("a")}) - scope.produce("a") - scope.produce_source("b", _make_source("getattr_a_0", "attr")) - self.assertEqual(scope.outputs, ["a", "b"]) - self.assertEqual( - scope.output_edges, - { - edge_models.OutputTarget(port="a"): _make_input("a"), - edge_models.OutputTarget(port="b"): _make_source("getattr_a_0", "attr"), - }, - ) - - class TestSymbolScopeFork(unittest.TestCase): def test_fork_with_remap(self): scope = SymbolScope({"xs": _make_input("xs"), "const": _make_input("const")}) diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index 5fc6e19e..b43481d6 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1416,52 +1416,37 @@ def wf(x0: int): with self.assertRaises(ValueError): workflow_parser.parse_workflow(wf) - def test_return_single_access(self): + def test_return_of_access_raises(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) return dc.a - node = workflow_parser.parse_workflow(wf) - self.assertEqual(node.outputs, ["a"]) - self.assertEqual( - node.output_edges[edge_models.OutputTarget(port="a")], - edge_models.SourceHandle(node="getattr_a_0", port="attr"), - ) + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + message = str(ctx.exception) + self.assertIn("bind", message.lower()) + self.assertIn("dc.a", message) - def test_return_chain(self): + def test_return_of_chain_raises(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) return dc.a.val - node = workflow_parser.parse_workflow(wf) - self.assertEqual(node.outputs, ["val"]) - - def test_return_access_with_explicit_output_label(self): - def wf(x0: int, comp: library.ComplexData): - dc = library.MyDataclass(comp, x0) - return dc.a - - node = workflow_parser.parse_workflow(wf, "thing") - self.assertEqual(node.outputs, ["thing"]) + with self.assertRaises(ValueError): + workflow_parser.parse_workflow(wf) - def test_return_symbol_and_access_in_order(self): + def test_bound_access_returns_under_its_symbol(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) - other_symbol = x0 - return dc.a, other_symbol + v = dc.a + return v node = workflow_parser.parse_workflow(wf) - self.assertEqual(node.outputs, ["a", "other_symbol"]) - - def test_return_duplicate_attribute_port_raises(self): - def wf(x0: int, comp: library.ComplexData, comp2: library.ComplexData): - dc = library.MyDataclass(comp, x0) - dc2 = library.MyDataclass(comp2, x0) - return dc.a, dc2.a - - with self.assertRaises(ValueError) as ctx: - workflow_parser.parse_workflow(wf) - self.assertIn("unique", str(ctx.exception).lower()) + self.assertEqual(node.outputs, ["v"]) + self.assertEqual( + node.output_edges[edge_models.OutputTarget(port="v")], + edge_models.SourceHandle(node="getattr_a_0", port="attr"), + ) def test_return_no_value_raises(self): def wf(x0: int): From 18067d60c266412c878f1e14c4758325189b4621 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 12:58:38 -0700 Subject: [PATCH 04/14] Add untracked test file for sugar Co-authored-by: Claude Signed-off-by: Liam Huber --- tests/unit/compiler/test_sugar.py | 93 +++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 tests/unit/compiler/test_sugar.py diff --git a/tests/unit/compiler/test_sugar.py b/tests/unit/compiler/test_sugar.py new file mode 100644 index 00000000..3d749509 --- /dev/null +++ b/tests/unit/compiler/test_sugar.py @@ -0,0 +1,93 @@ +import unittest + +from flowrep import edge_models +from flowrep.compiler import sugar +from flowrep.parsers import workflow_parser +from flowrep.prospective import constant_recipe, std + +from flowrep_static import library, makers + + +def _wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + v = dc.a + return v + + +class TestIsStdGetattr(unittest.TestCase): + def test_true_for_std_getattr_recipe(self): + self.assertTrue(sugar.is_std_getattr(std.getattr_.recipe)) + + def test_false_for_other_atomic(self): + self.assertFalse(sugar.is_std_getattr(library.my_add.flowrep_recipe)) + + def test_false_for_constant_recipe(self): + self.assertFalse( + sugar.is_std_getattr(constant_recipe.ConstantRecipe(constant=1)) + ) + + def test_false_for_workflow_recipe(self): + recipe = makers.reference_free(_wf) + self.assertFalse(sugar.is_std_getattr(recipe)) + + +class TestIsAttributeSyntax(unittest.TestCase): + def test_true_cases(self): + for value in ("a", "inputs", "_x", "__class__"): + with self.subTest(value): + self.assertTrue(sugar.is_attribute_syntax(value)) + + def test_false_for_keyword(self): + self.assertFalse(sugar.is_attribute_syntax("class")) + + def test_false_for_non_identifiers(self): + for value in ("1a", "a b", ""): + with self.subTest(value): + self.assertFalse(sugar.is_attribute_syntax(value)) + + def test_false_for_non_string_types(self): + for value in (3, None): + with self.subTest(value): + self.assertFalse(sugar.is_attribute_syntax(value)) + + +class TestAttributeName(unittest.TestCase): + def test_returns_name_for_parsed_access(self): + recipe = makers.reference_free(_wf) + self.assertEqual(sugar.attribute_name("getattr_a_0", recipe), "a") + + def test_none_when_name_port_fed_by_non_constant_node(self): + recipe = workflow_parser.parse_workflow(_wf).model_copy( + update={"reference": None} + ) + # Rewire the name port to come from a non-constant node output instead of a + # constant peer -- a shape only hand-construction produces. + target = edge_models.TargetHandle(node="getattr_a_0", port="name") + new_edges = dict(recipe.edges) + new_edges[target] = edge_models.SourceHandle( + node="MyDataclass_0", port="instance" + ) + recipe = recipe.model_copy(update={"edges": new_edges}) + self.assertIsNone(sugar.attribute_name("getattr_a_0", recipe)) + + def test_none_when_constant_is_non_identifier_string(self): + recipe = workflow_parser.parse_workflow(_wf).model_copy( + update={"reference": None} + ) + new_nodes = dict(recipe.nodes) + new_nodes["constant_0"] = constant_recipe.ConstantRecipe(constant="not an id!") + recipe = recipe.model_copy(update={"nodes": new_nodes}) + self.assertIsNone(sugar.attribute_name("getattr_a_0", recipe)) + + def test_none_when_name_port_unwired(self): + recipe = workflow_parser.parse_workflow(_wf).model_copy( + update={"reference": None} + ) + target = edge_models.TargetHandle(node="getattr_a_0", port="name") + new_edges = {k: v for k, v in recipe.edges.items() if k != target} + recipe = recipe.model_copy(update={"edges": new_edges}) + self.assertIsNone(sugar.attribute_name("getattr_a_0", recipe)) + + +if __name__ == "__main__": + unittest.main() From c2276d1f605310615bc7fcfbfb1cd843c186bb86 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 13:41:24 -0700 Subject: [PATCH 05/14] Centralize and strengthen non-symbol rejection This actually goes too far, I think we can relax it. But Claude's first pass being more relaxed had bugs, so let's tighten all the way down then loosen it up one case at a time. I'll tolerate the churn for a bit of precision. Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/attribute_parser.py | 36 ++++++++++++++++++++++ src/flowrep/parsers/parser_helpers.py | 19 ++++++------ tests/unit/compiler/test_compiler.py | 20 ++++++++++++ tests/unit/parsers/test_workflow_parser.py | 29 +++++++++++++++-- 4 files changed, 92 insertions(+), 12 deletions(-) diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py index 41f9c1ef..e9399849 100644 --- a/src/flowrep/parsers/attribute_parser.py +++ b/src/flowrep/parsers/attribute_parser.py @@ -1,3 +1,39 @@ +"""Parse attribute access on workflow data into injected ``std.getattr_`` nodes. + +An attribute chain rooted at a known *symbol* (``dc.a``, ``dc.a.val``) becomes one +``std.getattr_`` node per link, each with a ``ConstantRecipe`` peer carrying the +attribute name. The symbol map deliberately shadows the object scope, so a workflow +input named ``os`` makes ``os.path`` an attribute access on that input, exactly as it +would be at runtime. + +Attribute access is allowed in exactly two places: the right-hand side of an +assignment, and the arguments of an ordinary node call. Everywhere else it must be +bound to a symbol first (see :func:`reject_unbound_attribute`). + +The reason is that flowrep takes port names from two different places: + +=============================== ========================================== +Port Name comes from +=============================== ========================================== +Workflow output port the returned symbol +Flow-control input port the enclosing symbol feeding it +For-body output port the appended symbol +Atomic/workflow node input port the *callee's* own parameter +=============================== ========================================== + +An attribute chain has no symbol. Passing ``mul(dc.x, y)`` is fine -- the injected +getattr node feeds ``mul``'s own ``a`` port, whose name comes from ``mul``. But +returning a chain, appending one to an accumulator, or passing one to a flow-control +condition would each require *inventing* a port name, and a port name the user cannot +predict from reading their own source is an unpleasant interface. So we refuse, and +ask for the binding. + +The binding costs one line and changes the recipe not at all -- the getattr node +exists either way. For a condition it produces exactly the graph one would want +anyway: a getattr node sitting as a peer of the flow-control node, feeding it through +a normally-named input port. +""" + from __future__ import annotations import ast diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index df574d12..d43ba8ed 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -180,9 +180,12 @@ def consume_call_arguments( Data-attribute arguments are injected ahead of the call by ``attribute_parser.hoist_call_arguments`` and arrive here in *hoisted*, mapping - the argument's AST node to the ``SourceHandle`` of its outermost getattr node. In - condition mode there is no room for a peer getattr node inside a flow-control - condition, so a data-attribute argument is rejected instead. + the argument's AST node to the ``SourceHandle`` of its outermost getattr node. The + call to ``attribute_parser.reject_unbound_attribute`` below is a no-op outside + condition mode: in normal mode every data-attribute argument was already hoisted + and returns from the first branch above, so it can only fire on a condition + argument, where there is no room for a peer getattr node inside the flow-control + node itself. """ reserved = set() if reserved_ports is None else reserved_ports already_hoisted = {} if hoisted is None else hoisted @@ -194,13 +197,9 @@ def _consume(arg_node: ast.expr, consumer_port: str) -> None: if isinstance(arg_node, ast.Name): scope.consume(arg_node.id, child.label, consumer_port) return - if attribute_parser.is_data_attribute(arg_node, scope): - raise ValueError( - f"Attribute access on workflow data is not supported in flow-control " - f"condition arguments; for input '{consumer_port}' of condition node " - f"'{child.label}' found '{ast.unparse(arg_node)}'. Bind it to a " - f"symbol before the flow control and pass that symbol instead." - ) + attribute_parser.reject_unbound_attribute( + arg_node, scope, "passed to a flow-control condition" + ) is_literal, value = constant_parser.try_parse_constant(arg_node) if not is_literal: raise TypeError( diff --git a/tests/unit/compiler/test_compiler.py b/tests/unit/compiler/test_compiler.py index 04b565be..77bb0d37 100644 --- a/tests/unit/compiler/test_compiler.py +++ b/tests/unit/compiler/test_compiler.py @@ -2510,6 +2510,26 @@ def test_hand_built_non_sugarable_getattr_emits_call_and_executes(self): fn = rendered.build() self.assertEqual(fn(library.ComplexData(val=42), "val"), 42) + def test_bound_access_as_condition_input_round_trips(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + flag = dc.x + if library.is_positive(flag): + y = library.identity(x0) + else: + y = library.negate(x0) + return y + + free = makers.reference_free(wf) + rendered = source._workflow2python(free) + self.assertRegex(rendered.source, r"\bflag = \w+\.x\b") + fn = rendered.build() + self.assertEqual(fn(3, library.ComplexData(val=7)), 3) + self.assertEqual(fn(-3, library.ComplexData(val=7)), 3) + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index b43481d6..888a2bfa 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1376,7 +1376,9 @@ def wf(x0: int, comp: library.ComplexData): with self.assertRaises(ValueError) as ctx: workflow_parser.parse_workflow(wf) - self.assertIn("bind", str(ctx.exception).lower()) + message = str(ctx.exception) + self.assertIn("has no symbol to take it from", message) + self.assertIn("dc.x", message) def test_attribute_chain_in_while_condition_argument_raises(self): def wf(x0: int, comp: library.ComplexData): @@ -1387,7 +1389,30 @@ def wf(x0: int, comp: library.ComplexData): with self.assertRaises(ValueError) as ctx: workflow_parser.parse_workflow(wf) - self.assertIn("bind", str(ctx.exception).lower()) + message = str(ctx.exception) + self.assertIn("has no symbol to take it from", message) + self.assertIn("dc.x", message) + + def test_bound_access_as_if_condition_input(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + flag = dc.x + if library.is_positive(flag): + y = library.identity(x0) + else: + y = library.negate(x0) + return y + + node = workflow_parser.parse_workflow(wf) + # The getattr node is a peer of the `if`, not inside it. + self.assertIn("getattr_x_0", node.nodes) + self.assertIn("if_0", node.nodes) + # ...and it feeds the flow control through a port named after the symbol. + self.assertIn("flag", node.nodes["if_0"].inputs) + self.assertEqual( + node.edges[edge_models.TargetHandle(node="if_0", port="flag")], + edge_models.SourceHandle(node="getattr_x_0", port="attr"), + ) def test_multi_target_attribute_assignment_raises(self): def wf(x0: int, comp: library.ComplexData): From 3e3630c6d6c4143fd8b9cbbe7d88e8ae57a80d19 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 13:52:58 -0700 Subject: [PATCH 06/14] Use single source of truth on std recipe labels Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/compiler/sugar.py | 18 +++++------ src/flowrep/parsers/attribute_parser.py | 13 +++----- tests/unit/compiler/test_sugar.py | 12 ++++++++ tests/unit/parsers/test_attribute_parser.py | 34 +++++++++++++-------- 4 files changed, 48 insertions(+), 29 deletions(-) diff --git a/src/flowrep/compiler/sugar.py b/src/flowrep/compiler/sugar.py index f472074f..40ff57e9 100644 --- a/src/flowrep/compiler/sugar.py +++ b/src/flowrep/compiler/sugar.py @@ -12,14 +12,14 @@ workflow_recipe, ) -OBJ_PORT = "obj" -NAME_PORT = "name" -ATTR_PORT = "attr" - # `LabeledRecipe.recipe` is a discriminated union; the cast narrows it for mypy. -_GETATTR_FQN = cast( - atomic_recipe.AtomicRecipe, std.getattr_.recipe -).reference.info.fully_qualified_name +_GETATTR = cast(atomic_recipe.AtomicRecipe, std.getattr_.recipe) +_GETATTR_FQN = _GETATTR.reference.info.fully_qualified_name + +# Port names are derived from the recipe, never spelled out: editing `std.getattr_` +# must not require editing strings anywhere else in the source. +OBJ_PORT, NAME_PORT = _GETATTR.inputs +(ATTR_PORT,) = _GETATTR.outputs def is_std_getattr(node: union_types.RecipeDiscrimination) -> bool: @@ -32,8 +32,8 @@ def is_std_getattr(node: union_types.RecipeDiscrimination) -> bool: return ( isinstance(node, atomic_recipe.AtomicRecipe) and node.reference.info.fully_qualified_name == _GETATTR_FQN - and node.inputs == [OBJ_PORT, NAME_PORT] - and node.outputs == [ATTR_PORT] + and node.inputs == _GETATTR.inputs + and node.outputs == _GETATTR.outputs ) diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py index e9399849..97ea4e97 100644 --- a/src/flowrep/parsers/attribute_parser.py +++ b/src/flowrep/parsers/attribute_parser.py @@ -42,9 +42,11 @@ from flowrep.parsers import constant_parser, label_helpers, symbol_scope from flowrep.prospective import std, union_types -_OBJ_PORT = "obj" -_NAME_PORT = "name" -_ATTR_PORT = "attr" +# Port names are derived from the recipe, never spelled out: editing `std.getattr_` +# must not require editing strings anywhere else in the source. +_GETATTR = std.getattr_.recipe +_OBJ_PORT, _NAME_PORT = _GETATTR.inputs +(_ATTR_PORT,) = _GETATTR.outputs def chain_root(node: ast.expr) -> ast.Name | None: @@ -67,11 +69,6 @@ def is_data_attribute(node: ast.expr, symbol_map: symbol_scope.SymbolScope) -> b return root is not None and root.id in symbol_map -def attribute_name(node: ast.Attribute) -> str: - """The outermost attribute name, e.g. ``dc.a.val`` -> ``"val"``.""" - return node.attr - - def inject_attribute_chain( node: ast.expr, symbol_map: symbol_scope.SymbolScope, diff --git a/tests/unit/compiler/test_sugar.py b/tests/unit/compiler/test_sugar.py index 3d749509..4c6cea88 100644 --- a/tests/unit/compiler/test_sugar.py +++ b/tests/unit/compiler/test_sugar.py @@ -89,5 +89,17 @@ def test_none_when_name_port_unwired(self): self.assertIsNone(sugar.attribute_name("getattr_a_0", recipe)) +class TestPortConstantsTrackTheRecipe(unittest.TestCase): + """Editing std.getattr_'s ports must not require editing strings in src/.""" + + def test_input_ports_come_from_the_recipe(self): + self.assertEqual( + (sugar.OBJ_PORT, sugar.NAME_PORT), tuple(std.getattr_.recipe.inputs) + ) + + def test_output_port_comes_from_the_recipe(self): + self.assertEqual((sugar.ATTR_PORT,), tuple(std.getattr_.recipe.outputs)) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_attribute_parser.py b/tests/unit/parsers/test_attribute_parser.py index c2268ade..14194f36 100644 --- a/tests/unit/parsers/test_attribute_parser.py +++ b/tests/unit/parsers/test_attribute_parser.py @@ -59,18 +59,6 @@ def test_symbol_scope_shadows_object_scope(self): self.assertTrue(attribute_parser.is_data_attribute(_expr("os.path"), scope)) -class TestAttributeName(unittest.TestCase): - def test_outermost_link(self): - node = _expr("dc.a.val") - assert isinstance(node, ast.Attribute) - self.assertEqual(attribute_parser.attribute_name(node), "val") - - def test_single_link(self): - node = _expr("dc.a") - assert isinstance(node, ast.Attribute) - self.assertEqual(attribute_parser.attribute_name(node), "a") - - class TestInjectAttributeChain(unittest.TestCase): def test_single_link_adds_getattr_and_constant(self): scope = symbol_scope.SymbolScope( @@ -179,6 +167,28 @@ def test_noop_for_plain_call(self): attribute_parser.reject_method_call(call, scope) # does not raise +class TestInjectionUsesTheRecipePorts(unittest.TestCase): + """The injected edges must target whatever ports std.getattr_ actually declares.""" + + def test_edges_and_handle_track_the_recipe(self): + obj_port, name_port = std.getattr_.recipe.inputs + (attr_port,) = std.getattr_.recipe.outputs + scope = symbol_scope.SymbolScope( + {"dc": _make_source("MyDataclass_0", "instance")} + ) + nodes: dict = {} + + handle = attribute_parser.inject_attribute_chain(_expr("dc.a"), scope, nodes) + + self.assertEqual(handle, _make_source("getattr_a_0", attr_port)) + self.assertIn( + edge_models.TargetHandle(node="getattr_a_0", port=obj_port), scope.edges + ) + self.assertIn( + edge_models.TargetHandle(node="getattr_a_0", port=name_port), scope.edges + ) + + class TestRejectUnboundAttribute(unittest.TestCase): def _scope(self) -> symbol_scope.SymbolScope: return symbol_scope.SymbolScope( From 0dc663e2d67a0fde1bd6e8f3018f8d9f33dd3b3a Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 14:00:38 -0700 Subject: [PATCH 07/14] Test mixed workflow return Co-authored-by: Claude Signed-off-by: Liam Huber --- tests/unit/parsers/test_workflow_parser.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index 888a2bfa..f642fcd8 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1460,6 +1460,18 @@ def wf(x0: int, comp: library.ComplexData): with self.assertRaises(ValueError): workflow_parser.parse_workflow(wf) + def test_return_of_tuple_mixing_symbol_and_access_raises(self): + def wf(x0: int, comp: library.ComplexData): + dc = library.MyDataclass(comp, x0) + v = dc.a + return v, dc.x + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + message = str(ctx.exception) + self.assertIn("has no symbol to take it from", message) + self.assertIn("dc.x", message) + def test_bound_access_returns_under_its_symbol(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) From 37aa4883ccfa2e94ddf99de84ee8782d1c9a62b2 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 15:03:33 -0700 Subject: [PATCH 08/14] Squash bug on constants in condition calls Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/parser_helpers.py | 19 ++++--- .../parsers/test_parsing_constants.py | 49 ++++++++++++++++++- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index d43ba8ed..1b6fbb98 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -237,16 +237,19 @@ def _bind_condition_constant( ) -> None: """Expose a literal condition argument as a synthetic flow-input port. - The synthetic port name is unique across the condition's real argument symbols - (``scope.inputs``) and every synthetic port already allocated for this - flow-control chain (*reserved_ports*), so it is a deterministic function of the - source and round-trips exactly. The ``ConstantRecipe`` is built eagerly (so a - non-JSON literal such as a tuple raises ``ConstantParseError`` with call-site - context here, matching non-condition timing) and handed up for the enclosing - walker to attach as a peer. + The port name is unique against every symbol in the enclosing scope + (``set(scope)``) and every port already generated for this flow-control chain + (*reserved_ports*), so it is a deterministic function of the source and + round-trips exactly. Deduping against the scope rather than the condition's own + arguments is what makes it airtight: a flow-control node's inputs are its + condition's inputs *plus its body's*, and a body input port is always an + enclosing symbol, so a port that dodges every symbol cannot collide with a real + one. The ``ConstantRecipe`` is built eagerly (so a non-JSON literal such as a + tuple raises ``ConstantParseError`` with call-site context here, matching + non-condition timing) and handed up for the enclosing walker to attach as a peer. """ synthetic_port = label_helpers.unique_suffix( - constant_recipe.ConstantRecipe.std_label, set(scope.inputs) | reserved_ports + constant_recipe.ConstantRecipe.std_label, set(scope) | reserved_ports ) reserved_ports.add(synthetic_port) condition_bindings[synthetic_port] = constant_parser.make_constant( diff --git a/tests/integration/parsers/test_parsing_constants.py b/tests/integration/parsers/test_parsing_constants.py index 9ded0218..6e602626 100644 --- a/tests/integration/parsers/test_parsing_constants.py +++ b/tests/integration/parsers/test_parsing_constants.py @@ -2,7 +2,7 @@ import pydantic -from flowrep import wfms +from flowrep import edge_models, wfms from flowrep.compiler import source from flowrep.parsers import workflow_parser from flowrep.prospective import union_types @@ -38,5 +38,52 @@ def test_parse_serialize_run_compile(self): ) +@workflow_parser.workflow +def shadowed_constant_symbol(x): + """A user symbol named like a generated port must not be clobbered by one.""" + constant_0 = library.negate(x) + if library.my_condition(x, 3): + y = library.my_add(constant_0, x) + else: + y = library.identity(x) + return y + + +class TestGeneratedPortDodgesUserSymbols(unittest.TestCase): + """A flow-control node's inputs include its *body's* inputs, so a generated port + must dodge every enclosing symbol -- not just the condition's arguments.""" + + def test_generated_port_dodges_the_user_symbol(self): + recipe = shadowed_constant_symbol.flowrep_recipe + # The literal 3's port steps aside for the user's `constant_0`. + self.assertEqual(recipe.nodes["if_0"].inputs, ["x", "constant_1", "constant_0"]) + + def test_body_reads_the_user_symbol_not_the_literal(self): + recipe = shadowed_constant_symbol.flowrep_recipe + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="if_0", port="constant_0")], + edge_models.SourceHandle(node="negate_0", port="output_0"), + ) + + def test_generated_port_reads_the_constant_peer(self): + recipe = shadowed_constant_symbol.flowrep_recipe + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="if_0", port="constant_1")], + edge_models.SourceHandle(node="constant_0", port="constant"), + ) + + def test_executes_via_run_recipe(self): + result = wfms.run_recipe(shadowed_constant_symbol.flowrep_recipe, x=1) + self.assertEqual(result.output_ports["y"].value, shadowed_constant_symbol(1)) + + def test_round_trips_through_source(self): + free = makers.reference_free(shadowed_constant_symbol) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + if __name__ == "__main__": unittest.main() From 846a69deeacd064d881cd8891b9daefba23a0eaa Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 15:10:46 -0700 Subject: [PATCH 09/14] Refactor: introduce type for flow control bindings Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/case_helpers.py | 6 +-- src/flowrep/parsers/for_parser.py | 23 ++++++----- src/flowrep/parsers/if_parser.py | 10 ++--- src/flowrep/parsers/parser_helpers.py | 20 ++++++++-- src/flowrep/parsers/try_parser.py | 21 +++++----- src/flowrep/parsers/while_parser.py | 4 +- src/flowrep/parsers/workflow_parser.py | 53 +++++++++++++++----------- 7 files changed, 83 insertions(+), 54 deletions(-) diff --git a/src/flowrep/parsers/case_helpers.py b/src/flowrep/parsers/case_helpers.py index c79b3658..c0207bc0 100644 --- a/src/flowrep/parsers/case_helpers.py +++ b/src/flowrep/parsers/case_helpers.py @@ -14,7 +14,7 @@ parser_protocol, symbol_scope, ) -from flowrep.prospective import constant_recipe, helper_models, union_types +from flowrep.prospective import helper_models, union_types def parse_case( @@ -28,7 +28,7 @@ def parse_case( ) -> tuple[ helper_models.LabeledRecipe, edge_models.InputEdges, - dict[str, constant_recipe.ConstantRecipe], + parser_helpers.FlowControlBindings, ]: """ Parse a conditional expression. @@ -50,7 +50,7 @@ def parse_case( ) scope_copy = symbol_map.fork() - condition_bindings: dict[str, constant_recipe.ConstantRecipe] = {} + condition_bindings: parser_helpers.FlowControlBindings = {} parser_helpers.consume_call_arguments( scope_copy, test, diff --git a/src/flowrep/parsers/for_parser.py b/src/flowrep/parsers/for_parser.py index 5a401aef..3c57cd8d 100644 --- a/src/flowrep/parsers/for_parser.py +++ b/src/flowrep/parsers/for_parser.py @@ -4,7 +4,7 @@ from typing import NamedTuple from flowrep import edge_models -from flowrep.parsers import parser_protocol, symbol_scope +from flowrep.parsers import parser_helpers, parser_protocol, symbol_scope from flowrep.prospective import for_recipe, helper_models FOR_BODY_LABEL: str = "body" @@ -25,7 +25,7 @@ class _IterationAxis(NamedTuple): def parse_for_node( walker: parser_protocol.BodyWalker, tree: ast.For -) -> for_recipe.ForEachRecipe: +) -> tuple[for_recipe.ForEachRecipe, parser_helpers.FlowControlBindings]: """ Walk a for-loop. @@ -65,14 +65,17 @@ def parse_for_node( label=FOR_BODY_LABEL, recipe=body_walker.build_model() ) - return for_recipe.ForEachRecipe( - inputs=inputs, - outputs=outputs, - body_node=body_node, - input_edges=input_edges, - output_edges=output_edges, - nested_ports=nested_ports, - zipped_ports=zipped_ports, + return ( + for_recipe.ForEachRecipe( + inputs=inputs, + outputs=outputs, + body_node=body_node, + input_edges=input_edges, + output_edges=output_edges, + nested_ports=nested_ports, + zipped_ports=zipped_ports, + ), + {}, ) diff --git a/src/flowrep/parsers/if_parser.py b/src/flowrep/parsers/if_parser.py index c057d56c..65137d5e 100644 --- a/src/flowrep/parsers/if_parser.py +++ b/src/flowrep/parsers/if_parser.py @@ -4,8 +4,8 @@ import dataclasses from flowrep import edge_models -from flowrep.parsers import case_helpers, parser_protocol -from flowrep.prospective import constant_recipe, helper_models, if_recipe +from flowrep.parsers import case_helpers, parser_helpers, parser_protocol +from flowrep.prospective import helper_models, if_recipe IF_CONDITION_LABEL_PREFIX: str = "condition" IF_BODY_LABEL_PREFIX: str = "body" @@ -19,12 +19,12 @@ class _CaseComponents: condition: helper_models.LabeledRecipe condition_input_edges: edge_models.InputEdges body: case_helpers.WalkedBranch - condition_bindings: dict[str, constant_recipe.ConstantRecipe] + condition_bindings: parser_helpers.FlowControlBindings def parse_if_node( walker: parser_protocol.BodyWalker, tree: ast.If -) -> tuple[if_recipe.IfRecipe, dict[str, constant_recipe.ConstantRecipe]]: +) -> tuple[if_recipe.IfRecipe, parser_helpers.FlowControlBindings]: """ Walk an if/elif/else chain. @@ -83,7 +83,7 @@ def parse_if_node( for cc in cases ] - condition_bindings: dict[str, constant_recipe.ConstantRecipe] = {} + condition_bindings: parser_helpers.FlowControlBindings = {} for cc in cases: condition_bindings.update(cc.condition_bindings) diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index 1b6fbb98..4ec4cff7 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -6,7 +6,7 @@ import textwrap from collections.abc import Callable, Iterable from types import FunctionType -from typing import Any, TypeVar, cast +from typing import Any, TypeAlias, TypeVar, cast from flowrep import base_models, edge_models from flowrep.parsers import ( @@ -24,6 +24,20 @@ class SourceCodeUnavailableError(ValueError): ... _D = TypeVar("_D") +FlowControlBindings: TypeAlias = dict[ + str, constant_recipe.ConstantRecipe | edge_models.SourceHandle +] +"""Peers the enclosing walker must wire into a flow-control node's generated ports. + +A flow-control recipe has no room to host a peer node inside it, so a literal or an +attribute chain in a condition becomes a peer of the flow-control node *one level up*, +reaching it through a generated input port. + +A ``ConstantRecipe`` value means "create this peer node and wire it to the port"; a +``SourceHandle`` means "the peer already exists in your ``nodes`` -- just wire it". +""" + + def apply_label_decorator( func: _D | str | None, output_labels: tuple[str, ...], @@ -161,7 +175,7 @@ def consume_call_arguments( child: helper_models.LabeledRecipe, nodes: union_types.Recipes, *, - condition_bindings: dict[str, constant_recipe.ConstantRecipe] | None = None, + condition_bindings: FlowControlBindings | None = None, reserved_ports: set[str] | None = None, hoisted: dict[ast.expr, edge_models.SourceHandle] | None = None, ) -> None: @@ -232,7 +246,7 @@ def _bind_condition_constant( child: helper_models.LabeledRecipe, consumer_port: str, value: Any, - condition_bindings: dict[str, constant_recipe.ConstantRecipe], + condition_bindings: FlowControlBindings, reserved_ports: set[str], ) -> None: """Expose a literal condition argument as a synthetic flow-input port. diff --git a/src/flowrep/parsers/try_parser.py b/src/flowrep/parsers/try_parser.py index 753477bc..8112310e 100644 --- a/src/flowrep/parsers/try_parser.py +++ b/src/flowrep/parsers/try_parser.py @@ -4,7 +4,7 @@ from pyiron_snippets import versions -from flowrep.parsers import case_helpers, object_scope, parser_protocol +from flowrep.parsers import case_helpers, object_scope, parser_helpers, parser_protocol from flowrep.prospective import helper_models, try_recipe TRY_BODY_LABEL: str = "try_body" @@ -13,7 +13,7 @@ def parse_try_node( walker: parser_protocol.BodyWalker, tree: ast.Try -) -> try_recipe.TryRecipe: +) -> tuple[try_recipe.TryRecipe, parser_helpers.FlowControlBindings]: """ Walk a try/except block. @@ -59,13 +59,16 @@ def parse_try_node( for exceptions, branch in zip(exception_groups, except_branches, strict=True) ] - return try_recipe.TryRecipe( - inputs=inputs, - outputs=outputs, - try_node=try_branch.to_labeled_node(), - exception_cases=exception_cases, - input_edges=input_edges, - prospective_output_edges=prospective_output_edges, + return ( + try_recipe.TryRecipe( + inputs=inputs, + outputs=outputs, + try_node=try_branch.to_labeled_node(), + exception_cases=exception_cases, + input_edges=input_edges, + prospective_output_edges=prospective_output_edges, + ), + {}, ) diff --git a/src/flowrep/parsers/while_parser.py b/src/flowrep/parsers/while_parser.py index 339d6422..3b166f1c 100644 --- a/src/flowrep/parsers/while_parser.py +++ b/src/flowrep/parsers/while_parser.py @@ -5,7 +5,7 @@ from flowrep import edge_models from flowrep.parsers import case_helpers, parser_helpers, parser_protocol -from flowrep.prospective import constant_recipe, helper_models, while_recipe +from flowrep.prospective import helper_models, while_recipe WHILE_CONDITION_LABEL: str = "condition" WHILE_BODY_LABEL: str = "body" @@ -13,7 +13,7 @@ def parse_while_node( walker: parser_protocol.BodyWalker, tree: ast.While -) -> tuple[while_recipe.WhileRecipe, dict[str, constant_recipe.ConstantRecipe]]: +) -> tuple[while_recipe.WhileRecipe, parser_helpers.FlowControlBindings]: """ Walk a while-loop. diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index 9dea1134..0029a641 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -347,52 +347,61 @@ def _digest_flow_control( self, label_prefix: str, node: union_types.RecipeDiscrimination, - condition_bindings: dict[str, constant_recipe.ConstantRecipe] | None = None, + bindings: parser_helpers.FlowControlBindings | None = None, ) -> None: label = label_helpers.unique_suffix(label_prefix, self.nodes) self.nodes[label] = node - self._connect_node_to_enclosing_scope(label, node, condition_bindings) + self._connect_node_to_enclosing_scope(label, node, bindings) def _connect_node_to_enclosing_scope( self, label: str, node: union_types.RecipeDiscrimination, - condition_bindings: dict[str, constant_recipe.ConstantRecipe] | None = None, + bindings: parser_helpers.FlowControlBindings | None = None, ): - bindings = condition_bindings or {} + bound = bindings or {} for port in node.inputs: - if port in bindings: - constant_label = constant_recipe.ConstantRecipe.std_label - peer_label = label_helpers.unique_suffix(constant_label, self.nodes) - self.nodes[peer_label] = bindings[port] - self.symbol_map.consume_source( - edge_models.SourceHandle(node=peer_label, port=constant_label), - label, - port, - ) - else: + binding = bound.get(port) + if binding is None: self.symbol_map.consume(port, label, port) + elif isinstance(binding, edge_models.SourceHandle): + self.symbol_map.consume_source(binding, label, port) + else: + self._attach_constant_peer(label, port, binding) labeled_node = helper_models.LabeledRecipe(label=label, recipe=node) self.symbol_map.register(new_symbols=node.outputs, child=labeled_node) + def _attach_constant_peer( + self, label: str, port: str, recipe: constant_recipe.ConstantRecipe + ) -> None: + """Create a constant peer of the flow-control node and feed it into *port*.""" + constant_label = constant_recipe.ConstantRecipe.std_label + peer_label = label_helpers.unique_suffix(constant_label, self.nodes) + self.nodes[peer_label] = recipe + self.symbol_map.consume_source( + edge_models.SourceHandle(node=peer_label, port=constant_label), + label, + port, + ) + def visit_For(self, tree: ast.For) -> None: - for_recipe = for_parser.parse_for_node(self, tree) + for_recipe, bindings = for_parser.parse_for_node(self, tree) # Accumulators consumed by the for body are no longer available here self.symbol_map.declared_accumulators -= set(for_recipe.outputs) - self._digest_flow_control("for_each", for_recipe) + self._digest_flow_control("for_each", for_recipe, bindings) def visit_While(self, tree: ast.While) -> None: - while_recipe, condition_bindings = while_parser.parse_while_node(self, tree) - self._digest_flow_control("while", while_recipe, condition_bindings) + while_recipe, bindings = while_parser.parse_while_node(self, tree) + self._digest_flow_control("while", while_recipe, bindings) def visit_If(self, tree: ast.If) -> None: - if_recipe, condition_bindings = if_parser.parse_if_node(self, tree) - self._digest_flow_control("if", if_recipe, condition_bindings) + if_recipe, bindings = if_parser.parse_if_node(self, tree) + self._digest_flow_control("if", if_recipe, bindings) def visit_Try(self, tree: ast.Try) -> None: - try_recipe = try_parser.parse_try_node(self, tree) - self._digest_flow_control("try", try_recipe) + try_recipe, bindings = try_parser.parse_try_node(self, tree) + self._digest_flow_control("try", try_recipe, bindings) def visit_Expr(self, stmt: ast.Expr) -> None: if is_append_call(stmt.value): From 9b95dfa04f3ec7366fd7c1cb4f0e25787a2981f7 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 15:28:19 -0700 Subject: [PATCH 10/14] Relax using attribute accesses Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/attribute_parser.py | 18 +++ src/flowrep/parsers/case_helpers.py | 6 + src/flowrep/parsers/parser_helpers.py | 55 +++++-- src/flowrep/parsers/while_parser.py | 51 ++++++- tests/flowrep_static/library.py | 10 ++ .../parsers/test_parsing_attribute_access.py | 139 ++++++++++++++++++ tests/unit/parsers/test_workflow_parser.py | 26 +--- 7 files changed, 272 insertions(+), 33 deletions(-) diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py index 97ea4e97..4968a330 100644 --- a/src/flowrep/parsers/attribute_parser.py +++ b/src/flowrep/parsers/attribute_parser.py @@ -37,6 +37,7 @@ from __future__ import annotations import ast +from collections.abc import Iterable from flowrep import edge_models from flowrep.parsers import constant_parser, label_helpers, symbol_scope @@ -134,6 +135,23 @@ def hoist_call_arguments( return hoisted +def generate_port_name(node: ast.expr, taken: Iterable[str]) -> str: + """A deterministic port name for an attribute chain that has no symbol. + + Takes the outermost attribute name -- what the author would have called the + binding, and what the compiler will literally emit as the binding symbol -- and + unique-suffixes it against *taken*. ``x.a.val`` gives ``val_0``. + + *taken* must include every symbol in the enclosing scope, not merely the ports + allocated so far: a flow-control node's inputs are its condition's inputs *plus + its body's*, and a body input port is always an enclosing symbol. + """ + if not isinstance(node, ast.Attribute): # pragma: no cover - callers gate on + # is_data_attribute, which implies ast.Attribute + raise TypeError(f"Not an attribute chain: {ast.dump(node)}") + return label_helpers.unique_suffix(node.attr, taken) + + def reject_unbound_attribute( node: ast.expr, symbol_map: symbol_scope.SymbolScope, context: str ) -> None: diff --git a/src/flowrep/parsers/case_helpers.py b/src/flowrep/parsers/case_helpers.py index c0207bc0..5cf68a15 100644 --- a/src/flowrep/parsers/case_helpers.py +++ b/src/flowrep/parsers/case_helpers.py @@ -49,6 +49,11 @@ def parse_case( f"truthy), but got {condition.recipe.outputs}" ) + # A flow-control recipe has no room to host a peer, so an attribute argument + # becomes a getattr peer of the flow-control node in the *enclosing* scope, and + # reaches the condition through a generated input port. + hoisted = attribute_parser.hoist_call_arguments(test, symbol_map, nodes) + scope_copy = symbol_map.fork() condition_bindings: parser_helpers.FlowControlBindings = {} parser_helpers.consume_call_arguments( @@ -58,6 +63,7 @@ def parse_case( nodes, condition_bindings=condition_bindings, reserved_ports=reserved_ports, + hoisted=hoisted, ) relabeled_node, relabeled_inputs = _relabel_node_data( condition, scope_copy.input_edges, label diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index 4ec4cff7..860c025e 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -193,27 +193,34 @@ def consume_call_arguments( multiple literals -- across an if/elif chain -- get distinct, deterministic ports. Data-attribute arguments are injected ahead of the call by - ``attribute_parser.hoist_call_arguments`` and arrive here in *hoisted*, mapping - the argument's AST node to the ``SourceHandle`` of its outermost getattr node. The - call to ``attribute_parser.reject_unbound_attribute`` below is a no-op outside - condition mode: in normal mode every data-attribute argument was already hoisted - and returns from the first branch above, so it can only fire on a condition - argument, where there is no room for a peer getattr node inside the flow-control - node itself. + ``attribute_parser.hoist_call_arguments`` and arrive here in *hoisted*, mapping the + argument's AST node to the ``SourceHandle`` of its outermost getattr node. Outside + condition mode the consuming node is a peer of that getattr and simply reads from + it. In condition mode the consumer lives *inside* a flow-control recipe, which has + no room for a peer, so the handle is routed through a generated input port and + recorded in *condition_bindings* -- exactly as a literal is. """ reserved = set() if reserved_ports is None else reserved_ports already_hoisted = {} if hoisted is None else hoisted def _consume(arg_node: ast.expr, consumer_port: str) -> None: if (handle := already_hoisted.get(arg_node)) is not None: - scope.consume_source(handle, child.label, consumer_port) + if condition_bindings is None: + scope.consume_source(handle, child.label, consumer_port) + else: + _bind_condition_source( + scope, + arg_node, + child, + consumer_port, + handle, + condition_bindings, + reserved, + ) return if isinstance(arg_node, ast.Name): scope.consume(arg_node.id, child.label, consumer_port) return - attribute_parser.reject_unbound_attribute( - arg_node, scope, "passed to a flow-control condition" - ) is_literal, value = constant_parser.try_parse_constant(arg_node) if not is_literal: raise TypeError( @@ -275,6 +282,32 @@ def _bind_condition_constant( ) +def _bind_condition_source( + scope: symbol_scope.SymbolScope, + arg_node: ast.expr, + child: helper_models.LabeledRecipe, + consumer_port: str, + handle: edge_models.SourceHandle, + condition_bindings: FlowControlBindings, + reserved_ports: set[str], +) -> None: + """Expose a hoisted attribute chain as a generated flow-input port. + + The getattr peer already sits in the enclosing walker's ``nodes`` -- the + flow-control node just needs a port to receive it through. The ``SourceHandle`` + twin of :func:`_bind_condition_constant`; see :func:`attribute_parser.generated_port` + for why the name is deduped against every enclosing symbol. + """ + generated = attribute_parser.generate_port_name( + arg_node, set(scope) | reserved_ports + ) + reserved_ports.add(generated) + condition_bindings[generated] = handle + scope.consume_input_source( + edge_models.InputSource(port=generated), child.label, consumer_port + ) + + def reject_input_alias_outputs( body_symbol_map: symbol_scope.SymbolScope, candidate_outputs: Iterable[str], diff --git a/src/flowrep/parsers/while_parser.py b/src/flowrep/parsers/while_parser.py index 3b166f1c..9757c882 100644 --- a/src/flowrep/parsers/while_parser.py +++ b/src/flowrep/parsers/while_parser.py @@ -4,7 +4,13 @@ from ast import While from flowrep import edge_models -from flowrep.parsers import case_helpers, parser_helpers, parser_protocol +from flowrep.parsers import ( + attribute_parser, + case_helpers, + parser_helpers, + parser_protocol, + symbol_scope, +) from flowrep.prospective import helper_models, while_recipe WHILE_CONDITION_LABEL: str = "condition" @@ -41,6 +47,7 @@ def parse_while_node( reassigned_symbols = body_walker.symbol_map.reassigned_symbols _validate_some_output_exists(reassigned_symbols) + _reject_looped_attribute_roots(tree.test, walker.symbol_map, reassigned_symbols) parser_helpers.reject_input_alias_outputs( body_walker.symbol_map, reassigned_symbols, "while-loop" ) @@ -85,6 +92,48 @@ def _validate_some_output_exists(reassigned_symbols: list[str]): ) +def _reject_looped_attribute_roots( + test: ast.expr, + symbol_map: symbol_scope.SymbolScope, + reassigned_symbols: list[str], +) -> None: + """Raise if a condition attribute chain is rooted at a symbol the body reassigns. + + Python re-evaluates a while condition every iteration, so ``x.val`` is re-read + against the *updated* ``x``. A flowrep condition is a single call fed by hoisted + inputs: its getattr peer sits outside the loop, is never re-read, and is not a + while output, so it never feeds back. Rather than silently diverge from Python we + refuse. An attribute on a symbol the loop does not touch hoists faithfully and is + allowed -- which is why the guard is on the *root*, not on attribute access. + + Runs after the body walk, because ``reassigned_symbols`` is the parser's ground + truth (it includes symbols reassigned by nested flow control, not just bare + assignments). By then the getattr peers have already been injected into the + enclosing scope; that is harmless, since the exception aborts the whole parse. + """ + if not isinstance( + test, ast.Call + ): # pragma: no cover - parse_case rejects non-calls + return + looped = set(reassigned_symbols) + arguments = list(test.args) + [kw.value for kw in test.keywords] + for argument in arguments: + if not attribute_parser.is_data_attribute(argument, symbol_map): + continue + root = attribute_parser.chain_root(argument) + if root is not None and root.id in looped: + chain = ast.unparse(argument) + raise ValueError( + f"While-condition attribute access {chain!r} is rooted at " + f"{root.id!r}, which the loop body reassigns. Python would re-read " + f"{chain!r} every iteration, but a flowrep while-condition is a " + f"single call fed by hoisted inputs -- there is no place inside the " + f"loop for the attribute access. Either bind it outside the loop " + f"(e.g. `v = {chain}`) if you meant to read it once, or move the " + f"attribute access into the condition function itself." + ) + + def _wire_inputs( body_walker: parser_protocol.BodyWalker, condition_inputs: edge_models.InputEdges, diff --git a/tests/flowrep_static/library.py b/tests/flowrep_static/library.py index 5e6ad214..54f704e5 100644 --- a/tests/flowrep_static/library.py +++ b/tests/flowrep_static/library.py @@ -161,6 +161,16 @@ def __init__(self, val: int = 0): self.val = val +class Payload: + """A plain (non-recipe) payload class with a list and two scalars, reached via + attribute access.""" + + def __init__(self, xs: list | None = None, num: int = 1, den: int = 1): + self.xs = [] if xs is None else xs + self.num = num + self.den = den + + @dataclass_parser.dataclass class MyDataclass: a: ComplexData diff --git a/tests/integration/parsers/test_parsing_attribute_access.py b/tests/integration/parsers/test_parsing_attribute_access.py index 66bb4b12..f014e113 100644 --- a/tests/integration/parsers/test_parsing_attribute_access.py +++ b/tests/integration/parsers/test_parsing_attribute_access.py @@ -82,5 +82,144 @@ def test_compiled_source_is_sugared(self): self.assertNotIn("_getattr_wrapper", rendered.source) +@workflow_parser.workflow +def if_attribute_condition(comp: library.ComplexData, x: int): + if library.my_condition(comp.val, 3): + y = library.increment(x) + else: + y = library.decrement(x) + return y + + +@workflow_parser.workflow +def if_bound_condition(comp: library.ComplexData, x: int): + """The form the parser forced yesterday, and the form the compiler still emits.""" + val_0 = comp.val + if library.my_condition(val_0, 3): # noqa: SIM108 + y = library.increment(x) + else: + y = library.decrement(x) + return y + + +@workflow_parser.workflow +def while_unlooped_attribute_condition(comp: library.ComplexData, seed: int): + """`comp` is never reassigned, so hoisting the getattr is faithful to Python.""" + x = library.identity(seed) + while library.my_condition(x, comp.val): + x = library.loop_inc(x) + return x + + +class TestAttributeInCondition(unittest.TestCase): + def test_generated_port_is_named_for_the_attribute(self): + recipe = if_attribute_condition.flowrep_recipe + self.assertEqual(recipe.nodes["if_0"].inputs, ["val_0", "constant_0", "x"]) + + def test_getattr_peer_feeds_the_generated_port(self): + recipe = if_attribute_condition.flowrep_recipe + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="if_0", port="val_0")], + edge_models.SourceHandle(node="getattr_val_0", port="attr"), + ) + + def test_identical_to_the_bound_form(self): + """The headline invariant: the sugar is not merely equivalent, it is equal.""" + self.assertEqual( + makers.dump_no_refs(makers.reference_free(if_attribute_condition)), + makers.dump_no_refs(makers.reference_free(if_bound_condition)), + ) + + def test_executes_via_run_recipe(self): + comp = library.ComplexData(val=7) + result = wfms.run_recipe(if_attribute_condition.flowrep_recipe, comp=comp, x=1) + self.assertEqual( + result.output_ports["y"].value, if_attribute_condition(comp, 1) + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(if_attribute_condition) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_compiles_back_to_the_bound_form(self): + rendered = source._workflow2python( + makers.reference_free(if_attribute_condition) + ) + self.assertRegex(rendered.source, r"\n\s*val_0 = comp\.val\n") + + +class TestAttributeInWhileCondition(unittest.TestCase): + def test_unlooped_root_is_allowed(self): + recipe = while_unlooped_attribute_condition.flowrep_recipe + self.assertEqual(recipe.nodes["while_0"].inputs, ["x", "val_0"]) + + def test_executes_via_run_recipe(self): + comp = library.ComplexData(val=7) + result = wfms.run_recipe( + while_unlooped_attribute_condition.flowrep_recipe, comp=comp, seed=1 + ) + self.assertEqual( + result.output_ports["x"].value, + while_unlooped_attribute_condition(comp, 1), + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(while_unlooped_attribute_condition) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + def test_looped_root_raises(self): + """Python would re-read `x.val` each iteration; a hoisted getattr cannot.""" + + def wf(comp: library.ComplexData): + x = library.identity(comp) + while library.my_condition(x, x.val): + x = library.identity(x) + return x + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + message = str(ctx.exception) + self.assertIn("x.val", message) + self.assertIn("re-read", message) + # Both honest fixes must be named -- only the author knows which they meant. + self.assertIn("bind it outside the loop", message.lower()) + self.assertIn("condition function", message.lower()) + + +@workflow_parser.workflow +def elif_two_attribute_conditions(a: library.ComplexData, b: library.ComplexData, x): + if library.my_condition(a.val, 3): + y = library.increment(x) + elif library.my_condition(b.val, 4): + y = library.decrement(x) + else: + y = library.negate(x) + return y + + +class TestAttributeInElifChain(unittest.TestCase): + def test_generated_ports_are_distinct_across_the_chain(self): + recipe = elif_two_attribute_conditions.flowrep_recipe + inputs = recipe.nodes["if_0"].inputs + self.assertEqual(inputs[:4], ["val_0", "constant_0", "val_1", "constant_1"]) + + def test_executes_via_run_recipe(self): + a, b = library.ComplexData(val=7), library.ComplexData(val=2) + result = wfms.run_recipe( + elif_two_attribute_conditions.flowrep_recipe, a=a, b=b, x=1 + ) + self.assertEqual( + result.output_ports["y"].value, elif_two_attribute_conditions(a, b, 1) + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index f642fcd8..911a7062 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1365,33 +1365,17 @@ def wf(comp: library.ComplexData): workflow_parser.parse_workflow(wf) self.assertIn("method", str(ctx.exception).lower()) - def test_attribute_chain_in_if_condition_argument_raises(self): + def test_attribute_chain_in_if_condition_argument_generates_a_port(self): def wf(x0: int, comp: library.ComplexData): dc = library.MyDataclass(comp, x0) if library.is_positive(dc.x): # noqa: SIM108 - y = x0 + y = library.identity(x0) else: - y = x0 + y = library.identity(x0) return y - with self.assertRaises(ValueError) as ctx: - workflow_parser.parse_workflow(wf) - message = str(ctx.exception) - self.assertIn("has no symbol to take it from", message) - self.assertIn("dc.x", message) - - def test_attribute_chain_in_while_condition_argument_raises(self): - def wf(x0: int, comp: library.ComplexData): - dc = library.MyDataclass(comp, x0) - while library.is_positive(dc.x): - x0 = x0 - return x0 - - with self.assertRaises(ValueError) as ctx: - workflow_parser.parse_workflow(wf) - message = str(ctx.exception) - self.assertIn("has no symbol to take it from", message) - self.assertIn("dc.x", message) + node = workflow_parser.parse_workflow(wf) + self.assertEqual(node.nodes["if_0"].inputs[0], "x_0") def test_bound_access_as_if_condition_input(self): def wf(x0: int, comp: library.ComplexData): From 082e89cb0aef29086dadeb7c431ca7962eb9a2c0 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 15:44:46 -0700 Subject: [PATCH 11/14] Accept attribute access in for-headers Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/for_parser.py | 129 ++++++++++++------ src/flowrep/parsers/parser_helpers.py | 5 +- src/flowrep/parsers/symbol_scope.py | 19 ++- .../parsers/test_parsing_attribute_access.py | 118 ++++++++++++++++ tests/unit/parsers/test_for_parser.py | 91 +++++++----- 5 files changed, 283 insertions(+), 79 deletions(-) diff --git a/src/flowrep/parsers/for_parser.py b/src/flowrep/parsers/for_parser.py index 3c57cd8d..38b8193e 100644 --- a/src/flowrep/parsers/for_parser.py +++ b/src/flowrep/parsers/for_parser.py @@ -4,17 +4,29 @@ from typing import NamedTuple from flowrep import edge_models -from flowrep.parsers import parser_helpers, parser_protocol, symbol_scope -from flowrep.prospective import for_recipe, helper_models +from flowrep.parsers import ( + attribute_parser, + parser_helpers, + parser_protocol, + symbol_scope, +) +from flowrep.prospective import for_recipe, helper_models, union_types FOR_BODY_LABEL: str = "body" class _IterationAxis(NamedTuple): - """Holding the variable, x, and the collection xs, in statements like for x in xs""" + """One axis of `for x in xs`. + + *variable* is the loop variable (a *body* input port). *port* is the for-node + input port feeding it: a bare symbol lends its own name, an attribute chain gets a + generated one. *binding* is the getattr peer the enclosing walker must wire into + *port*, or ``None`` when the collection was a plain symbol. + """ variable: str - collection: str + port: str + binding: edge_models.SourceHandle | None = None AccumulatorMap = dict[str, str] @@ -34,14 +46,19 @@ def parse_for_node( tree: The top-level ``ast.For`` node (may contain immediately nested for-headers that declare additional iteration axes). """ - # Parse the iteration header — pure AST, no parser state needed - nested_iters, zipped_iters, body_tree = _parse_for_iterations(tree) + # Parse the iteration header — pure AST plus attribute-chain injection into the + # enclosing walker's own nodes/scope + nested_iters, zipped_iters, body_tree = _parse_for_iterations( + tree, walker.symbol_map, walker.nodes + ) all_iters = nested_iters + zipped_iters # When we fork the scope here, we replace iterated-over symbols with iteration - # variables, all as InputSources from the body's perspective + # variables, all as InputSources from the body's perspective. An attribute-chain + # axis has no parent symbol to remap, so its variable is added fresh instead. body_symbol_map = walker.symbol_map.fork( - {src: var for var, src in all_iters}, + {axis.port: axis.variable for axis in all_iters if axis.binding is None}, + added_symbols=[axis.variable for axis in all_iters if axis.binding is not None], available_accumulators=walker.symbol_map.declared_accumulators.copy(), ) @@ -55,8 +72,8 @@ def parse_for_node( all_iters, body_walker, consumed, walker.symbol_map ) - nested_ports = [var for var, _ in nested_iters] - zipped_ports = [var for var, _ in zipped_iters] + nested_ports = [axis.variable for axis in nested_iters] + zipped_ports = [axis.variable for axis in zipped_iters] inputs, input_edges = _wire_inputs(body_walker, all_iters) outputs, output_edges = _wire_outputs(body_walker, input_edges) @@ -65,6 +82,10 @@ def parse_for_node( label=FOR_BODY_LABEL, recipe=body_walker.build_model() ) + bindings: parser_helpers.FlowControlBindings = { + axis.port: axis.binding for axis in all_iters if axis.binding is not None + } + return ( for_recipe.ForEachRecipe( inputs=inputs, @@ -75,7 +96,7 @@ def parse_for_node( nested_ports=nested_ports, zipped_ports=zipped_ports, ), - {}, + bindings, ) @@ -94,7 +115,7 @@ def _validate_no_unused_iterators( An unused iterator likely indicates a bug; if the user only needs the structural effect (e.g. repetition count), they should make the dependency explicit. """ - iterating_symbols = {var for var, _ in all_iters} + iterating_symbols = {axis.variable for axis in all_iters} consumed_symbols = set(body_walker.inputs) | set(consumed.values()) if unused := iterating_symbols - consumed_symbols: raise ValueError( @@ -117,7 +138,7 @@ def _validate_no_leaked_reassignments( body_reassigned = set(body_walker.symbol_map.reassigned_symbols) accumulator_outputs = set(consumed) unreturned_reassignments = ( - body_reassigned - accumulator_outputs - {var for var, _ in all_iters} + body_reassigned - accumulator_outputs - {axis.variable for axis in all_iters} ) leaked_reassignments = unreturned_reassignments.intersection(symbol_map.keys()) if leaked_reassignments: @@ -137,9 +158,9 @@ def _wire_inputs( s for s in body_walker.inputs if s not in set(consumed.values()) - and s not in {iterating_symbol for iterating_symbol, _ in all_iters} + and s not in {axis.variable for axis in all_iters} ] # Need to keep it consistently ordered, so don't use a simple set op - scattered_symbols = [scattered_symbol for _, scattered_symbol in all_iters] + scattered_symbols = [axis.port for axis in all_iters] inputs = broadcast_symbols + scattered_symbols broadcast_inputs = { edge_models.TargetHandle( @@ -149,9 +170,9 @@ def _wire_inputs( } scattered_inputs = { edge_models.TargetHandle( - node=FOR_BODY_LABEL, port=body_port - ): edge_models.InputSource(port=for_port) - for body_port, for_port in all_iters + node=FOR_BODY_LABEL, port=axis.variable + ): edge_models.InputSource(port=axis.port) + for axis in all_iters } input_edges = broadcast_inputs | scattered_inputs return inputs, input_edges @@ -176,26 +197,57 @@ def _wire_outputs( return outputs, output_edges +def _resolve_collection( + iter_expr: ast.expr, + symbol_map: symbol_scope.SymbolScope, + nodes: union_types.Recipes, + reserved_ports: set[str], +) -> tuple[str, edge_models.SourceHandle | None]: + """The for-node input port that feeds one iteration axis, plus any peer to wire. + + A for recipe has no room to host a peer, so an attribute chain becomes a getattr + peer of the for node in the enclosing scope and reaches it through a generated + port. See :func:`attribute_parser.generate_port_name` for the naming rule. + """ + if isinstance(iter_expr, ast.Name): + return iter_expr.id, None + if attribute_parser.is_data_attribute(iter_expr, symbol_map): + handle = attribute_parser.inject_attribute_chain(iter_expr, symbol_map, nodes) + port = attribute_parser.generate_port_name( + iter_expr, set(symbol_map) | reserved_ports + ) + reserved_ports.add(port) + return port, handle + raise ValueError( + f"For iteration must iterate over a symbol, or an attribute of one, but got " + f"'{ast.unparse(iter_expr)}'." + ) + + def _parse_for_iterations( for_stmt: ast.For, + symbol_map: symbol_scope.SymbolScope, + nodes: union_types.Recipes, ) -> tuple[list[_IterationAxis], list[_IterationAxis], ast.For]: """ Parse for-node iteration structure, handling zip and immediately nested iterations. - Returns (nested_iterations, zipped_iterations) where each is a list of - (variable_name, source_symbol) tuples. + Returns (nested_iterations, zipped_iterations, innermost_for_tree). """ nested: list[_IterationAxis] = [] zipped: list[_IterationAxis] = [] + reserved_ports: set[str] = set() current = for_stmt while isinstance(current, ast.For): - is_zip, pairs = _parse_single_for_header(current) + is_zip, axes = _parse_single_for_header( + current, symbol_map, nodes, reserved_ports + ) if is_zip: - zipped.extend(pairs) + zipped.extend(axes) else: - nested.extend(pairs) + nested.extend(axes) # Check for nested for-declaration (single statement that's another For) if len(current.body) >= 1 and isinstance(current.body[0], ast.For): @@ -208,11 +260,14 @@ def _parse_for_iterations( def _parse_single_for_header( for_stmt: ast.For, + symbol_map: symbol_scope.SymbolScope, + nodes: union_types.Recipes, + reserved_ports: set[str], ) -> tuple[bool, list[_IterationAxis]]: """ Parse a single for-header. - Returns (is_zipped, [(var, source), ...]). + Returns (is_zipped, axes). """ iter_expr = for_stmt.iter target = for_stmt.target @@ -226,30 +281,22 @@ def _parse_single_for_header( if len(vars_list) != len(target.elts): raise ValueError("zip() iteration targets must be simple names") - sources = [] - for arg in iter_expr.args: - if not isinstance(arg, ast.Name): - raise ValueError("zip() arguments must be simple symbols") - sources.append(arg.id) - - if len(vars_list) != len(sources): + if len(vars_list) != len(iter_expr.args): raise ValueError( f"zip() variable count ({len(vars_list)}) must match " - f"argument count ({len(sources)})" + f"argument count ({len(iter_expr.args)})" ) - return True, [ - _IterationAxis(v, s) for v, s in zip(vars_list, sources, strict=True) - ] + axes = [] + for variable, arg in zip(vars_list, iter_expr.args, strict=True): + port, binding = _resolve_collection(arg, symbol_map, nodes, reserved_ports) + axes.append(_IterationAxis(variable, port, binding)) + return True, axes - # Simple iteration: for x in xs - if not isinstance(iter_expr, ast.Name): - raise ValueError( - "For iteration must iterate over a symbol (not an inline expression)" - ) + port, binding = _resolve_collection(iter_expr, symbol_map, nodes, reserved_ports) if isinstance(target, ast.Name): - return False, [_IterationAxis(target.id, iter_expr.id)] + return False, [_IterationAxis(target.id, port, binding)] elif isinstance(target, ast.Tuple): # for a, b in items (tuple unpacking without zip) raise ValueError( diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index 860c025e..a607f0fe 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -295,8 +295,9 @@ def _bind_condition_source( The getattr peer already sits in the enclosing walker's ``nodes`` -- the flow-control node just needs a port to receive it through. The ``SourceHandle`` - twin of :func:`_bind_condition_constant`; see :func:`attribute_parser.generated_port` - for why the name is deduped against every enclosing symbol. + twin of :func:`_bind_condition_constant`; see + :func:`attribute_parser.generate_port_name` for why the name is deduped against + every enclosing symbol. """ generated = attribute_parser.generate_port_name( arg_node, set(scope) | reserved_ports diff --git a/src/flowrep/parsers/symbol_scope.py b/src/flowrep/parsers/symbol_scope.py index f93920fe..cd87d3b4 100644 --- a/src/flowrep/parsers/symbol_scope.py +++ b/src/flowrep/parsers/symbol_scope.py @@ -1,5 +1,5 @@ import dataclasses -from collections.abc import Mapping +from collections.abc import Iterable, Mapping from flowrep import edge_models from flowrep.prospective import helper_models @@ -292,6 +292,7 @@ def use_accumulator(self, accumulator_symbol: str, appended_symbol: str) -> None def fork( self, symbol_remap: dict[str, str] | None = None, + added_symbols: Iterable[str] | None = None, available_accumulators: set[str] | None = None, ) -> "SymbolScope": """ @@ -302,6 +303,11 @@ def fork( symbols in transit (e.g. a for-loop replacing the iterable symbol with the iteration variable). + *added_symbols* introduces symbols the child has but the parent does not: a + for-loop variable whose collection was an attribute chain, and so had no + parent symbol to remap. They shadow a parent symbol of the same name, exactly + as the loop variable does in plain Python. + Accumulator propagation is controlled explicitly via *available_accumulators*. For-loop bodies pass the parent's ``declared_accumulators`` so the body can ``.append()``; while-loop @@ -314,11 +320,14 @@ def fork( access is caught with a clear error rather than silently ignored. """ remap = {} if symbol_remap is None else symbol_remap + sources: dict[str, edge_models.InputSource | edge_models.SourceHandle] = { + (k := remap.get(key, key)): edge_models.InputSource(port=k) + for key in self._sources + } + for symbol in added_symbols or (): + sources[symbol] = edge_models.InputSource(port=symbol) return SymbolScope( - { - (k := remap.get(key, key)): edge_models.InputSource(port=k) - for key in self._sources - }, + sources, available_accumulators=available_accumulators, reserved_accumulators=self.reserved_accumulators | self.available_accumulators, diff --git a/tests/integration/parsers/test_parsing_attribute_access.py b/tests/integration/parsers/test_parsing_attribute_access.py index f014e113..7f0169ca 100644 --- a/tests/integration/parsers/test_parsing_attribute_access.py +++ b/tests/integration/parsers/test_parsing_attribute_access.py @@ -221,5 +221,123 @@ def test_executes_via_run_recipe(self): ) +@workflow_parser.workflow +def for_attribute_iterable(holder: library.Payload): + ys = [] + for n in holder.xs: + y = library.increment(n) + ys.append(y) + return ys + + +@workflow_parser.workflow +def for_bound_iterable(holder: library.Payload): + xs_0 = holder.xs + ys = [] + for n in xs_0: + y = library.increment(n) + ys.append(y) + return ys + + +@workflow_parser.workflow +def for_zipped_attribute_iterables(a: library.Payload, b: library.Payload): + zs = [] + for m, n in zip(a.xs, b.xs, strict=True): + z = library.my_add(m, n) + zs.append(z) + return zs + + +@workflow_parser.workflow +def for_nested_attribute_iterables(a: library.Payload, b: library.Payload): + zs = [] + for m in a.xs: + for n in b.xs: + z = library.my_add(m, n) + zs.append(z) + return zs + + +class TestAttributeAsForIterable(unittest.TestCase): + def test_generated_port_is_named_for_the_attribute(self): + recipe = for_attribute_iterable.flowrep_recipe + self.assertEqual(recipe.nodes["for_each_0"].inputs, ["xs_0"]) + + def test_getattr_peer_feeds_the_generated_port(self): + recipe = for_attribute_iterable.flowrep_recipe + self.assertEqual( + recipe.edges[edge_models.TargetHandle(node="for_each_0", port="xs_0")], + edge_models.SourceHandle(node="getattr_xs_0", port="attr"), + ) + + def test_identical_to_the_bound_form(self): + self.assertEqual( + makers.dump_no_refs(makers.reference_free(for_attribute_iterable)), + makers.dump_no_refs(makers.reference_free(for_bound_iterable)), + ) + + def test_executes_via_run_recipe(self): + holder = library.Payload(xs=[1, 2, 3]) + result = wfms.run_recipe(for_attribute_iterable.flowrep_recipe, holder=holder) + self.assertEqual( + result.output_ports["ys"].value, for_attribute_iterable(holder) + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(for_attribute_iterable) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + +class TestAttributesInZippedForIterables(unittest.TestCase): + def test_generated_ports_are_distinct(self): + recipe = for_zipped_attribute_iterables.flowrep_recipe + self.assertEqual(recipe.nodes["for_each_0"].inputs, ["xs_0", "xs_1"]) + + def test_executes_via_run_recipe(self): + a, b = library.Payload(xs=[1, 2]), library.Payload(xs=[10, 20]) + result = wfms.run_recipe( + for_zipped_attribute_iterables.flowrep_recipe, a=a, b=b + ) + self.assertEqual( + result.output_ports["zs"].value, for_zipped_attribute_iterables(a, b) + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(for_zipped_attribute_iterables) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + +class TestAttributesInNestedForIterables(unittest.TestCase): + def test_generated_ports_are_distinct(self): + recipe = for_nested_attribute_iterables.flowrep_recipe + self.assertEqual(recipe.nodes["for_each_0"].inputs, ["xs_0", "xs_1"]) + + def test_executes_via_run_recipe(self): + a, b = library.Payload(xs=[1, 2]), library.Payload(xs=[10, 20]) + result = wfms.run_recipe( + for_nested_attribute_iterables.flowrep_recipe, a=a, b=b + ) + self.assertEqual( + result.output_ports["zs"].value, for_nested_attribute_iterables(a, b) + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(for_nested_attribute_iterables) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_for_parser.py b/tests/unit/parsers/test_for_parser.py index 1303bca4..b7a42b89 100644 --- a/tests/unit/parsers/test_for_parser.py +++ b/tests/unit/parsers/test_for_parser.py @@ -11,7 +11,7 @@ import unittest from flowrep import edge_models -from flowrep.parsers import atomic_parser, for_parser, workflow_parser +from flowrep.parsers import atomic_parser, for_parser, symbol_scope, workflow_parser from flowrep.prospective import ( atomic_recipe, for_recipe, @@ -80,32 +80,40 @@ def test_attribute_call_not_zip(self): class TestParseSingleForHeader(unittest.TestCase): """Direct tests for every branch in _parse_single_for_header.""" + @staticmethod + def _call(stmt: ast.For): + scope = symbol_scope.SymbolScope({}) + nodes: dict = {} + return for_parser._parse_single_for_header(stmt, scope, nodes, set()) + # --- happy paths --- def test_simple_iteration(self): stmt = _parse_for_stmt("for x in xs: pass") - is_zip, pairs = for_parser._parse_single_for_header(stmt) + is_zip, pairs = self._call(stmt) self.assertFalse(is_zip) - self.assertEqual(pairs, [("x", "xs")]) + self.assertEqual(pairs, [("x", "xs", None)]) def test_zip_two_elements(self): stmt = _parse_for_stmt("for a, b in zip(xs, ys): pass") - is_zip, pairs = for_parser._parse_single_for_header(stmt) + is_zip, pairs = self._call(stmt) self.assertTrue(is_zip) - self.assertEqual(pairs, [("a", "xs"), ("b", "ys")]) + self.assertEqual(pairs, [("a", "xs", None), ("b", "ys", None)]) def test_zip_three_elements(self): stmt = _parse_for_stmt("for a, b, c in zip(xs, ys, zs): pass") - is_zip, pairs = for_parser._parse_single_for_header(stmt) + is_zip, pairs = self._call(stmt) self.assertTrue(is_zip) - self.assertEqual(pairs, [("a", "xs"), ("b", "ys"), ("c", "zs")]) + self.assertEqual( + pairs, [("a", "xs", None), ("b", "ys", None), ("c", "zs", None)] + ) # --- error paths --- def test_zip_without_tuple_unpacking_raises(self): stmt = _parse_for_stmt("for x in zip(xs, ys): pass") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("tuple unpacking", str(ctx.exception)) def test_zip_non_name_target_raises(self): @@ -113,31 +121,31 @@ def test_zip_non_name_target_raises(self): # Patch one target element to an Attribute so it isn't an ast.Name stmt.target.elts[1] = ast.Attribute(value=ast.Name(id="obj"), attr="field") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("simple names", str(ctx.exception)) def test_zip_non_symbol_arg_raises(self): stmt = _parse_for_stmt("for a, b in zip(xs, foo()): pass") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) - self.assertIn("simple symbols", str(ctx.exception)) + self._call(stmt) + self.assertIn("symbol", str(ctx.exception).lower()) def test_zip_var_arg_count_mismatch_raises(self): stmt = _parse_for_stmt("for a, b, c in zip(xs, ys): pass") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("variable count", str(ctx.exception)) def test_iteration_over_expression_raises(self): stmt = _parse_for_stmt("for x in range(10): pass") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("symbol", str(ctx.exception).lower()) def test_tuple_unpacking_without_zip_raises(self): stmt = _parse_for_stmt("for a, b in items: pass") with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("requires zip()", str(ctx.exception)) def test_unsupported_target_type_raises(self): @@ -147,9 +155,24 @@ def test_unsupported_target_type_raises(self): slice=ast.Constant(value=0), ) with self.assertRaises(ValueError) as ctx: - for_parser._parse_single_for_header(stmt) + self._call(stmt) self.assertIn("Unsupported", str(ctx.exception)) + def test_attribute_iterable_generates_a_port(self): + stmt = _parse_for_stmt("for x in holder.xs: pass") + scope = symbol_scope.SymbolScope( + {"holder": edge_models.SourceHandle(node="Payload_0", port="instance")} + ) + nodes: dict = {} + is_zip, axes = for_parser._parse_single_for_header(stmt, scope, nodes, set()) + self.assertFalse(is_zip) + self.assertEqual(axes[0].variable, "x") + self.assertEqual(axes[0].port, "xs_0") + self.assertEqual( + axes[0].binding, edge_models.SourceHandle(node="getattr_xs_0", port="attr") + ) + self.assertIn("getattr_xs_0", nodes) + # =================================================================== # parse_for_iterations @@ -159,42 +182,48 @@ def test_unsupported_target_type_raises(self): class TestParseForIterations(unittest.TestCase): """Tests for the iteration-header unwrapping logic.""" + @staticmethod + def _call(stmt: ast.For): + scope = symbol_scope.SymbolScope({}) + nodes: dict = {} + return for_parser._parse_for_iterations(stmt, scope, nodes) + def test_single_simple_iteration(self): stmt = _parse_for_stmt("for x in xs:\n pass") - nested, zipped, body = for_parser._parse_for_iterations(stmt) - self.assertEqual(nested, [("x", "xs")]) + nested, zipped, body = self._call(stmt) + self.assertEqual(nested, [("x", "xs", None)]) self.assertEqual(zipped, []) def test_single_zip_iteration(self): stmt = _parse_for_stmt("for a, b in zip(xs, ys):\n pass") - nested, zipped, body = for_parser._parse_for_iterations(stmt) + nested, zipped, body = self._call(stmt) self.assertEqual(nested, []) - self.assertEqual(zipped, [("a", "xs"), ("b", "ys")]) + self.assertEqual(zipped, [("a", "xs", None), ("b", "ys", None)]) def test_two_nested_simple_iterations(self): stmt = _parse_for_stmt("for x in xs:\n for y in ys:\n pass") - nested, zipped, body = for_parser._parse_for_iterations(stmt) - self.assertEqual(nested, [("x", "xs"), ("y", "ys")]) + nested, zipped, body = self._call(stmt) + self.assertEqual(nested, [("x", "xs", None), ("y", "ys", None)]) self.assertEqual(zipped, []) def test_nested_then_zip(self): code = "for x in xs:\n for a, b in zip(as_, bs):\n pass" stmt = _parse_for_stmt(code) - nested, zipped, body = for_parser._parse_for_iterations(stmt) - self.assertEqual(nested, [("x", "xs")]) - self.assertEqual(zipped, [("a", "as_"), ("b", "bs")]) + nested, zipped, body = self._call(stmt) + self.assertEqual(nested, [("x", "xs", None)]) + self.assertEqual(zipped, [("a", "as_", None), ("b", "bs", None)]) def test_zipped_then_nested(self): code = "for a, b in zip(as_, bs):\n for x in xs:\n pass" stmt = _parse_for_stmt(code) - nested, zipped, body = for_parser._parse_for_iterations(stmt) - self.assertEqual(nested, [("x", "xs")]) - self.assertEqual(zipped, [("a", "as_"), ("b", "bs")]) + nested, zipped, body = self._call(stmt) + self.assertEqual(nested, [("x", "xs", None)]) + self.assertEqual(zipped, [("a", "as_", None), ("b", "bs", None)]) def test_body_tree_is_innermost_for(self): code = "for x in xs:\n for y in ys:\n z = 1" stmt = _parse_for_stmt(code) - _, _, body = for_parser._parse_for_iterations(stmt) + _, _, body = self._call(stmt) self.assertIsInstance(body, ast.For) # The innermost for's body should contain the assignment self.assertIsInstance(body.body[0], ast.Assign) @@ -203,8 +232,8 @@ def test_stops_nesting_when_body_starts_with_non_for(self): """If body[0] is not a For, don't descend even if a For follows.""" code = "for x in xs:\n y = 1\n for z in zs:\n pass" stmt = _parse_for_stmt(code) - nested, zipped, body = for_parser._parse_for_iterations(stmt) - self.assertEqual(nested, [("x", "xs")]) + nested, zipped, body = self._call(stmt) + self.assertEqual(nested, [("x", "xs", None)]) self.assertEqual(zipped, []) # body is the outer for – its body contains the Assign then another For self.assertIsInstance(body.body[0], ast.Assign) @@ -354,7 +383,7 @@ def wf(xs): with self.assertRaises(ValueError) as ctx: workflow_parser.parse_workflow(wf) - self.assertIn("simple symbols", str(ctx.exception)) + self.assertIn("symbol", str(ctx.exception).lower()) # =================================================================== From 1d07ac05821630ea68380519b4de270915182014 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 21:18:39 -0700 Subject: [PATCH 12/14] Accept appending attribute access to accumulators Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/symbol_scope.py | 22 ++- src/flowrep/parsers/workflow_parser.py | 30 ++- .../parsers/test_parsing_attribute_access.py | 179 ++++++++++++++++++ tests/unit/parsers/test_attribute_parser.py | 17 +- tests/unit/parsers/test_workflow_parser.py | 8 +- 5 files changed, 242 insertions(+), 14 deletions(-) diff --git a/src/flowrep/parsers/symbol_scope.py b/src/flowrep/parsers/symbol_scope.py index cd87d3b4..bcbcba10 100644 --- a/src/flowrep/parsers/symbol_scope.py +++ b/src/flowrep/parsers/symbol_scope.py @@ -266,12 +266,30 @@ def consume_input_source( def produce(self, output_port: str, symbol: str | None = None) -> None: """Record that `output_port` is sourced from `symbol`.""" produced_symbol = output_port if symbol is None else symbol - if any(p.output_port == output_port for p in self._productions): - raise ValueError(f"Output port '{output_port}' already produced.") + self._reject_duplicate_production(output_port) self._productions.append( SymbolProduction(output_port=output_port, source=self[produced_symbol]) ) + def produce_source( + self, output_port: str, source: edge_models.SourceHandle + ) -> None: + """Record that `output_port` is sourced from a fixed ``SourceHandle``. + + The ``SourceHandle`` twin of :meth:`produce`, for an output port whose name was + generated rather than taken from a symbol (an attribute chain appended to an + accumulator), so no synthetic symbol enters ``_sources`` and risks colliding + with a user symbol. + """ + self._reject_duplicate_production(output_port) + self._productions.append( + SymbolProduction(output_port=output_port, source=source) + ) + + def _reject_duplicate_production(self, output_port: str) -> None: + if any(p.output_port == output_port for p in self._productions): + raise ValueError(f"Output port '{output_port}' already produced.") + def produce_symbols(self, symbols: list[str]) -> None: """Record that an output port of the same name is sources from each symbol.""" for symbol in symbols: diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index 0029a641..a1ed51b7 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -450,14 +450,15 @@ def _handle_appending_to_accumulator(self, append_call: ast.Call) -> None: ast.Name, cast(ast.Attribute, append_call.func).value ).id appended = append_call.args[0] - attribute_parser.reject_unbound_attribute( - appended, self.symbol_map, "appended to an accumulator" - ) + if attribute_parser.is_data_attribute(appended, self.symbol_map): + self._append_attribute(used_accumulator, appended) + return if not isinstance(appended, ast.Name): raise TypeError( - f"Workflow python definitions can only append a symbol to an " - f"accumulator, but '{used_accumulator}.append(...)' got " - f"{type(appended).__name__}. Bind the value to a symbol first." + f"Workflow python definitions can only append a symbol, or an " + f"attribute of one, to an accumulator, but " + f"'{used_accumulator}.append(...)' got {type(appended).__name__}. " + f"Bind the value to a symbol first." ) appended_symbol = appended.id self.symbol_map.use_accumulator(used_accumulator, appended_symbol) @@ -465,6 +466,23 @@ def _handle_appending_to_accumulator(self, append_call: ast.Call) -> None: if isinstance(appended_source, edge_models.SourceHandle): self.symbol_map.produce(appended_symbol) + def _append_attribute(self, used_accumulator: str, appended: ast.expr) -> None: + """Append an attribute chain, giving its output port a generated name. + + The getattr nodes go *inside* this body -- an ordinary workflow, which has room + for peers -- so they re-execute every iteration, exactly as the attribute access + would in Python. Only the port name is invented, and it is deduped against every + symbol in scope and every port already produced, so it cannot collide. + """ + handle = attribute_parser.inject_attribute_chain( + appended, self.symbol_map, self.nodes + ) + port = attribute_parser.generate_port_name( + appended, set(self.symbol_map) | set(self.symbol_map.outputs) + ) + self.symbol_map.use_accumulator(used_accumulator, port) + self.symbol_map.produce_source(port, handle) + def generic_visit(self, stmt: ast.AST) -> None: raise TypeError( f"Workflow python definitions can only interpret a subset of assignments, " diff --git a/tests/integration/parsers/test_parsing_attribute_access.py b/tests/integration/parsers/test_parsing_attribute_access.py index 7f0169ca..01c82686 100644 --- a/tests/integration/parsers/test_parsing_attribute_access.py +++ b/tests/integration/parsers/test_parsing_attribute_access.py @@ -339,5 +339,184 @@ def test_round_trips_through_source(self): ) +@workflow_parser.workflow +def for_appending_attribute(comp: library.ComplexData, ns: list): + xs = [] + for n in ns: + d = library.MyDataclass(comp, n) + xs.append(d.x) + return xs + + +@workflow_parser.workflow +def for_appending_bound_attribute(comp: library.ComplexData, ns: list): + xs = [] + for n in ns: + d = library.MyDataclass(comp, n) + x_0 = d.x + xs.append(x_0) + return xs + + +class TestAppendingAnAttribute(unittest.TestCase): + def test_body_output_port_is_named_for_the_attribute(self): + body = for_appending_attribute.flowrep_recipe.nodes["for_each_0"].body_node + self.assertEqual(body.recipe.outputs, ["x_0"]) + + def test_for_output_port_still_comes_from_the_accumulator(self): + recipe = for_appending_attribute.flowrep_recipe + self.assertEqual(recipe.nodes["for_each_0"].outputs, ["xs"]) + + def test_getattr_lives_inside_the_body(self): + """It must re-execute every iteration, exactly as the attribute access would.""" + body = for_appending_attribute.flowrep_recipe.nodes["for_each_0"].body_node + self.assertIn("getattr_x_0", body.recipe.nodes) + + def test_identical_to_the_bound_form(self): + self.assertEqual( + makers.dump_no_refs(makers.reference_free(for_appending_attribute)), + makers.dump_no_refs(makers.reference_free(for_appending_bound_attribute)), + ) + + def test_executes_via_run_recipe(self): + comp = library.ComplexData(val=7) + result = wfms.run_recipe( + for_appending_attribute.flowrep_recipe, comp=comp, ns=[1, 2, 3] + ) + self.assertEqual( + result.output_ports["xs"].value, for_appending_attribute(comp, [1, 2, 3]) + ) + + def test_round_trips_through_source(self): + free = makers.reference_free(for_appending_attribute) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + +class TestPreservedRejections(unittest.TestCase): + """The relaxation is bounded. These must keep raising.""" + + def test_returned_attribute_raises(self): + """A workflow's outputs are public IO; their names may not be generated.""" + + def wf(comp: library.ComplexData): + dc = library.MyDataclass(comp, 1) + return dc.x + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("returned from a workflow", str(ctx.exception)) + + def test_method_call_on_data_raises(self): + def wf(comp: library.ComplexData): + dc = library.MyDataclass(comp, 1) + y = dc.method(comp) + return y + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("method", str(ctx.exception).lower()) + + def test_literal_iterable_raises(self): + def wf(x): + ys = [] + for n in [1, 2, 3]: + y = library.my_add(n, x) + ys.append(y) + return ys + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("symbol", str(ctx.exception).lower()) + + def test_literal_append_raises(self): + def wf(ns: list): + ys = [] + for n in ns: + y = library.identity(n) # noqa: F841 + ys.append(3) + return ys + + with self.assertRaises(TypeError): + workflow_parser.parse_workflow(wf) + + def test_attribute_of_accumulator_raises(self): + """Accumulators are name-tracked, not graph data -- taking an attribute of + one directly (while it is still active) must raise. A plain post-loop + access such as ``ys.count`` is deliberately *not* this case: by then ``ys`` + is the finalised list output, an ordinary symbol like any other, and taking + its attribute is exactly what the error message below tells the reader to + do instead. To exercise the guard we shadow the accumulator's own name with + the loop variable, so the loop body's ``ys.count`` refers to the still-live + accumulator, not the eventual output.""" + + def wf(items: list): + ys = [] + for ys in items: + z = ys.count + w = library.identity(z) + ys.append(w) + return ys + + with self.assertRaises(ValueError) as ctx: + workflow_parser.parse_workflow(wf) + self.assertIn("accumulator", str(ctx.exception).lower()) + + +@workflow_parser.workflow +def try_attribute_in_branches(holder: library.Payload): + try: + y = library.divide(holder.num, holder.den) + except ZeroDivisionError: + y = library.identity(holder.num) + return y + + +class TestAttributeInsideTryBranches(unittest.TestCase): + def test_try_node_takes_the_root_symbol_not_a_generated_port(self): + recipe = try_attribute_in_branches.flowrep_recipe + self.assertEqual(recipe.nodes["try_0"].inputs, ["holder"]) + + def test_getattr_lives_inside_the_branch(self): + try_node = try_attribute_in_branches.flowrep_recipe.nodes["try_0"] + self.assertIn("getattr_num_0", try_node.try_node.recipe.nodes) + + def test_executes_both_branches(self): + ok = library.Payload(num=6, den=2) + boom = library.Payload(num=6, den=0) + for holder in (ok, boom): + result = wfms.run_recipe( + try_attribute_in_branches.flowrep_recipe, holder=holder + ) + self.assertEqual( + result.output_ports["y"].value, try_attribute_in_branches(holder) + ) + + @unittest.expectedFailure + def test_round_trips_through_source(self): + """Known gap: a getattr *inside a branch body* leaks into the branch outputs. + + The compiler expands every getattr into its own ``name = obj.attr`` statement, + and a branch body's outputs are every locally-registered symbol -- not just a + returned one, which is what keeps this invisible at the top level of a + ``@workflow``. So re-parsing promotes the compiler's own ``getattr_num`` to a + branch output that the original recipe never had. + + Predates this work and is independent of it: attribute access as an ordinary + call argument has always been legal, and a plain ``if`` branch with no relaxed + condition, for-header, or append reproduces it identically. Left executable + rather than deleted, so it flips to an unexpected success when fixed. + """ + free = makers.reference_free(try_attribute_in_branches) + rendered = source._workflow2python(free) + fn = rendered.build() + self.assertEqual( + makers.dump_no_refs(fn.flowrep_recipe), makers.dump_no_refs(free) + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_attribute_parser.py b/tests/unit/parsers/test_attribute_parser.py index 14194f36..04959e4a 100644 --- a/tests/unit/parsers/test_attribute_parser.py +++ b/tests/unit/parsers/test_attribute_parser.py @@ -198,10 +198,10 @@ def _scope(self) -> symbol_scope.SymbolScope: def test_raises_for_chain_on_known_symbol(self): with self.assertRaises(ValueError) as ctx: attribute_parser.reject_unbound_attribute( - _expr("dc.a"), self._scope(), "appended to an accumulator" + _expr("dc.a"), self._scope(), "returned from a workflow" ) message = str(ctx.exception) - self.assertIn("appended to an accumulator", message) + self.assertIn("returned from a workflow", message) self.assertIn("dc.a", message) self.assertIn("bind", message.lower()) @@ -223,5 +223,18 @@ def test_noop_for_chain_on_unknown_root(self): ) +class TestGeneratedPort(unittest.TestCase): + def test_takes_the_outermost_attribute(self): + self.assertEqual( + attribute_parser.generate_port_name(_expr("dc.a.val"), []), "val_0" + ) + + def test_dodges_taken_names(self): + self.assertEqual( + attribute_parser.generate_port_name(_expr("dc.val"), ["val_0", "val_1"]), + "val_2", + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_workflow_parser.py b/tests/unit/parsers/test_workflow_parser.py index 911a7062..0b36a470 100644 --- a/tests/unit/parsers/test_workflow_parser.py +++ b/tests/unit/parsers/test_workflow_parser.py @@ -1483,7 +1483,7 @@ def wf(x0: int): with self.assertRaises(TypeError): workflow_parser.parse_workflow(wf) - def test_accumulator_append_of_access_raises(self): + def test_accumulator_append_of_access_generates_a_port(self): def wf(items: list): xs = [] for item in items: @@ -1491,9 +1491,9 @@ def wf(items: list): xs.append(dc.a) return xs - with self.assertRaises(ValueError) as ctx: - workflow_parser.parse_workflow(wf) - self.assertIn("bind", str(ctx.exception).lower()) + node = workflow_parser.parse_workflow(wf) + for_node = node.nodes["for_each_0"] + self.assertEqual(for_node.body_node.recipe.outputs, ["a_0"]) def test_accumulator_append_of_non_symbol_raises(self): def wf(items: list): From c6589688e2bf275d03d7d8658c1bf4a1ebb8763c Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 21:18:50 -0700 Subject: [PATCH 13/14] Update module docstring Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/parsers/attribute_parser.py | 56 +++++++++++++++---------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/flowrep/parsers/attribute_parser.py b/src/flowrep/parsers/attribute_parser.py index 4968a330..5b8bcf70 100644 --- a/src/flowrep/parsers/attribute_parser.py +++ b/src/flowrep/parsers/attribute_parser.py @@ -6,32 +6,43 @@ input named ``os`` makes ``os.path`` an attribute access on that input, exactly as it would be at runtime. -Attribute access is allowed in exactly two places: the right-hand side of an -assignment, and the arguments of an ordinary node call. Everywhere else it must be -bound to a symbol first (see :func:`reject_unbound_attribute`). +Attribute access is allowed anywhere a value is, with one exception: the workflow's +``return``. The rule is -The reason is that flowrep takes port names from two different places: + a port name may be *generated* when the port is internal to the recipe; it must + come from a *symbol* only when the port is public IO. + +Ports get their names from four places: =============================== ========================================== Port Name comes from =============================== ========================================== Workflow output port the returned symbol -Flow-control input port the enclosing symbol feeding it -For-body output port the appended symbol +Flow-control input port the enclosing symbol feeding it, or -- for a + constant or an attribute chain, which have no + symbol -- a generated name (see + :func:`generate_port_name`) +For-body output port the appended symbol, or a generated name Atomic/workflow node input port the *callee's* own parameter =============================== ========================================== -An attribute chain has no symbol. Passing ``mul(dc.x, y)`` is fine -- the injected -getattr node feeds ``mul``'s own ``a`` port, whose name comes from ``mul``. But -returning a chain, appending one to an accumulator, or passing one to a flow-control -condition would each require *inventing* a port name, and a port name the user cannot -predict from reading their own source is an unpleasant interface. So we refuse, and -ask for the binding. - -The binding costs one line and changes the recipe not at all -- the getattr node -exists either way. For a condition it produces exactly the graph one would want -anyway: a getattr node sitting as a peer of the flow-control node, feeding it through -a normally-named input port. +Only the first is public IO. A workflow's output ports are its interface, and a +consumer wiring into them should be able to read their names straight off the source, +so ``return dc.a`` is refused (see :func:`reject_unbound_attribute`). Everywhere else +the port is internal wiring, and a generated name costs the reader nothing: the +compiler emits ``val_0 = x.val`` and re-parsing regenerates exactly that port, so the +sugared form and the bound form are not merely equivalent recipes -- they are the same +recipe. + +(``@atomic`` stays laxer about its own returns. That asymmetry is deliberate: atomic +parsing must swallow arbitrary Python functions, including ones the caller did not +write, whereas a ``@workflow`` author controls their own source.) + +Two things a generated name cannot rescue, both refused elsewhere: a method call on +workflow data (``dc.method(x)``), which needs a callable reference rather than a port +name, and an attribute in a ``while`` condition rooted at a symbol the loop body +reassigns (see :func:`while_parser._reject_looped_attribute_roots`), where hoisting the +getattr out of the loop would silently diverge from Python's per-iteration re-read. """ from __future__ import annotations @@ -155,12 +166,13 @@ def generate_port_name(node: ast.expr, taken: Iterable[str]) -> str: def reject_unbound_attribute( node: ast.expr, symbol_map: symbol_scope.SymbolScope, context: str ) -> None: - """Raise if *node* is an attribute chain used where a port name must be invented. + """Raise if *node* is an attribute chain used where the port name is public IO. - Flowrep names a workflow output port, a flow-control input port, and a for-body - output port after the *symbol* supplying them. An attribute chain has no symbol, - so there is no honest name to give the port. Rather than invent one the user - cannot predict from reading their own source, require the binding. + A workflow's output ports are its interface. Flowrep names them after the returned + symbol, and an attribute chain has no symbol -- so the port would carry a generated + name that a consumer cannot read off the source. Everywhere else in a workflow a + generated name is fine (the port is internal wiring); here it is not, so we ask for + the binding. """ if not is_data_attribute(node, symbol_map): return From f1ea2447f64bca98c5069e8faefc7e819e0d7b95 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Mon, 13 Jul 2026 23:05:21 -0700 Subject: [PATCH 14/14] Improve symbol hygiene This isn't strictly a problem with attribute access, but attribute access is how I found it. We now use a new field on the symbol scope to keep pinned names unavailable more robustly across scopes Co-authored-by: Claude Signed-off-by: Liam Huber --- src/flowrep/compiler/flow_control.py | 8 ++- src/flowrep/compiler/function.py | 68 ++++++++++++++++--------- src/flowrep/compiler/statements.py | 4 +- src/flowrep/parsers/for_parser.py | 2 +- src/flowrep/parsers/parser_helpers.py | 2 +- src/flowrep/parsers/symbol_scope.py | 23 +++++++++ src/flowrep/parsers/workflow_parser.py | 2 +- tests/flowrep_static/library.py | 12 +++++ tests/unit/compiler/test_compiler.py | 63 ++++++++++++++++++++++- tests/unit/parsers/test_symbol_scope.py | 43 ++++++++++++++++ 10 files changed, 194 insertions(+), 33 deletions(-) diff --git a/src/flowrep/compiler/flow_control.py b/src/flowrep/compiler/flow_control.py index e81818a1..2b55f480 100644 --- a/src/flowrep/compiler/flow_control.py +++ b/src/flowrep/compiler/flow_control.py @@ -69,10 +69,14 @@ def emit_flow_control( for port in node.outputs if (label, port) in required_by_handle } - # Allocate output symbols for all outputs, using required names where available. + # A flow-control node's output port is always named after an enclosing *symbol* the + # author wrote -- an accumulator, a branch assignment, a loop variable -- so the port + # name is a pin, not a hint. `reserve` rather than `fresh`: minting `b_0` for a symbol + # the source calls `b` would break the round trip, and would collide with the same + # symbol pinned elsewhere (a `while` inside a `try` surfaces one `b` through both). out_syms: dict[str, str] = {} for port in node.outputs: - name = required.get(port) or alloc.fresh(port) + name = required.get(port) or alloc.reserve(port) produced[(label, port)] = name out_syms[port] = name diff --git a/src/flowrep/compiler/function.py b/src/flowrep/compiler/function.py index 98f6aece..c7bdd88c 100644 --- a/src/flowrep/compiler/function.py +++ b/src/flowrep/compiler/function.py @@ -118,51 +118,69 @@ def referenced_top_level_bindings( yield info.module.split(".")[0] -def _inlined_loop_variables(recipe: workflow_recipe.WorkflowRecipe) -> set[str]: - """Loop-variable names emitted into this workflow's own scope. - - For-loop variables are pinned to body-port names and bypass the allocator, so - reserving them prevents allocator-minted symbols from shadowing them. Descends - through inlined flow-control bodies; stops at reference-free WorkflowRecipe peer - nodes (emitted as nested defs with their own scope) and referenced nodes. +def _inlined_pinned_symbols(recipe: workflow_recipe.WorkflowRecipe) -> set[str]: + """Every name the emitter is *forced* to use as a symbol in this function's scope. + + Python's flow control introduces no scope, so every inlined ``for``/``if``/``while``/ + ``try`` body shares one flat namespace with the enclosing ``def``. Most symbols in it + are the allocator's to choose, but some are pinned: round-tripping requires a symbol + to carry a port name verbatim, or re-parsing regenerates a different port. Pinned + names bypass the allocator, so it must learn them *up front* -- a pin emitted later + cannot retroactively protect a name the allocator already minted. + + The pins are flow-control output ports, for-loop variables, for-body output ports, and + the input ports of flow-control nodes (which force the feeding sibling's output symbol + to match). + + Descends through inlined flow-control bodies; stops at reference-free WorkflowRecipe + peer nodes (emitted as nested defs, with their own scope) and referenced nodes. """ - names: set[str] = set() + names: set[str] = set(statements.flow_control_input_requirements(recipe).values()) for node in recipe.nodes.values(): - _collect_loop_variables(node, names) + _collect_pinned_symbols(node, names) return names -def _collect_loop_variables( +def _collect_pinned_symbols( node: union_types.RecipeDiscrimination, names: set[str] ) -> None: + if not isinstance(node, flow_control.FLOW_CONTROL_TYPES): + return # atomic / referenced / reference-free workflow peer: not inlined here + + # Every flow-control output port is named for an enclosing symbol, and + # `emit_flow_control` pins it to that name. + names.update(node.outputs) + if isinstance(node, for_recipe.ForEachRecipe): names.update(node.iterated_ports) - _collect_loop_variables_from_body(node.body_node.recipe, names) + # `_emit_for_each` pins every body output to its port name. + names.update(node.body_node.recipe.outputs) + _collect_pinned_symbols_from_body(node.body_node.recipe, names) elif isinstance(node, if_recipe.IfRecipe): for body_case in node.cases: - _collect_loop_variables_from_body(body_case.body.recipe, names) + _collect_pinned_symbols_from_body(body_case.body.recipe, names) if node.else_case is not None: - _collect_loop_variables_from_body(node.else_case.recipe, names) + _collect_pinned_symbols_from_body(node.else_case.recipe, names) elif isinstance(node, try_recipe.TryRecipe): - _collect_loop_variables_from_body(node.try_node.recipe, names) + _collect_pinned_symbols_from_body(node.try_node.recipe, names) for exception_case in node.exception_cases: - _collect_loop_variables_from_body(exception_case.body.recipe, names) + _collect_pinned_symbols_from_body(exception_case.body.recipe, names) elif isinstance(node, while_recipe.WhileRecipe): - _collect_loop_variables_from_body(node.case.body.recipe, names) - # atomic / referenced / reference-free workflow peer node: not inlined here + _collect_pinned_symbols_from_body(node.case.body.recipe, names) -def _collect_loop_variables_from_body( +def _collect_pinned_symbols_from_body( body: union_types.RecipeDiscrimination, names: set[str] ) -> None: """Recurse into an inlined body: a WorkflowRecipe's own nodes, or a nested flow-control node. Both are emitted into the current scope.""" if isinstance(body, workflow_recipe.WorkflowRecipe): + names.update(statements.flow_control_input_requirements(body).values()) for node in body.nodes.values(): - _collect_loop_variables(node, names) + _collect_pinned_symbols(node, names) elif isinstance(body, flow_control.FLOW_CONTROL_TYPES): - _collect_loop_variables(body, names) - # single atomic body: no loop variables + _collect_pinned_symbols(body, names) + # single atomic body: nothing pinned beyond what the caller already reserved def emit_nested_workflow_node( @@ -319,10 +337,10 @@ def emit_workflow_function( ) -> FunctionBuilder: alloc = NameAllocator() in_syms = {port: alloc.reserve(port) for port in recipe.inputs} - # Loop variables are pinned to body-port names and bypass the allocator; reserve - # them so node-output symbols never collide with (and get shadowed by) them. - for loop_variable in _inlined_loop_variables(recipe): - alloc.reserve(loop_variable) + # Pinned symbols bypass the allocator, so it has to know them before it mints + # anything, or a fresh name can land on one and be silently overwritten. + for pinned in _inlined_pinned_symbols(recipe): + alloc.reserve(pinned) params = _render_params(recipe, signature, emitter) lines, out_syms = statements.emit_workflow_body(recipe, in_syms, {}, emitter, alloc) # Output port names are pinned via the decorator (see FunctionBuilder.render), diff --git a/src/flowrep/compiler/statements.py b/src/flowrep/compiler/statements.py index 39c1c676..a774f224 100644 --- a/src/flowrep/compiler/statements.py +++ b/src/flowrep/compiler/statements.py @@ -115,7 +115,7 @@ def _topological_nodes( ) -def _flow_control_input_requirements( +def flow_control_input_requirements( recipe: workflow_recipe.WorkflowRecipe, ) -> dict[tuple[str, str], str]: """Required source-symbol names imposed by flow-control node inputs. @@ -238,7 +238,7 @@ def resolve(source) -> str: # stays inert and it is inlined in the topo loop below -- which is why the # conflict branch immediately below remains unreachable in parser-produced # recipes. - for handle, name in _flow_control_input_requirements(recipe).items(): + for handle, name in flow_control_input_requirements(recipe).items(): if ( # pragma: no cover - twin of the output-edge guard above; only a # hand-built recipe (one a parser never emits) can name a source for # both a flow-control input and an output differently. See diff --git a/src/flowrep/parsers/for_parser.py b/src/flowrep/parsers/for_parser.py index 38b8193e..935f5eac 100644 --- a/src/flowrep/parsers/for_parser.py +++ b/src/flowrep/parsers/for_parser.py @@ -214,7 +214,7 @@ def _resolve_collection( if attribute_parser.is_data_attribute(iter_expr, symbol_map): handle = attribute_parser.inject_attribute_chain(iter_expr, symbol_map, nodes) port = attribute_parser.generate_port_name( - iter_expr, set(symbol_map) | reserved_ports + iter_expr, symbol_map.unavailable_names | reserved_ports ) reserved_ports.add(port) return port, handle diff --git a/src/flowrep/parsers/parser_helpers.py b/src/flowrep/parsers/parser_helpers.py index a607f0fe..29f4a468 100644 --- a/src/flowrep/parsers/parser_helpers.py +++ b/src/flowrep/parsers/parser_helpers.py @@ -300,7 +300,7 @@ def _bind_condition_source( every enclosing symbol. """ generated = attribute_parser.generate_port_name( - arg_node, set(scope) | reserved_ports + arg_node, scope.unavailable_names | reserved_ports ) reserved_ports.add(generated) condition_bindings[generated] = handle diff --git a/src/flowrep/parsers/symbol_scope.py b/src/flowrep/parsers/symbol_scope.py index bcbcba10..0c2186ca 100644 --- a/src/flowrep/parsers/symbol_scope.py +++ b/src/flowrep/parsers/symbol_scope.py @@ -43,6 +43,7 @@ def __init__( sources: dict[str, edge_models.InputSource | edge_models.SourceHandle], available_accumulators: set[str] | None = None, reserved_accumulators: set[str] | None = None, + shadowed_symbols: set[str] | None = None, ): self._sources = dict(sources) self._consumptions: list[SymbolConsumption] = [] @@ -56,8 +57,24 @@ def __init__( self.reserved_accumulators: set[str] = ( set() if reserved_accumulators is None else reserved_accumulators ) + self.shadowed_symbols: set[str] = ( + set() if shadowed_symbols is None else shadowed_symbols + ) self.consumed_accumulators: dict[str, str] = {} + @property + def unavailable_names(self) -> set[str]: + """Every name a *generated* port name must avoid. + + The compiler inlines flow-control bodies into the enclosing ``def``, so a body's + ports and its enclosing scope's symbols share one Python namespace. A generated + name must therefore dodge more than this scope's own symbols: it must also dodge + :attr:`shadowed_symbols` -- names live in the enclosing emitted scope but absent + here, chiefly the collection symbol that :meth:`fork` remapped into the loop + variable. + """ + return set(self._sources) | self.shadowed_symbols | set(self.outputs) + @property def inputs(self) -> list[str]: """Ordered unique input ports consumed from InputSources.""" @@ -349,4 +366,10 @@ def fork( available_accumulators=available_accumulators, reserved_accumulators=self.reserved_accumulators | self.available_accumulators, + # Pre-remap names: `for x in coll` hands the child `x` and drops `coll`, but + # `coll` is still very much alive in the emitted Python -- it is the thing + # being iterated. A name generated in the child must not land on it. + shadowed_symbols=( + set(self._sources) | self.all_accumulators | self.shadowed_symbols + ), ) diff --git a/src/flowrep/parsers/workflow_parser.py b/src/flowrep/parsers/workflow_parser.py index a1ed51b7..11464b8b 100644 --- a/src/flowrep/parsers/workflow_parser.py +++ b/src/flowrep/parsers/workflow_parser.py @@ -478,7 +478,7 @@ def _append_attribute(self, used_accumulator: str, appended: ast.expr) -> None: appended, self.symbol_map, self.nodes ) port = attribute_parser.generate_port_name( - appended, set(self.symbol_map) | set(self.symbol_map.outputs) + appended, self.symbol_map.unavailable_names ) self.symbol_map.use_accumulator(used_accumulator, port) self.symbol_map.produce_source(port, handle) diff --git a/tests/flowrep_static/library.py b/tests/flowrep_static/library.py index 54f704e5..4da70386 100644 --- a/tests/flowrep_static/library.py +++ b/tests/flowrep_static/library.py @@ -175,3 +175,15 @@ def __init__(self, xs: list | None = None, num: int = 1, den: int = 1): class MyDataclass: a: ComplexData x: int = 1 + + +@atomic_parser.atomic +def val(x): + """Deliberately named for ``ComplexData.val``. + + The compiler names a node's output symbol after its label base, so two ``val`` + nodes mint ``val`` and ``val_0`` -- and ``val_0`` is exactly the port name the + parser generates for an attribute chain ending in ``.val``. That coincidence is + what forces the compiler-side namespace collision. + """ + return x diff --git a/tests/unit/compiler/test_compiler.py b/tests/unit/compiler/test_compiler.py index 77bb0d37..321255d1 100644 --- a/tests/unit/compiler/test_compiler.py +++ b/tests/unit/compiler/test_compiler.py @@ -1956,7 +1956,7 @@ def wf(rows): return out recipe = makers.reference_free(wf) - reserved = function._inlined_loop_variables(recipe) + reserved = function._inlined_pinned_symbols(recipe) self.assertIn("row", reserved) self.assertIn("cell", reserved) rebuilt = source._workflow2python(recipe).build() @@ -2531,5 +2531,66 @@ def wf(x0: int, comp: library.ComplexData): ) +class TestPinnedSymbolReservation(unittest.TestCase): + """The allocator must know every pinned name *before* it mints anything. + + A pin bypasses the allocator, and a pin emitted later cannot retroactively protect a + name the allocator already minted -- so an unreserved pin silently overwrites it. + """ + + def test_flow_control_input_ports_are_reserved(self): + @workflow_parser.workflow + def wf(comp, seed): + a = library.val(1) + b = library.val(2) + if library.is_positive(comp.val): + m = library.identity(seed) + else: + m = library.negate(seed) + c = library.my_add(a, b) + return m, c + + recipe = makers.reference_free(wf) + self.assertIn( + "val_0", + function._inlined_pinned_symbols(recipe), + "the if-node's generated input port pins a getattr to `val_0`", + ) + + def test_for_body_output_ports_are_reserved(self): + @workflow_parser.workflow + def wf(payload, comp): + ys = [] + for n in payload.xs: + d = library.MyDataclass(comp, n) + ys.append(d.x) + return ys + + recipe = makers.reference_free(wf) + reserved = function._inlined_pinned_symbols(recipe) + self.assertIn("n", reserved, "loop variable") + self.assertIn("x_0", reserved, "the for-body's generated output port") + self.assertIn("ys", reserved, "the for node's output port") + + def test_allocator_does_not_mint_over_a_pin(self): + """The regression itself: `b` must survive the loop that follows it.""" + + @workflow_parser.workflow + def wf(comp, seed): + a = library.val(1) + b = library.val(2) + if library.is_positive(comp.val): + m = library.identity(seed) + else: + m = library.negate(seed) + c = library.my_add(a, b) + return m, c + + recipe = makers.reference_free(wf) + rebuilt = source._workflow2python(recipe).build() + comp = library.ComplexData(val=7) + self.assertEqual(rebuilt(comp, 4), wf(comp, 4)) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/parsers/test_symbol_scope.py b/tests/unit/parsers/test_symbol_scope.py index 04feb620..709664c5 100644 --- a/tests/unit/parsers/test_symbol_scope.py +++ b/tests/unit/parsers/test_symbol_scope.py @@ -364,5 +364,48 @@ def test_inputs_reports_source_port_not_alias_name(self): self.assertEqual(scope.inputs, ["x"]) +class TestShadowedSymbols(unittest.TestCase): + """`fork` must not lose names that stay alive in the emitted Python scope. + + The compiler inlines a for-body into the enclosing ``def``, so the collection + symbol is still bound there even though the child scope knows it only by the loop + variable's name. A generated port name must dodge it anyway. + """ + + def test_fork_shadows_pre_remap_symbols(self): + parent = SymbolScope( + {"coll": _make_input("coll"), "other": _make_input("other")} + ) + child = parent.fork(symbol_remap={"coll": "x"}) + self.assertNotIn("coll", child, "the remap renames it away, as before") + self.assertIn("x", child) + self.assertIn( + "coll", + child.shadowed_symbols, + "but the emitted Python still binds `coll` -- it is being iterated", + ) + self.assertIn("other", child.shadowed_symbols) + + def test_fork_shadows_accumulators(self): + parent = SymbolScope({"a": _make_input("a")}) + parent.register_accumulator("acc") + child = parent.fork(available_accumulators={"acc"}) + self.assertIn("acc", child.shadowed_symbols) + + def test_shadowing_is_transitive(self): + """A doubly-nested body must dodge names from *both* enclosing levels.""" + grandparent = SymbolScope({"coll": _make_input("coll")}) + parent = grandparent.fork(symbol_remap={"coll": "row"}) + child = parent.fork(symbol_remap={"row": "cell"}) + self.assertIn("coll", child.shadowed_symbols) + self.assertIn("row", child.shadowed_symbols) + + def test_unavailable_names_unions_scope_shadowed_and_outputs(self): + scope = SymbolScope({"a": _make_input("a")}, shadowed_symbols={"gone"}) + scope.register(["b"], _make_labeled_node("node_0", ["out"])) + scope.produce("b") + self.assertEqual(scope.unavailable_names, {"a", "b", "gone"}) + + if __name__ == "__main__": unittest.main()