Repository navigation
feat: list a program's calls and literals for hosts - #32
Conversation
….Literal Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Approve: The walk covers every node type that validate.go accepts, pipes are desugared before the walk, and the shared caches are read-only at run time, so the design holds; two Should-fix items can land after merge.
This is the expr side of workflow#86 (decision edges, PRD workflow#95).
Should fix
S1. A named binding does not shadow a call (calls.go:62)
The doc on Calls (lines 25-27) says a name shadowed by a named binding is not a call. The walk drops the binding (coll, _, body), so map(xs, ask, ask("a?", ask)) reports the inner ask. evalCall skips the registered function when the env shadows its name (program.go:1432), so that call never reaches the function at Run. The over-report is the safe direction for a host that prepares answers, but it contradicts the doc. Fix: pass the binding name down and skip calls of it, as identifiers.go:110 does for element names, or change the doc to match. Add a test for the shadowed case.
S2. A bound argument has no it to read (calls.go:12)
Args says each argument runs like any program. For a call with Bound: true, an argument that reads it, index, or the named element has no value outside the loop, so Run on it fails. The PR example only runs top-level arguments, and no test runs a bound one. Fix: say in the Args and Bound docs and in docs/reference/spec.md that bound arguments run only per element, and add a test that pins the error. See Q1 for whether the API needs more.
Questions
- Q1. Does workflow#86 need to evaluate a
decidesubject inside amaporanybody, such asany(xs, decide("q", it))? If yes, S2 needs an API change, not only a doc note. (calls.go:13)
Nits
- N1. The spec example ends with
_, _, _ = q, isLit, subject. Drop the unused variables and show the call the way the PR description does. (docs/reference/spec.md)
Sensitive paths
CLAUDE.md: adds thecalls.gorow to the layout table, which matches the file.AGENTS.mdis a symlink to it, so the two cannot drift.
Prompt for your AI tool: fix S1, S2
Fix these review findings in deepnoodle-ai/expr, PR #32.
1. S1, calls.go, func (p *Program) walkCalls: the named binding of a
three-arg iterating form is dropped (`coll, _, body`), so a call
whose name matches the binding is still reported. Wanted: a call
whose name is shadowed by the binding is not reported, as the doc
comment on Calls says (or change the doc comment to match). Test:
Calls("ask") on `map(xs, ask, ask("a?", ask))` returns no calls.
2. S2, calls.go, type Call: Args says each argument runs like any
program, but a Bound argument that reads it, index, or the named
element cannot run outside the loop. Wanted: the Args and Bound
docs and docs/reference/spec.md say bound arguments run only per
element. Test: Run on a bound argument that reads `it` fails with
an error; pin that in calls_test.go.
Run the existing tests after the changes.
…ents Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replies to the review. All fixes are in 62456f9.
Fixed in 62456f9.
Fixed in 62456f9. The
No. workflow rejects a decision call with
Fixed in 62456f9. The example now runs the subject and prints it, as the PR description does. |
There was a problem hiding this comment.
Re-review at 62456f9. Approve.
- S1 (named binding shadowing): fixed.
walkCallsnow carriesboundIdents, the same stackIdentifiersuses. A call of the binding's name inside the body is skipped, and its arguments are still walked.hashandles a nil receiver, so the top-level path is safe. - S2 (bound arguments): fixed in the
Argsdoc and in spec.md.TestCalls_BoundArgumentRunsOnlyPerElementpins theErrEvaluateresult when the argument runs alone. - Q1: agreed. A decision call with
Bound: trueis already rejected at Load in workflow, so the doc note is enough and the API stays as it is. - N1: fixed. The spec example now runs the subject and prints it.
Not run: tests, build, and lint, per the review rules. The new tests are checked by reading only.
What changes for the user
A host can now list the calls of chosen functions in a compiled expression, before it runs it.
Program.Calls(names...)returns each call with its arguments as programs of their own.Program.Literal()tells a host whether an expression is only a literal, and returns its value.Issue
Needed by deepnoodle-ai/workflow#86 (decision edges, PRD PR deepnoodle-ai/workflow#95). The workflow engine must find every
decide(question, subject)call in an edge condition before it runs it. It also needs each question as a literal and each subject as something it can evaluate.See it
Why
Program.Identifiers()already gives hosts the names an expression reads. Some hosts also need to see calls: what each call asks, and with which arguments. Run cannot show them all, because&&,||,ifandtryskip branches. Without this, a host must parse expr's syntax itself and copy its rewrite rules (?., JSON literals, keyword fields, pipes).Decisions to check
*Program. It runs, type-checks and reportsIdentifiers()like any program. Rejected: return argument source text, which a host would compile again with the same options.Boundmarks a call in an iterating form's body, where it runs once per element. Rejected: skip those calls, which would hide them from the host.x | f("q")reportsf(x, "q"), which is what Run calls. Rejected: aPipedflag. No caller needs it yet.-1,2 * 3) count as literals. Compile already folds them. Rejected: a stricter "as written" rule, which would need a second pass.How to verify
go test -run 'TestCalls|TestLiteral' ./..., thengo test ./...andgo vet ./....Risk and rollout
Additive API. No change to Compile, Run or Check. An argument program shares its parent's caches, which are read-only after compile. A release tag (v1.4.0) needs the maintainer's approval.
🤖 Generated with Claude Code