Repository navigation
fix(check): report a registered function used as a value - #33
Conversation
Check typed a bare function name (`first.id`, `[upper]`) as Any, so it passed and then failed at run time on the function value. It now reports the name. An env entry with the same name still wins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Comment: Add the new Check rule to docs/reference/spec.md before merge, because CLAUDE.md requires spec updates in the same change as any language behavior change.
The code change is small and does what the description says. Identifiers and Run are unchanged, and first.id fails at Check the same way it fails at Run.
Should fix
S1. Spec does not list the new Check rule (docs/reference/spec.md:745)
The spec lists what Check reports, and a function used as a value is missing from it. The spec also says Check agrees with Run. This rule now rejects [upper, 1] and if(upper, 1, 2), which Run still evaluates. Say in the spec and CLAUDE.md that a bare function name is the one intended exception. Fix: add a bullet after line 745 for a registered function used as a value, and note the exception where CLAUDE.md says Check must agree with Run.
S2. Check comment says Run fails in more cases than it does (check.go:267)
The comment says a function value "fails wherever Run uses it". Run does not fail for a condition: IsTruthy returns true for a non-nil func (truthy.go), and a list literal holding one evaluates without error. Only a field read and a stored result fail. Fix: reword the comment to name those two cases, so the next reader does not treat the rule as a Run error.
Nits
- N1. The new
llms.txtline runs to about 123 characters, and the paragraph around it wraps near 72. Rewrap the paragraph. (llms.txt:476)
Prompt for your AI tool: fix S1, S2
Fix these review findings in deepnoodle-ai/expr, PR #33.
1. S1, docs/reference/spec.md, section "Static type checking", the list
of what Check reports (around line 745): the list has no entry for a
registered function used as a value. Wanted: add a bullet that says
Check reports a registered function name used as a value instead of
called, with a short example. Also state in this section, and in the
CLAUDE.md convention that Check must agree with Run, that this is the
one intended exception: Run still returns the function value. Test:
none needed; doc change only.
2. S2, check.go, func (c *checker) ident, comment above the
c.report call (around line 267): the comment says a function value
fails wherever Run uses it. Wanted: reword it to say the value fails
at Run for a field read and when the host stores the result. Do not
claim failure for conditions or list literals, since truthy.go and
evalCompositeLit accept them. Test: none needed; comment change only.
Run the existing tests after the changes.
Also check llms.txt line 476 when you change S1: rewrap the paragraph to match the surrounding lines (N1).
…/Run exception Add the rule to the spec's list of Check problems, state in the spec and CLAUDE.md that it is the one intended disagreement with Run, narrow the check.go comment to the cases where Run fails, and rewrap llms.txt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixed in d63a461. The spec lists the rule under "Check reports", and the spec and CLAUDE.md name it as the one intended exception to Check agreeing with Run.
Fixed in d63a461. The comment names the field read and the stored result as the Run failures, and says Run accepts a condition or a list literal.
Fixed in d63a461. The paragraph is rewrapped to match the surrounding lines. |
There was a problem hiding this comment.
Verdict: Approve. No Blocking findings. Reviewed at head d63a461, which adds one commit on top of e4cc3a5.
Previous findings, now resolved
- S1 (spec did not list the new Check rule): resolved.
docs/reference/spec.mdnow lists the rule under the Check section, names the one exception (Run accepts a bare function value in a condition or list literal, and Check rejects it), andCLAUDE.mdrecords the same exception forTestCheckMatchesRun. - S2 (checker comment overstated Run failures): resolved. The
check.gocomment now says Run fails on a field read or when a host stores the result, and accepts the value in a condition or list literal. That matches the runtime behavior I traced earlier. - N1 (long
llms.txtline): resolved. The changed lines are rewrapped to about 70 characters.
Checked in this pass
TestCheckRejectsholds the two new cases. It only asserts that Check errors, so it does not hit the Run-must-fail rule inTestCheckMatchesRun.- The checker still returns
Anyafter reporting, so the new error does not cascade.
Non-blocking
CLAUDE.mdis an unusual place to record a language exception in a bug-fix PR. It is reasonable to keep it, since that file is where contributors look for the Check/Run rule.
Not done here: I did not run go test, go vet, or any linter, per the review setup. CI is the test gate for this head. I also did not check whether any existing test expects a bare registered function to pass Check. I read the tests but did not execute them.
Problem
Program.Checktyped a registered function used as a value, not called, asAny.first.id,[upper, 1]or a barejoinpassed Check. At run time the value is the function, so a field read, a comparison, a condition or a host that stores the result fails (cannot select "id" on expr.Func,json: unsupported type: expr.Func).Identifiersalso leaves out registered names, so a host can't catch this itself. deepnoodle-ai/workflow hit this in its Load soundness promise (deepnoodle-ai/workflow#119).Change
checker.identnow reports"first" is a function, not a value; call itwhen a name resolves only to a registered function. An env entry or a bound name with the same name still wins, as it does in Run.Identifiersand Run are unchanged.Trade-off: a host that passes a registered function as an argument to another Go function now gets a Check problem for that use. Run behaves as before. The guides already call a bare function value "rarely what you want".
Tests
Two new
TestCheckRejectscases:first.idand[upper, 1].go test ./...andgo vet ./...pass.llms.txtand the spec list the new check. The spec and CLAUDE.md name it as the one intended exception to Check agreeing with Run: Run still accepts a function value in a condition or a list literal.Not tagged or released. A release needs approval.
🤖 Generated with Claude Code