Skip to content

BUG: Make save('name') in queries mark the query for output - #137

Open
demianw wants to merge 1 commit into
masterfrom
fix-save-in-queries
Open

demianw wants to merge 1 commit into
masterfrom
fix-save-in-queries

Conversation

@demianw

@demianw demianw commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner

What was wrong

EvaluateQueries.visit_Call has a save('name') branch. It has been there since 5cd2746 (2012, "Added outlier rejection and the interactive shell"), but it has never worked:

  • it tests isinstance(node.args, ast.Constant), and node.args is a list, so the branch is never taken;
  • visit_Expr rejects bare calls before visit_Call is reached.

On master, a |= 1 / save('a') fails with Invalid expression at line: 2.

What save was meant to do

The CLI writes one output file per name in EvaluateQueries.queries_to_save, which eval_queries returns. A name gets into queries_to_save in two ways:

  • name = <query> evaluates the query and adds it;
  • a line containing only name adds an already evaluated query.

name |= <query> evaluates the query without adding it, for intermediate results.

The dead branch only did self.queries_to_save.add(<string>). So save('name') was meant to be a third way to request an evaluated query for output. Unlike a bare name, it also works for dotted names: visit_Expr only accepts ast.Name, so cst.left alone on a line is rejected. With save, a query defined as cst.side |= ... can be written with save('cst.left').

Change (option A: fix, this PR)

  • Remove the dead branch from visit_Call.
  • In visit_Expr, accept save('<name>') (exactly one string argument) next to the bare-name case. It uses the same lookup and the same Query X not known line: N error for unknown names.
  • save is only valid as a statement: x = save('a') is still rejected with Invalid query in line N. Names are matched after preprocessing, so they are case-insensitive like everything else in WMQL. Glob patterns are not expanded.

Behaviour change: save('name') lines that used to fail now work. Nothing else changes.

Not changed: in the interactive shell (--interactive), save('a') is now accepted but does not write a file. The shell writes queries through its own SaveQueries visitor and its save <name> command, not through queries_to_save. Writing files from the shell would need about 3 more lines in SaveQueries.visit_Expr.

Option B: remove instead

If you would rather not have a second way to do what a bare name does, the alternative is to delete the six dead lines in visit_Call (the elif ... 'save' branch) and keep save(...) an error. I chose the fix because the semantics are unambiguous from the code (add the name to queries_to_save), it is small, and it fills a real gap: saving dotted |= queries. save is not documented and no packaged .qry file uses it, so either choice is safe.

Testing

  • New test_save_query in tract_querier/tests/test_query_eval.py. It checks that A |= 0; B.left |= 1; C |= 0; A; save('B.left') returns exactly {a, b.left}, and that save('B') (unknown query) and X = save('A') raise TractQuerierSyntaxError. It fails on master with Invalid expression at line: 5.
  • python -m pytest -q -p no:cacheprovider tract_querier: 43 passed, 1 skipped.

Merge order: touches visit_Call / visit_Expr in query_processor.py, as do #133 (#103: visit_Attribute, visit_For, relative terms) and #139 (#115: the starargs lines in visit_Call). Expect textual conflicts at most.

Closes #104

🤖 Generated with Claude Code

`save('name')` was meant to add a query to the queries written by the CLI
(`queries_to_save`), like writing the query name on its own line. It never
worked: `visit_Call` tested `isinstance(node.args, ast.Constant)` on the
argument list, and `visit_Expr` rejected bare calls before `visit_Call`
was reached.

Handle `save('name')` as a statement in `visit_Expr`, next to the bare
name case it mirrors. Unlike a bare name it also accepts dotted names such
as `save('cst.left')`, for queries defined with `|=`. An unknown name
raises the same error as an unknown bare name, and `x = save(...)` is still
rejected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: save(...) in queries can never work

1 participant