diff --git a/AGENTS.md b/AGENTS.md index 8c6f20c..768ba0d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -818,6 +818,14 @@ permissive rule applies: `Lot`'s "close_date without close_price" fires only for stock lots because realized P&L is only computed for them, and each such scope limit cites its gate in a comment. +Rules that span records (limits, "at most one of") go in the schema's +`fileCheck(data: []const u8, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void` +instead, which gets the whole file and must set `sink.line` per record. +When a parser already enforces such rules, have `fileCheck` run the +parser with the sink rather than re-implement them - `projections.srf` +does this: `parseWith` reports through a `Reporter` that logs normally +and records findings under the lint, so each rule has one definition. + ### Adding a new provider 1. Create `src/providers/newprovider.zig` following the existing struct pattern diff --git a/docs/guides/audit-against-brokerage.md b/docs/guides/audit-against-brokerage.md index ad2dd96..e3dd6fe 100644 --- a/docs/guides/audit-against-brokerage.md +++ b/docs/guides/audit-against-brokerage.md @@ -103,7 +103,7 @@ With no flags, `zfin audit` first prints a health report: `update_cadence` (see [accounts.srf](set-up-accounts.md#3-tune-the-maintenance-cadence)). - **Brokerage files** it discovered, which it then reconciles. - **Config file problems** -- field-name typos in your `.srf` files, impossible - lot dates, and `accounts.srf` values zfin had to ignore. + lot dates, and values zfin had to ignore or adjust. ``` Portfolio hygiene @@ -145,7 +145,7 @@ kinds of problem, and prints nothing when there aren't any: ``` Each finding names the line of the record it came from, and the date -and account findings also say what zfin is doing instead of what you +and value findings also say what zfin is doing instead of what you wrote. #### 1. Field names (every file) @@ -195,10 +195,14 @@ normal -- that is the field for a CD or option that ends on a known date. Watch lots are exempt, since zfin reads only their `symbol`. See [Closed lots](../reference/config/portfolio-srf.md#closed-lots). -#### 3. Account values (`accounts.srf`) +#### 3. Values zfin rejects or adjusts (`accounts.srf`, `projections.srf`) -Values zfin rejects and replaces with a default, so the account loads -- -just not the way you wrote it. +Values that parse but can't be used as written. zfin replaces or clamps +them so the file still loads -- just not the way you wrote it. Each of +these is also logged to stderr when the file loads; the audit is where +it gets a line number and can't scroll past. + +**`accounts.srf`** | Report | What zfin does instead | |-----------------------------------------------|-------------------------------------------------------------------------| @@ -211,8 +215,26 @@ a finite number, none may name the account's own `tax_type`, and together they must sum to less than 100. See [`accounts.srf`](../reference/config/accounts-srf.md). -These three are also logged to stderr whenever `accounts.srf` loads; -the audit is where they get a line number and can't scroll past. +**`projections.srf`** + +| Report | What zfin does instead | +|---------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------| +| `return_cap`, `annual_contribution`, `target_spending`, or `survivor_spending_pct` `must be >= 0` | Ignores that value; an earlier line's value, or the default, stands. | +| `horizon`, `horizon_age`, or `max_accumulation_years` `must be > 0` | Ignores that value. | +| `spending_change capped` | Clamps it to 10%/yr either way -- a larger figure is almost always a units slip. | +| `max_accumulation_years capped` | Clamps it to 100. | +| `benchmark_stock` / `benchmark_bond` `must be 1..16 chars` | Ignores the symbol and keeps `SPY` / `AGG`. | +| `retirement_target must be 90, 95, or 99` | Ignores the annotation on that horizon. | +| `retirement_target set on multiple horizons` | Ignores **all** of them and uses the default promotion rule. Names both lines. | +| `horizon limit` / `horizon_age limit` (more than 8 of either) | Ignores the extras. | +| `event limit` (more than 16), or an event with `start_age 0` | Ignores that event. | +| `birthdate person index ... exceeds limit` (more than 4 people) | Ignores that birthdate. | +| `skipping malformed record` | Drops the whole line -- e.g. a misspelled `type::`, or text where a number belongs. | +| `stopped reading` | Ignores that line **and everything after it**. | + +The last two matter most: this file never fails to load, so a line zfin +couldn't read otherwise just quietly reverts to defaults. See +[`projections.srf`](../reference/config/projections-srf.md). #### Also in `zfin doctor` diff --git a/src/analytics/projections.zig b/src/analytics/projections.zig index 4de2a44..3454af6 100644 --- a/src/analytics/projections.zig +++ b/src/analytics/projections.zig @@ -15,19 +15,45 @@ const log = std.log.scoped(.projections); const shiller = @import("../data/shiller.zig"); const srf = @import("srf"); const srf_opts = @import("../srf_opts.zig"); +const srf_lint = @import("../srf_lint.zig"); const Date = @import("../Date.zig"); -/// `log.warn` wrapper that no-ops under `zig build test`. Used for -/// validation warnings emitted while parsing user-supplied -/// `projections.srf` records: the test suite intentionally feeds -/// invalid inputs to `validRetirementTarget` and the config-loader -/// to verify they're rejected, but the resulting stderr noise -/// pollutes test output. Keep using `log.warn` directly when the -/// warning is interesting in tests too. -fn warnUser(comptime fmt: []const u8, args: anytype) void { - if (builtin.is_test) return; - log.warn(fmt, args); -} +/// Where `projections.srf` validation problems go. +/// +/// The parser is the ONLY definition of these rules. Several depend on +/// state across the whole file - the horizon / horizon_age / event / +/// person limits, and "at most one `retirement_target`" - so a separate +/// lint copy would have to re-implement the parse loop, and could then +/// drift from it. Instead the one loop reports through this, either way: +/// +/// - no sink (every normal load): `log.warn`, as it always has. Silent +/// under `zig build test`, whose fixtures feed invalid values on +/// purpose. +/// - a sink (`srf_schema.fileCheck`, run by `zfin doctor` and +/// `zfin audit`): a lint finding carrying the record's line number, +/// and nothing on stderr. +/// +/// `args: anytype` is a `std.fmt` argument tuple, forwarded unchanged to +/// `log.warn` / `std.fmt.allocPrint` - the same shape as `std.log` +/// itself, and as the `warnUser` helper this replaced. +const Reporter = struct { + sink: ?*srf_lint.Sink = null, + + /// Stamp subsequent findings with `line`. No-op without a sink. + fn at(self: Reporter, line: u32) void { + if (self.sink) |s| s.line = line; + } + + /// `key` is the field the problem is about. It is used for rollup + /// identity only - the message itself names it too. + fn warn(self: Reporter, key: []const u8, comptime fmt: []const u8, args: anytype) error{OutOfMemory}!void { + if (self.sink) |s| { + try s.addOwned(.semantic, key, try std.fmt.allocPrint(s.allocator, fmt, args)); + } else if (!builtin.is_test) { + log.warn("projections: " ++ fmt, args); + } + } +}; // ── Life events ───────────────────────────────────────────────── @@ -734,6 +760,17 @@ pub const srf_schema = struct { pub const Record = SrfProjection; pub const file_label: []const u8 = "projections.srf"; pub const doc_path: []const u8 = "reference/config/projections-srf.md"; + + /// Run the real parser with a sink attached, so every rule it + /// enforces is reported with a line number instead of logged. See + /// `Reporter` for why this is a `fileCheck` (whole file) rather than + /// a per-record `semanticCheck`: half the rules are limits and + /// "at most one" constraints that span records. + pub fn fileCheck(data: []const u8, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void { + // No date rules in this file, so `today` is unused. + _ = ctx; + _ = try parseWith(data, .{ .sink = sink }); + } }; /// Clamp on the magnitude of `spending_change` (10%/yr real, in @@ -796,6 +833,18 @@ pub fn benchmarkSymbols(io: std.Io, arena: std.mem.Allocator, path: []const u8) /// type::birthdate,date::1975-03-15 /// type::event,name::Social Security,start_age:num:67,amount:num:38400 pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { + // With no sink the reporter only logs, and nothing else in the parse + // can fail, so OutOfMemory is unreachable. An exhaustive switch + // rather than `catch unreachable`: a new error in `parseWith`'s set + // becomes a compile error here instead of a silent assumption. + return parseWith(data, .{}) catch |err| switch (err) { + error.OutOfMemory => unreachable, + }; +} + +/// `parseProjectionsConfig`, reporting validation problems through +/// `rep`. Only a sink-backed reporter can fail (allocating a finding). +fn parseWith(data: ?[]const u8, rep: Reporter) error{OutOfMemory}!UserConfig { var config = UserConfig{}; const raw = data orelse return config; if (raw.len == 0) return config; @@ -817,17 +866,28 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { // rule. A single bad value (not in {90,95,99}) is treated as // "no annotation on this record" and doesn't poison the others. var annotation_count: u8 = 0; + // Where the first two annotations were, so the "more than one" + // finding can point at both. + var first_annotation_line: u32 = 0; + var second_annotation_line: u32 = 0; + + while (true) { + const next = it.next() catch |err| { + // Used to be `catch null`, which dropped the rest of the + // file without a word. + rep.at(@intCast(it.state.line)); + try rep.warn("record", "stopped reading ({s}); this and every later record are ignored", .{@errorName(err)}); + break; + }; + const field_it = next orelse break; + const line: u32 = @intCast(it.state.line); + rep.at(line); - while (it.next() catch null) |field_it| { const rec = field_it.to(SrfProjection, srf_opts.user_edited) catch |err| { // Skip the record rather than losing the whole file, but // name the error: a dropped record reverts that setting to - // its default without saying so. Quiet under - // `zig build test`, where fixtures feed malformed records - // on purpose. - if (!builtin.is_test) { - log.warn("projections.srf: skipping malformed record: {s}", .{@errorName(err)}); - } + // its default without saying so. + try rep.warn("record", "skipping malformed record ({s}); its settings revert to defaults", .{@errorName(err)}); continue; }; switch (rec) { @@ -841,7 +901,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { if (cap >= 0) { config.return_cap = cap; } else { - warnUser("projections: return_cap must be >= 0 (got {d}); ignoring record", .{cap}); + try rep.warn("return_cap", "return_cap must be >= 0 (got {d}); value ignored", .{cap}); } } if (c.horizon) |h| { @@ -850,14 +910,16 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { saw_horizon = true; } if (h == 0) { - log.warn("projections: horizon must be > 0; ignoring record", .{}); + try rep.warn("horizon", "horizon must be > 0; value ignored", .{}); } else if (config.horizon_count >= UserConfig.max_horizons) { - log.warn("projections: horizon limit reached ({d}); ignoring extra horizon record (value {d})", .{ UserConfig.max_horizons, h }); + try rep.warn("horizon", "horizon limit reached ({d}); ignoring extra horizon (value {d})", .{ UserConfig.max_horizons, h }); } else { config.horizons[config.horizon_count] = h; - if (validRetirementTarget(c.retirement_target)) |conf| { + if (try validRetirementTarget(c.retirement_target, rep)) |conf| { config.horizon_targets[config.horizon_count] = conf; annotation_count += 1; + if (annotation_count == 1) first_annotation_line = line; + if (annotation_count == 2) second_annotation_line = line; } config.horizon_count += 1; } @@ -874,14 +936,16 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { saw_horizon = true; } if (age == 0) { - log.warn("projections: horizon_age must be > 0; ignoring record", .{}); + try rep.warn("horizon_age", "horizon_age must be > 0; value ignored", .{}); } else if (config.horizon_age_count >= UserConfig.max_horizons) { - log.warn("projections: horizon_age limit reached ({d}); ignoring extra horizon_age record (value {d})", .{ UserConfig.max_horizons, age }); + try rep.warn("horizon_age", "horizon_age limit reached ({d}); ignoring extra horizon_age (value {d})", .{ UserConfig.max_horizons, age }); } else { config.horizon_ages[config.horizon_age_count] = age; - if (validRetirementTarget(c.retirement_target)) |conf| { + if (try validRetirementTarget(c.retirement_target, rep)) |conf| { config.horizon_age_targets[config.horizon_age_count] = conf; annotation_count += 1; + if (annotation_count == 1) first_annotation_line = line; + if (annotation_count == 2) second_annotation_line = line; } config.horizon_age_count += 1; } @@ -894,7 +958,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { if (amt >= 0) { config.annual_contribution = amt; } else { - warnUser("projections: annual_contribution must be >= 0 (got {d}); ignoring record", .{amt}); + try rep.warn("annual_contribution", "annual_contribution must be >= 0 (got {d}); value ignored", .{amt}); } } if (c.contribution_inflation_adjusted) |b| { @@ -904,7 +968,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { if (amt >= 0) { config.target_spending = amt; } else { - warnUser("projections: target_spending must be >= 0 (got {d}); ignoring record", .{amt}); + try rep.warn("target_spending", "target_spending must be >= 0 (got {d}); value ignored", .{amt}); } } if (c.target_spending_inflation_adjusted) |b| { @@ -918,10 +982,10 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { const frac = pct / 100.0; const cap = max_abs_spending_real_change; if (frac > cap) { - warnUser("projections: spending_change capped at +{d:.0}%/yr (got {d}%)", .{ cap * 100.0, pct }); + try rep.warn("spending_change", "spending_change capped at +{d:.0}%/yr (got {d}%)", .{ cap * 100.0, pct }); config.spending_real_change = cap; } else if (frac < -cap) { - warnUser("projections: spending_change capped at -{d:.0}%/yr (got {d}%)", .{ cap * 100.0, pct }); + try rep.warn("spending_change", "spending_change capped at -{d:.0}%/yr (got {d}%)", .{ cap * 100.0, pct }); config.spending_real_change = -cap; } else { config.spending_real_change = frac; @@ -937,7 +1001,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { if (pct >= 0) { config.survivor_spending_pct = pct; } else { - warnUser("projections: survivor_spending_pct must be >= 0 (got {d}); ignoring record", .{pct}); + try rep.warn("survivor_spending_pct", "survivor_spending_pct must be >= 0 (got {d}); value ignored", .{pct}); } } if (c.max_accumulation_years) |n| { @@ -945,12 +1009,12 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { // A zero-year search ceiling is degenerate (it // would only ever ask "can I retire today?"). // Almost certainly a typo; keep the default. - warnUser("projections: max_accumulation_years must be > 0; ignoring record", .{}); + try rep.warn("max_accumulation_years", "max_accumulation_years must be > 0; value ignored", .{}); } else if (n > max_configurable_accumulation_years) { // Respect the intent (the user wants a large // ceiling) but clamp to keep the search bounded // and inside the historical data span. - warnUser("projections: max_accumulation_years capped at {d} (got {d})", .{ max_configurable_accumulation_years, n }); + try rep.warn("max_accumulation_years", "max_accumulation_years capped at {d} (got {d})", .{ max_configurable_accumulation_years, n }); config.max_accumulation_years = max_configurable_accumulation_years; } else { config.max_accumulation_years = n; @@ -958,7 +1022,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { } if (c.benchmark_stock) |sym| { if (sym.len == 0 or sym.len > config.benchmark_stock_buf.len) { - warnUser("projections: benchmark_stock must be 1..{d} chars (got {d}); ignoring record", .{ config.benchmark_stock_buf.len, sym.len }); + try rep.warn("benchmark_stock", "benchmark_stock must be 1..{d} chars (got {d}); value ignored", .{ config.benchmark_stock_buf.len, sym.len }); } else { // Copy into our own buffer + length so the value // outlives both the SRF iterator's backing data @@ -972,7 +1036,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { } if (c.benchmark_bond) |sym| { if (sym.len == 0 or sym.len > config.benchmark_bond_buf.len) { - warnUser("projections: benchmark_bond must be 1..{d} chars (got {d}); ignoring record", .{ config.benchmark_bond_buf.len, sym.len }); + try rep.warn("benchmark_bond", "benchmark_bond must be 1..{d} chars (got {d}); value ignored", .{ config.benchmark_bond_buf.len, sym.len }); } else { @memcpy(config.benchmark_bond_buf[0..sym.len], sym); config.benchmark_bond_len = @intCast(sym.len); @@ -987,15 +1051,15 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { config.birthdates[idx] = b.date; if (idx >= config.birthdate_count) config.birthdate_count = idx + 1; } else { - log.warn("projections: birthdate person index {d} exceeds limit ({d}); ignoring record", .{ idx + 1, UserConfig.max_persons }); + try rep.warn("person", "birthdate person index {d} exceeds limit ({d}); ignoring record", .{ idx + 1, UserConfig.max_persons }); } birthdate_seq += 1; }, .event => |e| { if (e.start_age == 0) { - log.warn("projections: event '{s}' has start_age 0; ignoring record", .{e.name}); + try rep.warn("start_age", "event '{s}' has start_age 0; ignoring record", .{e.name}); } else if (config.event_count >= UserConfig.max_events) { - log.warn("projections: event limit reached ({d}); ignoring extra event '{s}'", .{ UserConfig.max_events, e.name }); + try rep.warn("event", "event limit reached ({d}); ignoring extra event '{s}'", .{ UserConfig.max_events, e.name }); } else { var ev = LifeEvent{ .start_age = e.start_age, @@ -1020,7 +1084,8 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { // fall back to the default rule. Logged as a warning so the // user knows their override was ignored. if (annotation_count > 1) { - warnUser("projections: retirement_target set on multiple horizons; ignoring all annotations and using default promotion rule", .{}); + rep.at(second_annotation_line); + try rep.warn("retirement_target", "retirement_target set on multiple horizons (first on line {d}); ignoring all annotations and using default promotion rule", .{first_annotation_line}); config.horizon_targets = @splat(0); config.horizon_age_targets = @splat(0); } @@ -1030,13 +1095,13 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { /// Validate a `retirement_target` SRF value. Returns the value /// unchanged if it's exactly 90, 95, or 99; returns null otherwise -/// (logged as a warning so the user notices the typo). Used at parse +/// (reported through `rep` so the user notices the typo). Used at parse /// time; the view-layer `pickPromotedCell` trusts whatever lands in /// `horizon_targets`. -fn validRetirementTarget(raw: ?u8) ?u8 { +fn validRetirementTarget(raw: ?u8, rep: Reporter) error{OutOfMemory}!?u8 { const v = raw orelse return null; if (v == 90 or v == 95 or v == 99) return v; - warnUser("projections: retirement_target must be 90, 95, or 99 (got {d}); annotation ignored", .{v}); + try rep.warn("retirement_target", "retirement_target must be 90, 95, or 99 (got {d}); annotation ignored", .{v}); return null; } @@ -4014,13 +4079,13 @@ test "parseProjectionsConfig: multiple retirement_target annotations all dropped } test "validRetirementTarget: 90/95/99 pass, others fail" { - try std.testing.expectEqual(@as(?u8, 90), validRetirementTarget(90)); - try std.testing.expectEqual(@as(?u8, 95), validRetirementTarget(95)); - try std.testing.expectEqual(@as(?u8, 99), validRetirementTarget(99)); - try std.testing.expectEqual(@as(?u8, null), validRetirementTarget(null)); - try std.testing.expectEqual(@as(?u8, null), validRetirementTarget(0)); - try std.testing.expectEqual(@as(?u8, null), validRetirementTarget(85)); - try std.testing.expectEqual(@as(?u8, null), validRetirementTarget(100)); + try std.testing.expectEqual(@as(?u8, 90), try validRetirementTarget(90, .{})); + try std.testing.expectEqual(@as(?u8, 95), try validRetirementTarget(95, .{})); + try std.testing.expectEqual(@as(?u8, 99), try validRetirementTarget(99, .{})); + try std.testing.expectEqual(@as(?u8, null), try validRetirementTarget(null, .{})); + try std.testing.expectEqual(@as(?u8, null), try validRetirementTarget(0, .{})); + try std.testing.expectEqual(@as(?u8, null), try validRetirementTarget(85, .{})); + try std.testing.expectEqual(@as(?u8, null), try validRetirementTarget(100, .{})); } // ── oldestBirthdate / oldestAge tests ────────────────────────── @@ -4464,3 +4529,231 @@ test "benchmarkSymbols: an override is honoured and outlives the config" { try std.testing.expectEqualStrings("VTI", pair[0]); try std.testing.expectEqualStrings("BND", pair[1]); } + +// ── srf_schema.fileCheck: parse-time rules reported through the lint ── + +/// Run both lint passes over `data` as projections.srf and return the +/// semantic findings as "line: message" lines, in line order. Caller +/// frees. +fn lintProjections(data: []const u8) ![]u8 { + const allocator = std.testing.allocator; + var r = try srf_lint.check(allocator, data, srf_lint.shapeOfSchema(srf_schema)); + defer r.deinit(); + try srf_lint.checkSemantic(srf_schema, &r, data, .{ .today = Date.fromYmd(2026, 1, 1) }); + r.sort(); + + var aw: std.Io.Writer.Allocating = .init(allocator); + errdefer aw.deinit(); + var buf: [srf_lint.Finding.describe_max]u8 = undefined; + for (r.findings) |f| { + if (f.kind != .semantic) continue; + try aw.writer.print("{d}: {s}\n", .{ f.first_line, f.describe(&buf) }); + } + return aw.toOwnedSlice(); +} + +fn expectProjectionFindings(data: []const u8, want: []const u8) !void { + const got = try lintProjections(data); + defer std.testing.allocator.free(got); + try std.testing.expectEqualStrings(want, got); +} + +test "fileCheck: a clean projections.srf reports nothing" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,target_stock_pct:num:80,return_cap:num:30 + \\type::config,horizon:num:30,retirement_target:num:95 + \\type::config,horizon_age:num:95 + \\type::config,spending_change:num:-2,survivor_spending_pct:num:75 + \\type::birthdate,date::1975-03-15 + \\type::event,name::Social Security,start_age:num:67,amount:num:38400 + \\ + , ""); +} + +test "fileCheck: negative amounts are rejected" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,return_cap:num:-5 + \\type::config,annual_contribution:num:-100 + \\type::config,target_spending:num:-1 + \\type::config,survivor_spending_pct:num:-10 + \\ + , + \\2: return_cap must be >= 0 (got -5); value ignored + \\3: annual_contribution must be >= 0 (got -100); value ignored + \\4: target_spending must be >= 0 (got -1); value ignored + \\5: survivor_spending_pct must be >= 0 (got -10); value ignored + \\ + ); +} + +test "fileCheck: zero horizons and a zero accumulation ceiling are rejected" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,horizon:num:0 + \\type::config,horizon_age:num:0 + \\type::config,max_accumulation_years:num:0 + \\ + , + \\2: horizon must be > 0; value ignored + \\3: horizon_age must be > 0; value ignored + \\4: max_accumulation_years must be > 0; value ignored + \\ + ); +} + +test "fileCheck: out-of-range values report the clamp that was applied" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,spending_change:num:20 + \\type::config,spending_change:num:-20 + \\type::config,max_accumulation_years:num:150 + \\ + , + \\2: spending_change capped at +10%/yr (got 20%) + \\3: spending_change capped at -10%/yr (got -20%) + \\4: max_accumulation_years capped at 100 (got 150) + \\ + ); +} + +test "fileCheck: benchmark symbols longer than the buffer are rejected" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,benchmark_stock::ABCDEFGHIJKLMNOPQ + \\type::config,benchmark_bond::ABCDEFGHIJKLMNOPQ + \\ + , + \\2: benchmark_stock must be 1..16 chars (got 17); value ignored + \\3: benchmark_bond must be 1..16 chars (got 17); value ignored + \\ + ); +} + +test "fileCheck: an invalid retirement_target is ignored" { + try expectProjectionFindings( + \\#!srfv1 + \\type::config,horizon:num:30,retirement_target:num:85 + \\ + , + \\2: retirement_target must be 90, 95, or 99 (got 85); annotation ignored + \\ + ); +} + +test "fileCheck: retirement_target on two horizons points at both" { + // A cross-record rule - the reason this is a `fileCheck` and not a + // per-record `semanticCheck`. + try expectProjectionFindings( + \\#!srfv1 + \\type::config,horizon:num:30,retirement_target:num:95 + \\type::config,horizon_age:num:90,retirement_target:num:99 + \\ + , + \\3: retirement_target set on multiple horizons (first on line 2); ignoring all annotations and using default promotion rule + \\ + ); +} + +test "fileCheck: horizon and horizon_age limits" { + var aw: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer aw.deinit(); + try aw.writer.writeAll("#!srfv1\n"); + for (0..UserConfig.max_horizons + 1) |i| try aw.writer.print("type::config,horizon:num:{d}\n", .{10 + i}); + for (0..UserConfig.max_horizons + 1) |i| try aw.writer.print("type::config,horizon_age:num:{d}\n", .{80 + i}); + // Line 1 is the header, so the ninth horizon is line 10 and the + // ninth horizon_age is line 19. + try expectProjectionFindings(aw.written(), + \\10: horizon limit reached (8); ignoring extra horizon (value 18) + \\19: horizon_age limit reached (8); ignoring extra horizon_age (value 88) + \\ + ); +} + +test "fileCheck: birthdate beyond the person limit" { + try expectProjectionFindings( + \\#!srfv1 + \\type::birthdate,date::1980-01-01,person:num:5 + \\ + , + \\2: birthdate person index 5 exceeds limit (4); ignoring record + \\ + ); +} + +test "fileCheck: event with start_age 0, and the event limit" { + var aw: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer aw.deinit(); + try aw.writer.writeAll("#!srfv1\ntype::event,name::Pension,amount:num:100\n"); + for (0..UserConfig.max_events + 1) |i| { + try aw.writer.print("type::event,name::Event {d},start_age:num:{d},amount:num:1\n", .{ i, 60 + i }); + } + // Line 2 has no start_age (defaults to 0). Lines 3..18 fill the 16 + // slots; line 19 is the seventeenth. + try expectProjectionFindings(aw.written(), + \\2: event 'Pension' has start_age 0; ignoring record + \\19: event limit reached (16); ignoring extra event 'Event 16' + \\ + ); +} + +test "fileCheck: a record that fails to coerce is reported, not silently dropped" { + // `doctor`'s parse-check for this file is structural only, because + // the parser is infallible - so before this, nothing reported it. + try expectProjectionFindings( + \\#!srfv1 + \\type::config,horizon::thirty + \\type::confg,horizon:num:30 + \\ + , + \\2: skipping malformed record (InvalidCharacter); its settings revert to defaults + \\3: skipping malformed record (ActiveTagDoesNotExist); its settings revert to defaults + \\ + ); +} + +test "fileCheck: a stream error is reported instead of truncating the file silently" { + // A directive after data is a hard SRF error; the parser used to + // `catch null` it and quietly drop every record after it. + const got = try lintProjections( + \\#!srfv1 + \\type::config,horizon:num:30 + \\#!expires=1767225600 + \\type::config,target_stock_pct:num:60 + \\ + ); + defer std.testing.allocator.free(got); + try std.testing.expect(std.mem.indexOf(u8, got, "stopped reading (ParseFailed); this and every later record are ignored") != null); +} + +test "fileCheck: attaching a sink does not change what the parser produces" { + // The lint runs the real parser; it must not also alter it. Same + // bad input through both modes, compared field by field. + const data = + \\#!srfv1 + \\type::config,return_cap:num:-5,target_stock_pct:num:70 + \\type::config,horizon:num:0 + \\type::config,horizon:num:30,retirement_target:num:95 + \\type::config,horizon_age:num:90,retirement_target:num:99 + \\type::config,spending_change:num:20 + \\type::event,name::Pension,amount:num:100 + \\ + ; + const plain = parseProjectionsConfig(data); + + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + var sink: srf_lint.Sink = .{ .allocator = arena_state.allocator(), .findings = .empty }; + const linted = try parseWith(data, .{ .sink = &sink }); + + try std.testing.expect(sink.findings.items.len > 0); + try std.testing.expectEqual(plain.target_stock_pct, linted.target_stock_pct); + try std.testing.expectEqual(plain.return_cap, linted.return_cap); + try std.testing.expectEqual(plain.horizon_count, linted.horizon_count); + try std.testing.expectEqualSlices(u16, &plain.horizons, &linted.horizons); + try std.testing.expectEqualSlices(u8, &plain.horizon_targets, &linted.horizon_targets); + try std.testing.expectEqualSlices(u8, &plain.horizon_age_targets, &linted.horizon_age_targets); + try std.testing.expectEqual(plain.spending_real_change, linted.spending_real_change); + try std.testing.expectEqual(plain.event_count, linted.event_count); +} diff --git a/src/srf_lint.zig b/src/srf_lint.zig index 33fa50c..82fe641 100644 --- a/src/srf_lint.zig +++ b/src/srf_lint.zig @@ -267,8 +267,9 @@ pub const Sink = struct { allocator: std.mem.Allocator, findings: std.ArrayList(Finding), truncated: bool = false, - /// Line of the record being walked. Set by `check`; read by - /// `semanticCheck` implementations via `add*`. + /// Line of the record being walked, stamped on every finding added. + /// Set by `check` and `checkSemantic` before each record; a + /// schema's `fileCheck` walks its own records and must set it. line: u32 = 0, /// Add a finding whose `detail` is a static string. @@ -483,6 +484,15 @@ fn scopesOfSchema(comptime S: type) Scopes { /// pub const scope_discriminator = "security_type"; // with field_rules /// pub const field_rules = [_]srf_lint.Rule{ ... }; // with scope_discriminator /// pub fn semanticCheck(rec: Record, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void +/// pub fn fileCheck(data: []const u8, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void +/// +/// `semanticCheck` sees one record at a time and suits rules that are +/// about that record alone. `fileCheck` gets the whole file, for rules +/// that depend on state ACROSS records - limits, "at most one of", and +/// ordering. It walks the records itself and sets `sink.line` as it +/// goes. Its intended use is a model whose parser already enforces +/// such rules: pass the sink to the parser, so the rules have one +/// definition rather than a parser copy and a lint copy that can drift. pub fn validateSchema(comptime S: type) void { comptime { const kind = "SRF schema"; @@ -527,6 +537,17 @@ pub fn validateSchema(comptime S: type) void { "pub fn semanticCheck(rec: Record, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void", ); } + if (@hasDecl(S, "fileCheck")) { + comptime_validator.expectFnInferredError( + kind, + name, + S, + "fileCheck", + &.{ []const u8, Context, *Sink }, + void, + "pub fn fileCheck(data: []const u8, ctx: srf_lint.Context, sink: *srf_lint.Sink) !void", + ); + } } } @@ -796,34 +817,43 @@ fn describeOnly( return aw.toOwnedSlice(); } -/// Run a schema's model-owned `semanticCheck` over `data`, appending -/// to `result`. +/// Run a schema's model-owned `semanticCheck` and/or `fileCheck` over +/// `data`, appending to `result`. /// /// A SECOND typed pass, separate from `check`'s raw one, because SRF's /// iterators are single-pass: `to()` drains the fields that the raw -/// walk needs. Records that fail to coerce are skipped silently - the -/// typed parser's own diagnostics (and `doctor`'s parse-check) already -/// report those, and duplicating them here would double every message. +/// walk needs. In the per-record pass, records that fail to coerce are +/// skipped silently - the typed parser's own diagnostics (and +/// `doctor`'s parse-check) already report those, and duplicating them +/// here would double every message. A `fileCheck` owns its own walk, +/// so what it reports about an uncoercible record is its decision. pub fn checkSemantic(comptime S: type, result: *Result, data: []const u8, ctx: Context) !void { - if (!@hasDecl(S, "semanticCheck")) return; + const has_record = @hasDecl(S, "semanticCheck"); + const has_file = @hasDecl(S, "fileCheck"); + if (!has_record and !has_file) return; const allocator = result.arena.allocator(); var sink: Sink = .{ .allocator = allocator, .findings = .empty }; try sink.findings.appendSlice(allocator, result.findings); sink.truncated = result.truncated; + if (has_record) try recordPass(S, &sink, data, ctx); + if (has_file) try S.fileCheck(data, ctx, &sink); + + result.findings = try sink.findings.toOwnedSlice(allocator); + result.truncated = sink.truncated; +} + +fn recordPass(comptime S: type, sink: *Sink, data: []const u8, ctx: Context) !void { var reader = std.Io.Reader.fixed(data); - var it = srf.iterator(&reader, allocator, .{ .parse_allocator = .none }) catch return; + var it = srf.iterator(&reader, sink.allocator, .{ .parse_allocator = .none }) catch return; defer it.deinit(); while (it.next() catch null) |fields| { sink.line = @intCast(it.state.line); const rec = fields.to(S.Record, @import("srf_opts.zig").user_edited) catch continue; - try S.semanticCheck(rec, ctx, &sink); + try S.semanticCheck(rec, ctx, sink); } - - result.findings = try sink.findings.toOwnedSlice(allocator); - result.truncated = sink.truncated; } /// Write the valid field names for `shape` to `w`, wrapped to `width` @@ -1800,3 +1830,67 @@ test "Result.hasNameFindings: only name problems warrant the valid-field list" { try testing.expect(!r.hasNameFindings()); } } + +// ── fileCheck ───────────────────────────────────────────────── + +const LimitedRecord = struct { name: []const u8 = "" }; + +/// A cross-record rule no per-record hook can express: at most two +/// records. Walks its own records and stamps `sink.line`, per the +/// `fileCheck` contract. +const limited_schema = struct { + pub const Record = LimitedRecord; + pub const file_label: []const u8 = "limited.srf"; + pub const doc_path: []const u8 = "reference/config/limited-srf.md"; + + pub fn fileCheck(data: []const u8, ctx: Context, sink: *Sink) !void { + _ = ctx; + var reader = std.Io.Reader.fixed(data); + var it = srf.iterator(&reader, sink.allocator, .{ .parse_allocator = .none }) catch return; + defer it.deinit(); + var n: usize = 0; + while (it.next() catch null) |fields| { + sink.line = @intCast(it.state.line); + _ = fields.to(Record, .{}) catch continue; + n += 1; + if (n > 2) try sink.addStatic(.semantic, "name", "limit of 2 records reached; ignoring record"); + } + } +}; + +test "checkSemantic: fileCheck sees every record and reports cross-record rules" { + const data = + \\#!srfv1 + \\name::a + \\name::b + \\name::c + \\name::d + \\ + ; + var r = try check(testing.allocator, data, shapeOfSchema(limited_schema)); + defer r.deinit(); + try checkSemantic(limited_schema, &r, data, test_ctx); + // Records 3 and 4 trip the same rule with the same detail, so they + // roll up into one finding carrying both line numbers. + try testing.expectEqual(@as(usize, 1), r.findings.len); + try testing.expectEqual(@as(u32, 2), r.findings[0].count); + try testing.expectEqual(@as(u32, 4), r.findings[0].first_line); + try testing.expectEqual(@as(u32, 5), r.findings[0].extra_lines[0]); +} + +test "checkSemantic: fileCheck findings merge with raw-pass findings" { + const data = + \\#!srfv1 + \\name::a + \\name::b + \\name::c,nmae::x + \\ + ; + var r = try check(testing.allocator, data, shapeOfSchema(limited_schema)); + defer r.deinit(); + try checkSemantic(limited_schema, &r, data, test_ctx); + r.sort(); + try testing.expectEqual(@as(usize, 2), r.findings.len); + try testing.expectEqual(Kind.semantic, r.findings[0].kind); // key "name" < "nmae" + try testing.expectEqual(Kind.unknown, r.findings[1].kind); +}