Skip to content

Address review: expand named arguments during command macros - #72

Merged
oflatt merged 1 commit into
egraphs-good:oflatt-named-argsfrom
oflatt-claude:named-args-review
Sep 22, 2026
Merged

oflatt merged 1 commit into
egraphs-good:oflatt-named-argsfrom
oflatt-claude:named-args-review

Conversation

@oflatt-claude

Copy link
Copy Markdown
Contributor

Addresses the review on #57. Also merges current main into the branch, which had conflicts.

The three correctness issues share one cause

Field names were registered while parsing. egglog parses a whole program before running any of it, so a parse-time macro sees declarations in source order, never execution order. Moving expansion into a CommandMacro — which runs after every preceding command has taken effect — fixes all three together.

[P1] Ellipsis variables could collide. Each ... now mints fresh variables from a fixed hint, the way wildcard parsing does, instead of from the field names (hint x at count 1 and hint x1 at count 0 both produced x1). Worth noting: the pinned egglog now makes SymbolGen collision-free on its own, so the reviewer's repro already passes without this change — the fixed hint removes the dependency on that fix. Regression test: tests/named-args-ellipsis-fresh.egg.

[P2] Named calls failed after included declarations. A call now sees declarations pulled in by an earlier (include ...), because the include has actually run by the time the caller is expanded. Regression test: tests/named-args-include.egg (+ tests/named-args-included-defs.egg). Verified failing on the previous implementation with Unbound symbol :x.

[P3] Field metadata survived scoped redeclarations. Names are recorded per (name, arity), and every call resolves against the arity the e-graph currently reports for that name, so a declaration that has been popped or redeclared can no longer rewrite calls. Regression test: tests/named-args-scope.egg, covering both the reviewer's (push)/(pop)/different-arity case and redeclaration at the same arity. Verified failing on the previous implementation.

On the storage suggestion: extension_state is only reachable from a UserDefinedCommand, and CommandMacro::transform receives just symbol_gen and type_info. Since type_info is itself push/pop-scoped, resolving through it gives the same guarantee without new egglog API. The registry is not itself scoped, but a stale entry is unreachable — it can only be found through an arity the live type info still reports. This is documented in the module.

Succinctness

  • No parse-time macro is left. :name and ... already parse as ordinary variables, and :name Sort pairs already parse as a longer sort list, so the declaration commands no longer need shadowing. The set/delete/subsume macros are gone too; those store the table name beside its arguments rather than as a call, so their arguments are rewritten in place.
  • Every implementation type is now private; register_named_args is the only public item.
  • One split_fields validator is shared between schemas and datatype variants.
  • register_named_call is gone with the rest of the parse-time path.
  • Net: named_args.rs drops from 607 to 494 lines while gaining the fixes.
  • The dependency upgrade and the stale "local dev against the forked egglog" comment are gone. The branch now just follows main's pin (upstream egglog 9063586), and the lockfile matches the manifest, so --locked builds work.

API change

register_named_args now takes &mut EGraph rather than &mut Parser, since expansion needs the command stage. new_experimental_egraph() registers it; experimental_parser() on its own no longer provides named arguments.

Verification

make test and make nits both pass: 152 nextest tests, doctests, cargo clippy --tests -- -D warnings, cargo fmt --check.

🤖 Generated with Claude Code

All three correctness issues in the review share one cause: field names were
registered while parsing. egglog parses a whole program before running any of
it, so a parse-time macro sees declarations in source order, not execution
order.

Expanding in a `CommandMacro` fixes them together, because command macros run
after every preceding command has taken effect:

- A call now sees declarations pulled in by an earlier `(include ...)`, which
  had not been read yet when the caller was parsed.
- Field names are recorded per `(name, arity)`, and each call resolves against
  the arity the e-graph currently reports for that name, so a declaration that
  has been popped or redeclared can no longer rewrite calls.
- Each `...` binds fresh variables from a fixed hint instead of the field
  names, which could collide with each other ("x" at 1 and "x1" at 0 both
  gave "x1"). The pinned egglog also makes `SymbolGen` collision-free, so this
  no longer depends on that fix.

No parse-time macro is left: `:name` and `...` already parse as ordinary
variables, and `:name Sort` pairs already parse as a longer sort list, so the
declaration commands no longer need shadowing. That drops the `set`/`delete`/
`subsume` macros (their arguments are rewritten in place instead), makes every
implementation type private, and shares one field-name validator between
schemas and datatype variants. Named arguments are now registered on the
e-graph rather than the parser, since expansion needs the command stage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oflatt-claude
oflatt-claude requested a review from a team as a code owner September 22, 2026 18:19
@oflatt-claude
oflatt-claude requested review from saulshanabrook and removed request for a team September 22, 2026 18:19
@oflatt
oflatt merged commit 9749ea5 into egraphs-good:oflatt-named-args Sep 22, 2026
3 checks passed
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.

2 participants