From a7f3c2eab427397846f619dba7abb72378ac2ce9 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Sun, 23 Jul 2023 17:19:50 -0700 Subject: [PATCH 1/3] InternPool: fix coerced func hash/eql same as uncoerced Since the same Key.Func data structure is used for coerced function bodies as well as uncoerced function bodies, there is danger of them being hashed and equality-checked as the same. When that happens, the type of a function body value might be wrong, causing lots of problems. In this instance, it causes an assertion failure. This commit fixes it by introducing an `uncoerced_ty` field which is different than `ty` in the case of `func_coerced` and is used to differentiate when doing hashing and equality checking. I have a new behavior test to cover this bug, but it revealed *another* bug at the same time, so I will fix it in the next commit and and add the new test therein. --- src/InternPool.zig | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/InternPool.zig b/src/InternPool.zig index b8eaafceab93da422e26683f0d3e3fed1ae01907..0e9e1d45594920a31ea15e871f6b42f3c40caf79 100644 --- a/src/InternPool.zig +++ b/src/InternPool.zig @@ -594,6 +594,13 @@ pub const Key = union(enum) { /// In the case of a generic function, this type will potentially have fewer parameters /// than the generic owner's type, because the comptime parameters will be deleted. ty: Index, + /// If this is a function body that has been coerced to a different type, for example + /// ``` + /// fn f2() !void {} + /// const f: fn()anyerror!void = f2; + /// ``` + /// then it contains the original type of the function body. + uncoerced_ty: Index, /// Index into extra array of the `FuncAnalysis` corresponding to this function. /// Used for mutating that data. analysis_extra_index: u32, @@ -990,11 +997,15 @@ pub const Key = union(enum) { // otherwise we would get false negatives for interning generic // function instances which have inferred error sets. - if (func.generic_owner == .none and func.resolved_error_set_extra_index == 0) - return Hash.hash(seed, asBytes(&func.owner_decl) ++ asBytes(&func.ty)); + if (func.generic_owner == .none and func.resolved_error_set_extra_index == 0) { + const bytes = asBytes(&func.owner_decl) ++ asBytes(&func.ty) ++ + [1]u8{@intFromBool(func.uncoerced_ty == func.ty)}; + return Hash.hash(seed, bytes); + } var hasher = Hash.init(seed); std.hash.autoHash(&hasher, func.generic_owner); + std.hash.autoHash(&hasher, func.uncoerced_ty == func.ty); for (func.comptime_args.get(ip)) |arg| std.hash.autoHash(&hasher, arg); if (func.resolved_error_set_extra_index == 0) { std.hash.autoHash(&hasher, func.ty); @@ -1122,6 +1133,12 @@ pub const Key = union(enum) { )) return false; } + if ((a_info.ty == a_info.uncoerced_ty) != + (b_info.ty == b_info.uncoerced_ty)) + { + return false; + } + if (a_info.ty == b_info.ty) return true; @@ -3371,6 +3388,7 @@ fn extraFuncDecl(ip: *const InternPool, extra_index: u32) Key.Func { const func_decl = ip.extraDataTrail(P, extra_index); return .{ .ty = func_decl.data.ty, + .uncoerced_ty = func_decl.data.ty, .analysis_extra_index = extra_index + std.meta.fieldIndex(P, "analysis").?, .zir_body_inst_extra_index = extra_index + std.meta.fieldIndex(P, "zir_body_inst").?, .resolved_error_set_extra_index = if (func_decl.data.analysis.inferred_error_set) func_decl.end else 0, @@ -3392,6 +3410,7 @@ fn extraFuncInstance(ip: *const InternPool, extra_index: u32) Key.Func { const func_decl = ip.funcDeclInfo(fi.data.generic_owner); return .{ .ty = fi.data.ty, + .uncoerced_ty = fi.data.ty, .analysis_extra_index = extra_index + std.meta.fieldIndex(P, "analysis").?, .zir_body_inst_extra_index = func_decl.zir_body_inst_extra_index, .resolved_error_set_extra_index = if (fi.data.analysis.inferred_error_set) fi.end else 0, -- 2.54.0 From cc6964c5dc7a4d4c3081db06697b8ba0d7900b85 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Sun, 23 Jul 2023 17:46:50 -0700 Subject: [PATCH 2/3] InternPool: add func_coerced handling to funcIesResolved --- src/InternPool.zig | 11 +++++++++++ test/behavior/generics.zig | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/InternPool.zig b/src/InternPool.zig index 0e9e1d45594920a31ea15e871f6b42f3c40caf79..8a160d9e39bdfd1ad15f8591f0d7578a78809b14 100644 --- a/src/InternPool.zig +++ b/src/InternPool.zig @@ -7069,6 +7069,17 @@ pub fn funcIesResolved(ip: *const InternPool, func_index: Index) *Index { const extra_index = switch (tags[@intFromEnum(func_index)]) { .func_decl => func_start + @typeInfo(Tag.FuncDecl).Struct.fields.len, .func_instance => func_start + @typeInfo(Tag.FuncInstance).Struct.fields.len, + .func_coerced => i: { + const uncoerced_func_index: Index = @enumFromInt(ip.extra.items[ + func_start + std.meta.fieldIndex(Tag.FuncCoerced, "func").? + ]); + const uncoerced_func_start = datas[@intFromEnum(uncoerced_func_index)]; + break :i switch (tags[@intFromEnum(uncoerced_func_index)]) { + .func_decl => uncoerced_func_start + @typeInfo(Tag.FuncDecl).Struct.fields.len, + .func_instance => uncoerced_func_start + @typeInfo(Tag.FuncInstance).Struct.fields.len, + else => unreachable, + }; + }, else => unreachable, }; return @ptrCast(&ip.extra.items[extra_index]); diff --git a/test/behavior/generics.zig b/test/behavior/generics.zig index a8a7d3c0ed450efa0adac60cc4d653090dff62c4..095461522331d5da0f315cc7a1d0bb7a3db153bc 100644 --- a/test/behavior/generics.zig +++ b/test/behavior/generics.zig @@ -456,3 +456,23 @@ test "return type of generic function is function pointer" { try expect(null == S.b(void)); } + +test "coerced function body has inequal value with its uncoerced body" { + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; + + const S = struct { + const A = B(i32, c); + fn c() !i32 { + return 1234; + } + fn B(comptime T: type, comptime d: ?fn () anyerror!T) type { + return struct { + fn do() T { + return d.?() catch @panic("fail"); + } + }; + } + }; + try expect(S.A.do() == 1234); +} -- 2.54.0 From d92cca9324e71705af9134de70e1e995ba8aa57a Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Sun, 23 Jul 2023 18:39:58 -0700 Subject: [PATCH 3/3] InternPool: add func_coerced handling to getFuncInstanceIes Oops, there was a missing call to `unwrapCoercedFunc`. --- src/InternPool.zig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/InternPool.zig b/src/InternPool.zig index 8a160d9e39bdfd1ad15f8591f0d7578a78809b14..0c01c9988c47ca44e3b29cf381a06aea4342d98f 100644 --- a/src/InternPool.zig +++ b/src/InternPool.zig @@ -4819,6 +4819,8 @@ pub fn getFuncInstanceIes( assert(arg.bare_return_type != .none); for (arg.param_types) |param_type| assert(param_type != .none); + const generic_owner = unwrapCoercedFunc(ip, arg.generic_owner); + // The strategy here is to add the function decl unconditionally, then to // ask if it already exists, and if so, revert the lengths of the mutated // arrays. This is similar to what `getOrPutTrailingString` does. @@ -4854,7 +4856,7 @@ pub fn getFuncInstanceIes( .owner_decl = undefined, .ty = func_ty, .branch_quota = 0, - .generic_owner = arg.generic_owner, + .generic_owner = generic_owner, }); ip.extra.appendAssumeCapacity(@intFromEnum(Index.none)); // resolved error set ip.extra.appendSliceAssumeCapacity(@ptrCast(arg.comptime_args)); @@ -4927,7 +4929,7 @@ pub fn getFuncInstanceIes( return finishFuncInstance( ip, gpa, - arg.generic_owner, + generic_owner, func_index, func_extra_index, arg.generation, -- 2.54.0