Fix five defects the port uncovered in the reference parser and scanner
Chained subtraction and exponentiation fold left instead of abandoning the rest of the line -- `1 - 2 - 3` gave -1 and dropped the `- 3`. A unary leaf's operand moves to .left, so a negative literal is one argument again and a second argument no longer overwrites it. An operator in a line's final column survives. Hex literals reach the parser whole. A leading zero is padding, not a radix. Item 16 stays pinned: the fix works and costs an upstream golden case, which is a decision rather than a defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
113
TODO.md
113
TODO.md
@@ -767,54 +767,89 @@ asserts the correct contract. Until fixed, the correct-contract test lives in
|
||||
`stopWaiting`, leaving the parent waiting on a `NEXT` that will never come.
|
||||
9. **`basicruntime.go:149`** — `newVariable()` and `BasicRuntime.variables[MAX_VARIABLES]` are
|
||||
dead; variables live in the environment's map. Do not port them.
|
||||
10. **`basicgrammar.go:224`** — `newLiteralInt` selects base 8 whenever the lexeme starts with
|
||||
`0`. *Consequence:* `PRINT 08` is a parse error and `PRINT 010` prints 8. Commodore BASIC
|
||||
has no octal literals. Fix: base 10 unless prefixed `0x`.
|
||||
10. ~~**`basicgrammar.go:224`** — `newLiteralInt` selects base 8 whenever the lexeme starts
|
||||
with `0`.~~ **Fixed.** Base 10 unless prefixed `0x`; a leading zero in a listing is padding,
|
||||
not a radix. `PRINT 010` prints 10 and `PRINT 08` parses. Nothing in the upstream corpus
|
||||
had a leading-zero literal, which is why nothing there noticed. Regression tests in
|
||||
`tests/grammar_leaves.c` and `tests/language/numeric/octal_literal.bas`, which was rewritten
|
||||
from pinning the defect to pinning the fix.
|
||||
11. **`basicruntime.go:121` / `basicparser_commands.go:124`** — environments are never freed.
|
||||
Addressed structurally by §1.4; no separate fix needed, but the C pool **will** exhaust
|
||||
where Go merely leaked, so `tests/environment_scope.c`'s exhaustion assertion matters.
|
||||
|
||||
### Found during the port
|
||||
|
||||
Five more, none of which the reference's own corpus catches. Each is reproduced faithfully,
|
||||
pinned by an assertion in the relevant unit test, and asserted *correctly* in
|
||||
`tests/known_reference_defects.c`.
|
||||
Five more, none of which the reference's own corpus catches. **Four are now fixed**, each with
|
||||
a regression test in the suite that pinned it; the fifth is described below and is not a coding
|
||||
question.
|
||||
|
||||
12. **`basicparser.go:329` — `subtraction()` returns after one operator where `addition()`
|
||||
loops, so `1 - 2 - 3` computes `1 - 2` and abandons the rest of the line.** The remaining
|
||||
`- 3` is left in the token stream and the statement loop picks it up as a second statement,
|
||||
which evaluates and is discarded. *Consequence:* a chain of two or more subtractions
|
||||
silently produces the wrong answer. This is the highest-impact item on the list — it is a
|
||||
wrong result, not a refused one. The same shape appears in `exponent()`, so `2 ^ 3 ^ 2`
|
||||
has it too. Fix: make both loop the way `addition()` does. Pinned in
|
||||
`tests/parser_expressions.c`.
|
||||
12. ~~**`basicparser.go:329` — `subtraction()` returns after one operator where `addition()`
|
||||
loops.**~~ **Fixed.** `1 - 2 - 3` computed `1 - 2` and abandoned the rest of the line; the
|
||||
remaining `- 3` was left in the token stream, picked up by the statement loop as a second
|
||||
statement, evaluated and discarded. A wrong answer, silently, which made this the
|
||||
highest-impact item on the list. `exponent()` has the identical shape — a `for` loop with a
|
||||
`return` inside it, in both the reference and the port — so `2 ^ 3 ^ 2` had it too. Both
|
||||
now fold left, which is what a C128 does with a run of same-precedence operators.
|
||||
Regression tests in `tests/parser_expressions.c`.
|
||||
|
||||
13. **`basicparser.go:565` — a unary-minus argument inflates a function's arity count.** The
|
||||
counter walks the `.right` chain, and a unary leaf keeps its operand on `.right`, so
|
||||
`ABS(-9)` is counted as two arguments and refused with "function ABS takes 1 arguments,
|
||||
received 2". *Consequence:* no builtin can be called with a negative literal. The corpus
|
||||
hides it — `sgn.bas` assigns `-1` to a variable first. Fix: count only the top-level
|
||||
argument chain, not whatever a leaf hangs off `.right`. Pinned in
|
||||
`tests/runtime_evaluate.c`.
|
||||
Worth recording for whoever reads the reference next: `exponent()` there is
|
||||
`for self.match(CARAT) { ... return expr, nil }` (`basicparser.go:519-539`), so the loop is
|
||||
a loop in shape only. `subtraction()` is the same at `:357-377`. Neither is a
|
||||
transcription slip in the port.
|
||||
|
||||
14. **`basicscanner.go:272` — `matchNextChar` returns without setting a token type when it
|
||||
cannot peek past the end of the line, so a comparison operator in the final column is
|
||||
silently dropped.** `A# =` produces one token, not two. *Consequence:* a line ending in a
|
||||
bare `<`, `>` or `=` loses it, and the parse error that follows points at the wrong place.
|
||||
Fix: set `falsetype` before returning on the peek failure. Pinned in
|
||||
`tests/scanner_tokens.c`.
|
||||
13. ~~**`basicparser.go:565` — a unary-minus argument inflates a function's arity count.**~~
|
||||
**Fixed**, and the fix is structural rather than a smarter counter. The counter walked
|
||||
`.right`, which is where a unary leaf keeps its operand *and* where an argument list chains
|
||||
its arguments; the two meanings were indistinguishable, so `ABS(-9)` counted as two
|
||||
arguments and was refused with "function ABS takes 1 arguments, received 2". No builtin
|
||||
could be called with a negative literal at all. The corpus hid it — `sgn.bas` assigns `-1`
|
||||
to a variable first.
|
||||
|
||||
15. **`basicscanner.go:318` — hex literals do not survive the scanner.** `matchNumber` allows
|
||||
`x` through but not the hex digits after it, so `0xff` lexes as `0x` and `ff` is scanned
|
||||
as a separate identifier. *Consequence:* the base-16 branch in `newLiteralInt`
|
||||
(`basicgrammar.go:224`) is unreachable, and the README's hex support does not exist. Fix:
|
||||
once `x` has been seen, accept `[0-9a-fA-F]`. Pinned in `tests/scanner_tokens.c`.
|
||||
**A unary leaf's operand now hangs off `.left`.** A unary leaf had no other use for that
|
||||
field, so the two meanings separate for the cost of one nobody was using — and counting
|
||||
then needs no special case at all. That also fixes the half of the defect the original
|
||||
entry did not mention: chaining the *next* argument through `.right` overwrote the operand,
|
||||
so `MOD(-7, 3)` destroyed its own first argument. Three readers moved (`src/runtime.c`,
|
||||
`src/grammar.c`'s renderer, `src/runtime_commands.c`'s `LIST` range parser) and that is the
|
||||
whole change. Regression tests in `tests/runtime_evaluate.c`, both halves.
|
||||
|
||||
**The same collision remains for array subscripts**, which is why §4 still lists
|
||||
"array references in parameter lists" as outstanding: an identifier keeps its subscript
|
||||
list on `.right` too. The same move — onto `.left` — is very likely the fix, and it is not
|
||||
made here because it wants its own commit and its own tests.
|
||||
|
||||
14. ~~**`basicscanner.go:272` — `matchNextChar` returns without setting a token type when it
|
||||
cannot peek past the end of the line.**~~ **Fixed.** A comparison operator in the final
|
||||
column was silently dropped: `A# =` produced one token, not two, and the parse error that
|
||||
followed pointed somewhere else entirely. `falsetype` is now set before returning on the
|
||||
peek failure. Regression tests in `tests/scanner_tokens.c`, for `=`, `<` and `>`.
|
||||
|
||||
15. ~~**`basicscanner.go:318` — hex literals do not survive the scanner.**~~ **Fixed.**
|
||||
`matchNumber` let `x` through but not the hex digits after it, so `0xff` lexed as `0x`
|
||||
followed by an identifier `ff`, the base-16 branch in `newLiteralInt` was unreachable, and
|
||||
the README's hex support did not exist. Once the `x` is seen the run continues over hex
|
||||
digits. Accepted anywhere in a number run rather than only after a leading `0`, which is
|
||||
the reference's rule and is worth keeping: it is what makes `1x2` one malformed token that
|
||||
the line-number conversion diagnoses by name. Regression tests in `tests/scanner_tokens.c`
|
||||
and `tests/language/numeric/octal_literal.bas`.
|
||||
|
||||
16. **`basicscanner.go:349` — the "Reserved word in variable name" check never fires.** The
|
||||
lexeme still carries its type suffix when the keyword tables are searched, so `PRINT$`
|
||||
misses every table and becomes an ordinary string variable. *Consequence:* the diagnostic
|
||||
is dead code and `PRINT$ = 1` is accepted. Fix: strip the suffix before the lookup. Pinned
|
||||
in `tests/scanner_tokens.c`.
|
||||
is dead code and `PRINT$ = 1` is accepted.
|
||||
|
||||
**Deliberately not fixed, and this is the interesting one.** The fix is four lines — strip
|
||||
the suffix before the lookup — and it works. It also breaks a golden case: the reference's
|
||||
own `deps/basicinterpret/tests/examples/strreverse.bas` names a variable `INPUT$`, and with
|
||||
the check working that line is refused.
|
||||
|
||||
A real C128 refuses it too, so the reference *program* is the wrong one here, not the
|
||||
diagnostic. But the corpus is the acceptance contract (§1.8), it lives in a submodule this
|
||||
repository may not edit, and reproducing it byte for byte is goal 1's headline claim. The
|
||||
two cannot both hold. Fixing this means deciding that the diagnostic is worth one
|
||||
permanently-excluded upstream case, which is a call for the author rather than for whoever
|
||||
is next through this file. Until then it stays pinned in
|
||||
`tests/known_reference_defects.c`, which is now the only thing left in there.
|
||||
|
||||
### Ours, not the reference's
|
||||
|
||||
@@ -1052,8 +1087,14 @@ What remains, in priority order:
|
||||
deliberately does not clone. Two apt packages against two more submodules is an easy trade.
|
||||
|
||||
Every step was verified from a clean clone before being written down, not inferred.
|
||||
5. **§6 items 12–17** — the defects the port uncovered. Item 12 is the one that produces a
|
||||
*wrong answer* rather than a refused one, and should be fixed first.
|
||||
5. **§6 items 16 and 17.** Items 10 and 12–15 are fixed, each with a regression test in the
|
||||
suite that pinned it. What is left is not more of the same:
|
||||
- **Item 16** is a *decision*, not a fix. Making the "Reserved word in variable name" check
|
||||
fire costs an upstream golden case, because the reference's own `strreverse.bas` names a
|
||||
variable `INPUT$`. §6 states the trade; somebody has to make it.
|
||||
- **Item 17** is ours: a host cannot reliably create a script variable while a script is
|
||||
suspended. The fix is `akbasic_runtime_global()` and roughly fifteen lines. §6 describes
|
||||
it and names the one design question to settle first.
|
||||
6. **Mutation survivors in `src/value.c`.** The harness is in place (`scripts/mutation_test.py`,
|
||||
the `mutation` CMake target, and a CI job on `src/symtab.c`), and a partial run over
|
||||
`src/value.c` turned up test gaps worth closing. These are *not* equivalent mutants; each
|
||||
|
||||
Reference in New Issue
Block a user