Merge pull request #8414 from roc-lang/propagate_is_eq

Fix constraints for `==` and `!=`

Author: rtfeldman

src/PROFILING/bench_repeated_check.roc

@@ -1,6 +1,6 @@
 # Minimal test file - original benchmark temporarily disabled
 # See bench_repeated_check_ORIGINAL.roc for the full version
-# TODO: Re-enable once `is_ne` is supported for all numeric types
+# TODO: Re-enable once `!=` (which desugars to is_eq().not()) is fully working
 
 app [main] { pf: platform "../../test/str/platform/main.roc" }
 

src/base/Ident.zig

@@ -6,6 +6,7 @@
 //! in constant time. Storing IDs in each IR instead of strings also uses less memory in the IRs.
 
 const std = @import("std");
+const builtin = @import("builtin");
 const serialization = @import("serialization");
 const collections = @import("collections");
 
@@ -16,6 +17,10 @@ const CompactWriter = collections.CompactWriter;
 
 const Ident = @This();
 
+/// Whether to enable debug store tracking. This adds runtime checks to verify
+/// that Idx values are only looked up in the store that created them.
+const enable_store_tracking = builtin.mode == .Debug;
+
 /// Method name for parsing integers from digit lists - used by numeric literal type checking
 pub const FROM_INT_DIGITS_METHOD_NAME = "from_int_digits";
 /// Method name for parsing decimals from digit lists - used by numeric literal type checking
@@ -105,12 +110,159 @@ pub const Attributes = packed struct(u3) {
     }
 };
 
