From 5618ed0a7daab396e40a4e83eb652f9db06976e6 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Tue, 25 Nov 2025 20:34:54 -0500 Subject: [PATCH] Improve violation detection --- build.zig | 51 +++++++++++++----- src/builtins/str.zig | 124 +++++++++++++++++++++---------------------- 2 files changed, 101 insertions(+), 74 deletions(-) diff --git a/build.zig b/build.zig index 672c62b5c4..cffb898989 100644 --- a/build.zig +++ b/build.zig @@ -81,7 +81,7 @@ const TestsSummaryStep = struct { /// 2. They are brittle to changes that type-checking should not be sensitive to /// /// Instead, we always compare indices - either into node stores or to interned string indices. -/// This step enforces that rule by failing the build if `std.mem.` is found in src/check/. +/// This step enforces that rule by failing the build if `std.mem.` is found in src/check/ or src/layout/. const CheckTypeCheckerPatternsStep = struct { step: Step, @@ -106,13 +106,16 @@ const CheckTypeCheckerPatternsStep = struct { var violations = std.ArrayList(Violation).empty; defer violations.deinit(allocator); - // Recursively scan src/check/ for .zig files - var dir = std.fs.cwd().openDir("src/check", .{ .iterate = true }) catch |err| { - return step.fail("Failed to open src/check directory: {}", .{err}); - }; - defer dir.close(); + // Recursively scan src/check/ and src/layout/ for .zig files + const dirs_to_scan = [_][]const u8{ "src/check", "src/layout" }; + for (dirs_to_scan) |dir_path| { + var dir = std.fs.cwd().openDir(dir_path, .{ .iterate = true }) catch |err| { + return step.fail("Failed to open {s} directory: {}", .{ dir_path, err }); + }; + defer dir.close(); - try scanDirectory(allocator, dir, "src/check", &violations); + try scanDirectory(allocator, dir, dir_path, &violations); + } if (violations.items.len > 0) { std.debug.print("\n", .{}); @@ -121,7 +124,7 @@ const CheckTypeCheckerPatternsStep = struct { std.debug.print("=" ** 80 ++ "\n\n", .{}); std.debug.print( - \\The type checker code in src/check/ must NOT use std.mem.* functions. + \\The type checker code in src/check/ and src/layout/ must NOT use std.mem.* functions. \\ \\WHY THIS RULE EXISTS: \\ During type checking, we NEVER do string or byte comparisons because: @@ -161,7 +164,7 @@ const CheckTypeCheckerPatternsStep = struct { std.debug.print("\n" ++ "=" ** 80 ++ "\n", .{}); return step.fail( - "Found {d} uses of std.mem.* in src/check/. " ++ + "Found {d} uses of std.mem.* in src/check/ or src/layout/. " ++ "See above for details on why this is forbidden and what to do instead.", .{violations.items.len}, ); @@ -187,6 +190,11 @@ const CheckTypeCheckerPatternsStep = struct { if (entry.kind != .file) continue; if (!std.mem.endsWith(u8, entry.path, ".zig")) continue; + // Skip test files - they may legitimately need string comparison for assertions + if (std.mem.endsWith(u8, entry.path, "_test.zig")) continue; + if (std.mem.indexOf(u8, entry.path, "test/") != null) continue; + if (std.mem.startsWith(u8, entry.path, "test")) continue; + const full_path = try std.fmt.allocPrint(allocator, "{s}/{s}", .{ path_prefix, entry.path }); const file = dir.openFile(entry.path, .{}) catch continue; @@ -202,11 +210,30 @@ const CheckTypeCheckerPatternsStep = struct { if (char == '\n') { const line = content[line_start..i]; - // Check for std.mem. usage (but allow std.mem.Allocator which is a type) + // Check for std.mem. usage (but allow safe patterns) if (std.mem.indexOf(u8, line, "std.mem.")) |idx| { - // Check if it's just "std.mem.Allocator" which is allowed const after_match = line[idx + 8 ..]; - if (!std.mem.startsWith(u8, after_match, "Allocator")) { + + // Allow these safe patterns that don't involve string/byte comparison: + // - std.mem.Allocator: a type, not a comparison + // - std.mem.Alignment: a type, not a comparison + // - std.mem.sort: sorting by custom comparator, not string comparison + // - std.mem.asBytes: type punning, not string comparison + // - std.mem.reverse: reversing arrays, not string comparison + // - std.mem.alignForward: memory alignment arithmetic, not string comparison + // - std.mem.order: sort ordering (used by sort comparators), not string comparison + // - std.mem.copyForwards: byte copying, not string comparison + const is_allowed = + std.mem.startsWith(u8, after_match, "Allocator") or + std.mem.startsWith(u8, after_match, "Alignment") or + std.mem.startsWith(u8, after_match, "sort") or + std.mem.startsWith(u8, after_match, "asBytes") or + std.mem.startsWith(u8, after_match, "reverse") or + std.mem.startsWith(u8, after_match, "alignForward") or + std.mem.startsWith(u8, after_match, "order") or + std.mem.startsWith(u8, after_match, "copyForwards"); + + if (!is_allowed) { const trimmed = std.mem.trim(u8, line, " \t"); // Skip comments if (!std.mem.startsWith(u8, trimmed, "//")) { diff --git a/src/builtins/str.zig b/src/builtins/str.zig index dd74669c83..3082d5b890 100644 --- a/src/builtins/str.zig +++ b/src/builtins/str.zig @@ -213,7 +213,7 @@ pub const RocStr = extern struct { } } - pub fn eq(self: RocStr, other: RocStr) bool { + pub fn eql(self: RocStr, other: RocStr) bool { // If they are byte-for-byte equal, they're definitely equal! if (self.bytes == other.bytes and self.length == other.length) { return true; @@ -478,7 +478,7 @@ pub fn init( // Str.equal /// TODO: Document strEqual. pub fn strEqual(self: RocStr, other: RocStr) callconv(.c) bool { - return self.eq(other); + return self.eql(other); } // Str.numberOfBytes @@ -1655,7 +1655,7 @@ test "RocStr.eq: small, equal" { const str2_ptr: [*]u8 = &str2; var roc_str2 = RocStr.init(str2_ptr, str2_len, test_env.getOps()); - try std.testing.expect(roc_str1.eq(roc_str2)); + try std.testing.expect(roc_str1.eql(roc_str2)); roc_str1.decref(test_env.getOps()); roc_str2.decref(test_env.getOps()); @@ -1680,7 +1680,7 @@ test "RocStr.eq: small, not equal, different length" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(!roc_str1.eq(roc_str2)); + try std.testing.expect(!roc_str1.eql(roc_str2)); } test "RocStr.eq: small, not equal, same length" { @@ -1702,7 +1702,7 @@ test "RocStr.eq: small, not equal, same length" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(!roc_str1.eq(roc_str2)); + try std.testing.expect(!roc_str1.eql(roc_str2)); } test "RocStr.eq: large, equal" { @@ -1718,7 +1718,7 @@ test "RocStr.eq: large, equal" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(roc_str1.eq(roc_str2)); + try std.testing.expect(roc_str1.eql(roc_str2)); } test "RocStr.eq: large, different lengths, unequal" { @@ -1735,7 +1735,7 @@ test "RocStr.eq: large, different lengths, unequal" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(!roc_str1.eq(roc_str2)); + try std.testing.expect(!roc_str1.eql(roc_str2)); } test "RocStr.eq: large, different content, unequal" { @@ -1752,7 +1752,7 @@ test "RocStr.eq: large, different content, unequal" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(!roc_str1.eq(roc_str2)); + try std.testing.expect(!roc_str1.eql(roc_str2)); } test "RocStr.eq: large, garbage after end, equal" { @@ -1775,7 +1775,7 @@ test "RocStr.eq: large, garbage after end, equal" { roc_str2.decref(test_env.getOps()); } - try std.testing.expect(roc_str1.eq(roc_str2)); + try std.testing.expect(roc_str1.eql(roc_str2)); } test "strSplitHelp: empty delimiter" { @@ -1812,7 +1812,7 @@ test "strSplitHelp: empty delimiter" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); + try std.testing.expect(array[0].eql(expected[0])); } test "strSplitHelp: no delimiter" { @@ -1849,7 +1849,7 @@ test "strSplitHelp: no delimiter" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); + try std.testing.expect(array[0].eql(expected[0])); } test "strSplitHelp: empty start" { @@ -1891,8 +1891,8 @@ test "strSplitHelp: empty start" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); } test "strSplitHelp: empty end" { @@ -1936,9 +1936,9 @@ test "strSplitHelp: empty end" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); - try std.testing.expect(array[2].eq(expected[2])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); + try std.testing.expect(array[2].eql(expected[2])); } test "strSplitHelp: string equals delimiter" { @@ -1972,8 +1972,8 @@ test "strSplitHelp: string equals delimiter" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); } test "strSplitHelp: delimiter on sides" { @@ -2016,9 +2016,9 @@ test "strSplitHelp: delimiter on sides" { } try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); - try std.testing.expect(array[2].eq(expected[2])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); + try std.testing.expect(array[2].eql(expected[2])); } test "strSplitHelp: three pieces" { @@ -2060,9 +2060,9 @@ test "strSplitHelp: three pieces" { } try std.testing.expectEqual(expected_array.len, array.len); - try std.testing.expect(array[0].eq(expected_array[0])); - try std.testing.expect(array[1].eq(expected_array[1])); - try std.testing.expect(array[2].eq(expected_array[2])); + try std.testing.expect(array[0].eql(expected_array[0])); + try std.testing.expect(array[1].eql(expected_array[1])); + try std.testing.expect(array[2].eql(expected_array[2])); } test "strSplitHelp: overlapping delimiter 1" { @@ -2089,8 +2089,8 @@ test "strSplitHelp: overlapping delimiter 1" { // strings are all small so we ignore freeing the memory try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); } test "strSplitHelp: overlapping delimiter 2" { @@ -2118,9 +2118,9 @@ test "strSplitHelp: overlapping delimiter 2" { // strings are all small so we ignore freeing the memory try std.testing.expectEqual(array.len, expected.len); - try std.testing.expect(array[0].eq(expected[0])); - try std.testing.expect(array[1].eq(expected[1])); - try std.testing.expect(array[2].eq(expected[2])); + try std.testing.expect(array[0].eql(expected[0])); + try std.testing.expect(array[1].eql(expected[1])); + try std.testing.expect(array[2].eql(expected[2])); } test "countSegments: long delimiter" { @@ -2244,7 +2244,7 @@ test "substringUnsafe: start" { const actual = substringUnsafe(str, 0, 3, test_env.getOps()); - try std.testing.expect(RocStr.eq(actual, expected)); + try std.testing.expect(RocStr.eql(actual, expected)); } test "substringUnsafe: middle" { @@ -2259,7 +2259,7 @@ test "substringUnsafe: middle" { const actual = substringUnsafe(str, 1, 3, test_env.getOps()); - try std.testing.expect(RocStr.eq(actual, expected)); + try std.testing.expect(RocStr.eql(actual, expected)); } test "substringUnsafe: end" { @@ -2274,7 +2274,7 @@ test "substringUnsafe: end" { const actual = substringUnsafe(str, 23, 37 - 23, test_env.getOps()); - try std.testing.expect(RocStr.eq(actual, expected)); + try std.testing.expect(RocStr.eql(actual, expected)); } test "startsWith: food starts with foo" { @@ -2381,7 +2381,7 @@ test "RocStr.concat: small concat small" { defer result.decref(test_env.getOps()); - try std.testing.expect(roc_str3.eq(result)); + try std.testing.expect(roc_str3.eql(result)); } test "RocStr.joinWith: result is big" { @@ -2420,7 +2420,7 @@ test "RocStr.joinWith: result is big" { defer result.decref(test_env.getOps()); - try std.testing.expect(roc_result.eq(result)); + try std.testing.expect(roc_result.eql(result)); } test "validateUtf8Bytes: ascii" { @@ -2499,7 +2499,7 @@ test "fromUtf8Lossy: ascii, emoji" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r💖c", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } fn expectErr( @@ -2710,7 +2710,7 @@ test "fromUtf8Lossy: invalid start byte" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r�c", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } test "fromUtf8Lossy: overlong encoding" { @@ -2724,7 +2724,7 @@ test "fromUtf8Lossy: overlong encoding" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r💖�c", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } test "fromUtf8Lossy: expected continuation" { @@ -2738,7 +2738,7 @@ test "fromUtf8Lossy: expected continuation" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r�c", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } test "fromUtf8Lossy: unexpected end" { @@ -2752,7 +2752,7 @@ test "fromUtf8Lossy: unexpected end" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r�", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } test "fromUtf8Lossy: encodes surrogate" { @@ -2771,7 +2771,7 @@ test "fromUtf8Lossy: encodes surrogate" { defer res.decref(test_env.getOps()); const expected = RocStr.fromSlice("r�c", test_env.getOps()); defer expected.decref(test_env.getOps()); - try std.testing.expect(expected.eq(res)); + try std.testing.expect(expected.eql(res)); } test "isWhitespace" { @@ -2794,7 +2794,7 @@ test "withAsciiLowercased: small str" { defer str_result.decref(test_env.getOps()); try std.testing.expect(str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "withAsciiLowercased: non small str" { @@ -2811,7 +2811,7 @@ test "withAsciiLowercased: non small str" { const str_result = strWithAsciiLowercased(original, test_env.getOps()); try std.testing.expect(!str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "withAsciiLowercased: seamless slice" { @@ -2830,7 +2830,7 @@ test "withAsciiLowercased: seamless slice" { const str_result = strWithAsciiLowercased(original, test_env.getOps()); try std.testing.expect(!str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "withAsciiUppercased: small str" { @@ -2847,7 +2847,7 @@ test "withAsciiUppercased: small str" { defer str_result.decref(test_env.getOps()); try std.testing.expect(str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "withAsciiUppercased: non small str" { @@ -2864,7 +2864,7 @@ test "withAsciiUppercased: non small str" { const str_result = strWithAsciiUppercased(original, test_env.getOps()); try std.testing.expect(!str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "withAsciiUppercased: seamless slice" { @@ -2883,7 +2883,7 @@ test "withAsciiUppercased: seamless slice" { const str_result = strWithAsciiUppercased(original, test_env.getOps()); try std.testing.expect(!str_result.isSmallStr()); - try std.testing.expect(str_result.eq(expected)); + try std.testing.expect(str_result.eql(expected)); } test "caselessAsciiEquals: same str" { @@ -2965,7 +2965,7 @@ test "strTrim: empty" { var test_env = TestEnv.init(std.testing.allocator); defer test_env.deinit(); const trimmedEmpty = strTrim(RocStr.empty(), test_env.getOps()); - try std.testing.expect(trimmedEmpty.eq(RocStr.empty())); + try std.testing.expect(trimmedEmpty.eql(RocStr.empty())); } test "strTrim: null byte" { @@ -2987,7 +2987,7 @@ test "strTrim: null byte" { const trimmed = strTrim(original.clone(test_env.getOps()), test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(original.eq(trimmed)); + try std.testing.expect(original.eql(trimmed)); } test "strTrim: blank" { @@ -3000,7 +3000,7 @@ test "strTrim: blank" { const trimmed = strTrim(original, test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(trimmed.eq(RocStr.empty())); + try std.testing.expect(trimmed.eql(RocStr.empty())); } test "strTrim: large to large" { @@ -3021,7 +3021,7 @@ test "strTrim: large to large" { const trimmed = strTrim(original, test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); } test "strTrim: large to small sized slice" { @@ -3043,7 +3043,7 @@ test "strTrim: large to small sized slice" { const trimmed = strTrim(original, test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(!trimmed.isSmallStr()); } @@ -3065,7 +3065,7 @@ test "strTrim: small to small" { const trimmed = strTrim(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(trimmed.isSmallStr()); } @@ -3074,7 +3074,7 @@ test "strTrimStart: empty" { defer test_env.deinit(); const trimmedEmpty = strTrimStart(RocStr.empty(), test_env.getOps()); - try std.testing.expect(trimmedEmpty.eq(RocStr.empty())); + try std.testing.expect(trimmedEmpty.eql(RocStr.empty())); } test "strTrimStart: blank" { @@ -3087,7 +3087,7 @@ test "strTrimStart: blank" { const trimmed = strTrimStart(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(RocStr.empty())); + try std.testing.expect(trimmed.eql(RocStr.empty())); } test "strTrimStart: large to large" { @@ -3108,7 +3108,7 @@ test "strTrimStart: large to large" { const trimmed = strTrimStart(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); } test "strTrimStart: large to small" { @@ -3130,7 +3130,7 @@ test "strTrimStart: large to small" { const trimmed = strTrimStart(original, test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(!trimmed.isSmallStr()); } @@ -3152,7 +3152,7 @@ test "strTrimStart: small to small" { const trimmed = strTrimStart(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(trimmed.isSmallStr()); } @@ -3160,7 +3160,7 @@ test "strTrimEnd: empty" { var test_env = TestEnv.init(std.testing.allocator); defer test_env.deinit(); const trimmedEmpty = strTrimEnd(RocStr.empty(), test_env.getOps()); - try std.testing.expect(trimmedEmpty.eq(RocStr.empty())); + try std.testing.expect(trimmedEmpty.eql(RocStr.empty())); } test "strTrimEnd: blank" { @@ -3172,7 +3172,7 @@ test "strTrimEnd: blank" { const trimmed = strTrimEnd(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(RocStr.empty())); + try std.testing.expect(trimmed.eql(RocStr.empty())); } test "strTrimEnd: large to large" { @@ -3192,7 +3192,7 @@ test "strTrimEnd: large to large" { const trimmed = strTrimEnd(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); } test "strTrimEnd: large to small" { @@ -3214,7 +3214,7 @@ test "strTrimEnd: large to small" { const trimmed = strTrimEnd(original, test_env.getOps()); defer trimmed.decref(test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(!trimmed.isSmallStr()); } @@ -3236,7 +3236,7 @@ test "strTrimEnd: small to small" { const trimmed = strTrimEnd(original, test_env.getOps()); - try std.testing.expect(trimmed.eq(expected)); + try std.testing.expect(trimmed.eql(expected)); try std.testing.expect(trimmed.isSmallStr()); }