From 4a11170bfaff848849f60bd3ded7dab640fb622b Mon Sep 17 00:00:00 2001 From: Corey Ryan Dean Date: Sat, 30 May 2026 05:44:58 -0500 Subject: [PATCH 1/2] Make 'Release' a contextual keyword so legacy identifiers compile 'Release' is a BlitzForge addition (the GC release operator `Release.Type `); stock Blitz3D had no such keyword and freely used `release` as an ordinary variable. Reserving it globally broke drop-in source compatibility (INTENT #1) -- e.g. spindisc.bb's `If release = 1` failed with "Expecting identifier". This is the Phase 2 explicitly deferred by the contextual-'Test' work (PR #77). The operator form always has a type identifier immediately after the keyword (with an optional '.'): `Release.Type operand` / `Release Type operand`. So 'Release' is the keyword iff the next token is '.' or an identifier; otherwise it is a plain identifier. Apply that disambiguator at the three sites #77 used for 'Test': - parseIdent() accepts RELEASE (so `release` works as a parameter / Local / Global / Field name and type-instance var); - statement dispatch routes `release =`/`release(`/`release\f` through the IDENT path, keeping `Release.Type` as the operator; - the expression primary handles `Release.Type operand` when a '.'/ident follows, else falls through to the IDENT case. No tokenizer change; ReleaseNode semantics and the `Release.Type` grammar are untouched. Scope is 'Release' only -- 'Object' is a base Blitz3D keyword, a separate problem class, left deferred. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/blitzrc/compiler/parser.cpp | 35 ++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/blitzrc/compiler/parser.cpp b/src/blitzrc/compiler/parser.cpp index e6a132f..5ea70ab 100644 --- a/src/blitzrc/compiler/parser.cpp +++ b/src/blitzrc/compiler/parser.cpp @@ -63,11 +63,12 @@ void Parser::exp( const string &s ){ } string Parser::parseIdent(){ - // 'Test' is a contextual keyword: it is only the test-block opener at - // statement start (handled in parseStmtSeq). In every identifier position -- - // parameter names, Local/Global/Field declarations, type tags, labels -- it is - // an ordinary identifier, so legacy programs using `test` as a name compile. - if( toker->curr()!=IDENT && toker->curr()!=TEST ) exp( "identifier near: " + toker->text() ); + // 'Test' and 'Release' are contextual keywords: 'Test' is only the test-block + // opener at statement start, and 'Release' is only the GC release operator + // (`Release.Type `). In every identifier position -- parameter names, + // Local/Global/Field declarations, type tags, labels -- both are ordinary + // identifiers, so legacy programs using `test`/`release` as names compile. + if( toker->curr()!=IDENT && toker->curr()!=TEST && toker->curr()!=RELEASE ) exp( "identifier near: " + toker->text() ); string t=toker->text(); if (t.find(":") != std::string::npos) ex( "Identifiers cannot include the : character"); toker->next(); @@ -161,6 +162,11 @@ void Parser::parseStmtSeq( StmtSeqNode *stmts,int scope ){ // source compatibility). Route those uses through the IDENT statement path. int stmtTok = toker->curr(); if( stmtTok==TEST && !(scope==STMTS_PROG && toker->lookAhead(1)==IDENT) ) stmtTok=IDENT; + // 'Release' is contextual the same way: the GC release operator is always + // `Release.Type ...` or `Release Type ...` (a '.' or identifier follows). + // A statement like `release = ...`, `release(...)`, or `release\field` is an + // ordinary identifier use -- route it through the IDENT statement path. + if( stmtTok==RELEASE && !(toker->lookAhead(1)=='.' || toker->lookAhead(1)==IDENT) ) stmtTok=IDENT; switch( stmtTok ){ case INCLUDE: @@ -1029,12 +1035,6 @@ ExprNode *Parser::parseUniExpr( bool opt ){ result=parseUniExpr( false ); result=d_new RecastNode( result,t ); break; - case RELEASE: - if( toker->next()=='.' ) toker->next(); - t=parseIdent(); - result=parseUniExpr( false ); - result=d_new ReleaseNode( result,t ); - break; case REFERENCE: toker->next(); result=parseUniExpr( false ); @@ -1132,6 +1132,19 @@ ExprNode *Parser::parsePrimary( bool opt ){ case BBFALSE: result=d_new IntConstNode( 0 ); toker->next();break; + case RELEASE: + // 'Release' is a contextual keyword: `Release[.]Type ` is the GC + // release operator, but a bare `release` (followed by '=', '\', '(', '[', an + // operator, ...) is an ordinary identifier. Disambiguate on the next token: + // the operator form always has a '.' or a type identifier immediately after. + if( toker->lookAhead(1)=='.' || toker->lookAhead(1)==IDENT ){ + if( toker->next()=='.' ) toker->next(); + t=parseIdent(); + result=parseUniExpr( false ); + result=d_new ReleaseNode( result,t ); + break; + } + // else fall through: treat 'release' as an ordinary identifier case TEST: // contextual keyword: usable as an identifier in expressions case IDENT: ident=toker->text(); From e1c3f811b67b2c675cf32fe29de4bb7cfef071a5 Mon Sep 17 00:00:00 2001 From: Corey Ryan Dean Date: Sat, 30 May 2026 05:45:01 -0500 Subject: [PATCH 2/2] Pin 'release'-as-identifier + clear spindisc from the corpus allowlist Extend LegacyKeywordIdentifierTest.bb with `release` in every identifier position (local, assignment, If-condition operand, parameter, field-typed var) plus a positive test that the `Release.Type` GC operator still releases an object (RefCount 1 -> 0). Remove spindisc.bb from the corpus allowlist (it compiles now); the sweep goes 325 -> 326 compiled with 0 new failures. Record the working note. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/notes/legacy-release-keyword.md | 110 +++++++++++++++++++++++++++ scripts/corpus_compile_allowlist.txt | 9 ++- tests/LegacyKeywordIdentifierTest.bb | 59 ++++++++++++++ 3 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 docs/notes/legacy-release-keyword.md diff --git a/docs/notes/legacy-release-keyword.md b/docs/notes/legacy-release-keyword.md new file mode 100644 index 0000000..3657c9b --- /dev/null +++ b/docs/notes/legacy-release-keyword.md @@ -0,0 +1,110 @@ +# Working note — `Release` as a contextual keyword (legacy-identifier Phase 2) + +Branch: `align/legacy-release-keyword-identifier` +Type: Feature / Alignment (drop-in source compatibility, INTENT #1) + +## Goal (restated) + +BlitzForge globally reserves `Release` in the tokenizer, so a classic Blitz3D +program using `release` as an ordinary variable fails to compile. `Release` is a +*fork addition* (stock Blitz3D had no such keyword — release was `x = Null` / +`Delete`), so `release`-as-identifier is a genuine fork-introduced regression +against INTENT #1. PR #77 made `Test` contextual the same way and explicitly named +`Release` as the deferred Phase 2; this delivers it. + +Scope is `Release` ONLY. `Object` (the other deferred token) is a *base Blitz3D* +keyword (`Object.Type(handle)`), so `object`-as-identifier would not have compiled +in stock Blitz3D either — a different, riskier problem class, deferred for separate +investigation. + +## Root cause / current behavior (verified) + +- `toker.cpp:81` reserves `Release` → `RELEASE` (unconditional). +- `parser.cpp:518` statement `case RELEASE` passes through to `parseExpr` (so the + keyword statement form `Release.Type x` is handled by the expression parser). +- `parser.cpp:1032` expression primary `case RELEASE` handles `Release[.]Type + operand` → `ReleaseNode` (semant casts operand to BBPointer, calls `__bbRelease`). +- `parser.cpp:70` `parseIdent` rejects everything but `IDENT`/`TEST`, so `release` + can't be a declared name. +- Repro: `samples/Blitz 2D Samples/spindisc.bb:82` `release=1` → + "Expecting identifier near: = Got: =". (Confirmed `release` is its only blocker.) + +The keyword form ALWAYS has an identifier immediately after `Release` (a type name, +with an optional `.`): `Release.Type operand` or `Release Type operand`. So: +**`Release` is the keyword iff the next token is `.` or IDENT; otherwise it is an +ordinary identifier** (`release=`, `release\f`, `release(`, `release[`, `If +release=1`, ...). This is the exact disambiguator, mirroring #77's `Test` lookahead. + +## Approach (specific, in order) — mirrors PR #77 + +1. `parser.cpp:70` (`parseIdent`): accept `RELEASE` as an identifier (add + `&& toker->curr()!=RELEASE`), so `release` works as a parameter / Local / Global / + Field name and type-instance var. Update the comment. +2. `parser.cpp:162-163` (statement dispatch): after the `TEST` disambiguator add + `if( stmtTok==RELEASE && !(toker->lookAhead(1)=='.' || toker->lookAhead(1)==IDENT) ) stmtTok=IDENT;` + — routing `release = ...` / `release(...)` / `release\f` statements through the + IDENT statement path (which reads `toker->text()` directly, so it handles the + `release` spelling without further change). `Release.Type x` keeps `stmtTok==RELEASE`. +3. `parser.cpp:1032` expression primary: make the keyword handling conditional and + fall through to the IDENT primary when it's an identifier use. Relocate the + `case RELEASE:` body to sit immediately above `case TEST: case IDENT:` (line 1135) + so the identifier path is a clean fall-through: + ```cpp + case RELEASE: + if( toker->lookAhead(1)=='.' || toker->lookAhead(1)==IDENT ){ + if( toker->next()=='.' ) toker->next(); + t=parseIdent(); + result=parseUniExpr( false ); + result=d_new ReleaseNode( result,t ); + break; + } + // else fall through: 'release' is an ordinary identifier + case TEST: + case IDENT: + ... + ``` +4. Remove the old standalone `case RELEASE:` at 1032. + +No tokenizer change; `Release.Type` grammar and `ReleaseNode` semantics untouched. + +## Files touched + +- `src/blitzrc/compiler/parser.cpp` — 3 sites (parseIdent, statement dispatch, + expression primary). +- `tests/LegacyKeywordIdentifierTest.bb` — `release` as identifier in every position + + a POSITIVE `Release.Type` keyword test proving the operator still works. +- `scripts/corpus_compile_allowlist.txt` — remove the `spindisc.bb` line. +- `docs/notes/legacy-release-keyword.md` — this note. + +## Migration / rollback + +Additive compatibility — every newly-accepted `release`-as-identifier form was a +hard compile error before, so no program can depend on the old behavior. Rollback = +revert the parser hunks + restore the allowlist line. + +## Trade-offs considered and rejected + +- **Include `Object` too** (the recon majority): rejected this iteration — `Object` + is a base-Blitz3D keyword, not a fork addition, so it's a different problem class + needing its own investigation. Keeping scope to the clear-cut fork regression. +- **Tokenizer-level soft keyword / general framework**: rejected — over-engineering; + the per-site `.`/IDENT lookahead is the proven #77 pattern. + +## Acceptance criteria + +- `printf 'release=1\nIf release=1 Then release=0\nEnd\n' | blitzcc -c +q` exits 0. +- `release` works as Local, assignment target, expression operand, field-typed var, + and function parameter (pinned by `tests/LegacyKeywordIdentifierTest.bb`). +- The `Release.Type obj` GC operator still parses AND works (positive test: + RefCount drops after release) — keyword not broken. +- `spindisc.bb` compiles under `blitzcc -c +q`; its allowlist line is removed and + `scripts/sweep_corpus.sh` stays green (one fewer known-fail, 0 new-fail). +- Full `blitzcc -t tests/*.bb` green. + +## Deferred follow-ups + +- `Object` contextual-identifier investigation (base-keyword problem class). +- `Recast`/`Reference` are also expression-position keywords with the same grammar + shape — no corpus collision, but the same pattern now applies if ever needed. +- Modern-features docs as a CI-gated executable contract + BBList docs (strong + next-iteration candidate; best anti-pile-on pick with compounding value). diff --git a/scripts/corpus_compile_allowlist.txt b/scripts/corpus_compile_allowlist.txt index 80b3f0f..5fdc292 100644 --- a/scripts/corpus_compile_allowlist.txt +++ b/scripts/corpus_compile_allowlist.txt @@ -20,10 +20,11 @@ # that classic Blitz3D allowed as identifiers. The HIGH-priority contextual-keyword # fix would re-enable these. (`object` is a legacy Blitz3D keyword, not a fork # addition, but the program still uses it as an identifier.) -# NOTE: the four `test`-collision files (fileio/global/mirror/deathisland) were -# removed once `Test` became a contextual keyword (PR: align/legacy-test-keyword-identifier); -# they now compile. `release` and `object` remain (see below). -samples/Blitz 2D Samples/spindisc.bb # 'release=1' — Release is still a hard keyword (Phase 2: expression-position contextual) +# NOTE: the `test`-collision files (fileio/global/mirror/deathisland) were removed +# once `Test` became a contextual keyword (PR: align/legacy-test-keyword-identifier), +# and `spindisc.bb` once `Release` became contextual (PR: align/legacy-release-keyword-identifier). +# `object` remains: it is a *base* Blitz3D keyword (Object.Type(handle)), not a fork +# addition, so it is a different problem class -- deferred for separate investigation. samples/Blitz 3D Samples/RobHutchinson/CraftFlare/CraftFlare.bb # 'object' identifier (legacy Blitz3D keyword, not a fork addition) # --- Class B: disabled / unregistered builtin -------------------------------- diff --git a/tests/LegacyKeywordIdentifierTest.bb b/tests/LegacyKeywordIdentifierTest.bb index 1e79b53..d1ceb4e 100644 --- a/tests/LegacyKeywordIdentifierTest.bb +++ b/tests/LegacyKeywordIdentifierTest.bb @@ -15,6 +15,25 @@ Function withTestParam( test ) Return test + 1 End Function +; --- 'Release' as a legacy identifier --- +; 'Release' is BlitzForge's GC release operator (`Release.Type `), but legacy +; Blitz3D programs use `release` as an ordinary variable/parameter name (e.g. the +; spindisc.bb `If release = 1` case). It must work as an identifier everywhere it +; is not the release operator, while `Release.Type ` still parses and works. + +Type ReleaseHolder + Field value +End Type + +Type ReleaseTarget + Field var +End Type + +; 'release' as a function parameter name, used as an expression operand in the body. +Function withReleaseParam( release ) + Return release + 1 +End Function + Test legacyTestAsLocalAndAssignment() ; Local declaration named `test`, statement-start assignment, and use as an ; expression operand -- all positions that previously tokenized as the TEST @@ -34,3 +53,43 @@ Test legacyTestFieldAccess() test\value = 7 Assert( test\value = 7 ) End Test + +Test legacyReleaseAsLocalAndAssignment() + ; Local declaration named `release`, statement-start assignment, and use as an + ; expression operand -- all positions that previously tokenized as the RELEASE + ; keyword and failed. + Local release = 41 + release = release + 1 + Assert( release = 42 ) +End Test + +Test legacyReleaseInIfCondition() + ; The spindisc.bb real-world case: `release` as an operand in an If condition. + Local release = 1 + Assert( release = 1 ) + If release = 1 + release = release + 1 + EndIf + Assert( release = 2 ) +End Test + +Test legacyReleaseAsParameter() + Assert( withReleaseParam( 41 ) = 42 ) +End Test + +Test legacyReleaseFieldAccess() + ; `release` as a type-instance variable plus field access on it. + Local release.ReleaseHolder = New ReleaseHolder() + release\value = 7 + Assert( release\value = 7 ) +End Test + +Test releaseOperatorStillWorks() + ; Positive: the GC operator `Release.Type ` must still parse and run. + ; Mirrors GarbageCollectionTest's testRef/testRel: New gives RefCount 1, and + ; after Release the object is gone so RefCount(First ReleaseTarget) = 0. + Local obj.ReleaseTarget = New ReleaseTarget() + Assert( RefCount(First ReleaseTarget) = 1 ) + Release.ReleaseTarget obj + Assert( RefCount(First ReleaseTarget) = 0 ) +End Test