diff --git a/TODO.md b/TODO.md index 1c02694..cab611a 100644 --- a/TODO.md +++ b/TODO.md @@ -1385,6 +1385,40 @@ deviations from the reference's *program*: `main.go` and the SDL half of the last, as they always have. That is a separate decision with its own corpus risk and it is not this one. +64. **Branching by number to a line the program did not number is refused before it runs.** + The other half of deviation 63, and the reason `akbasic_SourceLine` carries a flag rather + than the loader just picking slots quietly. In a script written without line numbers + `GOTO 100` finds the hundredth line and branches there: plausible, silent and wrong, which + is the worst thing to hand somebody at run time. + + `akbasic_runtime_check_targets()` is a fourth prescan, run beside the label, `DATA` and + `TYPE` ones on every entry into `AKBASIC_MODE_RUN` — the earliest the check can be made and + the only place all four ways a program arrives pass through. It refuses with + `AKBASIC_ERR_SYNTAX`, naming the line and saying what to do instead: + + ```text + ? 1 : PARSE ERROR Line 1: branch to line 4, which the program did not number. + Branch by LABEL, or RENUMBER first + ``` + + Two things are deliberately **not** refused. A target naming an *empty* line, for the same + reason `RENUMBER` leaves one alone — `GOTO 9999` in a program with no line 9999 is already + broken and inventing a rule about it would hide that. And a target in a fully numbered + program, which is every program that existed before this, so nothing checked in changed. + + **It shares `RENUMBER`'s walk rather than repeating it.** `src/renumber.c` grew an + `akbasic_TargetWalk` — a `self` pointer and a `visit` function — and `rewrite_line()` now + takes one. `RENUMBER`'s visitor substitutes the number a line moved to; the check's + substitutes the number unchanged and raises if it names an assigned line. One walk, so the + two cannot disagree about what a branch target is, which they would have within a release. + + Worth knowing: the check points `environment->lineno` at the line it is walking, so the + `? N :` prefix names the offending line. **The other three prescans do not**, so a + malformed `TYPE` or a bad `DATA` item still reports whichever line the loader stopped on — + usually the last line of the program. They work around it by putting `Line %d:` in the + message text, which is why the message above says the number twice. Worth fixing in + `akbasic_runtime_set_mode()`'s wrapper rather than in four places; not fixed here. + --- --- diff --git a/include/akbasic/runtime.h b/include/akbasic/runtime.h index fadd39f..b4763f8 100644 --- a/include/akbasic/runtime.h +++ b/include/akbasic/runtime.h @@ -388,6 +388,29 @@ akerr_ErrorContext AKERR_NOIGNORE *akbasic_runtime_settime(akbasic_Runtime *obj, */ akerr_ErrorContext AKERR_NOIGNORE *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int64_t increment, int64_t oldstart); +/** + * @brief Refuse a program that branches by number to a line it did not number. + * + * The fourth prescan, run beside the label, `DATA` and `TYPE` ones on every entry + * into AKBASIC_MODE_RUN. Since a loaded line may be given its number rather than + * carry one, `GOTO 100` in a script written without numbers finds the hundredth + * line and branches there -- plausible, silent and wrong. This says so before any + * line executes, which is the earliest it can be said and the only place that + * covers all four ways a program arrives. + * + * It walks the same `GOTO`, `GOSUB`, `RUN`, `RESTORE`, `TRAP`, `ON ... GOTO` and + * `COLLISION` targets akbasic_renumber() rewrites, sharing that walk rather than + * repeating it. A target naming an *empty* line is allowed through, for the same + * reason RENUMBER leaves one alone. A `GOTO` to a label never reaches here. + * + * @param obj Object to initialize, inspect, or modify. + * @return `NULL` on success, otherwise an error context owned by the caller. + * @throws AKERR_NULLPOINTER When `obj` is NULL. + * @throws AKBASIC_ERR_SYNTAX When a numeric target names a line the program did not number. + * @throws AKBASIC_ERR_BOUNDS When a line does not fit the walk's buffer. + */ +akerr_ErrorContext AKERR_NOIGNORE *akbasic_runtime_check_targets(akbasic_Runtime *obj); + /** * @brief File every `LABEL` in the stored program before any of it runs. * diff --git a/src/renumber.c b/src/renumber.c index 1bc8c82..bb38f51 100644 --- a/src/renumber.c +++ b/src/renumber.c @@ -1,18 +1,26 @@ /** * @file renumber.c - * @brief RENUMBER: move every line, and rewrite every reference to one. + * @brief Walking a program's branch targets, and the two things that do it. * - * Moving the lines is easy -- source is filed under its line number in - * `source[]`, so it is a permutation. Rewriting every `GOTO`, `GOSUB`, `RUN`, - * `RESTORE`, `TRAP` and `COLLISION` target to match is the job, and it is why - * this was deferred rather than written early: a `RENUMBER` that moved lines - * without fixing their references would silently break every branch in the - * program, which is worse than not having the verb at all. + * **RENUMBER** moves every line and rewrites every reference to one. Moving the + * lines is easy -- source is filed under its line number in `source[]`, so it is + * a permutation. Rewriting every `GOTO`, `GOSUB`, `RUN`, `RESTORE`, `TRAP` and + * `COLLISION` target to match is the job, and it is why this was deferred rather + * than written early: a `RENUMBER` that moved lines without fixing their + * references would silently break every branch in the program, which is worse + * than not having the verb at all. * - * The rewrite is textual, and the only thing it has to understand about the rest - * of the language is where a string literal starts and stops -- `PRINT "GOTO 10"` + * **akbasic_runtime_check_targets()** walks the same targets without moving + * anything, refusing a program that branches by number to a line the program + * never numbered. It shares this file because it needs exactly the same walk, + * and a second copy of it would drift. + * + * The walk is textual, and the only thing it has to understand about the rest of + * the language is where a string literal starts and stops -- `PRINT "GOTO 10"` * must come through untouched. That is the same statement-walking the label and - * DATA prescans do. + * DATA prescans do. What the two callers differ in is one function pointer: + * given a target, say what text should stand in its place, and raise if it is not + * acceptable. */ #include @@ -43,6 +51,21 @@ static const char *BRANCH_VERBS[] = { "GOTO", "GOSUB", "RUN", "RESTORE", "TRAP" /** @brief How many entries #BRANCH_VERBS has. */ #define BRANCH_VERB_COUNT ((int)(sizeof(BRANCH_VERBS) / sizeof(BRANCH_VERBS[0]))) +/** + * @brief What a walk does with each numeric branch target it finds. + * + * The one thing RENUMBER and the target check differ in. `visit` is handed a + * target and writes into @p dest the text that should stand in its place -- + * the new number for RENUMBER, the same number for a check -- and may raise to + * refuse the line outright. + */ +typedef struct akbasic_TargetWalk akbasic_TargetWalk; +struct akbasic_TargetWalk +{ + void *self; + akerr_ErrorContext *(*visit)(akbasic_TargetWalk *walk, int64_t target, char *dest, size_t len); +}; + /** * @brief Where a line moved to, or the line itself when it did not move. * @@ -59,25 +82,26 @@ static int64_t mapped(const int64_t *map, int64_t line) } /** - * @brief Copy a run of comma-separated line numbers, rewriting each. + * @brief Copy a run of comma-separated line numbers, visiting each. * * `ON X GOTO 100, 200, 300` is why this takes a list rather than one number: the * targets follow the `GOTO`, so handling `GOTO` handles `ON` for free. * - * @param map Old line to new line. + * @param walk What to substitute for each target, and whether to allow it. * @param src Reads from here; advanced past what was consumed. * @param dest Writes to here; advanced past what was written. * @param limit One past the last byte @p dest may write. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKBASIC_ERR_BOUNDS When the rewritten line does not fit. */ -static akerr_ErrorContext *rewrite_targets(const int64_t *map, const char **src, char **dest, const char *limit) +static akerr_ErrorContext *rewrite_targets(akbasic_TargetWalk *walk, const char **src, char **dest, const char *limit) { PREPARE_ERROR(errctx); const char *p = *src; char *out = *dest; for ( ;; ) { + char replacement[32]; int64_t line = 0; int written = 0; @@ -101,7 +125,8 @@ static akerr_ErrorContext *rewrite_targets(const int64_t *map, const char **src, line = (line * 10) + (*p - '0'); p += 1; } - written = snprintf(out, (size_t)(limit - out), "%" PRId64, mapped(map, line)); + PASS(errctx, walk->visit(walk, line, replacement, sizeof(replacement))); + written = snprintf(out, (size_t)(limit - out), "%s", replacement); FAIL_ZERO_RETURN(errctx, (written > 0 && out + written < limit), AKBASIC_ERR_BOUNDS, "RENUMBER: a rewritten line does not fit"); out += written; @@ -136,14 +161,14 @@ static akerr_ErrorContext *rewrite_targets(const int64_t *map, const char **src, /** * @brief Rewrite every branch target in one line of source. * - * @param map Old line to new line. + * @param walk What to substitute for each target, and whether to allow it. * @param code The line to read. * @param dest Where the rewritten line goes. * @param len Capacity of @p dest. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKBASIC_ERR_BOUNDS When the rewritten line does not fit. */ -static akerr_ErrorContext *rewrite_line(const int64_t *map, const char *code, char *dest, size_t len) +static akerr_ErrorContext *rewrite_line(akbasic_TargetWalk *walk, const char *code, char *dest, size_t len) { PREPARE_ERROR(errctx); const char *p = code; @@ -210,7 +235,7 @@ static akerr_ErrorContext *rewrite_line(const int64_t *map, const char *code, ch memcpy(out, p, verblen); out += verblen; p += verblen; - PASS(errctx, rewrite_targets(map, &p, &out, limit)); + PASS(errctx, rewrite_targets(walk, &p, &out, limit)); statementstart = false; continue; } @@ -235,7 +260,7 @@ static akerr_ErrorContext *rewrite_line(const int64_t *map, const char *code, ch *out = ','; out += 1; p += 1; - PASS(errctx, rewrite_targets(map, &p, &out, limit)); + PASS(errctx, rewrite_targets(walk, &p, &out, limit)); } statementstart = false; continue; @@ -251,11 +276,24 @@ static akerr_ErrorContext *rewrite_line(const int64_t *map, const char *code, ch SUCCEED_RETURN(errctx); } +/* ----------------------------------------------------------- RENUMBER -- */ + +/** @brief RENUMBER's visitor: substitute the number the line moved to. */ +static akerr_ErrorContext *visit_renumber(akbasic_TargetWalk *walk, int64_t target, char *dest, size_t len) +{ + PREPARE_ERROR(errctx); + const int64_t *map = (const int64_t *)walk->self; + + snprintf(dest, len, "%" PRId64, mapped(map, target)); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int64_t increment, int64_t oldstart) { PREPARE_ERROR(errctx); static int64_t map[AKBASIC_MAX_SOURCE_LINES]; static akbasic_SourceLine rewritten[AKBASIC_MAX_SOURCE_LINES]; + akbasic_TargetWalk walk = { map, visit_renumber }; int64_t next = newstart; int64_t i = 0; @@ -305,7 +343,7 @@ akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int * `oldstart` can branch into the region that moved. */ target = mapped(map, i); - PASS(errctx, rewrite_line(map, obj->source[i].code, + PASS(errctx, rewrite_line(&walk, obj->source[i].code, rewritten[target].code, sizeof(rewritten[target].code))); rewritten[target].lineno = target; /* @@ -320,3 +358,88 @@ akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int memcpy(obj->source, rewritten, sizeof(obj->source)); SUCCEED_RETURN(errctx); } + +/* ------------------------------------------------- the target prescan -- */ + +/** @brief What visit_check() needs to name the line it refused. */ +typedef struct +{ + const akbasic_Runtime *runtime; + int64_t lineno; +} CheckState; + +/** + * @brief The prescan's visitor: allow the target, or refuse the program. + * + * A number that names a line the program never numbered is refused. In a script + * written without line numbers `GOTO 100` finds the hundredth line and branches + * there -- plausible, silent, and wrong, which is the worst kind of defect to be + * handed at run time. + * + * A target naming an *empty* slot is allowed through, for the same reason + * mapped() leaves one alone: `GOTO 9999` in a program with no line 9999 is + * already broken, and refusing it here would be inventing a rule about lines + * that do not exist. The substitution is the number itself, because nothing is + * being rewritten. + */ +static akerr_ErrorContext *visit_check(akbasic_TargetWalk *walk, int64_t target, char *dest, size_t len) +{ + PREPARE_ERROR(errctx); + CheckState *state = (CheckState *)walk->self; + + snprintf(dest, len, "%" PRId64, target); + if ( target >= 0 && target < AKBASIC_MAX_SOURCE_LINES ) { + FAIL_NONZERO_RETURN(errctx, + (state->runtime->source[target].code[0] != '\0' + && !state->runtime->source[target].numbered), + AKBASIC_ERR_SYNTAX, + "Line %" PRId64 ": branch to line %" PRId64 + ", which the program did not number. Branch by LABEL, or RENUMBER first", + state->lineno, target); + } + SUCCEED_RETURN(errctx); +} + +akerr_ErrorContext *akbasic_runtime_check_targets(akbasic_Runtime *obj) +{ + PREPARE_ERROR(errctx); + /* + * Static for the same reason akbasic_renumber()'s buffers are: a line's worth + * of rewrite does not belong on a default stack alongside the rest of a + * step(). Nothing is read back out of it -- the walk needs somewhere to put + * the text it would have written, and this is it. + */ + static char discard[AKBASIC_MAX_LINE_LENGTH * 2]; + CheckState state = { NULL, 0 }; + akbasic_TargetWalk walk = { &state, visit_check }; + int64_t entry = 0; + int64_t i = 0; + + FAIL_ZERO_RETURN(errctx, (obj != NULL), AKERR_NULLPOINTER, "NULL runtime in check_targets"); + state.runtime = obj; + entry = obj->environment->lineno; + + for ( i = 0; i < AKBASIC_MAX_SOURCE_LINES; i++ ) { + if ( obj->source[i].code[0] == '\0' ) { + continue; + } + state.lineno = i; + /* + * Point the runtime at the line being walked, so the `? N :` prefix + * akbasic_runtime_error() builds from environment->lineno names the + * offending line rather than whichever one the loader stopped on. The + * message repeats the number because the other three prescans do not do + * this and get it wrong -- see TODO.md. + * + * PASS inside the loop, never CATCH: CATCH expands to a break, which + * would leave this loop rather than an enclosing ATTEMPT. That also means + * the cursor is deliberately *not* restored on the failure path, which is + * the whole point of setting it. + */ + obj->environment->lineno = i; + PASS(errctx, rewrite_line(&walk, obj->source[i].code, discard, sizeof(discard))); + } + /* Nothing was refused, so leave the cursor as the caller had it. */ + obj->environment->lineno = entry; + SUCCEED_RETURN(errctx); +} diff --git a/src/runtime.c b/src/runtime.c index 8513e37..e03ff4e 100644 --- a/src/runtime.c +++ b/src/runtime.c @@ -380,7 +380,7 @@ akerr_ErrorContext *akbasic_runtime_set_mode(akbasic_Runtime *obj, int mode) */ if ( obj->mode == AKBASIC_MODE_RUN && obj->environment != NULL ) { /* - * All three prescans, and all three inside one ATTEMPT. + * All four prescans, and all four inside one ATTEMPT. * * **A prescan failure is the program's mistake, not the host's**, so it * has to leave here as a BASIC error line rather than as a raised @@ -417,6 +417,16 @@ akerr_ErrorContext *akbasic_runtime_set_mode(akbasic_Runtime *obj, int mode) * skipped the lines that declared it. */ CATCH(errctx, akbasic_structtype_scan(obj)); + /* + * Then the branch targets, last because it is the only one that reads + * nothing into the runtime -- it only refuses. A loaded line may be + * *given* its number rather than carry one, so `GOTO 100` in a script + * written without numbers would otherwise find the hundredth line and + * branch there, which is plausible, silent and wrong. Said here rather + * than at the branch, because before the program runs is earlier and + * every path into a run comes through this function. + */ + CATCH(errctx, akbasic_runtime_check_targets(obj)); } CLEANUP { } PROCESS(errctx) { } HANDLE_DEFAULT(errctx) { diff --git a/tests/language/statements/unnumbered.bas b/tests/language/statements/unnumbered.bas new file mode 100644 index 0000000..bf92982 --- /dev/null +++ b/tests/language/statements/unnumbered.bas @@ -0,0 +1,20 @@ +REM A whole program with no line numbers at all, read through RUNSTREAM. +REM Blank lines cost nothing, and every branch is by LABEL. + +PRINT "START" + +FOR I# = 1 TO 3 + PRINT I# * I# +NEXT I# + +GOSUB GREET +GOTO FINISH + +PRINT "NOT REACHED" + +LABEL GREET +PRINT "HELLO" +RETURN + +LABEL FINISH +PRINT "END" diff --git a/tests/language/statements/unnumbered.txt b/tests/language/statements/unnumbered.txt new file mode 100644 index 0000000..94800b6 --- /dev/null +++ b/tests/language/statements/unnumbered.txt @@ -0,0 +1,6 @@ +START +1 +4 +9 +HELLO +END diff --git a/tests/unnumbered.c b/tests/unnumbered.c index bee215f..13c7161 100644 --- a/tests/unnumbered.c +++ b/tests/unnumbered.c @@ -217,6 +217,105 @@ static void test_prompt_still_runs_direct_mode(void) harness_stop(); } +/** + * @brief Branching by number to a line nobody numbered is refused before the run. + * + * `GOTO 2` in a program with no line numbers finds the second line and branches + * there. The prescan says so instead, and says it before any line executes -- so + * the program produces the error and nothing else, not the error partway through + * its output. + */ +static void test_numeric_branch_to_an_assigned_line_is_refused(void) +{ + TEST_REQUIRE_OK(run_program("PRINT \"A\"\n" + "PRINT \"B\"\n" + "GOTO 2\n")); + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "did not number") != NULL, + "expected a refusal naming the unnumbered target, got \"%s\"", HARNESS_OUTPUT); + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "A\n") == NULL, + "the program should not have run at all, got \"%s\"", HARNESS_OUTPUT); + harness_stop(); + + /* The refusal names the offending line, not wherever the loader stopped. */ + TEST_REQUIRE_OK(run_program("GOTO 4\n" + "PRINT \"B\"\n" + "PRINT \"C\"\n" + "PRINT \"D\"\n" + "PRINT \"E\"\n")); + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "? 1 :") != NULL, + "expected the error to name line 1, got \"%s\"", HARNESS_OUTPUT); + harness_stop(); +} + +/** @brief A written number is still a legal target, in a mixed program too. */ +static void test_numeric_branch_to_a_written_line_is_allowed(void) +{ + TEST_REQUIRE_OK(run_program("10 PRINT \"A\"\n" + "20 IF A# = 0 THEN A# = 1 : GOTO 10\n" + "30 PRINT \"OK\"\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "A\nA\nOK\n"); + harness_stop(); + + /* Mixed: line 10 was written, so a branch to it stands. */ + TEST_REQUIRE_OK(run_program("10 PRINT \"A\"\n" + "PRINT \"B\"\n" + "20 IF A# = 0 THEN A# = 1 : GOTO 10\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "A\nB\nA\nB\n"); + harness_stop(); +} + +/** @brief A branch to an empty line is left alone, as RENUMBER leaves it alone. */ +static void test_numeric_branch_to_an_empty_line_is_allowed(void) +{ + TEST_REQUIRE_OK(run_program("PRINT \"A\"\n" + "GOTO 500\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "A\n"); + harness_stop(); +} + +/** @brief A label target never reaches the check, so it cannot be refused by it. */ +static void test_label_targets_are_never_refused(void) +{ + TEST_REQUIRE_OK(run_program("GOTO DONE\n" + "PRINT \"SKIPPED\"\n" + "LABEL DONE\n" + "PRINT \"OK\"\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK\n"); + harness_stop(); +} + +/** @brief A number inside a string literal is not a branch target. */ +static void test_a_number_in_a_string_is_not_a_target(void) +{ + TEST_REQUIRE_OK(run_program("PRINT \"A\"\n" + "PRINT \"GOTO 2\"\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "A\nGOTO 2\n"); + harness_stop(); +} + +/** + * @brief RENUMBER makes a numberless program branchable by number again. + * + * It moves line 3 to line 30 *and* rewrites `GOTO 3` to `GOTO 30`, and every line + * it touches comes out numbered -- so the branch the check refused a moment ago + * is now naming a number the program owns. That is the remedy the refusal names. + */ +static void test_renumber_makes_numeric_branches_legal(void) +{ + TEST_REQUIRE_OK(harness_start(NULL)); + TEST_REQUIRE_OK(akbasic_runtime_load(&HARNESS_RUNTIME, + "PRINT \"A\"\n" + "GOTO 3\n" + "PRINT \"B\"\n")); + /* Before: line 3 was assigned a number, so branching to it is refused. */ + TEST_REQUIRE_STATUS(akbasic_runtime_check_targets(&HARNESS_RUNTIME), AKBASIC_ERR_SYNTAX); + + TEST_REQUIRE_OK(akbasic_renumber(&HARNESS_RUNTIME, 10, 10, 0)); + TEST_REQUIRE_STR(HARNESS_RUNTIME.source[20].code, "GOTO 30"); + TEST_REQUIRE_OK(akbasic_runtime_check_targets(&HARNESS_RUNTIME)); + harness_stop(); +} + /** @brief RENUMBER numbers everything, which is what asking for it means. */ static void test_renumber_marks_every_line_numbered(void) { @@ -246,6 +345,12 @@ int main(void) test_runstream_assigns_the_same_slots(); test_trailing_blank_line_keeps_the_last_line(); test_prompt_still_runs_direct_mode(); + test_numeric_branch_to_an_assigned_line_is_refused(); + test_numeric_branch_to_a_written_line_is_allowed(); + test_numeric_branch_to_an_empty_line_is_allowed(); + test_label_targets_are_never_refused(); + test_a_number_in_a_string_is_not_a_target(); + test_renumber_makes_numeric_branches_legal(); test_renumber_marks_every_line_numbered(); return akbasic_test_failures; }