+/// Debug-only info for store provenance tracking.
+const StoreDebugInfo = struct {
+    store_id: []const u8,
+    known_idxs: std.AutoHashMapUnmanaged(u32, void),
+};
+
+/// Global counter for generating unique store IDs.
+/// This counter survives struct copies because the ID is stored in the Store struct itself.
+/// Using u32 for cross-platform compatibility (wasm32 doesn't support 64-bit atomics).
+var debug_store_id_counter: if (enable_store_tracking) std.atomic.Value(u32) else void =
+    if (enable_store_tracking) std.atomic.Value(u32).init(1) else {};
+
+/// Global map from Store's unique debug_id to debug info.
+/// Protected by a mutex for thread safety.
+var debug_store_map: if (enable_store_tracking) std.AutoHashMapUnmanaged(u32, StoreDebugInfo) else void = if (enable_store_tracking) .{} else {};
+
+/// Mutex protecting the debug_store_map.
+var debug_store_mutex: if (enable_store_tracking) std.Thread.Mutex else void = if (enable_store_tracking) .{} else {};
+
 /// An interner for identifier names.
 pub const Store = struct {
     interner: SmallStringInterner,
     attributes: collections.SafeList(Attributes) = .{},
     next_unique_name: u32 = 0,
 
+    /// Debug-only: unique ID for this store instance.
+    /// This ID is assigned on first insert and survives struct copies.
+    /// 0 means unassigned.
+    debug_id: if (enable_store_tracking) u32 else void = if (enable_store_tracking) 0 else {},
+
+    /// Debug-only: get or assign a unique ID for this store.
+    fn getOrAssignDebugId(self: *Store, src: std.builtin.SourceLocation) u32 {
+        if (enable_store_tracking) {
+            if (self.debug_id == 0) {
+                // If this store already has idents (e.g., deserialized), we can't
+                // fully track it because existing idents weren't registered.
+                // Keep debug_id at 0 to skip verification for this store.
+                if (self.interner.entry_count > 0) {
+                    return 0;
+                }
+
+                // Assign a new unique ID
+                self.debug_id = debug_store_id_counter.fetchAdd(1, .monotonic);
+
+                // Register in the global map with source location info
+                const store_id = std.fmt.allocPrint(std.heap.page_allocator, "{s}:{d}:{d}", .{
+                    src.file,
+                    src.line,
+                    src.column,
+                }) catch "unknown";
+
+                debug_store_map.put(std.heap.page_allocator, self.debug_id, .{
+                    .store_id = store_id,
+                    .known_idxs = .{},
+                }) catch {};
+            }
+            return self.debug_id;
+        } else {
+            return 0;
+        }
+    }
+
+    /// Debug-only: unregister this store from the global debug map.
+    fn unregisterFromTracking(self: *Store) void {
+        if (enable_store_tracking) {
+            if (self.debug_id == 0) return; // Never registered
+
+            debug_store_mutex.lock();
+            defer debug_store_mutex.unlock();
+
+            if (debug_store_map.fetchRemove(self.debug_id)) |entry| {
+                // Free the heap-allocated store_id (if it's not the static "unknown" string)
+                if (entry.value.store_id.ptr != @as([*]const u8, "unknown".ptr)) {
+                    std.heap.page_allocator.free(entry.value.store_id);
+                }
+                // Copy the known_idxs to make it mutable for deinit
+                var known_idxs = entry.value.known_idxs;
+                known_idxs.deinit(std.heap.page_allocator);
+            }
+        }
+    }
+
+    /// Debug-only: track an Idx as belonging to this store.
+    fn trackIdx(self: *Store, idx: Idx, src: std.builtin.SourceLocation) void {
+        if (enable_store_tracking) {
+            debug_store_mutex.lock();
+            defer debug_store_mutex.unlock();
+
+            const debug_id = self.getOrAssignDebugId(src);
+            if (debug_store_map.getPtr(debug_id)) |info| {
+                // We don't fail on OOM in debug tracking - just skip tracking
+                info.known_idxs.put(std.heap.page_allocator, @bitCast(idx), {}) catch {};
+            }
+        }
+    }
+
+    /// Debug-only: verify an Idx belongs to this store.
+    fn verifyIdx(self: *const Store, idx: Idx) void {
+        if (enable_store_tracking) {
+            if (self.debug_id == 0) {
+                // Store was never registered (e.g., deserialized store).
+                // Skip verification.
+                return;
+            }
+
+            debug_store_mutex.lock();
+            defer debug_store_mutex.unlock();
+
+            const info = debug_store_map.get(self.debug_id) orelse {
+                // Store not in map (shouldn't happen if debug_id != 0)
+                return;
+            };
+
+            const idx_bits: u32 = @bitCast(idx);
+            if (!info.known_idxs.contains(idx_bits)) {
+                std.debug.panic(
+                    "Ident.Idx lookup in wrong store: Idx {d} (0x{x}) not found in store '{s}' (debug_id={d}). " ++
+                        "This Idx was created by a different store.",
+                    .{ idx.idx, idx_bits, info.store_id, self.debug_id },
+                );
+            }
+        }
+    }
+
+    /// Check if an Idx was created by this store.
+    /// In debug builds with store tracking enabled, this checks the known_idxs set.
+    /// In release builds or when tracking is disabled, this returns true (assumes valid).
+    /// Use this to determine which store to use for lookups when idents may come from
+    /// multiple sources (e.g., during type unification with builtins).
+    pub fn containsIdx(self: *const Store, idx: Idx) bool {
+        if (enable_store_tracking) {
+            if (self.debug_id == 0) {
+                // Store was never registered (e.g., deserialized store).
+                // Can't verify, assume true.
+                return true;
+            }
+
+            debug_store_mutex.lock();
+            defer debug_store_mutex.unlock();
+
+            const info = debug_store_map.get(self.debug_id) orelse {
+                // Store not in map
+                return true;
+            };
+
+            const idx_bits: u32 = @bitCast(idx);
+            return info.known_idxs.contains(idx_bits);
+        } else {
+            // No tracking, can't determine - assume true
+            return true;
+        }
+    }
+
     /// Serialized representation of an Ident.Store
     /// Uses extern struct to guarantee consistent field layout across optimization levels.
     pub const Serialized = extern struct {
@@ -142,11 +294,14 @@ pub const Store = struct {
                 .next_unique_name = self.next_unique_name,
             };
 
+            // Note: We don't register deserialized stores for debug tracking.
+            // This is fine because the debug tracking is meant to catch bugs during fresh compilation.
+
             return store;
         }
     };
 
