diff --git a/TODO.md b/TODO.md index 4be714c..26e9ce2 100644 --- a/TODO.md +++ b/TODO.md @@ -2511,9 +2511,24 @@ reduced against `build/basic`, the stdio build, unless it says otherwise. `include/akbasic/value.h`. A test belongs in `tests/user_functions.c`: call a `DEF` ten thousand times and then `DIM` an array. -2. **A `BEGIN` block that is skipped leaks a scope for every loop inside it**, because `FOR` - and `DO` create their environment at *parse* time and the skip is decided at *evaluate* - time. +2. ~~**A `BEGIN` block that is skipped leaks a scope for every loop inside it**, because + `FOR` and `DO` create their environment at *parse* time and the skip is decided at + *evaluate* time.~~ **Done** — the skip now releases what parsing pushed + (`akbasic_runtime_interpret()`, `src/runtime.c`), taking the second of the two routes + below. `tests/structure_verbs.c` covers both loop forms inside a routine and the + forty-skip top-level case. + + **It is narrower than it looks, and the golden corpus is why.** The first attempt + released the scope on *any* skip, which broke + `tests/reference/language/flowcontrol/nestedforloopwaitingforcommand.bas`: a zero- + iteration `FOR` skips its body by the same mechanism, and there the orphan is + load-bearing -- it is what absorbs the inner `NEXT` so the outer one still finds its + `FOR`. Releasing it turns that case into "NEXT outside the context of FOR". So the + release is conditional on the skip being a *block* skip, which is testable because a + skipped block can never contain a live `NEXT` wait. Both halves are now asserted side by + side in `tests/structure_verbs.c`. + + The original report: ```basic N# = 0 diff --git a/docs/18-tutorial-breakout-artwork.md b/docs/18-tutorial-breakout-artwork.md index 7766a0d..e258e9a 100644 --- a/docs/18-tutorial-breakout-artwork.md +++ b/docs/18-tutorial-breakout-artwork.md @@ -343,9 +343,10 @@ The game sets `DPLAY# = 1` or `DHUD# = 1` when something changes and never draws directly. One consequence is visible and deliberate: **the score lags the bricks by one frame**, because the field goes first. At thirty frames a second nobody can see it. -Those are `LABEL`s and `GOTO`s rather than `BEGIN` blocks, and again that is not a style -choice — a `RETURN` inside a block leaves the interpreter with no `GOSUB` to return from. -The last trap in this chapter is exactly that. +Those are `LABEL`s and `GOTO`s rather than `BEGIN` blocks, and when this was written that +was not a style choice: a loop inside a block that was skipped left the interpreter with +no `GOSUB` to return from. That is fixed — see trap 3 below — and the shape is kept +because it costs nothing and reads the same. ## Step 6: Draw the lettering, because text has no colour @@ -672,7 +673,13 @@ NEXT I# RETURN ``` -### 3. A skipped `BEGIN` block containing a loop breaks the enclosing `RETURN` +### 3. A skipped `BEGIN` block containing a loop — *fixed* + +This one is here as history rather than as a warning. A loop inside a block that was not +taken used to leave a scope behind, and inside a routine the orphan sat between it and +its caller — so the `RETURN` after the block reported `RETURN outside the context of +GOSUB` from a routine that plainly *was* entered by a `GOSUB`. The error named the one +construct that was not at fault, which is why it cost an evening. ```basic T# = 0 @@ -689,18 +696,13 @@ RETURN ``` ```output -? 11 : RUNTIME ERROR RETURN outside the context of GOSUB - +CAME BACK ``` -The routine plainly *was* called by a `GOSUB`. `FOR` creates its environment when the -line is **parsed**, and the block skip is decided when it is **evaluated**, so a skipped -loop pushes a scope that its skipped `NEXT` never pops — and the orphan sits between the -routine and its caller. Outside a routine the same thing exhausts the pool of 32 after -thirty-two skips instead. - -Guard loops with `GOTO` rather than wrapping them in a block, which is what every draw -routine in this game does. +`FOR` creates its environment when the line is **parsed** and the skip is decided when it +is **evaluated**; the skip now releases what parsing pushed. **The listing still guards +its loops with `GOTO`**, which is where this chapter's own history shows: written that way +because it had to be, and left that way because it works. ### 4. `SSHAPE` and `GSHAPE` ignore the subscript on a string array diff --git a/src/runtime.c b/src/runtime.c index 8badaa9..bef1d63 100644 --- a/src/runtime.c +++ b/src/runtime.c @@ -816,6 +816,45 @@ akerr_ErrorContext *akbasic_runtime_interpret(akbasic_Runtime *obj, akbasic_ASTL if ( akbasic_environment_is_waiting_for_any(obj->environment) ) { if ( expr->leaftype != AKBASIC_LEAF_COMMAND || !akbasic_environment_is_waiting_for(obj->environment, expr->identifier) ) { + /* + * **A skipped loop has already pushed its scope, and this is where it + * comes back.** + * + * akbasic_parse_for() and akbasic_parse_do() create the environment + * while the line is *parsed*; whether to skip is decided here, after + * parsing. So a `FOR` inside a block that is not taken pushes a scope, + * its body is skipped, and the `NEXT` that would pop it is skipped + * too. Nothing else ever will. + * + * Left behind, thirty-two of them exhausted the pool -- and inside a + * routine it was much more confusing than that: the orphan sat between + * the routine and its caller, so the `RETURN` after the block reported + * "RETURN outside the context of GOSUB" from a routine that plainly + * *was* entered by a `GOSUB`. The error named the one construct that + * was not at fault. TODO.md section 9 item 2. + * + * `loopFirstLine` is what makes this safe: it is set by both parse + * handlers and by nothing else, so it identifies the scope this very + * line pushed rather than whatever happened to be on top. + * + * **Only a block skip**, which is what the `BEND` test is for. A + * zero-iteration `FOR` skips its body the same way, and there the + * orphan is load-bearing: `tests/reference/.../nestedforloopwaiting + * forcommand.bas` nests a loop inside one that runs zero times, and + * the inner scope is what absorbs the inner `NEXT` so the outer `NEXT` + * still finds its `FOR`. Popping it there turns that case into "NEXT + * outside the context of FOR". A `FOR` inside a skipped block cannot + * be in that position, because nothing in a skipped block ever runs to + * arm a `NEXT` wait in the first place. + */ + if ( expr->leaftype == AKBASIC_LEAF_COMMAND && + obj->environment->loopFirstLine != 0 && + obj->environment->waitingForCommand[0] == '\0' && + akbasic_environment_is_waiting_for(obj->environment, "BEND") && + (strcmp(expr->identifier, "FOR") == 0 || + strcmp(expr->identifier, "DO") == 0) ) { + PASS(errctx, akbasic_runtime_prev_environment(obj)); + } *dest = &obj->staticTrueValue; SUCCEED_RETURN(errctx); } diff --git a/tests/structure_verbs.c b/tests/structure_verbs.c index 1771e67..9508131 100644 --- a/tests/structure_verbs.c +++ b/tests/structure_verbs.c @@ -17,19 +17,31 @@ #include "harness.h" #include "testutil.h" -/** @brief Run a program to completion in RUN mode, from a string. */ -static akerr_ErrorContext AKERR_NOIGNORE *run_program(const char *source) +/** @brief Run a program to completion in RUN mode, under an explicit step budget. */ +static akerr_ErrorContext AKERR_NOIGNORE *run_program_bounded(const char *source, int64_t steps) { PREPARE_ERROR(errctx); PASS(errctx, harness_start(NULL)); PASS(errctx, akbasic_runtime_load(&HARNESS_RUNTIME, source)); PASS(errctx, akbasic_runtime_start(&HARNESS_RUNTIME, AKBASIC_MODE_RUN)); - /* - * Bounded. A defect in any of these verbs is a loop that never ends, and a - * suite that hangs tells you far less than one that fails. - */ - PASS(errctx, akbasic_runtime_run(&HARNESS_RUNTIME, 2000)); + PASS(errctx, akbasic_runtime_run(&HARNESS_RUNTIME, steps)); + SUCCEED_RETURN(errctx); +} + +/** + * @brief Run a program to completion in RUN mode, from a string. + * + * Bounded. A defect in any of these verbs is a loop that never ends, and a suite + * that hangs tells you far less than one that fails. 2000 steps is generous for + * every case here but the forty-skip one, which names its own: a *skipped* line + * is not free, and forty passes over a five-line block cost about 2700. + */ +static akerr_ErrorContext AKERR_NOIGNORE *run_program(const char *source) +{ + PREPARE_ERROR(errctx); + + PASS(errctx, run_program_bounded(source, 2000)); SUCCEED_RETURN(errctx); } @@ -273,6 +285,95 @@ static void test_loop_without_do(void) harness_stop(); } +/** + * @brief A block that is not taken gives back the scope its loop pushed. + * + * `FOR` and `DO` create their environment while the line is *parsed*; the skip + * is decided when it is *evaluated*. So a loop inside a block that is not taken + * used to push a scope whose `NEXT` or `LOOP` was skipped along with it, and + * nothing ever popped it. + * + * Inside a routine that was much worse than a leak. The orphan sat between the + * routine and its caller, so the `RETURN` after the block reported "RETURN + * outside the context of GOSUB" -- from a routine that plainly *was* entered by + * a `GOSUB`, naming the one construct that was not at fault. TODO.md section 9 + * item 2, found by writing a game against the interpreter. + */ +static void test_skipped_block_releases_its_loop_scope(void) +{ + /* Both loop forms, because both parse handlers push. */ + TEST_REQUIRE_OK(run_program("10 T# = 0\n" + "20 GOSUB DOIT\n" + "30 PRINT \"CAME BACK\"\n" + "40 END\n" + "50 LABEL DOIT\n" + "60 IF 1 = 0 THEN BEGIN\n" + "70 FOR I# = 0 TO 2\n" + "80 T# = T# + 1\n" + "90 NEXT I#\n" + "100 BEND\n" + "110 RETURN\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "CAME BACK\n"); + harness_stop(); + + TEST_REQUIRE_OK(run_program("10 T# = 0\n" + "20 GOSUB DOIT\n" + "30 PRINT \"CAME BACK\"\n" + "40 END\n" + "50 LABEL DOIT\n" + "60 IF 1 = 0 THEN BEGIN\n" + "70 DO\n" + "80 T# = T# + 1\n" + "90 LOOP UNTIL T# > 2\n" + "100 BEND\n" + "110 RETURN\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "CAME BACK\n"); + harness_stop(); + + /* + * At the top level it announced itself as pool exhaustion instead, after + * thirty-two skips -- AKBASIC_MAX_ENVIRONMENTS. Forty here, so a fix that + * merely raised the bound would not pass. + */ + TEST_REQUIRE_OK(run_program_bounded("10 N# = 0\n" + "20 LABEL AGAIN\n" + "30 N# = N# + 1\n" + "40 IF 1 = 0 THEN BEGIN\n" + "50 FOR I# = 0 TO 2\n" + "60 N# = N# + 1000\n" + "70 NEXT I#\n" + "80 BEND\n" + "90 IF N# < 40 THEN GOTO AGAIN\n" + "100 PRINT \"OK \" + N#\n", 6000)); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK 40\n"); + harness_stop(); +} + +/** + * @brief A zero-iteration loop still keeps the scope its inner loop pushed. + * + * The counter-case, and the reason the fix above tests for a `BEND` wait rather + * than for any wait at all. A `FOR` that runs zero times skips its body by the + * same mechanism, and *there* the orphan is load-bearing: it is what absorbs the + * inner `NEXT`, so the outer `NEXT` still finds its own `FOR`. Releasing it + * turns this into "NEXT outside the context of FOR". + * + * tests/reference/language/flowcontrol/nestedforloopwaitingforcommand.bas is the + * golden case that caught exactly that, and this is the same shape asserted + * where a reader of this file will see it. + */ +static void test_zero_iteration_loop_keeps_its_inner_scope(void) +{ + TEST_REQUIRE_OK(run_program("10 FOR I# = 1 TO 0\n" + "20 FOR J# = 2 TO 4\n" + "30 PRINT \"THE BODY MUST NOT RUN\"\n" + "40 NEXT J#\n" + "50 NEXT I#\n" + "60 PRINT \"SUCCESS\"\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "SUCCESS\n"); + harness_stop(); +} + int main(void) { test_do_while(); @@ -287,5 +388,7 @@ int main(void) test_end(); test_nested_do(); test_loop_without_do(); + test_skipped_block_releases_its_loop_scope(); + test_zero_iteration_loop_keeps_its_inner_scope(); return akbasic_test_failures; }