diff --git a/src/check/Check.zig b/src/check/Check.zig index d30445bc161..39b6e0e2c22 100644 --- a/src/check/Check.zig +++ b/src/check/Check.zig @@ -419,57 +419,92 @@ fn unify(self: *Self, a: Var, b: Var, env: *Env) std.mem.Allocator.Error!unifier return self.unifyInContext(a, b, env, .none); } -/// Unify two types where `a` is the expected type and `b` is the actual type -/// Accepts a full unifier config for fine-grained control +/// Context builder for unifyBuildingContext that wraps an already-constructed context value. +/// Use this when the context is cheap to build upfront and no lazy evaluation is needed. +const EagerContextBuilder = struct { + ctx: problem.Context, + fn build(self: @This()) std.mem.Allocator.Error!problem.Context { + return self.ctx; + } +}; + +/// Context builder for unifyBuildingContext when checking dot access on a record. +/// Lazily checks whether the field name is actually a method on a nominal type, +/// so the expensive isMethodOnNominalType lookup only runs on unification failure. +const RecordAccessContextBuilder = struct { + checker: *Self, + field_name: Ident.Idx, + field_region: base.Region, + receiver_var: Var, + fn build(self: @This()) std.mem.Allocator.Error!problem.Context { + const is_method = try self.checker.isMethodOnNominalType(self.receiver_var, self.field_name); + return .{ .record_access = .{ + .field_name = self.field_name, + .field_region = self.field_region, + .is_method = is_method, + } }; + } +}; + +/// Unify two types where `a` is the expected type and `b` is the actual type. +/// Delegates to unifyBuildingContext (not unifier.unifyInContext — Check uses unifyRaw +/// directly so it can control when vars are clobbered and problems are recorded). +/// For lazy context construction (e.g. to skip expensive lookups on the success path), +/// use unifyBuildingContext instead. fn unifyInContext(self: *Self, a: Var, b: Var, env: *Env, ctx: problem.Context) std.mem.Allocator.Error!unifier.Result { + return self.unifyBuildingContext(a, b, env, EagerContextBuilder{ .ctx = ctx }); +} + +/// Like unifyInContext, but the context is built lazily via a callback, only if unification +/// fails. This avoids expensive context computation on the success path. +/// +/// `ctx_builder` must have a `build() !problem.Context` method. It is only called on failure, +/// while the type variables are still live (before they are clobbered to `.err`). +fn unifyBuildingContext(self: *Self, a: Var, b: Var, env: *Env, ctx_builder: anytype) std.mem.Allocator.Error!unifier.Result { + comptime { + const T = @TypeOf(ctx_builder); + if (!@hasDecl(T, "build")) @compileError(@typeName(T) ++ " must have a `build() !problem.Context` method"); + } const trace = tracy.trace(@src()); defer trace.end(); - // Unify - const result = try unifier.unifyInContext( + const raw = try unifier.unifyRaw( self.cir, self.types, - &self.problems, &self.snapshots, &self.type_writer, &self.unify_scratch, &self.occurs_scratch, a, b, - ctx, ); - // Set regions and add to the current rank all variables created during unification. - // - // We assign all fresh variables the region of `b` (the "actual" type), since `a` is - // typically the "expected" type from an annotation. This heuristic works well for - // most cases but can be imprecise for deeply nested unifications where fresh variables - // are created for sub-components (e.g., record fields, tag payloads). In those cases, - // error messages may point to the outer expression rather than the specific field. - // - // A more precise solution would track the origin of each fresh variable during - // unification and propagate that back, but the current approach is sufficient for - // typical error reporting scenarios. + // Process fresh vars and deferred constraints regardless of success or failure. const region = self.getRegionAt(b); for (self.unify_scratch.fresh_vars.items.items) |fresh_var| { - // Set the rank const fresh_rank = self.types.resolveVar(fresh_var).desc.rank; try env.var_pool.addVarToRank(fresh_var, fresh_rank); - - // Set the region try self.fillInRegionsThrough(fresh_var); self.setRegionAt(fresh_var, region); } - - // Copy any constraints created during unification into our own array for (self.unify_scratch.deferred_constraints.items.items) |deferred_constraint| { _ = try env.deferred_static_dispatch_constraints.append(self.gpa, deferred_constraint); } - - // Ensure arrays are in sync self.debugAssertArraysInSync(); - return result; + switch (raw) { + .ok => return .ok, + .mismatch => |types_pair| { + // Vars are still live here — build the context before clobbering them. + const context = try ctx_builder.build(); + const problem_idx = try self.problems.appendProblem(self.cir.gpa, .{ .type_mismatch = .{ + .types = types_pair, + .context = context, + } }); + self.types.union_(a, b, .{ .content = .err, .rank = Rank.generalized }); + return .{ .problem = problem_idx }; + }, + } } /// Check if a variable contains an infinite type after solving a definition. @@ -4380,11 +4415,14 @@ fn checkExpr(self: *Self, expr_idx: CIR.Expr.Idx, env: *Env, expected: Expected) .record = .{ .fields = record_field_range, .ext = record_ext_var }, } }, env, expr_region); - // Then, unify the actual receiver type with the expected record - _ = try self.unifyInContext(record_being_accessed, receiver_var, env, .{ .record_access = .{ + // Use unifyBuildingContext so the is_method check only runs on failure, + // while receiver_var is still live (not yet clobbered to .err). + _ = try self.unifyBuildingContext(record_being_accessed, receiver_var, env, RecordAccessContextBuilder{ + .checker = self, .field_name = dot_access.field_name, .field_region = dot_access.field_name_region, - } }); + .receiver_var = receiver_var, + }); _ = try self.unify(expr_var, record_field_var, env); } }, @@ -5933,6 +5971,44 @@ fn checkConstraints(self: *Self, env: *Env) std.mem.Allocator.Error!void { self.constraints.items.clearRetainingCapacity(); } +/// Returns true if `method_ident` is a known method on the nominal type of `receiver_var`. +/// Used to detect when a user writes `receiver.method` (missing parentheses) instead of +/// `receiver.method()`. +fn isMethodOnNominalType(self: *Self, receiver_var: Var, method_ident: Ident.Idx) std.mem.Allocator.Error!bool { + const nominal = switch (self.types.resolveVar(receiver_var).desc.content) { + .structure => |s| switch (s) { + .nominal_type => |n| n, + else => return false, + }, + else => return false, + }; + + const origin_module_ident = nominal.origin_module; + // Find the module env that owns the type's method definitions. + const original_env: *const ModuleEnv = blk: { + // Type is defined in the module we're currently compiling. + if (origin_module_ident == self.builtin_ctx.module_name) break :blk self.cir; + // Type is defined in the Builtin module (and we're not compiling it). + if (origin_module_ident == self.cir.idents.builtin_module) { + break :blk if (self.builtin_ctx.builtin_module) |builtin_env| builtin_env else self.cir; + } + // Type is defined in an imported module; find it by matching idents. + // insertIdent converts the module name string to an Ident.Idx for + // index-based comparison (same approach as checkStaticDispatchConstraints). + for (self.imported_modules) |imported_env| { + const imported_name = if (!imported_env.qualified_module_ident.isNone()) + imported_env.getIdent(imported_env.qualified_module_ident) + else + imported_env.module_name; + const imported_module_ident = try self.cir.insertIdent(base.Ident.for_text(imported_name)); + if (imported_module_ident == origin_module_ident) break :blk imported_env; + } + return false; // Origin module not found; cannot determine if it's a method. + }; + + return original_env.lookupMethodIdentFromEnvConst(self.cir, nominal.ident.ident_idx, method_ident) != null; +} + /// Check static dispatch constraints /// /// Note that new constraints can be added as we are processing. For example: diff --git a/src/check/problem/context.zig b/src/check/problem/context.zig index 3c8e6dfb074..f902dfba14d 100644 --- a/src/check/problem/context.zig +++ b/src/check/problem/context.zig @@ -181,6 +181,9 @@ pub const Context = union(enum) { /// We have to use the real region here, because the field does not /// have its own CIR node field_region: base.Region, + /// True if field_name is actually a method on the receiver type, + /// i.e. the user wrote `receiver.method` instead of `receiver.method()` + is_method: bool = false, }; /// Context for record update type errors diff --git a/src/check/report.zig b/src/check/report.zig index f38421452cb..e0f265efd14 100644 --- a/src/check/report.zig +++ b/src/check/report.zig @@ -224,6 +224,8 @@ pub const ReportBuilder = struct { const ProblemRegion = union(enum) { simple: Region.Idx, focused: struct { outer: Region.Idx, highlight: Region.Idx }, + /// A direct Region value, for when only a Region (not a Region.Idx) is available. + region: Region, }; /// Region for source highlighting - can be either a Region.Idx or a direct Region @@ -279,6 +281,7 @@ pub const ReportBuilder = struct { bytes: []const u8, link: []const u8, ident: Ident.Idx, + ident_call: Ident.Idx, num_ord: u32, num: u32, }, @@ -314,6 +317,15 @@ pub const ReportBuilder = struct { }; } + /// Create a Doc that renders as "name()" — for showing a method call suggestion. + fn identCall(b: Ident.Idx) Doc { + return Doc{ + .type_ = .{ .ident_call = b }, + .preceding_space = true, + .annotation = null, + }; + } + /// Create a Doc containing an ordinal number (1st, 2nd, 3rd, etc.). fn num_ord(b: u32) Doc { return Doc{ @@ -367,6 +379,16 @@ pub const ReportBuilder = struct { try report.document.addReflowingText(ident_bytes); } }, + .ident_call => |i| { + var buf: [512]u8 = undefined; + const with_parens = try std.fmt.bufPrint(&buf, "{s}()", .{builder.can_ir.getIdent(i)}); + const owned = try report.addOwnedString(with_parens); + if (doc.annotation) |annotation| { + try report.document.addAnnotated(owned, annotation); + } else { + try report.document.addReflowingText(owned); + } + }, .num_ord => |ord| { const ord_bytes = try builder.getOrdinalOwned(report, ord); if (doc.annotation) |annotation| { @@ -435,6 +457,7 @@ pub const ReportBuilder = struct { .focused => |ctx| { try self.addFocusedSourceHighlight(&report, ctx.outer, ctx.highlight); }, + .region => |r| try self.addSourceHighlightRegion(&report, r), } try report.document.addLineBreak(); @@ -498,6 +521,7 @@ pub const ReportBuilder = struct { switch (region) { .simple => |region_idx| try self.addSourceHighlight(&report, region_idx), .focused => |ctx| try self.addFocusedSourceHighlight(&report, ctx.outer, ctx.highlight), + .region => |r| try self.addSourceHighlightRegion(&report, r), } try report.document.addLineBreak(); @@ -541,6 +565,7 @@ pub const ReportBuilder = struct { .focused => |ctx| { try self.addFocusedSourceHighlight(&report, ctx.outer, ctx.highlight); }, + .region => |r| try self.addSourceHighlightRegion(&report, r), } // Print static hints @@ -2118,6 +2143,22 @@ pub const ReportBuilder = struct { const region = ProblemRegion{ .simple = regionIdxFrom(types.actual_var) }; switch (record) { .not_a_record => { + if (ctx.is_method) { + return try self.makeCustomReport( + ProblemRegion{ .region = ctx.field_region }, + &.{ + D.ident(ctx.field_name).withAnnotation(.inline_code), + D.bytes("is a method, not a record field. Did you forget the parentheses?"), + }, + &.{ + &.{ + D.bytes("Method calls require parentheses even with no arguments. Use"), + D.identCall(ctx.field_name).withAnnotation(.inline_code), + D.bytes("instead."), + }, + }, + ); + } return try self.makeBadTypeReport( region, &.{D.bytes("This is not a record, so it does not have any fields to access:")}, diff --git a/src/check/test/type_checking_integration.zig b/src/check/test/type_checking_integration.zig index 149449a4223..fcf3a4be74f 100644 --- a/src/check/test/type_checking_integration.zig +++ b/src/check/test/type_checking_integration.zig @@ -1810,6 +1810,30 @@ test "check type - record - access - not a record" { try checkTypesModule(source, .fail, "TYPE MISMATCH"); } +test "check type - record - access - method missing parentheses" { + const source = + \\MyType := [A].{ + \\ my_method = || 42 + \\} + \\ + \\x = MyType.A + \\y = x.my_method + ; + try checkTypesModule(source, .fail_with, + \\**TYPE MISMATCH** + \\`my_method` is a method, not a record field. Did you forget the parentheses? + \\**test:6:6:6:16:** + \\```roc + \\y = x.my_method + \\``` + \\ ^^^^^^^^^^ + \\ + \\Method calls require parentheses even with no arguments. Use `my_method()` instead. + \\ + \\ + ); +} + // record update test "check type - record - update 1" { diff --git a/src/check/unify.zig b/src/check/unify.zig index cf3cc6ed7c3..597c3cc52b1 100644 --- a/src/check/unify.zig +++ b/src/check/unify.zig @@ -11,19 +11,25 @@ //! * numbers (polymorphic & compacted) //! * effect and purity tracking //! -//! The primary entrypoint is `unify`, which takes two type variables (`Var`) and attempts -//! to unify their structure in the given `Store`, using a mutable `Scratch` context -//! to hold intermediate buffers and allocations. +//! Entrypoints: +//! * `unify` — unify two vars with no context (no error reporting) +//! * `unifyInContext` — unify and record a problem with a pre-built context on failure +//! * `unifyRaw` — unify and capture snapshots on failure, without recording the problem +//! or clobbering vars; lets callers build the context lazily before committing +//! +//! All entrypoints take two type variables (`Var`) and attempt to unify their structure +//! in the given `Store`, using a mutable `Scratch` context to hold intermediate buffers +//! and allocations. //! //! The `Scratch` struct is designed to be reused across many unification calls. //! It owns internal scratch buffers for record fields, shared fields, and fresh variables. //! -//! Each call to `unify` will reset the scratch buffer. If the unification succeeds, +//! Each unification call will reset the scratch buffer. If the unification succeeds, //! the caller can access `scratch.fresh_vars` to retrieve all type variables created //! during that unification pass. These fresh variables are useful for later type //! generalization, let-binding, or monomorphization. //! -//! NOTE: Subsequent calls to `unify` will reset `fresh_vars`. It is up to the caller +//! NOTE: Subsequent unification calls will reset `fresh_vars`. It is up to the caller //! to use/store them if necessary. //! //! ### Example @@ -140,6 +146,56 @@ pub fn unify( ); } +/// The raw result of unification: either success, or a type mismatch with snapshots +/// captured before vars are clobbered. Does not include a context. +pub const RawResult = union(enum) { + ok, + mismatch: problem_mod.TypePair, +}; + +/// Runs unification and captures snapshots on failure, but does NOT record the problem, +/// build a context, or clobber vars. This enables lazy context construction on the failure +/// path (e.g. to skip expensive lookups on the success path). +/// +/// Callers must handle the `.mismatch` case by: building the context, recording the +/// problem via `problems.appendProblem`, and clobbering vars via `types.union_`. +pub fn unifyRaw( + module_env: *ModuleEnv, + types: *types_mod.Store, + snapshots: *snapshot_mod.Store, + type_writer: *types_mod.TypeWriter, + unify_scratch: *Scratch, + occurs_scratch: *occurs.Scratch, + /// The "expected" variable + a: Var, + /// The "actual" variable + b: Var, +) std.mem.Allocator.Error!RawResult { + const trace = tracy.trace(@src()); + defer trace.end(); + + unify_scratch.reset(); + + var unifier = Unifier.init(module_env, types, unify_scratch, occurs_scratch); + unifier.unifyGuarded(a, b) catch |err| { + switch (err) { + error.OutOfMemory => return error.OutOfMemory, + error.TypeMismatch => { + const expected_snapshot = try snapshots.snapshotVarForError(types, type_writer, a); + const actual_snapshot = try snapshots.snapshotVarForError(types, type_writer, b); + return .{ .mismatch = .{ + .expected_var = a, + .expected_snapshot = expected_snapshot, + .actual_var = b, + .actual_snapshot = actual_snapshot, + } }; + }, + } + }; + + return .ok; +} + /// Unify two type variables /// /// This function @@ -147,7 +203,9 @@ pub fn unify( /// * Compares variable contents for equality /// * Merges unified variables so 1 is "root" and the other is "redirect" /// -/// This function accepts a context and optional constraint origin var (for better error reporting) +/// Accepts a context for error reporting. For lazy context construction, call +/// unifyRaw directly and handle the mismatch case yourself. Note: Check.zig uses +/// unifyRaw via unifyBuildingContext rather than calling this function. pub fn unifyInContext( module_env: *ModuleEnv, types: *types_mod.Store, @@ -165,42 +223,18 @@ pub fn unifyInContext( const trace = tracy.trace(@src()); defer trace.end(); - // First reset the scratch store - unify_scratch.reset(); - - // Unify - var unifier = Unifier.init(module_env, types, unify_scratch, occurs_scratch); - unifier.unifyGuarded(a, b) catch |err| { - const problem: Problem = blk: { - switch (err) { - error.OutOfMemory => { - return error.OutOfMemory; - }, - error.TypeMismatch => { - const expected_snapshot = try snapshots.snapshotVarForError(types, type_writer, a); - const actual_snapshot = try snapshots.snapshotVarForError(types, type_writer, b); - - break :blk .{ .type_mismatch = .{ - .types = .{ - .expected_var = a, - .expected_snapshot = expected_snapshot, - .actual_var = b, - .actual_snapshot = actual_snapshot, - }, - .context = context, - } }; - }, - } - }; - const problem_idx = try problems.appendProblem(module_env.gpa, problem); - types.union_(a, b, .{ - .content = .err, - .rank = Rank.generalized, - }); - return Result{ .problem = problem_idx }; - }; - - return .ok; + const raw = try unifyRaw(module_env, types, snapshots, type_writer, unify_scratch, occurs_scratch, a, b); + switch (raw) { + .ok => return .ok, + .mismatch => |types_pair| { + const problem_idx = try problems.appendProblem(module_env.gpa, .{ .type_mismatch = .{ + .types = types_pair, + .context = context, + } }); + types.union_(a, b, .{ .content = .err, .rank = Rank.generalized }); + return .{ .problem = problem_idx }; + }, + } } /// A temporary unification context used to unify two type variables within a `Store`.