-    /// Initialize the memory for an `Ident.Store` with a specific capaicty.
+    /// Initialize the memory for an `Ident.Store` with a specific capacity.
     pub fn initCapacity(gpa: std.mem.Allocator, capacity: usize) std.mem.Allocator.Error!Store {
         return .{
             .interner = try SmallStringInterner.initCapacity(gpa, capacity),
@@ -157,12 +312,30 @@ pub const Store = struct {
     pub fn deinit(self: *Store, gpa: std.mem.Allocator) void {
         self.interner.deinit(gpa);
         self.attributes.deinit(gpa);
+        self.unregisterFromTracking();
     }
 
     /// Insert a new identifier into the store.
     pub fn insert(self: *Store, gpa: std.mem.Allocator, ident: Ident) std.mem.Allocator.Error!Idx {
         const idx = try self.interner.insert(gpa, ident.raw_text);
 
+        const result = Idx{
+            .attributes = ident.attributes,
+            .idx = @as(u29, @intCast(@intFromEnum(idx))),
+        };
+
+        self.trackIdx(result, @src());
+
+        return result;
+    }
+
+    /// Look up an identifier in the store without inserting.
+    /// Returns the index if found, null if not found.
+    /// Unlike insert, this never modifies the store (no resize, no insertion).
+    /// Useful for deserialized stores that cannot be grown.
+    pub fn lookup(self: *const Store, ident: Ident) ?Idx {
+        const idx = self.interner.lookup(ident.raw_text) orelse return null;
+
         return Idx{
             .attributes = ident.attributes,
             .idx = @as(u29, @intCast(@intFromEnum(idx))),
@@ -211,14 +384,19 @@ pub const Store = struct {
 
         _ = try self.attributes.append(gpa, attributes);
 
-        return Idx{
+        const result = Idx{
             .attributes = attributes,
             .idx = @truncate(@intFromEnum(idx)),
         };
+
+        self.trackIdx(result, @src());
+
+        return result;
     }
 
     /// Get the text for an identifier.
     pub fn getText(self: *const Store, idx: Idx) []u8 {
+        self.verifyIdx(idx);
         return self.interner.getText(@enumFromInt(@as(u32, idx.idx)));
     }
 

src/base/SmallStringInterner.zig

@@ -25,6 +25,9 @@ bytes: collections.SafeList(u8) = .{},
 hash_table: collections.SafeList(Idx) = .{},
 /// The current number of entries in the hash table.
 entry_count: u32 = 0,
+/// Debug-only flag to catch invalid inserts into deserialized interners.
+/// Deserialized interners should never have inserts - if they do, it's a bug.
+supports_inserts: if (std.debug.runtime_safety) bool else void = if (std.debug.runtime_safety) true else {},
 
 /// A unique index for a deduped string in this interner.
 pub const Idx = enum(u32) {
@@ -65,7 +68,13 @@ pub fn initCapacity(gpa: std.mem.Allocator, capacity: usize) std.mem.Allocator.E
 
 /// Free all memory consumed by this interner.
 /// Will invalidate all slices referencing the interner.
+/// NOTE: Do NOT call deinit on deserialized interners - their memory is owned by the deserialization buffer.
 pub fn deinit(self: *SmallStringInterner, gpa: std.mem.Allocator) void {
+    if (std.debug.runtime_safety) {
+        if (!self.supports_inserts) {
+            @panic("deinit called on deserialized interner - memory is owned by deserialization buffer");
+        }
+    }
     self.bytes.deinit(gpa);
     self.hash_table.deinit(gpa);
 }
@@ -131,30 +140,46 @@ fn resizeHashTable(self: *SmallStringInterner, gpa: std.mem.Allocator) std.mem.A
 
 /// Add a string to this interner, returning a unique, serial index.
 pub fn insert(self: *SmallStringInterner, gpa: std.mem.Allocator, string: []const u8) std.mem.Allocator.Error!Idx {
-    // Check if we need to resize the hash table (when 80% full = entry_count * 5 >= hash_table.len() * 4)
-    if (self.entry_count * 5 >= self.hash_table.len() * 4) {
-        try self.resizeHashTable(gpa);
-    }
-
-    // Find the string or the slot where it should be inserted
+    // First check if string exists
     const result = self.findStringOrSlot(string);
 
     if (result.idx) |existing_idx| {
         // String already exists
         return existing_idx;
-    } else {
-        // String doesn't exist, add it to bytes
-        const new_offset: Idx = @enumFromInt(self.bytes.len());
-
-        _ = try self.bytes.appendSlice(gpa, string);
-        _ = try self.bytes.append(gpa, 0);
+    }
 
-        // Add to hash table
-        self.hash_table.items.items[@intCast(result.slot)] = new_offset;
-        self.entry_count += 1;
+    // Debug assertion: deserialized interners should never need new inserts.
+    // If this fires, it's a bug - all idents should already exist in the interner.
+    if (std.debug.runtime_safety) {
+        if (!self.supports_inserts) {
+            @panic("insert called on deserialized interner - this is a bug, ident should already exist");
+        }
+    }
 
-        return new_offset;
+    // Check if resize needed.
+    if (self.entry_count * 5 >= self.hash_table.len() * 4) {
+        try self.resizeHashTable(gpa);
+        // After resize, need to find the slot again
+        const new_result = self.findStringOrSlot(string);
+        return self.insertAt(gpa, string, new_result.slot);
     }
+
+    // No resize needed, insert at the found slot
+    return self.insertAt(gpa, string, result.slot);
+}
+
+/// Insert a string at a specific slot (internal helper).
+fn insertAt(self: *SmallStringInterner, gpa: std.mem.Allocator, string: []const u8, slot: u64) std.mem.Allocator.Error!Idx {
+    const new_offset: Idx = @enumFromInt(self.bytes.len());
+
+    _ = try self.bytes.appendSlice(gpa, string);
+    _ = try self.bytes.append(gpa, 0);
+
+    // Add to hash table
+    self.hash_table.items.items[@intCast(slot)] = new_offset;
+    self.entry_count += 1;
+
+    return new_offset;
 }
 
 /// Check if a string is already interned in this interner, used for generating unique names.
@@ -163,6 +188,14 @@ pub fn contains(self: *const SmallStringInterner, string: []const u8) bool {
     return result.idx != null;
 }
 
+/// Look up a string in this interner and return its index if found.
+/// Unlike insert, this never modifies the interner (no resize, no insertion).
+/// Useful for deserialized interners that cannot be grown.
+pub fn lookup(self: *const SmallStringInterner, string: []const u8) ?Idx {
+    const result = self.findStringOrSlot(string);
+    return result.idx;
+}
+
 /// Get a reference to the text for an interned string.
 pub fn getText(self: *const SmallStringInterner, idx: Idx) []u8 {
     const bytes_slice = self.bytes.items.items;
@@ -209,6 +242,8 @@ pub const Serialized = extern struct {
     bytes: collections.SafeList(u8).Serialized,
     hash_table: collections.SafeList(Idx).Serialized,
     entry_count: u32,
+    /// Padding to maintain struct alignment
+    _padding: u32 = 0,
 
     /// Serialize a SmallStringInterner into this Serialized struct, appending data to the writer
     pub fn serialize(
@@ -223,6 +258,7 @@ pub const Serialized = extern struct {
         try self.hash_table.serialize(&interner.hash_table, allocator, writer);
         // Copy simple values directly
         self.entry_count = interner.entry_count;
+        self._padding = 0;
     }
 
     /// Deserialize this Serialized struct into a SmallStringInterner
@@ -234,6 +270,8 @@ pub const Serialized = extern struct {
             .bytes = self.bytes.deserialize(offset).*,
             .hash_table = self.hash_table.deserialize(offset).*,
             .entry_count = self.entry_count,
+            // Debug-only: mark as not supporting inserts - deserialized interners should never need new idents
+            .supports_inserts = if (std.debug.runtime_safety) false else {},
         };
 
         return interner;