From 583c0abbd21f7d4116a2f400c3fe0a6adbae6a6d Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Fri, 31 Jul 2026 12:52:00 -0400 Subject: [PATCH] Retire the byte-for-byte fidelity constraint, and fix what it was blocking The Go reference is deprecated and will not be updated, so the two projects are no longer required to match. Recorded as TODO.md section 0.1, first thing in the file, because it silently reverses the premise several later sections were written on -- an agent reading section 6 without it will park work that is no longer blocked. Section 6 item 16 was the item waiting on exactly this and is now fixed: the keyword tables are searched on the base name with any type suffix stripped, so PRINT$, LEN# and GOTO% are refused as variable names and the reference's own diagnostic stops being dead code. It cost the one golden case predicted, and the cost was nothing -- renaming a variable in strreverse.bas left its expected output byte-for-byte unchanged. AKBASIC_KNOWN_FAILING_TESTS is empty as a result and known_reference_defects.c is gone. Section 6 items 1-9 and 11 are reopened as an ordinary defect list. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 + CMakeLists.txt | 5 +- README.md | 6 +- TODO.md | 157 +++++++++++++++++------- src/scanner.c | 20 ++- tests/known_reference_defects.c | 46 ------- tests/reference/README.md | 55 +++++---- tests/reference/examples/strreverse.bas | 6 +- tests/scanner_tokens.c | 31 +++-- 9 files changed, 205 insertions(+), 127 deletions(-) delete mode 100644 tests/known_reference_defects.c diff --git a/CLAUDE.md b/CLAUDE.md index d01c65a..980dfb1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,6 +7,12 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co `akbasic` is a **C rewrite of the Go BASIC interpreter in `deps/basicinterpret`**, written in the same style as the `ak*` C libraries it builds on. +**That Go project is deprecated and will not be updated, and this interpreter is no longer required +to match it.** See `TODO.md` §0.1, which is the first thing to read: it retires the byte-for-byte +fidelity constraint that several sections of that file were written on. The reference's corpus is +checked in at `tests/reference/` and stays green as a *regression* suite; the Go source stays +readable as documentation of what the original did. Neither is binding any more. + The repository is currently **empty apart from its submodules** — no commits, no source tree, no build files. The staged content is `.gitmodules` plus four submodules under `deps/`. Everything below the "Project goals" section describes the material to build from, not code that exists here yet. diff --git a/CMakeLists.txt b/CMakeLists.txt index ce446c4..c070082 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -264,8 +264,11 @@ set(AKBASIC_TESTS set(AKBASIC_WILL_FAIL_TESTS ) +# Empty, and worth keeping declared rather than deleted -- libakstdlib left its +# AKSL_KNOWN_FAILING_TESTS in place for the same reason when 0.2.0 cleared it. +# The next defect that has to be reproduced before it can be fixed goes here, and +# the machinery to do that should not have to be reinvented. set(AKBASIC_KNOWN_FAILING_TESTS - known_reference_defects ) foreach(_test IN LISTS AKBASIC_TESTS AKBASIC_WILL_FAIL_TESTS AKBASIC_KNOWN_FAILING_TESTS) diff --git a/README.md b/README.md index 8542add..d18b73d 100644 --- a/README.md +++ b/README.md @@ -72,9 +72,11 @@ And the port is a good excuse to find out what the original actually does, as op # What Works? -Everything the Go version does. All 41 `.bas` files of the reference's own corpus produce **byte-identical** output from both implementations, including error messages and the trailing blank line after one. Each is a separate CTest case, so a failure names the file. +Everything the Go version does, and by now a little more. All 41 `.bas` files of the reference's own corpus produce the expected output, including error messages and the trailing blank line after one. Each is a separate CTest case, so a failure names the file. -That corpus lives at [`tests/reference/`](tests/reference/README.md), copied byte for byte out of the Go project. It was driven in place out of the submodule until the Go dependency started being deprecated; its README records where it came from, and that nobody is watching for drift any more because the thing it tracked has stopped moving. **Do not edit those expectations** — a failure there means this interpreter's behaviour changed. +The Go project is deprecated and will not be updated, so the two are **no longer required to match**. That corpus is kept as a regression suite rather than a specification — forty-one real BASIC programs with known-good output are worth having whatever their provenance. Where this interpreter has deliberately improved on the reference it is recorded in `TODO.md` §5, and any case whose expectation moved is listed in the corpus's own README. + +That corpus lives at [`tests/reference/`](tests/reference/README.md), copied byte for byte out of the Go project. Its README records where it came from and the rule for changing it: **diverge deliberately, never by accident.** A failure there means this interpreter's behaviour changed — nine times in ten that is a bug you just introduced, and the tenth time it is an improvement that should move the expectation, say why, and add a line to `TODO.md` §5. ## Case Sensitivity diff --git a/TODO.md b/TODO.md index 09d0177..727c196 100644 --- a/TODO.md +++ b/TODO.md @@ -11,6 +11,32 @@ parallel. ## 0. Agent protocol +### 0.1 The Go reference is deprecated. Stop matching it. + +**`deps/basicinterpret` is a dead project. It will not be updated, and this interpreter is no +longer required to reproduce its behaviour.** Recorded here first because it silently reverses +the premise several sections of this file were written on, and because an agent that reads them +without this will park work that is no longer blocked. + +What it changes: + +- **§6 is now an ordinary defect list.** "Port the behaviour first so the port is provably + faithful" is retired. Fix them because they are wrong, not when fidelity permits. +- **§1.8's message-text contract is now a convention.** Improving a message is allowed; it costs + a golden file, which is a cost rather than a veto. +- **§5's bar drops** from "defensible against the golden suite" to defensible on its own merits. +- **`tests/reference/` becomes a regression suite rather than a specification.** Diverging from + it is allowed and must be deliberate and recorded — see its README. + +What it does **not** change: + +- The corpus stays and stays green. Forty-one real BASIC programs with known-good output are + worth having whatever their provenance, and an unexplained change there is still a red flag. +- The Go source stays readable as documentation. It remains the best answer to "what did the + original actually do here", which is a question worth being able to answer even once the + answer stops being binding. +- Nothing about the `ak*` house rules, which never came from the reference. + **Read these before touching anything**, in this order: 1. `CLAUDE.md` in this repository — project goals, the `libakerror` convention, the error-code @@ -22,7 +48,8 @@ parallel. into. §1.9 below says which calls are cleared for use; that section is not optional reading, it bans a family of functions the port would otherwise reach for by reflex. 5. `deps/libakgl/AGENTS.md` — the no-`malloc` rule and the commit co-author requirement. -6. `deps/basicinterpret/README.md` — the language reference and the unimplemented list. +6. `deps/basicinterpret/README.md` — the language reference and the unimplemented list. Still + worth reading for the verb set and the semantics; no longer binding, per §0.1. **Rules for working this file:** @@ -250,7 +277,7 @@ and the process must not continue as though it does not. Do not call `akerr_init()` first. Every registry entry point calls it, and since 1.0.0 it no longer clears reservations made before it ran. -### 1.8 Error message text is part of the acceptance contract +### 1.8 Error message text is a convention, no longer a contract `tests/language/array_outofbounds.txt` is, verbatim: @@ -259,12 +286,19 @@ longer clears reservations made before it ran. ``` The trailing double newline is real: `basicError` builds a string ending in `\n` and hands it -to `Println`, which adds another. **Reproduce the message strings and the newline behaviour -byte for byte** or the golden suite fails for reasons that have nothing to do with the -interpreter. Where a Go message reads awkwardly, keep it awkward and note it here. +to `Println`, which adds another. -Numeric formatting must match too: integers via `%" PRId64 "`, floats via `%f` (Go's `%f` and -C's `%f` both give six decimals — `tests/language/arithmetic/float.txt` confirms). +**This used to be a hard contract and is now a default.** The Go implementation is deprecated +and will not be updated, so the two projects are no longer required to match — see §0.1. What +survives is the practical half: these strings and this newline behaviour are what every +expectation in `tests/reference/` was written against, so changing one means changing golden +files, and that is worth doing on purpose rather than by accident. A message that reads +awkwardly *may* now be improved; do it deliberately, move the expectations in the same commit, +and add a line to §5. + +Numeric formatting still matches the reference: integers via `%" PRId64 "`, floats via `%f` +(Go's `%f` and C's `%f` both give six decimals — `tests/reference/language/arithmetic/float.txt` +confirms). No reason to change it, which is different from not being allowed to. ### 1.9 Which `libakstdlib` calls are cleared for use — **the bans are lifted** @@ -322,7 +356,12 @@ ASCII so the symbol tables cannot reach it — see §1.3, which is still accurat ## 2. What exists — **the core port is complete and green** Phases 0 through 6 of the original plan are done. The interpreter builds clean under -`-Wall -Wextra`, reproduces the reference byte for byte, and passes under ASan and UBSan. +`-Wall -Wextra`, passes the reference's entire corpus, and passes under ASan and UBSan. + +It *did* reproduce the reference byte for byte, and that claim is retired rather than broken: +§0.1 released it, and one case has since diverged deliberately (§6 item 16, listed in +`tests/reference/README.md`). Everything else still matches, which is worth knowing but is no +longer a gate. ```sh cmake -S . -B build && cmake --build build --parallel @@ -626,8 +665,13 @@ Also on the queue, from the reference's own defect list: ## 5. Deliberate deviations from the reference -Keep this list current. Each of these changes structure without changing observable output, -and each one has to be defensible against the golden suite. +Keep this list current. Most of these change structure without changing observable output; +the ones that *do* change output say so and carry the golden file they moved. + +The bar has dropped since this list was started. It used to be "defensible against the golden +suite", because matching the Go implementation byte for byte was goal 1's headline claim. That +implementation is now deprecated (§0.1), so the bar is the ordinary one: defensible on its own +merits, recorded here, and tested. 1. Reflection → static dispatch table (§1.1). 2. Go maps → fixed open-addressed tables (§1.3). @@ -778,6 +822,17 @@ deviations from the reference's *program*: `main.go` and the SDL half of driven through the SDL binary, which is where most of the frontend's real coverage comes from. +### Deviations that change what a program is allowed to do + +30. **A verb name with a type suffix is not a variable name.** `PRINT$ = 1` is refused where + the reference accepts it, because its reserved-word check searched the keyword tables with + the suffix still attached and therefore never matched (§6 item 16). A real C128 refuses it + too, so this is the reference being wrong rather than this interpreter being strict. + + **What it costs a program:** a listing that used `INPUT$`, `LEN#` or `GOTO%` as a variable + stops parsing, and the fix is to rename the variable. One case in the reference's own corpus + did exactly that; see `tests/reference/README.md`. + ### Deviations in statement separation 29. **A branch decides who owns the rest of its line.** BASIC 7.0 scopes every statement after @@ -803,12 +858,21 @@ deviations from the reference's *program*: `main.go` and the SDL half of --- -## 6. Reference defects — ported faithfully, fix them deliberately +## 6. Reference defects — **fix them; fidelity is no longer a reason not to** -These are real bugs in `deps/basicinterpret`. **Port the behaviour first** so the golden suite -passes and the port is provably faithful, then fix each one in its own commit with a test that -asserts the correct contract. Until fixed, the correct-contract test lives in -`AKBASIC_KNOWN_FAILING_TESTS`. +These are real bugs in `deps/basicinterpret`, reproduced faithfully during the port. + +**The rule that governed this section is gone.** It used to read "port the behaviour first so +the golden suite passes and the port is provably faithful, then fix each one deliberately" — +and faithfulness was the reason several of these are still here. The Go implementation is +deprecated and will not be updated (§0.1), so there is no longer anything to be faithful *to*. +Each of these is now an ordinary defect, to be judged on whether the fix is right rather than on +whether it matches. + +What has not changed is the working method: one fix per commit, with a test that asserts the +correct contract, and a golden file moved in the same commit if the fix changes output. +`AKBASIC_KNOWN_FAILING_TESTS` is empty now and is kept declared for the next defect that has to +be reproduced before it can be fixed. 1. **`basicvariable.go:176`** — `toString()` tests `len(self.values) == 0` and then indexes `self.values[0]`. Inverted condition; on an empty variable it panics, and on a non-empty one @@ -907,23 +971,24 @@ question. 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. +16. ~~**`basicscanner.go:349` — the "Reserved word in variable name" check never fires.**~~ + **Fixed**, and it is the item §0.1 unblocked. The keyword tables are now searched on the + *base* name with any type suffix stripped, so `PRINT$`, `LEN#` and `GOTO%` are refused as + variable names and the reference's own diagnostic stops being dead code. A name that merely + contains a verb — `PRINTER$` — is still fine, which is what says the check compares the + whole base name rather than a prefix. Regression tests in `tests/scanner_tokens.c`. - **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. + **It cost one golden case, exactly as predicted, and the cost turned out to be nothing.** + The reference's `examples/strreverse.bas` names a variable `INPUT$`; the variable is renamed + to `SOURCE$` in `tests/reference/`, the expectation is byte-for-byte unchanged — the program + still prints `REVERSED: OLLEH` — and the case keeps every bit of its coverage. Recorded in + `tests/reference/README.md`'s divergence table, which this is the first entry in. - 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. + This item sat parked because the corpus was the acceptance contract and lived in a submodule + this repository could not edit. Both premises are gone: the corpus is checked in, and + matching the Go implementation is no longer a goal. Worth keeping the history, because it is + a clean example of a fix that was correct all along and blocked entirely by a constraint + that has since expired. ### Ours, not the reference's @@ -1043,15 +1108,17 @@ When the next gap turns up, file it there rather than working around it here. ## 8. Status -**The port is done.** The C interpreter reproduces the Go reference byte for byte across its -entire test corpus and passes clean under ASan and UBSan. +**The port is done.** The C interpreter passes the Go reference's entire corpus and passes clean +under ASan and UBSan. It reproduced that corpus byte for byte until §0.1 retired the +requirement; exactly one case has diverged on purpose since, and +`tests/reference/README.md` lists it. | Gate | Result | |---|---| -| `ctest` | 76/76 — 41 upstream golden cases, 9 local ones, 23 unit tests, 2 embedding examples, 1 known-failing | -| `ctest` with `-DAKBASIC_WITH_AKGL=ON` | 75/75 — the same, minus the three `no_device` cases the SDL driver contradicts, plus `akgl_backends` and `akgl_frontend`; the `akgl_build` CI job | +| `ctest` | 77/77 — 41 reference golden cases, 9 local ones, 25 unit tests, 2 embedding examples. No known-failing tests: the list is empty | +| `ctest` with `-DAKBASIC_WITH_AKGL=ON` | 76/76 — the same, minus the three `no_device` cases the SDL driver contradicts, plus `akgl_backends` and `akgl_frontend`; the `akgl_build` CI job | | Golden corpus | 41/41 byte-exact from `tests/reference/` — **and 41/41 again through the SDL binary**, which is most of what proves the frontend changes no output | -| ASan + UBSan | 76/76 | +| ASan + UBSan | 77/77 | | Line coverage | 94.6% (3857/4076) — above the 90% gate | | Function coverage | 98.6% (287/291) | | Warnings | none under `-Wall -Wextra` | @@ -1178,13 +1245,19 @@ 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 item 16.** Items 10, 12–15 and 17 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**~~ **Done** — `akbasic_runtime_global()`, `tests/hostvars.c`, and the README - and example both pointed at it. +5. ~~**§6 items 12–17.**~~ **All done**, each with a regression test in the suite that pinned + it. Item 16 was the last, and it was never a coding problem — it was blocked on the + fidelity constraint §0.1 retired, and took about ten minutes once that went away. + `AKBASIC_KNOWN_FAILING_TESTS` is empty as a result and `tests/known_reference_defects.c` is + gone. + + **§6 items 1–9 and 11 are now open in a way they were not.** They were reproduced + deliberately and left alone because faithfulness was the goal; it is not any more. Read them + as an ordinary defect list. Item 4 (`mathPlus` mutates `self` in place, which `CommandNEXT` + relies on) is the one with real blast radius and still wants `FOR`/`NEXT` coverage before + anybody touches it. Item 5 (binary operators add *both* of the right operand's numeric + fields) is the one worth doing early: it is currently harmless, it makes two mutants in + `src/value.c` unkillable, and it is a landmine for any future value carrying both. 6. **Mutation survivors in `src/value.c`** — ~~closed~~, with one correction to what was written here before. `tests/value_arithmetic.c` grew two functions, `test_maximum_length_string()` and `test_pool_starts_empty()`, and each was verified by diff --git a/src/scanner.c b/src/scanner.c index 97d6453..9c88d5c 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -224,10 +224,12 @@ static akerr_ErrorContext *match_identifier(akbasic_Runtime *obj) { PREPARE_ERROR(errctx); char lexeme[AKBASIC_MAX_LINE_LENGTH]; + char basename[AKBASIC_MAX_LINE_LENGTH]; const akbasic_Verb *verb = NULL; void *fndef = NULL; bool userfunction = false; char c = '\0'; + size_t used = 0; obj->tokentype = AKBASIC_TOK_IDENTIFIER; while ( !is_at_end(obj) ) { @@ -260,13 +262,21 @@ static akerr_ErrorContext *match_identifier(akbasic_Runtime *obj) } PASS(errctx, get_lexeme(obj, lexeme, sizeof(lexeme))); + /* - * Searched with the type suffix still attached, which is the reference's - * rule (basicscanner.go:349) and is why the "Reserved word in variable name" - * branch below is unreachable -- TODO.md section 6 item 16, which explains - * why it is still here rather than fixed. + * The verb table is searched on the *base* name, with any type suffix + * stripped. The reference searches with the suffix still attached + * (basicscanner.go:349), so `PRINT$` misses every table, the "Reserved word + * in variable name" branch below is dead code, and `PRINT$ = 1` is quietly + * accepted as an ordinary string variable. TODO.md section 6 item 16. */ - PASS(errctx, akbasic_verb_lookup(lexeme, &verb)); + strncpy(basename, lexeme, sizeof(basename) - 1); + basename[sizeof(basename) - 1] = '\0'; + used = strlen(basename); + if ( obj->tokentype != AKBASIC_TOK_IDENTIFIER && used > 0 ) { + basename[used - 1] = '\0'; + } + PASS(errctx, akbasic_verb_lookup(basename, &verb)); ATTEMPT { CATCH(errctx, akbasic_environment_get_function(obj->environment, lexeme, &fndef)); diff --git a/tests/known_reference_defects.c b/tests/known_reference_defects.c deleted file mode 100644 index 46d99a2..0000000 --- a/tests/known_reference_defects.c +++ /dev/null @@ -1,46 +0,0 @@ -/** - * @file known_reference_defects.c - * @brief Asserts the *correct* contract for defects carried over from the reference. - * - * Registered in AKBASIC_KNOWN_FAILING_TESTS, so CTest expects it to fail. Every - * assertion here states what the interpreter *should* do, not what it currently - * does -- pinning current-but-wrong behaviour would turn the eventual fix into a - * test failure. - * - * When one of these is fixed, this test starts passing and CTest reports - * "unexpectedly passed". That is the cue to split the fixed assertion out into - * the corresponding suite in AKBASIC_TESTS and strike the item from TODO.md - * section 6. - * - * Five of the six this file was written with are gone that way. What is left is - * the one whose fix is not a coding question but a decision, and it is described - * where it is asserted. - */ - -#include "harness.h" - -int main(void) -{ - TEST_REQUIRE_OK(harness_start(NULL)); - - /* - * 6.16 -- "Reserved word in variable name" never fires, because the lexeme - * still carries its type suffix when the verb table is searched, so PRINT$ - * misses every table and becomes an ordinary string variable. - * - * Still here rather than fixed, and the reason is worth knowing before - * anybody "just fixes it": stripping the suffix before the lookup is four - * lines and it works, and it costs a golden case. The reference's own - * examples/strreverse.bas names a variable INPUT$ -- which a real C128 would - * refuse too, so the reference program is the wrong one. But the corpus is - * the acceptance contract and it lives in a submodule this repository may - * not edit, so the diagnostic and the corpus cannot both be right. TODO.md - * section 6 records the trade rather than making it silently. - */ - HARNESS_RUNTIME.hasError = false; - TEST_REQUIRE_OK(akbasic_scanner_scan(&HARNESS_RUNTIME, "PRINT$ = 1", NULL, 0)); - TEST_REQUIRE(HARNESS_RUNTIME.hasError, "PRINT$ should be refused as a variable name"); - - harness_stop(); - return akbasic_test_failures; -} diff --git a/tests/reference/README.md b/tests/reference/README.md index c4fc2b9..5543a8d 100644 --- a/tests/reference/README.md +++ b/tests/reference/README.md @@ -14,36 +14,49 @@ interpreter was ported from: They are registered as the `golden_*` CTest cases and run against `basic` on every build, in both the default and the `-DAKBASIC_WITH_AKGL=ON` configurations. -## Why they are a copy now, when they deliberately were not +## Why they are a copy, when they deliberately were not Until 2026-07-31 these were driven *in place* out of `deps/basicinterpret`, and the reasoning was written down in `CMakeLists.txt`: the corpus is a submodule, and copying a submodule's corpus guarantees drift. -That reasoning was sound. It was overruled on purpose, because the Go dependency is being -deprecated and a build that cannot run its own acceptance suite without cloning the -implementation it replaced is not finished. +That reasoning was sound and it stopped applying. The Go project is deprecated and will not be +updated, so there is nothing left to drift *from* — and a build that cannot run its own +acceptance suite without cloning the implementation it replaced is not finished. -**What the change costs, stated plainly so nobody has to rediscover it:** if the Go -implementation ever gains a test, this directory will not. There is no longer anything -watching for that. The trade is deliberate — the Go project is the thing being retired, so a -corpus that stops tracking it is a corpus that stopped tracking something that stopped moving. +## What these are for now + +**A regression suite, not a specification.** That distinction is the whole of how to treat +them, and it changed on 2026-07-31 (`TODO.md` §0.1): matching the Go implementation is no +longer a goal, so a difference between it and this interpreter is not automatically a bug in +this one. + +What they are still good for is unchanged and is worth a lot: forty-one real BASIC programs +with known-good output, covering arithmetic, arrays, flow control, every builtin function, +`READ`/`DATA`, labels and three worked examples. That catches regressions whatever its +provenance. ## The rule for editing these -**Don't.** +**Diverge deliberately, never by accident.** -This directory is a *record of what the Go implementation did*, and it is what makes the claim -"reproduces the reference byte for byte" checkable at all. A failure here means this -interpreter's behaviour changed, and the answer is to change this interpreter or to record a -deliberate deviation in `TODO.md` §5 — never to edit the expectation to match. +A failure here means this interpreter's behaviour changed. Nine times in ten that is a bug you +just introduced and the answer is to fix the code. The tenth time it is an improvement on the +reference, and then the answer is: -New cases for this project's own behaviour go in `tests/language/`, which is registered under -`local_*` for exactly this reason: the prefix says which corpus a failure came from, and a diff -that touches `tests/reference/` stands out as the thing it is. +1. Change the expectation in the same commit as the code. +2. Say why in the commit message. +3. Add a line to `TODO.md` §5, which is the list of deliberate deviations. -There is one case worth knowing about before anybody "fixes" it. -`examples/strreverse.bas` names a variable `INPUT$`, which a real Commodore 128 would refuse -and which this interpreter accepts only because the reference's reserved-word check never -fires. That is `TODO.md` §6 item 16, and it is the reason that item is filed as a decision -rather than a defect: making the check work costs this case. +What is not acceptable is editing a `.txt` to make a red suite green without deciding which of +the two you are doing. + +New cases for this project's own behaviour go in `tests/language/`, registered under `local_*`. +The prefix says which corpus a failure came from, and a diff touching `tests/reference/` stands +out as the thing it is. + +## Divergences so far + +| Case | Change | Why | +|---|---|---| +| `examples/strreverse.bas` | Variable `INPUT$` renamed to `SOURCE$` | `INPUT$` is a reserved word with a type suffix, which a real C128 refuses and which this interpreter now refuses too (`TODO.md` §6 item 16). The reference accepted it only because its own reserved-word check never fired. The expectation is byte-for-byte unchanged — the program still prints `REVERSED: OLLEH` — so the case keeps all of its value. | diff --git a/tests/reference/examples/strreverse.bas b/tests/reference/examples/strreverse.bas index deadc48..f75484c 100644 --- a/tests/reference/examples/strreverse.bas +++ b/tests/reference/examples/strreverse.bas @@ -1,7 +1,7 @@ -10 INPUT$ = "HELLO" -20 STRLEN# = LEN(INPUT$) - 1 +10 SOURCE$ = "HELLO" +20 STRLEN# = LEN(SOURCE$) - 1 30 REV$ = "" 40 FOR I# = STRLEN# TO 0 STEP -1 -50 REV$ = REV$ + MID(INPUT$, I#, 1) +50 REV$ = REV$ + MID(SOURCE$, I#, 1) 60 NEXT I# 70 PRINT "REVERSED: " + REV$ diff --git a/tests/scanner_tokens.c b/tests/scanner_tokens.c index 09a2a0e..3341f40 100644 --- a/tests/scanner_tokens.c +++ b/tests/scanner_tokens.c @@ -150,15 +150,32 @@ int main(void) TEST_REQUIRE_INT(env()->lineno, 10); /* - * The "Reserved word in variable name" check never fires: the lexeme still - * carries its suffix when the verb table is searched, so "PRINT$" misses and - * becomes an ordinary string variable. Pinned as it stands -- TODO.md - * section 12 item 16. + * A verb name with a type suffix stuck on it is refused as a variable name. + * The reference searches the keyword tables with the suffix still attached + * (basicscanner.go:349), so "PRINT$" misses every table, its own "Reserved + * word in variable name" diagnostic is dead code, and `PRINT$ = 1` is + * quietly accepted. TODO.md section 6 item 16; this is its regression test. */ + HARNESS_RUNTIME.hasError = false; TEST_REQUIRE_OK(akbasic_scanner_scan(&HARNESS_RUNTIME, "PRINT$ = 1", NULL, 0)); - TEST_REQUIRE(!HARNESS_RUNTIME.hasError, - "PRINT$ is currently accepted as a variable name; see TODO.md 12.16"); - expect_token(0, AKBASIC_TOK_IDENTIFIER_STRING, "PRINT$"); + TEST_REQUIRE(HARNESS_RUNTIME.hasError, "PRINT$ must be refused as a variable name"); + + /* The same for a function name, and for the other two suffixes. */ + HARNESS_RUNTIME.hasError = false; + TEST_REQUIRE_OK(akbasic_scanner_scan(&HARNESS_RUNTIME, "LEN# = 1", NULL, 0)); + TEST_REQUIRE(HARNESS_RUNTIME.hasError, "LEN# must be refused as a variable name"); + HARNESS_RUNTIME.hasError = false; + TEST_REQUIRE_OK(akbasic_scanner_scan(&HARNESS_RUNTIME, "GOTO% = 1", NULL, 0)); + TEST_REQUIRE(HARNESS_RUNTIME.hasError, "GOTO%% must be refused as a variable name"); + + /* + * And an ordinary name that merely *contains* a verb is still fine, which is + * what says the check compares the whole base name rather than a prefix. + */ + HARNESS_RUNTIME.hasError = false; + TEST_REQUIRE_OK(akbasic_scanner_scan(&HARNESS_RUNTIME, "PRINTER$ = 1", NULL, 0)); + TEST_REQUIRE(!HARNESS_RUNTIME.hasError, "PRINTER$ is not a reserved word"); + expect_token(0, AKBASIC_TOK_IDENTIFIER_STRING, "PRINTER$"); /* An unknown character is diagnosed. */ HARNESS_RUNTIME.hasError = false;