diff --git a/TODO.md b/TODO.md index 59eb1b7..71fc847 100644 --- a/TODO.md +++ b/TODO.md @@ -1527,6 +1527,42 @@ the reference's semantics reproduced faithfully; the third is the port's own. listings read it there. Same known-failing test as item 19; the fix is a scoping decision about where a loop counter is created, not an ordering one. +### Found while auditing `akbasic_value_clone()` for the structures work + +22. ~~**The numeric operators' `else` was a catch-all, not a float branch.**~~ **Fixed.** + `math_minus`, `math_multiply` and `math_divide` were `if ( INTEGER ) … else `, and that `else` read `floatval` from whatever it was handed. A truth value + keeps its payload in `boolvalue` and leaves `floatval` zero, so a truth value on the + *left* computed into a field nothing reads and kept its `BOOLEAN` type: + + ``` + (A# == 1) - 1 printed true should be -2 + (A# == 1) * 3 printed true should be -3 + (A# == 1) + 1 correctly refused + ``` + + Wrong in value and in type, and silent. `math_plus` escaped only because it enumerates + its cases and ends in an error; the other three now share one `require_numeric()` guard + so a type added later is refused by all of them at once instead of quietly taking the + float branch in each. + + **The two operands have different rules, deliberately.** The left one picks the branch + and must be a number. The right one is read through `rval_as_int()`, which handles a + truth value on purpose — `5 - (A# == 1)` is 6, and that is the same property that lets + `AND` and `OR` double as logical operators. Refusing it on the right would have broken + every condition in the language, and `tests/language/numeric/truth_value_arithmetic.bas` + pins both halves. + + **Found by asking what a structure operand would do**, since `AKBASIC_TYPE_STRUCT` would + have taken exactly this path and read a `floatval` it does not carry. The structures + work did not create this defect; it made an already-reachable one worth chasing. Same + shape as item 5 and fixed the same way, with the assertions against the value API where + the bad value can actually be constructed. + + Two stale comments went with it: `src/value.c`'s file header and a duplicate block above + `rval_as_int()` both cited "TODO.md section 12", which does not exist — the defect list + is §6 — and the duplicate described the summing behaviour item 5 had already removed. + ### Found while writing the error-code appendix 21. **A dependency's status code reaches `ER#` where the interpreter has one of its own.** diff --git a/src/value.c b/src/value.c index 1c09af5..e111fa4 100644 --- a/src/value.c +++ b/src/value.c @@ -2,11 +2,10 @@ * @file value.c * @brief Implements the BASIC value type and its operators. * - * A faithful port of basicvalue.go. Where the reference does something - * arithmetically odd -- adding both of the right operand's numeric fields, for - * instance -- this reproduces it, because the golden corpus encodes the observed - * behaviour and a "fix" here is a silent behaviour change. Each one is catalogued - * in TODO.md section 12. + * Ported from basicvalue.go. It used to reproduce the reference's arithmetic + * oddities deliberately, because the golden corpus encoded the observed + * behaviour; TODO.md section 0.1 retired that constraint and section 6 records + * which ones have since been fixed and why. */ #include @@ -18,13 +17,6 @@ #include #include -/* - * The reference writes `rval.intval + int64(rval.floatval)` on every integer - * operation and the mirror image on every float one. It works only because the - * unused field of a typed value is always zero. Named so the intent -- "whatever - * numeric payload the right operand is carrying" -- is legible, and so there is - * one place to change when TODO.md section 12 item 5 is fixed. - */ /* * Selected on the type, not summed. The reference adds both numeric fields -- * `rval.intval + int64(rval.floatval)` -- which happens to give the right answer @@ -55,6 +47,72 @@ static double rval_as_float(akbasic_Value *rval) return (double)rval->intval; } +/** + * @brief What a value's type is called in a diagnostic. + * + * Indexed by akbasic_Type, so a new type added to the enum without a row here + * trips the negative-array-size assert below rather than printing an empty + * string into an error message nobody can act on. + */ +static const char *TYPE_NAMES[] = { + "an undefined value", /* AKBASIC_TYPE_UNDEFINED */ + "an integer", /* AKBASIC_TYPE_INTEGER */ + "a float", /* AKBASIC_TYPE_FLOAT */ + "a string", /* AKBASIC_TYPE_STRING */ + "a truth value" /* AKBASIC_TYPE_BOOLEAN */ +}; + +typedef char akbasic_assert_type_names_complete + [(sizeof(TYPE_NAMES) / sizeof(TYPE_NAMES[0]) == AKBASIC_TYPE_BOOLEAN + 1) ? 1 : -1]; + +static const char *type_name(akbasic_Type valuetype) +{ + if ( valuetype < 0 || valuetype > AKBASIC_TYPE_BOOLEAN ) { + return "a value of unknown type"; + } + return TYPE_NAMES[valuetype]; +} + +/** + * @brief Refuse an operand the numeric path cannot read. + * + * `math_minus`, `math_multiply` and `math_divide` are shaped + * `if ( INTEGER ) ... else `, and **that else is a catch-all + * rather than a float branch**: it reads `floatval` from whatever it is handed. + * A truth value carries its payload in `boolvalue` and leaves `floatval` zero, + * so `(A# == 1) - 1` computed `0 - 1` into a field nothing reads, left the type + * as BOOLEAN, and printed `true` instead of -2. Multiplication and division did + * the same; only `math_plus` escaped, because it enumerates its cases and ends + * in an error. + * + * Checked here rather than in each operator so there is one rule and one + * message, and so a type added later is refused by all three at once instead of + * silently taking the float branch in each. + * + * **The two operands have different rules, and deliberately.** The left one + * decides the branch, so it must be a number. The right one is read through + * rval_as_int()/rval_as_float(), which handle a truth value on purpose -- a + * comparison yields -1 or 0 and `5 - (A# == 1)` is 6, which is the same property + * that lets `AND` and `OR` double as logical operators. Refusing a truth value + * on the right would break that, so this permits exactly what those two readers + * can actually read and nothing else. + */ +static akerr_ErrorContext *require_numeric(akbasic_Value *self, akbasic_Value *rval, const char *what) +{ + PREPARE_ERROR(errctx); + + FAIL_ZERO_RETURN(errctx, + (self->valuetype == AKBASIC_TYPE_INTEGER || + self->valuetype == AKBASIC_TYPE_FLOAT), + AKBASIC_ERR_TYPE, "Cannot perform %s on %s", what, type_name(self->valuetype)); + FAIL_ZERO_RETURN(errctx, + (rval->valuetype == AKBASIC_TYPE_INTEGER || + rval->valuetype == AKBASIC_TYPE_FLOAT || + rval->valuetype == AKBASIC_TYPE_BOOLEAN), + AKBASIC_ERR_TYPE, "Cannot perform %s on %s", what, type_name(rval->valuetype)); + SUCCEED_RETURN(errctx); +} + /* Copy a string into a value's inline buffer. Truncation is an error. */ static akerr_ErrorContext *set_string(akbasic_Value *dest, const char *src) { @@ -448,6 +506,7 @@ akerr_ErrorContext *akbasic_value_math_minus(akbasic_Value *self, akbasic_Value FAIL_NONZERO_RETURN(errctx, (self->valuetype == AKBASIC_TYPE_STRING || rval->valuetype == AKBASIC_TYPE_STRING), AKBASIC_ERR_TYPE, "Cannot perform subtraction on strings"); + PASS(errctx, require_numeric(self, rval, "subtraction")); if ( self->valuetype == AKBASIC_TYPE_INTEGER ) { (*dest)->intval = self->intval - rval_as_int(rval); } else { @@ -464,6 +523,7 @@ akerr_ErrorContext *akbasic_value_math_divide(akbasic_Value *self, akbasic_Value FAIL_NONZERO_RETURN(errctx, (self->valuetype == AKBASIC_TYPE_STRING || rval->valuetype == AKBASIC_TYPE_STRING), AKBASIC_ERR_TYPE, "Cannot perform division on strings"); + PASS(errctx, require_numeric(self, rval, "division")); if ( self->valuetype == AKBASIC_TYPE_INTEGER ) { /* * Integer division by zero is UB in C where Go panics. Neither is @@ -510,12 +570,16 @@ akerr_ErrorContext *akbasic_value_math_multiply(akbasic_Value *self, akbasic_Val } buf[offset] = '\0'; PASS(errctx, set_string(*dest, buf)); + SUCCEED_RETURN(errctx); } /* - * Not an `else if`. The reference falls through to the numeric branches even - * for a string, where self->floatval is 0 and the write is a harmless no-op. - * Kept so the port is provably faithful. + * The reference falls through to the numeric branches even for a string, + * where self->floatval is 0 and the write lands in a field nothing reads. + * That was reproduced deliberately and is now a return, because the guard + * below would otherwise refuse the string repeat this branch just did -- + * and the fallthrough was only ever provably harmless, never useful. */ + PASS(errctx, require_numeric(self, rval, "multiplication")); if ( self->valuetype == AKBASIC_TYPE_INTEGER ) { (*dest)->intval = self->intval * rval_as_int(rval); } else { diff --git a/tests/language/numeric/truth_value_arithmetic.bas b/tests/language/numeric/truth_value_arithmetic.bas new file mode 100644 index 0000000..eb6d5f9 --- /dev/null +++ b/tests/language/numeric/truth_value_arithmetic.bas @@ -0,0 +1,16 @@ +10 REM A truth value carries its payload in boolvalue, not floatval. The three +20 REM numeric operators were `if ( INTEGER ) ... else `, and +30 REM that else was a catch-all rather than a float branch -- so a truth value +40 REM on the LEFT computed 0 - 1 into a field nothing reads, kept its BOOLEAN +50 REM type, and printed `true` instead of -2. Silent, and wrong twice over. +60 A# = 1 +70 PRINT (A# == 1) +80 REM On the RIGHT it stays legal and stays -1, which is the same property that +90 REM lets AND and OR double as logical operators. Refusing it here would have +100 REM broken every condition in the language. +110 PRINT 5 - (A# == 1) +120 PRINT 5 * (A# == 1) +130 PRINT 5 + (A# == 1) +140 PRINT 4 - (A# == 2) +150 REM And on the left it is refused by name rather than computed. +160 PRINT (A# == 1) - 1 diff --git a/tests/language/numeric/truth_value_arithmetic.txt b/tests/language/numeric/truth_value_arithmetic.txt new file mode 100644 index 0000000..04d8924 --- /dev/null +++ b/tests/language/numeric/truth_value_arithmetic.txt @@ -0,0 +1,7 @@ +true +6 +-5 +4 +4 +? 160 : RUNTIME ERROR Cannot perform subtraction on a truth value + diff --git a/tests/value_arithmetic.c b/tests/value_arithmetic.c index 3883a5b..10603b2 100644 --- a/tests/value_arithmetic.c +++ b/tests/value_arithmetic.c @@ -249,6 +249,52 @@ int main(void) TEST_REQUIRE_STATUS(akbasic_value_math_divide(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); TEST_REQUIRE_STATUS(akbasic_value_invert(&A, &SCRATCH, &out), AKBASIC_ERR_TYPE); + /* + * A left operand the numeric path cannot read is refused, not computed. + * + * `math_minus`, `math_multiply` and `math_divide` were `if ( INTEGER ) ... + * else `, and that else was a catch-all: it read `floatval` + * from whatever it was handed. A truth value keeps its payload in + * `boolvalue` and leaves `floatval` zero, so the operation computed into a + * field nothing reads and left the type alone -- `(A# == 1) - 1` rendered + * `true` rather than -2. Silent, and wrong in both value and type. + * + * `math_plus` never had it, because it enumerates its cases and ends in an + * error. These assert the other three now agree with it. + */ + TEST_REQUIRE_OK(akbasic_value_set_bool(&A, true)); + set_int(&B, 1); + TEST_REQUIRE_STATUS(akbasic_value_math_minus(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + TEST_REQUIRE_STATUS(akbasic_value_math_multiply(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + TEST_REQUIRE_STATUS(akbasic_value_math_divide(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + TEST_REQUIRE_STATUS(akbasic_value_math_plus(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + + /* An undefined value is refused for the same reason and by the same guard. */ + TEST_REQUIRE_OK(akbasic_value_zero(&A)); + TEST_REQUIRE_STATUS(akbasic_value_math_minus(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + TEST_REQUIRE_STATUS(akbasic_value_math_multiply(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + TEST_REQUIRE_STATUS(akbasic_value_math_divide(&A, &B, &SCRATCH, &out), AKBASIC_ERR_TYPE); + + /* + * But a truth value on the *right* stays legal, and that asymmetry is the + * point: the left operand picks the branch and must be a number, while the + * right is read through rval_as_int(), which handles -1/0 on purpose. It is + * the same property that lets AND and OR double as logical operators, so + * refusing it here would break `5 - (A# == 1)`, which is 6. + */ + set_int(&A, 5); + TEST_REQUIRE_OK(akbasic_value_set_bool(&B, true)); + TEST_REQUIRE_OK(akbasic_value_math_minus(&A, &B, &SCRATCH, &out)); + TEST_REQUIRE_INT(out->intval, 6); + TEST_REQUIRE_OK(akbasic_value_math_multiply(&A, &B, &SCRATCH, &out)); + TEST_REQUIRE_INT(out->intval, -5); + + /* And a string repeat is not caught by the numeric guard in front of it. */ + set_string(&A, "xy"); + set_int(&B, 2); + TEST_REQUIRE_OK(akbasic_value_math_multiply(&A, &B, &SCRATCH, &out)); + TEST_REQUIRE_STR(out->stringval, "xyxy"); + /* Division by zero raises rather than trapping or panicking. */ set_int(&A, 1); set_int(&B, 0);