Name the real cause when an expression attributes a comprehension variable - #301
Conversation
…iable
simpleeval binds comprehension targets in a closure-local scope reachable
only through the ast.Name handler it swaps into self.nodes for the
duration of the comprehension. _eval_attribute resolves an attribute's
root via super()._eval_name() to keep unknown names from being swallowed
by the warn-and-null override, but that deliberately bypasses the same
dispatch -- so a loop variable is indistinguishable from a typo and
`[c.code for c in codes]` reports:
Expression references unknown 'c' in 'c.code': no such source class,
join, or slot. Typo or stale reference?
There is no typo, and no amount of looking for one helps. Track
comprehension depth so the error can name the real cause and point at
class_derivations, which is the construct that should be used here
anyway: records built in an expression never pass through map_object, so
they skip range, cardinality, and datatype coercion.
Behavior is unchanged -- this only rewords the error. The root check
arrived in 0.5.3rc2; on 0.5.3rc1 the root resolved through self._eval(),
which respects comprehension scope.
There was a problem hiding this comment.
Pull request overview
This PR improves diagnostics from the expression evaluator when an attribute access uses a comprehension-bound loop variable (e.g., c.code inside [... for c in ...]), aiming to steer users toward class_derivations instead of misleading “typo/stale reference” wording.
Changes:
- Track comprehension nesting during evaluation to alter the unknown-root error message during comprehensions.
- Update the attribute-root resolution error message to explain why comprehension-bound attribute access is unsupported and what to do instead.
- Add regression tests covering the comprehension-bound case and ensuring the comprehension flag unwinds between evaluations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/linkml_map/utils/eval_utils.py |
Adds comprehension tracking and branches the unknown-root diagnostic in _eval_attribute. |
tests/test_utils/test_eval_utils.py |
Adds tests asserting the new comprehension-related error message and that the flag doesn’t leak across evaluations. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linkml_map/utils/eval_utils.py:745
- The
_eval_comprehensionoverride’s type annotation omitsast.SetComp. Python comprehensions includeSetComp, and the superclass implementation may dispatch set comprehensions through this method; a narrower override signature can break static type checking (and makes the annotation misleading).
def _eval_comprehension(self, node: ast.ListComp | ast.GeneratorExp | ast.DictComp) -> Any: # noqa: ANN401
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linkml_map/utils/eval_utils.py:767
- The slice deletion has an inconsistent space before the colon (
... - len(bound) :]), which is easy to misread and is the only instance of this slice style in the file. Tightening the slice improves readability and avoids future formatter churn.
del self._comprehension_targets[len(self._comprehension_targets) - len(bound) :]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_utils/test_eval_utils.py:1375
- The parametrized tuple-unpacking case (
[v.code for k, v in codes]) is currently evaluated withcodes=[{"code": "0"}], which will raise aValueErrorduringk, vunpacking (iterating a dict yields a single key like "code", not a 2-tuple). That prevents the test from reaching the intendedNameErrorpath for the comprehension-bound root.
("expr", "bound_name"),
[
("[c.code for c in codes]", "c"),
("[v.code for k, v in codes]", "v"),
("[inner.code for outer in codes for inner in outer]", "inner"),
[c.code for c in codes]fails with a message that sends you looking for a typo that doesn't exist:cis bound — by the comprehension.Why
simpleeval binds comprehension targets in a closure-local scope reachable only through the
ast.Namehandler it swaps intoself.nodesfor the duration of the comprehension (simpleeval.pyL994, restored L1027)._eval_attributeresolves an attribute's root viasuper()._eval_name()— deliberately, so unknown names surface asNameNotDefinedinstead of being swallowed by the warn-and-null override in_eval_name— but that bypasses exactly that dispatch. So a loop variable is structurally indistinguishable from a typo.This arrived in 0.5.3rc2; on 0.5.3rc1 the root resolved through
self._eval(), which respects comprehension scope. Bisect:[c.code for c in codes]_distributed_getattr, on dict data)NameError: ... unknown 'c' ... Typo or stale reference?What this does
Tracks comprehension depth and branches the message. No behavior change — the construct stays unsupported, which is right: records built in an
exprnever pass throughmap_object, so they skip range, cardinality, and datatype coercion. The error now says so and points atclass_derivations:The unknown-root diagnostic is unchanged for genuine typos.
Notes
self.nodes[ast.Name]againstself._eval_name— bound methods are recreated per attribute access, soisis always False there and==is subtle.validate-specis arguably the better home, since this is statically detectable and it already cross-references expression references. Happy to move it there instead — this is the small runtime version.Test plan
pytest tests/test_utils/ tests/test_transformer/— 620 passed, 2 skippedruff checkcleanContext
Found while migrating schema-automator's REDCap/Frictionless trans-specs to 0.5.3 (linkml/schema-automator#211). Rewriting them onto
class_derivationsmade them meaningfully simpler, so the restriction did its job — the message just didn't.