From 7b8060e8cc708be2a4acd90068719533f85d4392 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 06:25:15 +0000 Subject: [PATCH] =?UTF-8?q?Short-circuit=20`and`=20and=20`or`=20(=C2=A713.?= =?UTF-8?q?21,=20=C2=A714=20decision=2011)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §8.4 asked for both operands to be evaluated and the evaluator obeyed, so this was a stance rather than drift. §15 ranked it first: eight defects in the first library written against Ghost, two of them shipped, every one a null guard written the way Python, Ruby, JavaScript and PHP all teach it and crashing on the dereference it existed to prevent. target = null target == null or target.hint == '' // was: property error, cannot read property `hint` of null // now: true evaluateInfix routes `and`/`or` to evaluateLogicalInfix as soon as the left operand is evaluated, before the right one is touched. It requires the left operand to be a boolean, returns immediately when that settles the answer (`false and x`, `true or x`), and otherwise evaluates the right operand and answers with it - once the left has not decided, the result *is* the right one. The two cases are gone from evaluateBooleanInfix, which is handed both operands already evaluated and so cannot make this decision; they would be dead code. foldBooleanInfix still folds two literal booleans, where there is no evaluation to skip either way, and only its comment changed. The narrow reversal decision 11 described held: the truth table is untouched, both operands are still booleans, and the one observable loosening is that an unreached operand is no longer type-checked, so `false and 1` is now false where it was a type error. What the decision did not anticipate is that the error wording had to move with it. A wrong left operand can no longer be reported as "between null and boolean" - the right operand was deliberately never evaluated, and naming a type it might have had would be inventing one. logicalOperandError names the side at fault instead, on either side, and suggests comparing first when the operand is null. That reads better than what it replaced: a bare `if (x and x.foo)` now fails at the `and` with a type error rather than dying on the null dereference downstream of it. Tested in evaluator/logical_test.go: the truth table, short-circuiting proved both by a right operand that raises and by one whose side effect is counted and must not happen, the reached/unreached error wording with positions, and the null help line. All 41 programs in examples/ produce identical output before and after - mud.gs differs only in how many frames its non-terminating interactive loop renders inside the timeout, byte identical up to that point. Studio's 132-case suite, the codebase that reported this, passes unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EGsNMRiKwWmvoizxD619zA --- SPEC.md | 110 ++++++++++++++++-------- evaluator/boolean.go | 53 +++++++++++- evaluator/errors.go | 19 +++++ evaluator/infix.go | 7 ++ evaluator/logical_test.go | 172 ++++++++++++++++++++++++++++++++++++++ optimizer/fold.go | 6 +- 6 files changed, 325 insertions(+), 42 deletions(-) create mode 100644 evaluator/logical_test.go diff --git a/SPEC.md b/SPEC.md index 95e9e59..d8aaef5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -470,7 +470,7 @@ respectively, or it is a type error. There is no chained assignment (`a = b | Arithmetic | `+ - * / %` | On numbers: standard, with the int/float promotion rules above. On lists: elementwise with **NumPy-style broadcasting** — see below. On strings: only `+` (concatenation); `-`/`*`/`/`/`%` on strings are a type error. | | Comparison | `< <= > >=` | Numbers and strings only (strings compare lexicographically). **Not supported between two lists** — deliberately: neither an elementwise nor a lexicographic reading was judged obviously correct (`CLAUDE.md`). Dates support `< <= > >=` as instant ordering, independent of which time zone either `Date` is attached to (§9.5). | | Equality | `== !=` | See §8.5 — this is one of the language's most distinctive behaviors. | -| Logical | `and`, `or`, `!` | Word operators, not `&& \|\|` — there is no `&&`/`\|\|` token at all. `!` is the only prefix logical operator. Both operands of `and`/`or` are evaluated as ordinary booleans (no built-in short-circuit special-casing beyond ordinary infix evaluation order: left is evaluated, then right, then combined). | +| Logical | `and`, `or`, `!` | Word operators, not `&& \|\|` — there is no `&&`/`\|\|` token at all. `!` is the only prefix logical operator. **`and` and `or` short-circuit**: the right operand is evaluated only when the left one leaves the answer open, so `false and x` is `false` and `true or x` is `true` without ever reaching `x` (§13.21, §14 decision 11). Both operands are still booleans — an operand that *is* reached and is not one raises a `Type` fault naming the side at fault — but an operand that is never reached is never type-checked. | | Unary | `-`, `!` | `-` negates a number only. `!` follows Ghost's truthiness rules (§8.5), not "must be boolean." | | Range | `a..b` | Inclusive integer range, producing a `list`: `1..5` → `[1, 2, 3, 4, 5]`. Descending (`a > b`) produces an empty list rather than counting down. Not foldable at compile time (would require a shared mutable literal). | | Ternary | `cond ? a : b` | Standard. | @@ -2460,22 +2460,23 @@ fixing: the Chisel work no longer exists — noted here because that report is otherwise cited as a whole and this one item of it is stale. -### 13.21 `and`/`or` do not short-circuit, and the guard idiom every neighbouring language teaches therefore crashes +### 13.21 `and`/`or` do not short-circuit, and the guard idiom every neighbouring language teaches therefore crashes — done ```ghost target = null -if (target == null or target.hint == '') { // property error: cannot read - return null // property `hint` of null -} +if (target == null or target.hint == '') { // was: property error, cannot read + return null // property `hint` of null. +} // now: returns null, as written ``` -§8.4 states this outright — "no built-in short-circuit special-casing beyond -ordinary infix evaluation order: left is evaluated, then right, then -combined" — and `evaluator/infix.go`'s `evaluateInfix` matches it: both sides -are evaluated, and only then does the switch reach `evaluateBooleanInfix`. -So unlike §13.15 and §13.17 this is not drift; the implementation does what -this document asked for. **The callout is against the stance, not the code.** +§8.4 used to state this outright — "no built-in short-circuit special-casing +beyond ordinary infix evaluation order: left is evaluated, then right, then +combined" — and `evaluator/infix.go`'s `evaluateInfix` matched it: both sides +were evaluated, and only then did the switch reach `evaluateBooleanInfix`. +So unlike §13.15 and §13.17 this was never drift; the implementation did what +this document asked for. **The callout was against the stance, not the code**, +which is why closing it took a §14 decision (11) and not just a patch. The stance is wrong for one reason that no amount of documenting fixes. Ghost takes `and`/`or` from Python and Ruby, and the rest of a reader's Ghost @@ -2511,21 +2512,44 @@ or JavaScript: new concept in this tree-walker — it is one `case` that has not been written. -**Fix sketch.** Intercept `token.AND`/`token.OR` in `evaluateInfix` before -the right operand is evaluated: evaluate left, require it to be a `Boolean` -(the same `Type` fault the switch already raises, at the same token), and -return it unchanged when it decides the answer. `optimizer/fold.go`'s -`foldBooleanInfix` needs only its comment updated — it folds two literal -operands, where there is no side effect to skip either way. - -**One behavior loosens**, and it should be written down rather than -discovered: an unreached operand is no longer type-checked, so -`false and 1` becomes `false` where it is a type error today. That is the -same trade every short-circuiting language makes, and it is the point — the -unreached side is unreached. - -**Severity: high, shipped a crash twice.** This is the highest-priority item -in §15. +**Fix.** `evaluateInfix` (`evaluator/infix.go`) now routes `token.AND` and +`token.OR` to `evaluateLogicalInfix` (`evaluator/boolean.go`) as soon as the +left operand is evaluated, before the right one is touched. That function +requires the left operand to be a `Boolean`, returns immediately when it +settles the answer (`false and x`, `true or x`), and otherwise evaluates the +right operand and answers with it — because once the left operand has not +decided, the result *is* the right one: `true and x` is `x`, `false or x` is +`x`. `and`/`or` are gone from `evaluateBooleanInfix`, which is handed both +operands already evaluated and so cannot make this decision; the cases there +would be dead code. `optimizer/fold.go`'s `foldBooleanInfix` keeps folding +two literal booleans — with no evaluation to skip either way — and only its +comment changed. + +**Errors name the side at fault.** A non-boolean operand cannot be reported +as `cannot use `and` between null and boolean` any more, because when the +left operand is wrong the right one has deliberately not been evaluated and +naming a type it might have had would be inventing one. `logicalOperandError` +(`evaluator/errors.go`) reports `cannot use `and` with null on the left` +instead, on either side, and adds `help: compare it first, as in `x != null`` +when the offending operand is null — which is nearly always the truthy-guard +mistake this whole callout is about. The gain is not only wording: a bare +`if (x and x.foo)` now fails at the `and` with a `Type` fault, where it +previously died with a `property error` on the very dereference the guard +existed to prevent. + +**One behavior loosens**, as §14 decision 11 said it would: an unreached +operand is no longer type-checked, so `false and 1` is now `false` where it +was a type error. That is the same trade every short-circuiting language +makes, and it is the point — the unreached side is unreached. + +**Severity: high, shipped a crash twice.** Tested in +`evaluator/logical_test.go`: the truth table (unchanged), short-circuiting +proved by a right operand that raises and by one whose side effect is counted +and must not happen, the reached/unreached type-error wording with positions, +and the null help line. All 41 programs in `examples/` produce identical +output before and after — `mud.gs` differs only in how many frames its +non-terminating interactive loop renders inside the timeout, with its output +byte-identical up to that point. ### 13.22 A method's name shadows a same-named import, in every method of its class @@ -2764,7 +2788,7 @@ targets. fallback that keeps every existing file working is to treat a module that marks nothing as exporting everything, exactly as today. -11. **`and`/`or` short-circuit — reversing §8.4 (§13.21).** §8.4's +11. **`and`/`or` short-circuit — reversing §8.4 (§13.21) — done.** §8.4's non-short-circuiting rule was a decision this document made and the interpreter honoured; §13.21 is the evidence against it, and the evidence is strong enough to reverse it for 1.0. Eight real defects in @@ -2795,6 +2819,18 @@ targets. survives its own documentation is a design defect, not a teaching problem. + Implemented in `evaluator/boolean.go`'s `evaluateLogicalInfix`, reached + from `evaluator/infix.go`; §8.4 now documents short-circuiting as the + rule and §13.21 records what changed. The narrow reversal held: the truth + table is untouched, both operands are still booleans, and the single + observable loosening is the unreached operand going unchecked. One thing + the decision did not anticipate is that the error *wording* had to move + too — a wrong left operand can no longer be reported against a right + operand that was deliberately never evaluated, so `and`/`or` now name the + side at fault (`cannot use `and` with null on the left`). That reads + better than what it replaced: the truthy-guard mistake now fails at the + operator instead of at the null dereference downstream of it. + --- ## 15. Fix Priority for the Chisel/Studio Findings @@ -2811,8 +2847,9 @@ above findings that have been open longer. ### Closed since the report was written -Four items in `papercuts.md` no longer reproduce against this interpreter, -verified by running each one at `c31c79d`: +Five items in `papercuts.md` no longer reproduce against this interpreter. +The first four were verified by running each one at `c31c79d`; §13.21 was +closed here, by this section's own ranking: | Finding | Papercut severity | State | |---|---|---| @@ -2820,6 +2857,7 @@ verified by running each one at `c31c79d`: | §13.14 closures cannot capture a loop variable | high, silent | Fixed — §14 decision 9 | | §13.15 blocks do not introduce a scope | high, spec drift | Fixed — §14 decision 9 | | `list.length` hands back the method | low | Fixed — §13.20 | +| §13.21 `and`/`or` do not short-circuit | high, shipped twice | Fixed — §14 decision 11 | That report is cited elsewhere as a whole; these four are stale, and the architecture notes justifying workarounds for them (state on instances, @@ -2840,7 +2878,7 @@ halves of that decision paying for each other is not theoretical. | # | Finding | Severity | Cost | Why here | |---|---|---|---|---| -| 1 | §13.21 `and`/`or` do not short-circuit | high | small | Shipped twice; eight instances in one library; unchanged code keeps failing until the operator changes. Fix is one `case` in `evaluateInfix` (§14 decision 11). | +| ~~1~~ | ~~§13.21 `and`/`or` do not short-circuit~~ | high | small | **Done** — §14 decision 11. | | 2 | §13.22 a method's name shadows a same-named import | high | small | Shipped; silent until the call runs; the fault points at correct code. A diagnostic at class construction is the whole fix, and §13.18 wants the same check. | | 3 | §13.17 a bare sibling call loses the receiver | mid | small | §8.8 actively teaches the broken form, so the document is generating the bug. Contained in `unwrapCall`. | | 4 | §13.18 a field and a method may share one name | mid | small | Silent, and the behavior is the opposite of what a reader assumes. Shares its fix site with #2 — do them together. | @@ -2851,12 +2889,12 @@ halves of that decision paying for each other is not theoretical. | 9 | §12 `%=` | low | trivial | A table entry; the only compound operator missing. | | 10 | §13.19 module resolution is global and first-match-wins | mid | mid | Order-dependent and able to change under an unrelated import, but a full-path convention avoids it completely, and no reported bug has come from it yet. | -#1–#4 are four small, independent patches against known code paths, and -together they close every finding whose failure does not point at its own -cause — #1 reports at the dereference, #2 at the call site, #3 at the -callee, and #4 reports nothing at all. That is the sensible first session's -worth of work; #2 and #4 should land as one change, since a single check at -class construction catches both. +**The next item is #2.** #1–#4 were four small, independent patches against +known code paths that together close every finding whose failure does not +point at its own cause — #1 reported at the dereference, #2 at the call site, +#3 at the callee, and #4 reports nothing at all. #1 is now done; #2 and #4 +should still land as one change, since a single check at class construction +catches both. ### Already answered, no work outstanding diff --git a/evaluator/boolean.go b/evaluator/boolean.go index ed00e2f..f9fc814 100644 --- a/evaluator/boolean.go +++ b/evaluator/boolean.go @@ -15,11 +15,11 @@ func evaluateBooleanInfix(node *ast.Infix, left object.Object, right object.Obje leftValue := left.(*object.Boolean).Value rightValue := right.(*object.Boolean).Value + // `and` and `or` are absent deliberately: evaluateInfix routes them to + // evaluateLogicalInfix before this function is reached, because they have + // to decide whether to evaluate the right operand at all and everything + // here is handed both operands already evaluated. switch node.Operator { - case token.AND: - return toBooleanValue(leftValue && rightValue) - case token.OR: - return toBooleanValue(leftValue || rightValue) case token.EQUALEQUAL: return toBooleanValue(leftValue == rightValue) case token.BANGEQUAL: @@ -28,3 +28,48 @@ func evaluateBooleanInfix(node *ast.Infix, left object.Object, right object.Obje return object.NewError(fault.Type, node.Token, "cannot use `%s` between two booleans", node.Operator) } + +// evaluateLogicalInfix evaluates `and` and `or`, which short-circuit: the +// right operand is evaluated only when the left one leaves the answer open +// (§13.21, §14 decision 11). `false and x` is false and `true or x` is true +// whatever x turns out to be, so x is never reached - which is what lets a +// guard like `x == null or x.field` answer before the dereference it exists +// to prevent. +// +// Both operands are still booleans, exactly as before. What changes is only +// which operands there are: one that is reached and is not a boolean is a +// Type fault as it always was, and one that is never reached is never +// checked. +func evaluateLogicalInfix(node *ast.Infix, left object.Object, scope *object.Scope) object.Object { + condition, ok := left.(*object.Boolean) + + if !ok { + return logicalOperandError(node.Token, node.Operator, left, "left") + } + + // The left operand settles it on its own. Returning here without touching + // node.Right is the whole of the short-circuit. + if node.Operator == token.AND && !condition.Value { + return toBooleanValue(false) + } + + if node.Operator == token.OR && condition.Value { + return toBooleanValue(true) + } + + right := Evaluate(node.Right, scope) + + if isError(right) { + return right + } + + result, ok := right.(*object.Boolean) + + if !ok { + return logicalOperandError(node.Token, node.Operator, right, "right") + } + + // The left operand did not settle it, so the answer is whatever the right + // one says: `true and x` is x, and `false or x` is x. + return toBooleanValue(result.Value) +} diff --git a/evaluator/errors.go b/evaluator/errors.go index 484220e..4fce60c 100644 --- a/evaluator/errors.go +++ b/evaluator/errors.go @@ -182,6 +182,25 @@ func operatorError(tok token.Token, operator token.Type, left object.Object, rig return object.NewError(fault.Type, tok, "cannot use `%s` between %s and %s", operator, object.TypeName(left), object.TypeName(right)) } +// logicalOperandError reports a non-boolean operand of `and`/`or`. It names +// the side at fault rather than both types, because short-circuiting means +// there is not always another side to name: when the left operand is wrong +// the right one has deliberately not been evaluated, and reporting a type it +// might have had would be inventing one. +// +// The null case gets the help line because it is the mistake this wording +// exists for - a guard written as `x and x.field`, in the idiom of a language +// where `and` is truthy rather than boolean. +func logicalOperandError(tok token.Token, operator token.Type, operand object.Object, side string) *object.Error { + raised := object.NewError(fault.Type, tok, "cannot use `%s` with %s on the %s", operator, object.TypeName(operand), side) + + if object.TypeName(operand) == "null" { + raised.WithHelp("compare it first, as in `x != null`") + } + + return raised +} + // plural writes a type name as it reads when there is more than one of them. func plural(name string) string { if strings.HasSuffix(name, "s") || strings.HasSuffix(name, "x") || strings.HasSuffix(name, "ch") { diff --git a/evaluator/infix.go b/evaluator/infix.go index 2edac28..b0123f1 100644 --- a/evaluator/infix.go +++ b/evaluator/infix.go @@ -14,6 +14,13 @@ func evaluateInfix(node *ast.Infix, scope *object.Scope) object.Object { return left } + // `and` and `or` are the one infix pair that does not evaluate both sides + // up front: they short-circuit (§13.21, §14 decision 11), so the right + // operand is reached only when the left one leaves the answer open. + if node.Operator == token.AND || node.Operator == token.OR { + return evaluateLogicalInfix(node, left, scope) + } + right := Evaluate(node.Right, scope) if isError(right) { diff --git a/evaluator/logical_test.go b/evaluator/logical_test.go new file mode 100644 index 0000000..d081e25 --- /dev/null +++ b/evaluator/logical_test.go @@ -0,0 +1,172 @@ +package evaluator + +import ( + "testing" + + "ghostlang.org/x/ghost/object" +) + +// The tests in this file cover §13.21 — `and` and `or` short-circuit, per +// §14 decision 11, which reversed §8.4's original stance that both operands +// are always evaluated. +// +// The behavior that matters is not the truth table, which is unchanged, but +// which operands get evaluated: a guard whose left side already settles the +// answer must never reach its right side. That is what makes +// `x == null or x.field` safe to write, and it is the only reason this +// change exists. + +func TestLogicalOperatorsAnswerTheSameTruthTable(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {name: "true and true", input: "true and true", expected: true}, + {name: "true and false", input: "true and false", expected: false}, + {name: "false and true", input: "false and true", expected: false}, + {name: "false and false", input: "false and false", expected: false}, + {name: "true or true", input: "true or true", expected: true}, + {name: "true or false", input: "true or false", expected: true}, + {name: "false or true", input: "false or true", expected: true}, + {name: "false or false", input: "false or false", expected: false}, + { + name: "a computed left operand still decides", + input: "(1 == 2) or (3 < 4)", + expected: true, + }, + { + name: "chained operators associate left to right", + input: "false and true or true", + expected: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + isBooleanObject(t, evaluate(test.input), test.expected) + }) + } +} + +// TestLogicalOperatorsShortCircuit is the point of §13.21: the right operand +// is not evaluated when the left one has already settled the answer. Each +// case proves it by making evaluation of the right operand observable — it +// either raises, or it appends to a list the test then measures. +func TestLogicalOperatorsShortCircuit(t *testing.T) { + t.Run("a null guard does not reach the dereference it guards", func(t *testing.T) { + // The exact shape that crashed Chisel's Ui.paintTooltip() twice. + input := ` + target = null + target == null or target.hint == "" + ` + + isBooleanObject(t, evaluate(input), true) + }) + + t.Run("an and-guard does not reach its right side when the left is false", func(t *testing.T) { + input := ` + target = null + target != null and target.hint == "" + ` + + isBooleanObject(t, evaluate(input), false) + }) + + t.Run("a raising right operand is never reached", func(t *testing.T) { + isBooleanObject(t, evaluate("false and (1 / 0) == 0"), false) + isBooleanObject(t, evaluate("true or (1 / 0) == 0"), true) + }) + + t.Run("a side effect in the right operand does not happen", func(t *testing.T) { + input := ` + calls = [] + function touched() { calls.push(1) return true } + false and touched() + true or touched() + calls.length() + ` + + isNumberObject(t, evaluate(input), 0) + }) + + t.Run("the right operand is still evaluated when the left leaves it open", func(t *testing.T) { + input := ` + calls = [] + function touched() { calls.push(1) return true } + true and touched() + false or touched() + calls.length() + ` + + isNumberObject(t, evaluate(input), 2) + }) + + t.Run("an unreached operand is not type-checked", func(t *testing.T) { + // The one behavior §14 decision 11 loosens: these were type errors + // when both sides were always evaluated. + isBooleanObject(t, evaluate("false and 1"), false) + isBooleanObject(t, evaluate("true or 1"), true) + }) +} + +// TestLogicalOperandErrors pins the wording of a non-boolean operand that IS +// reached. `and`/`or` stay boolean-only (§14 decision 11), and the fault names +// the side at fault rather than both types, since short-circuiting means the +// other side may deliberately never have been evaluated. +func TestLogicalOperandErrors(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "a null left operand, the truthy-guard mistake", + input: "x = null x and true", + expected: "test.gs:1:12: type error: cannot use `and` with null on the left", + }, + { + name: "a number left operand", + input: "1 and 2", + expected: "test.gs:1:3: type error: cannot use `and` with number on the left", + }, + { + name: "a string left operand of or", + input: `"a" or false`, + expected: "test.gs:1:5: type error: cannot use `or` with string on the left", + }, + { + name: "a number right operand, reached because the left leaves it open", + input: "true and 1", + expected: "test.gs:1:6: type error: cannot use `and` with number on the right", + }, + { + name: "a null right operand of or, reached because the left is false", + input: "y = null false or y", + expected: "test.gs:1:16: type error: cannot use `or` with null on the right", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + isErrorObject(t, evaluate(test.input), test.expected) + }) + } +} + +// TestLogicalOperandErrorHelp checks the help line the null case carries, +// since a null operand is nearly always a guard written in the idiom of a +// language where `and` is truthy rather than boolean. +func TestLogicalOperandErrorHelp(t *testing.T) { + result := evaluate("x = null x and true") + + err, ok := result.(*object.Error) + + if !ok { + t.Fatalf("object is not Error. got=%T (%+v)", result, result) + } + + if err.Fault.Help != "compare it first, as in `x != null`" { + t.Errorf("help has wrong text. got=%q", err.Fault.Help) + } +} diff --git a/optimizer/fold.go b/optimizer/fold.go index dc5fdc0..3f1f6bc 100644 --- a/optimizer/fold.go +++ b/optimizer/fold.go @@ -162,8 +162,10 @@ func foldStringInfix(node *ast.Infix, left, right *ast.String) ast.ExpressionNod } func foldBooleanInfix(node *ast.Infix, left, right *ast.Boolean) ast.ExpressionNode { - // Ghost evaluates both sides of and/or before dispatching, so folding two - // literal operands cannot skip a side effect. + // `and`/`or` short-circuit at runtime (§13.21), so folding them here has + // to be able to skip the right operand's side effects - which it trivially + // is, because both operands are literal booleans by the time this is + // reached. There is no evaluation to skip either way. switch node.Operator { case token.AND: return booleanNode(node, left.Value && right.Value)