Conversation
The parser took exactly one statement for each branch of a single-line IF,
so everything after the first colon escaped the conditional:
X = 0
IF X = 1 THEN PRINT "A" : PRINT "B"
printed B. Inside a loop the leak was louder still -- three iterations of
`IF I = 2 THEN PRINT "two" : PRINT "x"` printed `x two x x` rather than
`two x`.
The ELSE form did not merely misbehave, it failed to compile. With the
second statement parsed as a sibling of the IF, the ELSE that followed
reached the top level and was rejected as "ELSE without matching IF", so a
legal QuickBASIC line could not be built at all.
Both branches now take the colon-separated list that QuickBASIC gives them:
everything after THEN up to ELSE or the end of the line, and everything
after ELSE. The guard on a trailing separator is load-bearing -- parse_statement
skips a leading newline, so without it `IF C THEN PRINT "x" :` would swallow
the statement on the line below.
Nothing in the suite used the form, which is why 314 tests stayed green over
it. Three now cover it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IF X < 0 THEN 900 is how GW-BASIC spells its commonest branch, and the form
runs through published listings. xbasic64 rejected it outright:
error: Unexpected token: Integer(900)
It was documented nowhere -- neither LANGREF as supported nor NONGOALS as
refused -- so a program using it hit a diagnostic that read like a typo.
A statement cannot otherwise begin with a number, so there is nothing to
disambiguate: in statement position within a single-line IF branch, an
Integer or LineNumber now parses as GOTO to that line. Both branches take
it, and it composes with the colon-separated lists they already accept.
LANGREF grows a worked example of each. tests/docs compiles every basic
block in that file, so the documentation is now its own regression test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conditions from DO and from LOOP were merged with
`condition.or(end_condition)`, so writing both silently discarded the one on
the LOOP:
I = 0
DO WHILE I < 3
I = I + 1
LOOP UNTIL I > 100
ran on the WHILE alone and printed 3. The UNTIL had no effect whatever, and
nothing said so.
A program that writes both is confused about which test is being applied, and
guessing on its behalf makes the confusion permanent. It is now an error that
names the two places. Every single-ended form is unaffected, and a test now
pins all five of them so the check cannot start rejecting DO loops wholesale.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four small things in the lexer, one of them a silent wrong answer. `num.parse().unwrap_or(0)` turned a line number too large to represent into label 0, so `99999999999 PRINT "hi"` defined a label nobody wrote. Any GOTO aimed at it failed separately and confusingly, because past LONG range the same digits lex as a Double rather than an integer. read_number and read_radix were both deliberately made strict about exactly this; the line-number path was missed. It now reports the number it could not represent. The '"' arm decremented self.pos and rebuilt the whole Peekable<Chars> from an input slice, purely to un-consume the quote it had just read so that read_string could read it again. Not consuming it in the first place removes the rewind -- and with it the only uses of the `input` and `pos` fields, so both are gone and advance() is now self.chars.next(). Token::Rem was unreachable: next_token intercepts REM before keyword lookup is ever reached, because it introduces a comment rather than a token. The variant and its table entry go together, since clippy -D warnings rejects either half alone. KEYWORDS becomes a match on &str instead of a LazyLock<HashMap> cloned into on every identifier. This is a simplification, not a speedup: compiling a 200k-line program still takes 0.80-0.84s against a 0.82s baseline, so keyword lookup was never where the time went. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The recursive-descent parser had no depth limit, so it did not refuse deep
input -- it died on it. 50,000 nested parentheses, or the same number of
unary minuses or NOTs, aborted the process:
thread 'main' has overflowed its stack
fatal runtime error: stack overflow, aborting (exit 134)
Deeply nested IF blocks did the same. Exit 134 is outside anything a caller
can interpret; the test harness's own is_clean_rejection() requires exit 1,
so these programs were not merely rejected badly, they were outside the
contract the suite is written against.
A MAX_DEPTH of 256 is far above what anyone writes and far below what the
stack takes, so the only programs it turns away are ones that were going to
crash. Both guards share one counter and one message, since whichever trips
first is a fact about the input, not about which parser function noticed.
Separately, parse_statement_kind recursed once per statement separator to
skip it. That is a tail call, so a release build optimized it away and only
a debug build overflowed -- on 200,000 colons. A bug that hides from the
profile CI tests is the worst kind to keep, so the separators are now skipped
in a loop, which costs no stack in either profile.
Verified in both profiles: debug now reports all four cases at exit 1, and a
60-deep expression still compiles and runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BASIC's block terminators are statements syntactically but belong to the
construct that opened the block, so parse_statement had to hand them back
somehow. It did so as ParseError::Block, which worked but meant `?` could not
be trusted: any `?` in the parser might be propagating an ordinary end-of-block
rather than a failure, and a stray terminator was caught by whichever enclosing
block parser happened to match it first rather than being reported where it
was written.
They now travel in Parsed<T>, so `?` carries only real errors. The thirteen
sites that produced them collapse into one try_block_end, which is also the
single place that knows END is only a terminator when IF, SUB, FUNCTION or
SELECT follows it.
That in turn allows the seven near-identical body loops to become one
parse_block_body -- and this is where the user-visible change is. None of the
seven checked for end of file. They stopped only because an unrecognised token
became "Unexpected token: Eof", so every unterminated block blamed the last
line of the file and named nothing:
prog.bas:3: error: Unexpected token: Eof
Now:
prog.bas:2: error: FOR is missing its NEXT
prog.bas:2: error: SUB 'FOO' is missing its END SUB
prog.bas:1: error: SELECT CASE is missing its END SELECT
The line is the opener's, which needed ParseError::ErrorAt to carry a line of
its own rather than inheriting wherever the parser stopped. Wording follows
the message TYPE already used. A block closed by the wrong terminator now says
so too -- "FOR needs NEXT to close it, but WEND came first" -- rather than
propagating to the top level and claiming the WEND had no matching WHILE.
The nine unmatched-terminator messages at top level are unchanged, and their
test still passes untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six error sites formatted the offending token with {:?}, so the compiler
showed the lexer's internal spelling to people reading their own program:
error: Expected To, got Integer(2)
for a FOR missing its TO, and `EndSelect`, `LParen`, `Ne` and `Newline` at
programmers who had written END SELECT, (, <> and pressed return. There was
already a describe_token helper for this, used in ten places and bypassed in
six; its own fallback arm was one of the six.
token_spelling now gives every fixed token the text it is written with, and
describe_token falls back to it. The same messages read:
error: expected TO, got 2
error: unexpected ) in an expression
error: expected a line number or label, got +
The table also makes expect() honest. It matches by variant, so a payload
would have been ignored -- expect(Token::Integer(0)) would have accepted any
integer. Only payload-free tokens have a spelling, so asking for one is now
what proves the caller passed a sensible token, and a payload-carrying one is
refused at the point of the mistake rather than silently over-matching.
Two tests: one pinning five specific messages, one sweeping the whole set for
leaked Rust names, since that is the class of regression rather than any
individual string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sema has always returned a Vec<Diagnostic>, so a program with five undefined
names is told about all five in one compile. The parser stopped at the first
error, so five typos cost five compiles -- the same program, reported two
different ways depending on which pass happened to find the mistake.
Panic-mode recovery closes the gap. BASIC makes it unusually reliable: it is
line-oriented, so a newline or colon ends a statement no matter what went
wrong before it, and the parser can resume at the next one.
Recovery runs inside block bodies as well as at the top level, which is the
part that decides whether this is useful or merely noisy. Recovering only at
the top level would let an error escape the SUB it happened in, strand that
SUB's END SUB, and bury the one real mistake under complaints about a block
that was closed perfectly well. Recovering in place costs the statement and
nothing else:
SUB Foo
X = ) <- prog.bas:2: error: unexpected ) in an expression
PRINT 1
END SUB <- not blamed
xbasic64: 1 error
Synchronizing stops *before* the boundary rather than past it, so a terminator
sharing the line is still read normally. A hard error that recovery cannot get
past, such as an unterminated block, still ends the parse -- but the errors
already found are reported alongside it rather than thrown away.
main.rs gains one `report` helper now shared by both stages, so the two cannot
drift apart in format again. The in-crate test helper joins the errors to keep
its Result<_, String> shape; none of the 74 parser unit tests needed changing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`A(1)` is an array element or a call, and the parser cannot tell. It guessed from the DIM statements it had read so far, which had two consequences. The AST depended on where the DIM was written: the same expression became FnCall before it and ArrayAccess after, so every consumer had to handle both shapes for one construct. And the guess ignored scope entirely -- a DIM inside a SUB was recorded in one flat set, so it changed how module-level code parsed. The parser now emits FnCall for all of `name(args)` and sema rewrites the ones that name an array, against a symbol table that is finished and scoped. Doing it as a rewrite rather than teaching codegen to accept FnCall is deliberate. codegen's expr_is_call_free treats every FnCall as opaque, so leaving array reads as calls would have silently switched off FOR-counter promotion; walk_array_uses collects names only from ArrayAccess, so loop-invariant descriptor hoisting would have stopped firing. Neither would have failed a test as anything but lost performance. Keeping ArrayAccess in the AST codegen sees means codegen needed no changes at all. for_each_expr_mut is exhaustive with no wildcard arm, so a future StmtKind that carries an expression is a compile error here rather than a variant the rewrite quietly skips -- the same discipline child_bodies already documents. This also exposed a real disagreement worth refusing. Sema resolved an array/procedure clash in favour of the array and codegen in favour of the procedure; the parser's heuristic hid it. `DIM F(5)` alongside `FUNCTION F(X)` compiled silently, as did `DIM LEN(5)`, where codegen's builtin table would answer first and the array would never be read. Both are now diagnosed rather than resolved by whichever pass looked first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
read_identifier uppercases every character of every name, and its entry gate is is_ascii_alphabetic, so there is no Unicode folding to worry about and the type suffixes are case-invariant. Every name reaching the parser, sema or codegen is therefore already upper case. Sixty-three call sites wrote `name.to_uppercase()` to say so anyway, each allocating a fresh String to produce the string it was handed. Worse than the waste, each was a separate restatement of the rule, and they had already begun to disagree: Symbols::lookup_array uppercases the name for its module-level lookup but not for its procedure-local one, and collect() builds Scope::Proc from a bare clone while other code builds it from an uppercased name. Those happen to agree today only because the invariant holds everywhere -- which is precisely the thing nothing was checking. `normalized()` states it in one place and asserts it. Twenty-one borrowed lookups now call it instead of allocating; the remaining sites need an owned String for a map key, where to_uppercase and to_string cost the same. This is not a speedup, and the numbers say so plainly: a 200k-line program compiles in 0.84-0.93s against a 0.82s baseline, which is noise. Profiling that program first is what showed why -- lex 56ms, parse 90ms, sema 53ms, codegen 456ms, emit 141ms. Name handling was never where the time went, so this change is worth making for the invariant it pins down, not for speed. The check is debug_assert, so it costs nothing shipped and fires during a debug `cargo test`. The whole 468-test suite runs clean under it, and a unit test confirms the guard is live rather than merely written down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Profiling a 200k-line program put 55% of compile time in codegen and 17% in
emitting the result, against 6-11% each for lex, parse and sema. That program
assembles to five million lines and 145 MB of text, which turns out to be the
whole story.
Three changes, none of which alters a byte of output.
`self.emit(&format!(...))` built a throwaway String per instruction only to
copy it into the buffer and drop it -- five million allocations spent handing
text to push_str. An `emit!` macro writes through fmt::Write instead. This is
301 sites and mechanical; correctness rests on the output being unchanged
rather than on reading all of them.
`generate` returned `self.output.clone()`, and main.rs then built
`format!("{}\n{}", asm, runtime_asm)`. Each made a further full copy of those
145 MB. The buffer is now handed over with mem::take and the runtime appended
onto it.
Measured, 200k-line program:
before 0.82 s 715 MB
after 0.68 s 571 MB
By phase, codegen 456 -> 364 ms and writing 141 -> 83 ms; lex, parse and sema
are unchanged at 55/89/53 ms, as expected since nothing touched them.
Verified by diffing generated assembly for all 13 examples, the 809-line
megatest, and a synthetic program exercising records, fixed strings, random
files, FIELD/LSET/RSET, GET/PUT, LOCK, SELECT CASE, PRINT USING and the string
builtins. All 15 are byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three statement parsers spelled out the same `.field.sub` loop verbatim -- parse_lvalue, and twice inside parse_assignment_or_call for the bare and subscripted forms. They are now one parse_field_path, next to the parse_field_chain that does the same job for expressions. The token-soup test is the one that matters. Every existing test feeds the compiler a program somebody thought about, which is why 314 of them coexisted with a stack overflow on nested parentheses: nobody writes that program, so nobody tested it. This one assembles 300 pseudo-random token sequences from the keyword and punctuation vocabulary and requires only that the compiler survive them -- rejecting is fine, exiting 101 or 134 is not. The generator is a fixed-seed xorshift rather than a dependency, so a failure is reproducible from the seed and the corpus does not drift between runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four of AND, OR, XOR and NOT were wrong, in two different ways, and every
existing test agreed with them because every test used operands that were
already 0 or -1.
AND, OR and XOR silently returned their LEFT operand whenever an operand was
Double -- which is what an unsuffixed variable is, so this was the ordinary
case:
A = 12 : B = 10
PRINT A AND B ' printed 12, not 8
C = A AND B ' C = 12
IF (A AND B) = 8 THEN ' false
The emitted assembly was correct. `cvttsd2si eax / cvttsd2si ecx / and eax, ecx`
computes 8. But promote_types had no case for these three, so the expression's
type came out Double, PRINT called _rt_file_print_float, and that reads xmm0 --
still holding the left operand. The answer in EAX was discarded. The same
function already returns Long for comparisons, `\` and MOD, which is exactly why
those three were right. Literal operands are constant-folded before this path,
which is how 470 tests missed it.
NOT was a *logical* not: `sete al / movzx / neg`, i.e. "0 gives -1, anything
else gives 0". So `NOT 12` was 0 where GW-BASIC gives -13. It now complements
the bits like the three operators beside it, and LANGREF's claim that these
"operate bitwise on integers" becomes true of all four rather than three.
That last change is visible: `IF NOT 1` is now taken, because NOT 1 is -2 and
-2 is non-zero. test_logical_operators asserted the opposite and has been
rewritten to say why, along with the note in LANGREF -- comparisons yield -1 or
0 and NOT maps those to each other, so `IF NOT (A > 0)` reads as expected while
`IF NOT 1` does not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three disagreements between the parser and the precedence table LANGREF publishes, all of them silent wrong answers rather than rejections. XOR had a level to itself that bound tighter than AND. LANGREF puts OR and XOR together at the bottom, as GW-BASIC does, so `-1 OR 0 XOR -1` should be `(-1 OR 0) XOR -1` = 0 and gave -1. NOT parsed its operand at the *caller's* precedence, which at statement level is the lowest there is, so NOT swallowed whatever followed it: `NOT A AND B` became `NOT (A AND B)`. It now takes an operand at the comparison level, which is where LANGREF puts it -- one step below the comparisons and one above AND. That keeps `NOT A = B` grouping as `NOT (A = B)`, the form that actually matters, while leaving AND to the precedence loop. `^` was right-associative. GW-BASIC and QuickBASIC evaluate equal-precedence operators left to right, so `2 ^ 3 ^ 2` is 64, not 512. Every operator now associates left to right and LANGREF says so explicitly -- its silence on associativity is what left this open to interpretation. Two existing tests asserted the old behavior and have been rewritten with the reasoning rather than just re-pointed: test_expr_power_right_associative (renamed) and test_expr_logical_operators, whose `A AND B OR C XOR D` now has XOR outermost because OR and XOR share a level. `-2 ^ 2` is still -4 and `2 ^ -2` still parses; both are pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The name after NEXT was parsed and thrown away, so it documented nothing and
checked nothing. With two loops open that is a silent structural miscompile:
FOR I = 1 TO 2
FOR J = 1 TO 2
NEXT I <- actually closed the J loop
NEXT J <- actually closed the I loop
compiled and ran, with a nesting nobody wrote. GW-BASIC calls this
"NEXT without FOR". A named NEXT must now name the loop it closes; a bare NEXT
still closes the innermost one, which LANGREF documents and mega.bas tests.
Carrying the names also makes `NEXT J, I` work, which was previously a syntax
error. BlockEnd::Next holds the list, parse_for takes the first and leaves the
rest in a queue that each enclosing block body picks up before reading another
token, so the terminator reaches every loop it names as the recursion unwinds.
Two details in that queue are load-bearing and both were got wrong first:
The queue must be consulted *before* the end-of-file guard in parse_block_body.
The NEXT has already been consumed by the time the names are queued, so for a
loop nest that ends the program the next token is Eof -- and checking after the
guard reported "FOR is missing its NEXT" with the terminator sitting right
there.
Names left over at the end of the program must be drained after the statement
loop, not only inside it. `FOR I .. NEXT I, J` as the last statement leaves the
stream at Eof with J still queued, so the loop condition exited and the extra
name vanished silently. It is now "NEXT J without matching FOR".
The queue is cleared in synchronize, so a half-parsed statement cannot leave a
stale name to surface as a terminator somewhere unrelated -- the token-soup
test feeds bare NEXT tokens and would have found that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DATA hello, world` was rejected -- and rejected confusingly. parse_data accepted only Integer, Float, String and a leading minus, and anything else simply broke the item loop, so the unquoted word was left in the stream to be parsed as a fresh statement. The error named the word, not the DATA. Empty items (`DATA 1,,3`) failed the same way. Reassembling items from tokens would not have fixed it. The lexer uppercases identifiers, so `DATA hello` would come back as HELLO; `DATA 007` and `DATA 1.50` would lose their spelling too. A DATA item is not an expression -- it is a literal run of characters -- so the lexer now hands the whole operand over verbatim as Token::DataText, exactly as it already intercepts REM, and the parser splits it. The rules are GW-BASIC's: quotes are needed only for an item containing a comma, a colon, or spaces that matter; an unquoted item is trimmed; an omitted one is empty, which reads as 0 or "". A doubled quote inside quotes is one quote character, as everywhere else in the language. Two details worth naming. Whitespace *outside* a quoted item is not data -- the first attempt kept it, and `DATA 10, "Hello"` read back as " Hello". And scanning stops at a colon outside quotes rather than running to end of line as GW-BASIC does, because `DATA 1,2 : PRINT 3` has always worked here and nothing in the wild relies on the other rule. Token::Data is gone: like REM's entry before it, the keyword-table entry became unreachable once next_token intercepted the word. A suffixed `Data$` is still an ordinary variable, which a test now pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DATA 42` followed by `READ A$` segfaulted. _rt_read_string ignored the entry's type tag and passed its second word to strlen whatever it held, so the integer 42 was measured as a string at address 42. Exit 139, no diagnostic. _rt_read_number has always done the mirror conversion -- it parses a string-tagged entry with strtod -- so the asymmetry was the bug, not the design. The numeric cases now render through _rt_str, which is the same text PRINT would produce, as GW-BASIC does. Both runtime trees, per the lockstep rule. The plan had been to diagnose this in sema instead. That cannot be done soundly: pairing a READ with the item it will consume requires the execution path, and RESTORE, GOTO and conditionals make it undecidable in general. Any conservative approximation strong enough to catch `DATA 42 / READ A$` also rejects `DATA 1, "two", 3.5, "four"` with `READ A, B$, C, D$`, which is legal, idiomatic, and already in the suite. Converting at the point of use is both sound and what the reference implementation does. With this, a DATA item's tag no longer has to match the variable it is read into, in either direction -- which is also what the previous commit needs, since an unquoted item that happens to look numeric is now tagged numeric. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LANGREF said `INPUT X` prompts with "? ". It did not, and no form did: the parser accepted either separator after the prompt and threw away which one it was, and no question mark existed anywhere in codegen or either runtime. GW-BASIC decides by the separator. `INPUT "p"; A` prints `p? `, `INPUT "p", A` prints `p` alone, and a promptless `INPUT A` prints a bare `? `. The separator is now kept in the AST and codegen folds the question mark into the prompt text, so this costs nothing at run time and needs no runtime change. LINE INPUT never adds one, whichever separator is written -- that is its documented difference from INPUT, and worth stating because the two statements otherwise parse alike. The leading `INPUT ; "p"; A` is now accepted. In GW-BASIC it suppresses the newline echoed when the operator presses Return; here that newline is the terminal's echo rather than anything the program emits, so there is nothing to suppress and the form parses with no effect. Refusing it would turn away a program for asking about a difference this implementation cannot observe, so LANGREF says plainly that it is ignored and why. Two existing tests asserted output that a promptless INPUT now prefixes with "? ". Both were asserting the absence of the bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`1e400` compiled to a silent infinity. parse::<f64>() answers `inf` for a literal too large to represent rather than failing, so the "malformed number" arm never fired -- the one numeric form in this lexer that still guessed, after read_radix and the line-number path were both made strict. A trailing `_` now continues a statement on the next line. The newline is swallowed so no Newline token is produced, but the line counter still advances, so an error three lines into a continued statement still names line 3. Nothing may follow the underscore, and saying so beats "Unexpected character: _". `IF X = 1 THEN :` opened a single-line IF, because the single-line test treated any token other than end-of-line as the start of a statement. A run of colons separates no statements, so the line ends there and the block form is what was meant; the END IF below is no longer left unmatched. And a failed block header no longer blames its own terminator. `FOR I = 1 2 3` reported "expected TO, got 2" and then "NEXT without matching FOR" about the FOR one line above it. Once anything has gone wrong a stray terminator is usually the perfectly good closer of a block whose header failed, so it is suppressed after the first error -- and still reported in a program that is otherwise clean, which a test pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways a program could reach a tool it should never have met. A lone RETURN produced `ld: undefined reference to _gosub_sp`. codegen defines the GOSUB return stack only when it has seen a GOSUB, so the RETURN emitted a reference to a symbol nothing declared and the failure surfaced from the linker. Sema exists precisely to stop the compiler inventing names that escape to `ld`, so it now refuses a RETURN in a program with no GOSUB anywhere, with a note pointing at EXIT SUB for the case where someone meant to leave a procedure. The check is whole-program on purpose. Deciding whether a *particular* RETURN is reachable from a *particular* GOSUB needs the control flow, and GOTO makes that undecidable; "no GOSUB at all" is sound and is the shape the mistake actually takes. CALL was not recognised at all, so `CALL MySub(1)` parsed as a paren-less call to a subroutine named CALL taking `MySub(1)` as its argument -- and the error talked about MySub having no value, which is true and useless. It is now the explicit call form GW-BASIC and QuickBASIC both provide, recognised only before a name in statement position like the random-access statement names, so a program may still use CALL for a variable of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`PRINT #3, "x"` on a number nobody OPENed died with SIGSEGV, exit 139. So did `PRINT #3, 5`, a bare `PRINT #3,` and `LINE INPUT #3, A$`. Four helpers loaded the handle-table slot and passed it straight to fprintf, fputc or fgets without looking at it. The machinery was already there and simply not used consistently: _rt_file_getc and _rt_file_eof have always checked for a NULL handle, GET # and the LOCK family already raise `?Bad file number`, and _rt_error prints a bare message when given line 0 -- so a guard needs no new argument and no signature change. These four now take the same path, in both runtime trees. before: Segmentation fault (core dumped) exit 139 after: ?Bad file number exit 1 Exit 139 is outside the contract the test harness is written against -- is_clean_rejection wants 1 -- so these were not merely rude, they were untestable. A test now pins all four. The four were found by probing every file helper against an unopened number rather than by reading: INPUT #, EOF, LOF, LOC and CLOSE all behaved, and only these four crashed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three cases where the runtime carried on with something it should have complained about, all confirmed by running them rather than by reading. A failed OPEN was stored in the handle table anyway, so opening a file that is not there appeared to succeed: `EOF()` answered -1, and the mistake surfaced later as a crash or as nothing at all. It is now `File not found`. On Windows this needed its own test -- CreateFileA reports failure as INVALID_HANDLE_VALUE, which is -1 rather than NULL, so the NULL guards added in the previous commit would not have caught it. Re-opening a file number that was still open silently rebound it, leaking the old handle and whatever it had buffered. Now `File already open`. Closing first still works, and OUTPUT and APPEND still create files that do not exist. Reading past the end returned an empty string forever, so a loop that forgot its `EOF()` test ran on quietly instead of saying what was wrong. Now `Input past end of file`. The Win64 reader needed care: it reads a byte at a time, so "nothing read" means past-the-end only when nothing had been read on that call at all -- a last line with no trailing newline already has characters in the buffer and must still be returned. All three in both runtime trees. The documented `WHILE NOT EOF(1)` loop is unaffected, and LANGREF now names the three errors next to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens xbasic64’s GW-BASIC/QuickBASIC compatibility and hardens the front-end/runtime against crashes by improving lexing/parsing semantics, adding semantic validation and AST rewrites, and expanding the integration test suite + LANGREF accordingly.
Changes:
- Front-end robustness: parser now reports multiple syntax errors, performs better error recovery, enforces nesting limits to avoid stack overflows, and fixes several GW-BASIC syntax/precedence behaviors (single-line IF tails, implied GOTO after THEN/ELSE line numbers,
NEXTvariable checking,CALL, etc.). - Lexer/runtime correctness: adds line continuation (
_), preserves rawDATAtext (case/spacing), fixes numeric overflow handling, and improves runtime error handling for file I/O andREADconversions. - Test + documentation expansion across variables/procedures/input/file I/O/errors/data/control/arrays/arithmetic, plus LANGREF updates to match the new behavior.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/variables/mod.rs | Adds coverage for _ line continuation. |
| tests/procedures/mod.rs | Adds coverage for CALL statement parsing and non-reservation of CALL. |
| tests/input/mod.rs | Updates/expands prompt separator semantics (; vs ,) and LINE INPUT behavior. |
| tests/file_io/mod.rs | Adds tests asserting clean runtime errors for unopened file numbers, failed OPEN, reuse of open file numbers, and read-past-EOF. |
| tests/errors/mod.rs | Adds extensive negative tests for diagnostics quality, recovery, tallying, and crash-proofing. |
| tests/data/mod.rs | Adds tests for GW-BASIC DATA quoting/splitting rules and cross-type READ conversions. |
| tests/control/mod.rs | Adds tests for single-line IF statement lists, implied GOTO, NEXT name checking/multi-close, and THEN trailing colons. |
| tests/arrays/mod.rs | Adds coverage ensuring array vs call resolution is scope-correct and DIM-order independent. |
| tests/arithmetic/mod.rs | Adds/updates tests for bitwise operator semantics, precedence/associativity, and NOT behavior. |
| src/sema.rs | Adds array-access resolution rewrite pass, name-collision checks, RETURN-without-GOSUB validation, and normalized identifier use in several lookups. |
| src/runtime/win64-native/file.s | Adds guards and diagnostics for unopened file numbers, failed OPEN, reuse of open file numbers, and read-past-EOF. |
| src/runtime/win64-native/error.s | Adds new runtime error strings for file-open and EOF cases. |
| src/runtime/win64-native/data.s | Makes _rt_read_string safely convert numeric DATA entries to strings. |
| src/runtime/sysv/file.s | Mirrors Win64 file I/O guards/diagnostics + read-past-EOF behavior on SysV runtime. |
| src/runtime/sysv/error.s | Adds new runtime error strings for file-open and EOF cases. |
| src/runtime/sysv/data.s | Mirrors Win64 _rt_read_string numeric-to-string conversion. |
| src/parser.rs | Major parser refactor: multi-error reporting, recovery, nesting limits, updated operator precedence/associativity, block terminator handling, DATA parsing from raw text, CALL, and control-flow syntax fixes. |
| src/main.rs | Unifies parser+sema diagnostic reporting and avoids large runtime concatenation copies. |
| src/lexer.rs | Adds normalized() invariant helper, keyword matching refactor, raw DATA operand capture, _ line continuation, and numeric/line-number overflow diagnostics. |
| LANGREF.md | Documents line continuation, updated INPUT prompt rules, single-line IF tails, NEXT rules, DATA rules, file I/O error behavior, and CALL. |
| e.txt | New file added (appears accidental). |
| a.txt | New empty file added (appears accidental). |
Suppressed comments (1)
src/sema.rs:992
resolve_expruppercasesFnCallnames withto_uppercase(), but identifiers are already normalized by the lexer and this PR addsnormalized()specifically to avoid these allocations. Usingnormalized(name)here avoids a newStringfor every call/array-access resolution.
if let Expr::FnCall { name, args } = expr {
let upper = name.to_uppercase();
if !symbols.procs.contains_key(&upper) && symbols.lookup_array(scope, &upper).is_some()
{
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three INPUT tests compared raw program output including the trailing newline, which is LF on System V and CRLF on Windows -- so they passed on Linux and failed the Windows job on nothing but the line ending. They now compare trim_end(), which is what the rest of the suite does with .trim() and .lines() and the reason none of those tests had the problem. trim_end still catches a spurious leading space, so nothing is given up. The stray a.txt and e.txt were mine, swept into a commit by `git add -A`. The fix is not to delete them: compile_and_run_flags spawned the compiled program without setting its working directory, so a program opening a file by a bare name wrote it wherever the test runner happened to be -- the repository root -- and every `cargo test` recreated them. compile_and_run_with_files already passed current_dir; now the other helper does too, and a full run leaves the tree clean. That also makes the note in .gitignore true. It says these files "never appear" under cargo test because it runs in a temporary directory; that held for the megatest's helper and not for this one, which is how three of them reached a commit once before and two did again. Review also flagged `Scope::Proc(name.to_uppercase())` in resolve_array_accesses. Fair: the lexer already uppercased the name, and `collect` and `check` both build this scope with `clone` -- a scope key normalized differently from theirs would simply fail to match. The companion `to_uppercase` in resolve_expr goes through `normalized` for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`READ A%` stored 0. So did `INPUT A%`, `INPUT A!`, `READ A&` and every other
narrow scalar those statements write -- and `SWAP A%, B%` did not swap the two
values, it erased both.
One cause. gen_store_lvalue, the path all three share, stored the incoming
Double whole, so a two-byte INTEGER slot received eight bytes of a double's bit
pattern; every later read is `movsx eax, WORD PTR`, which takes its low sixteen
bits, and for any ordinary value those are zero. Its own array-element path
narrowed correctly twenty lines further down, and plain `A% = 7` stores through
a different path entirely -- which is why only these three statements were
affected, only for `%`, `&` and `!`, and only for scalars.
Variables declared `AS INTEGER` rather than by suffix live in typed storage and
were wrong for a second reason: that storage was not consulted at all here.
They now go through gen_store_typed, the same helper an ordinary assignment to
them uses.
Separately, `SWAP P.N, P.V` on a record with a string field and a numeric one
crashed with SIGSEGV. Sema's type check compared `a.name.ends_with('$')`, and
for `P.N` that reads *P* -- a record, carrying no suffix -- so both sides looked
non-string, the mismatch reached codegen, and a string pointer was stored into
an INTEGER slot. It now compares what the operands resolve to, using the
expr_is_string that already walks field paths.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Assignment stored the source verbatim, so the declared width did nothing: `STRING * 5` given "abcdefgh" held all eight characters, given "ab" held two, and LEN answered 8 and 2. That is the whole purpose of a fixed-length string, and sema's own doc comment already described the intended behaviour -- "its declared length used only to pad or truncate on assignment" -- so this implements a design that was documented and never written. A new _rt_fixed builds the padded or truncated copy, in both runtime trees. The static stack-alignment check over runtime call sites earned its keep here: it caught four misaligned calls in the first draft, including that four pushes after rbp leave rsp aligned so the Win64 shadow space must be 32 rather than the 40 _rt_space uses. Writing the test turned up a second, older bug behind the same feature: sema decided "is this a string?" from the `$` suffix alone, so a variable declared `AS STRING * 4` was treated as numeric and both `LEN(S)` and `S = "xy"` were rejected outright. Confirmed against the previous commit before fixing, so it is not fallout from the padding change. Three existing tests asserted the unpadded results and have been updated with the reason rather than re-pointed: a `STRING * 20` field prints its padding and LEN answers 20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
INSTR with a start past the end of the string read past the end of the buffer.
The runtime subtracted `start - 1` from the remaining length without checking,
so the length went negative -- enormous, unsigned -- the "room for the needle"
test passed, and memcmp searched memory beyond the string. It answered 253 and
261 on two runs of the same program; the correct answer is 0. A start below 1
walked the pointer backwards out of the buffer instead. Both guarded, in both
runtime trees.
The rest were silent wrong answers of the same family. `LEFT$("abc", -1)`
returned "abc", because _rt_left compares the count against the length
unsigned and -1 clamps to the whole string. `MID$("abc", 1, -1)` returned
"abc" for a worse reason: -1 is this compiler's own sentinel for the
two-argument form, so a program writing it explicitly got "the rest of the
string" from a value GW-BASIC rejects. Only a count the program actually wrote
is checked, so `MID$(s, 3)` still works. `ASC("")` read whatever byte its
pointer happened at and answered 0.
`(-8) ^ 0.5` printed `-nan`. Testing the result for NaN rather than the
operands is both cheaper and exact -- ucomisd sets parity only for an unordered
compare, and a value is unordered with itself only when it is NaN -- and it
also catches every other pow that has no real answer. Integral exponents of a
negative base are unaffected and pinned by a test.
All of these go through the existing check machinery, so `--unsafe` elides them
exactly as it does the bounds and divide checks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h AS CHR$ kept only the low byte of its argument, so CHR$(256) was CHR$(0) and CHR$(-1) was CHR$(255) -- two wrong characters, silently. SPACE$ and STRING$ clamped a negative count to zero and returned the empty string. All three are an illegal function call in GW-BASIC and are now refused, with the whole legal range including both ends pinned by a test. The bound lives beside the builtin table rather than in the shared CallLong path, because HEX$ and OCT$ take any Long and only CHR$ and SPACE$ are narrower. `DIM X$ AS INTEGER` was accepted although the suffix and the AS clause say different things, leaving no way to tell which the program meant. The identical check has always guarded a FUNCTION's declared result type, and LANGREF states the rule for both; DIM simply never applied it. Agreeing suffixes and unsuffixed names are unaffected, and a record type is exempt since it has no suffix to agree with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Windows job died with 0xC0000005 on `MID$("abc", 0, 2)` and
`INSTR(0, "abc", "b")` -- the two programs whose new argument checks fail. Not
the checks themselves: both sequences push three callee-saved registers and
never pad, so rsp was 8 bytes short of a 16-byte boundary at every call made
from inside them.
That was latent. _rt_mid and _rt_instr are leaves and tolerated it, so nothing
noticed. An argument check jumps to an error trampoline, and that calls
_rt_error, which calls into the C library -- where a misaligned rsp meets an
aligned SSE store. Linux shrugged; Windows raised an access violation, and the
Linux CI could not have seen it.
STRING$ has always paired its odd push with `sub rsp, 8` and says why in a
comment. MID$ and INSTR now do the same.
tests/runtime already checks this property for the hand-written runtime;
nothing checked the code the compiler emits. A companion test now walks main's
body for a set of straight-line programs and requires every call -- and every
conditional jump to an error trampoline, which inherits the jump site's
alignment -- to be made at a 16-byte boundary. Reverting the fix makes it
report 19 sites, naming `jl .Lerr_dom_1` and `call _rt_mid` specifically, so it
fails for the right reason rather than merely passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
REDIM PRESERVE, string concatenation, PRINT USING, SWAP and a fixed-length string assignment all make calls from sequences that manipulate rsp, and all already pair an odd push with `sub rsp, 8`. Covering them keeps that true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.