From e3e7c61459ca36acd7537319bb1dcd5a8732e3d5 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Fri, 21 Aug 2026 15:25:31 -0700 Subject: [PATCH] human review: units/float_interp --- .kiro/specs/calculator/design.md | 18 +++- .kiro/specs/calculator/tasks.md | 43 ++++++++ engine/src/engine.zig | 1 + engine/src/units.zig | 179 +++++++++++++++++++++++++------ 4 files changed, 204 insertions(+), 37 deletions(-) diff --git a/.kiro/specs/calculator/design.md b/.kiro/specs/calculator/design.md index 6e60713..afa7f9d 100644 --- a/.kiro/specs/calculator/design.md +++ b/.kiro/specs/calculator/design.md @@ -907,9 +907,23 @@ table by canonical name and alias: 1. An **exact, case-sensitive** match is tried first. This is what keeps case-distinguished units meaningful: `K` is Kelvin (not a "kilo" something), - `B` is byte, and `kB` (1000) never silently becomes `KiB` (1024). + `B` is byte and `b` is bit, `Mb` is megabit and `MB` is megabyte, and `kB` + (1000) never silently becomes `KiB` (1024). 2. Only if nothing matches exactly does a **case-insensitive** pass run, which - is what makes forgiving input like `KM` or `Celsius` work. + is what makes forgiving input like `KM` or `Celsius` work. That pass must find + **exactly one** unit. Where a fold reaches two units that the tables + deliberately distinguish by case, the name is `AmbiguousUnit` rather than a + guess: `Mbps` is megabits per second and `MBps` is megabytes per second, so + `MBPS` is a factor of eight away from one of the two readings with no way to + tell which was meant. Returning the first fold match is how `MBPS` silently + became `Mbps`, and how `mB` became `MB`. + +Bits sit alongside bytes in `digital_storage` (`b`, `kb`, `Mb`, `Gb`, `Tb`, `Pb`), +because the category's whole convention is that lowercase `b` is a bit and +uppercase `B` a byte. Before they existed, `Mb` matched nothing exactly, fell +through to the fold, and `100 Mb to MB` answered `100 MB`: a megabit-to-megabyte +question returning an identity. A bit is an eighth of a byte, so every factor in +the category is still exact. Conversion goes through the category base unit: `to.fromBase(from.toBase(value))`. Both units must be in the same category, or the result is `IncompatibleUnits`. diff --git a/.kiro/specs/calculator/tasks.md b/.kiro/specs/calculator/tasks.md index 3850567..a7a4473 100644 --- a/.kiro/specs/calculator/tasks.md +++ b/.kiro/specs/calculator/tasks.md @@ -914,8 +914,51 @@ plans a `--raw` flag, but today it is exercised only by tests. 100% line coverage, engine 99.44%. CLI output byte-identical across all five rows, both byte orders, ASCII packing and the multi-base standard-mode view. +### Task 5.22: Bits are not bytes, and a fold that could mean two units says so + +Two findings from the `units.zig` review, one a silent wrong answer. + +**The prefixed bit units did not exist.** `digital_storage` had `bit` and the byte +ladder, but no `kb`/`Mb`/`Gb`, so `Mb` matched nothing exactly, fell through to the +case-insensitive pass, and resolved to `MB`: + +``` +100 Mb to MB -> 100 MB = 100 MB now: 100 Mb = 12.5 MB +1 kb to B -> 1 kB = 1,000 B now: 1 kb = 125 B +``` + +A megabit-to-megabyte question came back as an identity, and the echoed input said +`MB` where the user had written `Mb`. `findUnit`'s doc comment acknowledged the class +of problem and told the reader that "a user who means megabits has to write `Mbit`", +which was not true: no such unit existed. `b`, `kb`, `Mb`, `Gb`, `Tb` and `Pb` are now +in the table with their long forms, and since a bit is an eighth of a byte every +factor stays exact. The binary bit forms (`Kib`, `Mib`) are still absent, which is +deliberate: nothing asked for them and they are rarely written. + +**A fold onto two units is an error, not a guess.** The case-insensitive pass +returned the first match and stopped, so a spelling that folded onto two units the +tables distinguish by case got whichever came first in the file. In `data_rate` that +is an eightfold error: + +``` +100 MBPS to Mbps -> 100 Mbps = 100 Mbps now: error: ambiguous unit name +100 mB to MB -> 100 MB = 100 MB now: error: ambiguous unit name +``` + +`resolve` now returns `unit`, `unknown` or `ambiguous`, and the pass has to find +exactly one unit; two aliases of one unit are not a conflict, two units are. The new +`AmbiguousUnit` error carries "ambiguous unit name: case tells these units apart". +`findUnit` keeps its `?UnitDef` signature for the internal scans and returns null for +both failures, while `convert` and `parseRequest` report which. `splitTrailingUnit` +returns the error rather than skipping the candidate, because skipping would go +looking for a shorter suffix and answer a different question with no error at all. + +The forgiving input the fold exists for is untouched: `100 KM to mi`, +`32 F to celsius`, `1 KIB to B`, `5 in in cm` and `100 mm in in` all still resolve. + ### Task 5.21: One table of built-in functions + `evalFunction` dispatched through a chain of `if (mem.eql(u8, name, ...))` blocks grouped by argument count, spilling into `evalSingleArgFn` and `evalFinancialFn`, both of which returned `Error!?f64` where null meant "not my name". Four consequences: diff --git a/engine/src/engine.zig b/engine/src/engine.zig index b824fe3..88bd0f5 100644 --- a/engine/src/engine.zig +++ b/engine/src/engine.zig @@ -100,6 +100,7 @@ pub fn phrase(err: Error) []const u8 { // Units error.UnknownUnit => "unknown unit", + error.AmbiguousUnit => "ambiguous unit name: case tells these units apart", error.IncompatibleUnits => "incompatible units (different categories)", // Financial diff --git a/engine/src/units.zig b/engine/src/units.zig index c4f66db..310c88f 100644 --- a/engine/src/units.zig +++ b/engine/src/units.zig @@ -28,6 +28,9 @@ const std = @import("std"); /// the evaluator had. pub const Error = error{ UnknownUnit, + /// A name that matches no unit exactly and folds onto more than one. Honouring + /// it would be a guess between units the tables distinguish by case. + AmbiguousUnit, IncompatibleUnits, OutOfMemory, } || number_mod.Error; @@ -262,9 +265,21 @@ const time_units = define(.time, &.{ .{ .name = "yr", .aliases = &.{ "year", "years" }, .factor = "31557600" }, }); -// Base: byte. Decimal (kB) and binary (KiB) prefixes are both provided. +// Base: byte. Decimal (kB) and binary (KiB) prefixes are both provided, and bits +// alongside bytes: lowercase `b` is a bit and uppercase `B` a byte, which is the +// convention the whole category depends on. Before the prefixed bit units existed, +// `Mb` matched no unit exactly and fell through to the case-insensitive pass, so +// `100 Mb to MB` answered "100 MB" and a megabit-to-megabyte question came back as +// an identity. +// +// A bit is an eighth of a byte, so every factor here is exact. const digital_storage_units = define(.digital_storage, &.{ - .{ .name = "bit", .aliases = &.{"bits"}, .factor = "0.125" }, + .{ .name = "b", .aliases = &.{ "bit", "bits" }, .factor = "0.125" }, + .{ .name = "kb", .aliases = &.{ "kilobit", "kilobits" }, .factor = "125" }, + .{ .name = "Mb", .aliases = &.{ "megabit", "megabits" }, .factor = "1.25e5" }, + .{ .name = "Gb", .aliases = &.{ "gigabit", "gigabits" }, .factor = "1.25e8" }, + .{ .name = "Tb", .aliases = &.{ "terabit", "terabits" }, .factor = "1.25e11" }, + .{ .name = "Pb", .aliases = &.{ "petabit", "petabits" }, .factor = "1.25e14" }, .{ .name = "B", .aliases = &.{ "byte", "bytes" }, .factor = "1" }, .{ .name = "kB", .aliases = &.{ "kilobyte", "kilobytes" }, .factor = "1e3" }, .{ .name = "MB", .aliases = &.{ "megabyte", "megabytes" }, .factor = "1e6" }, @@ -418,32 +433,72 @@ fn matchesIgnoreCase(unit: UnitDef, name: []const u8) bool { return false; } +/// What a name resolved to. +const Resolution = union(enum) { + unit: UnitDef, + /// No unit has this name or alias, in any case. + unknown, + /// The name matches nothing exactly and folds onto more than one unit. Picking + /// whichever comes first in the tables answers a different question: `Mbps` is + /// megabits per second and `MBps` is megabytes per second, so `MBPS` is a factor + /// of eight away from one of the two readings and there is no way to tell which + /// was meant. + ambiguous, +}; + +/// Resolve a name to at most one unit. +/// +/// An exact (case-sensitive) match is tried first so that case-distinguished units +/// keep their meaning: "K" stays Kelvin, "B" stays byte, "Mb" stays megabit, and "kB" +/// does not become "KiB". +/// +/// Only if nothing matches exactly does a case-insensitive pass run, which is what +/// makes forgiving input like "KM" or "Celsius" work. That pass has to find exactly +/// one unit. It used to return the first fold match and stop, which is how "MBPS" +/// silently became "Mbps". +fn resolve(name: []const u8) Resolution { + if (name.len == 0) return .unknown; + + for (categories) |table| { + for (table) |unit| { + if (matchesExact(unit, name)) return .{ .unit = unit }; + } + } + + var found: ?UnitDef = null; + for (categories) |table| { + for (table) |unit| { + if (!matchesIgnoreCase(unit, name)) continue; + if (found) |first| { + // Two aliases of one unit are not a conflict; two units are. + if (!std.mem.eql(u8, first.name, unit.name)) return .ambiguous; + } else { + found = unit; + } + } + } + if (found) |unit| return .{ .unit = unit }; + return .unknown; +} + /// Look up a unit by canonical name or alias. /// -/// An exact (case-sensitive) match is tried first so that case-distinguished -/// units keep their meaning: "K" stays Kelvin, "B" stays byte, and "kB" does not -/// become "KiB". Only if nothing matches exactly does a case-insensitive pass run, -/// which is what makes forgiving input like "KM" or "Celsius" work. -/// -/// That fallback is deliberately permissive, so a spelling with no exact entry -/// resolves to whatever differs only in case: "mB" and "Mb" both reach "MB". A -/// user who means megabits has to write "Mbit" or "Mb" ... which is the same -/// string, so they have to write "Mbit". Tightening this would mean rejecting -/// case-variant input outright, which is a bigger behavioural decision than a -/// comment can carry. +/// Null covers both "no such unit" and "several, differing only in case". Callers +/// that report to a user go through `resolveOrError` instead, so the two come out as +/// different messages. pub fn findUnit(name: []const u8) ?UnitDef { - if (name.len == 0) return null; - for (categories) |table| { - for (table) |unit| { - if (matchesExact(unit, name)) return unit; - } - } - for (categories) |table| { - for (table) |unit| { - if (matchesIgnoreCase(unit, name)) return unit; - } - } - return null; + return switch (resolve(name)) { + .unit => |unit| unit, + .unknown, .ambiguous => null, + }; +} + +fn resolveOrError(name: []const u8) Error!UnitDef { + return switch (resolve(name)) { + .unit => |unit| unit, + .unknown => Error.UnknownUnit, + .ambiguous => Error.AmbiguousUnit, + }; } // -- Conversion -- @@ -460,8 +515,8 @@ pub fn convertUnits(value: f64, from: UnitDef, to: UnitDef) Error!f64 { /// Returns UnknownUnit if either name is unrecognized, or IncompatibleUnits if /// the units belong to different categories. pub fn convert(value: f64, from_name: []const u8, to_name: []const u8) Error!ConvertResult { - const from = findUnit(from_name) orelse return Error.UnknownUnit; - const to = findUnit(to_name) orelse return Error.UnknownUnit; + const from = try resolveOrError(from_name); + const to = try resolveOrError(to_name); const result = try convertUnits(value, from, to); // A single scaling factor only describes the relationship when neither @@ -548,7 +603,7 @@ fn collectSeparators(text: []const u8, out: *[max_separators]Span) usize { /// A candidate must begin at a token boundary: the preceding character may not /// be a letter. Without that rule the scan finds units buried inside words, e.g. /// taking the trailing "s" of "100 smoots" as seconds. -fn splitTrailingUnit(text: []const u8) ?struct { value_text: []const u8, unit: UnitDef } { +fn splitTrailingUnit(text: []const u8) Error!?struct { value_text: []const u8, unit: UnitDef } { var i: usize = 0; while (i < text.len) : (i += 1) { // The candidate must start exactly at i, so skip whitespace positions. @@ -557,7 +612,15 @@ fn splitTrailingUnit(text: []const u8) ?struct { value_text: []const u8, unit: U const candidate = std.mem.trim(u8, text[i..], whitespace); if (candidate.len == 0) break; - const unit = findUnit(candidate) orelse continue; + const unit = switch (resolve(candidate)) { + .unit => |found| found, + .unknown => continue, + // A candidate sitting at a token boundary that folds onto several units + // is the unit the user meant, spelled in a way that does not say which. + // Reported rather than skipped, because skipping it would go looking for + // a shorter suffix and answer some other question entirely. + .ambiguous => return Error.AmbiguousUnit, + }; const value_text = std.mem.trim(u8, text[0..i], whitespace); // A unit with nothing in front of it is not a conversion we can do. @@ -598,11 +661,18 @@ pub fn parseRequest(text: []const u8) Error!?ConversionRequest { const right = std.mem.trim(u8, text[sep.end..], whitespace); if (left.len == 0 or right.len == 0) continue; - const to_unit = findUnit(right) orelse { - failure = Error.UnknownUnit; + const to_unit = blk: { + switch (resolve(right)) { + .unit => |unit| break :blk unit, + .unknown => failure = Error.UnknownUnit, + .ambiguous => failure = Error.AmbiguousUnit, + } continue; }; - const split = splitTrailingUnit(left) orelse { + const split = (splitTrailingUnit(left) catch |err| { + failure = err; + continue; + }) orelse { failure = Error.UnknownUnit; continue; }; @@ -952,9 +1022,48 @@ test "exact match wins over case-insensitive match" { const k = findUnit("K").?; try testing.expectEqual(UnitCategory.temperature, k.category); try testing.expectEqualStrings("K", k.name); - // Likewise "B" is byte, not "b" - const b = findUnit("B").?; - try testing.expectEqualStrings("B", b.name); + // Likewise "B" is byte and "b" is bit, each by exact match. + try testing.expectEqualStrings("B", findUnit("B").?.name); + try testing.expectEqualStrings("b", findUnit("b").?.name); + try testing.expectEqualStrings("Mb", findUnit("Mb").?.name); + try testing.expectEqualStrings("MB", findUnit("MB").?.name); +} + +// -- Bits are not bytes, and case is how the tables say so -- + +test "prefixed bit units convert exactly against their byte counterparts" { + // `100 Mb to MB` used to answer "100 MB": there were no prefixed bit units, so + // "Mb" matched nothing exactly and the case-insensitive pass handed back "MB". + try expectConvert(12.5, 100, "Mb", "MB", 1e-12); + try expectConvert(125, 1, "kb", "B", 1e-12); + try expectConvert(1, 8, "b", "B", 1e-12); + try expectConvert(125, 1, "Gb", "MB", 1e-12); + try expectConvert(8, 1, "B", "b", 1e-12); + // The long forms too, which are unambiguous whatever the case. + try expectConvert(12.5, 100, "megabits", "megabytes", 1e-12); +} + +test "a name that folds onto two units is ambiguous, not a guess" { + // `Mbps` is megabits per second and `MBps` is megabytes per second, so "MBPS" + // is a factor of eight from one of them and there is no way to tell which. It + // used to resolve to whichever appeared first in the table. + try testing.expectError(Error.AmbiguousUnit, convert(100, "MBPS", "Mbps")); + try testing.expectError(Error.AmbiguousUnit, convert(100, "mB", "MB")); + try testing.expectError(Error.AmbiguousUnit, convert(100, "KB", "B")); + try testing.expect(findUnit("MBPS") == null); + try testing.expect(findUnit("mB") == null); + + // An exact spelling of either is still fine, and so is a fold that reaches + // exactly one unit. + try expectConvert(12.5, 100, "Mbps", "MBps", 1e-12); + try expectConvert(1000, 1, "KM", "M", 1e-9); +} + +test "an ambiguous unit in a conversion request reports itself" { + // Rather than skipping the candidate and resolving some shorter suffix, which + // would answer a different question with no error at all. + try testing.expectError(Error.AmbiguousUnit, parseRequest("100 MBPS to Mbps")); + try testing.expectError(Error.AmbiguousUnit, parseRequest("100 mB to MB")); } test "findUnit returns null for unknown names" {