| author |  Andrew Kelley <andrew@ziglang.org> 2022-04-14 00:36:54-07:00 |
| committer | Andrew Kelley <andrew@ziglang.org> 2022-04-14 06:08:28-07:00 |
| log | 2a00df9c091498268b58dd671f646a5590439b7a |
| tree | d1fe5b80b9180bd32a75c13e9d3f9ca7700817f3 |
| parent | 9b82e7f558d5aa66ac1dc285af2345d46cc3d4d6 |
Sema: fix generic instantiation false negatives
The problem was that types of non-anytype parameters were being included
as part of the check to see if generic function instantiations were
equal. Now, Module.Fn additionally stores the information for whether each
parameter is anytype or not. `generic_poison` cannot be used to signal
this because the type is still needed for comptime arguments; in such
case the type will not be present in the newly generated function
prototype.
This presented one additional challenge: we need to compare equality of
two values where one of them is post-coercion and the other is not. So
we make some minor adjustments to `Type.eql` to support this. I think
this small complexity tradeoff is worth it because it means the compiler
does much less work on the hot path that a generic function is called
and there is already an existing matching instantiation.
closes #111464 files changed, 178 insertions(+), 66 deletions(-)
test- behavior
src/Module.zig+28-14
| ... | ... | @@ -1394,7 +1394,15 @@ pub const Fn = struct { |
| 1394 | 1394 | /// there is a `TypedValue` here for each parameter of the function. |
| 1395 | 1395 | /// Non-comptime parameters are marked with a `generic_poison` for the value. |
| 1396 | 1396 | /// Non-anytype parameters are marked with a `generic_poison` for the type. |
| 1397 | comptime_args: ?[*]TypedValue = null, | |
| 1397 | /// These never have .generic_poison for the Type | |
| 1398 | /// because the Type is needed to pass to `Type.eql` and for inserting comptime arguments | |
| 1399 | /// into the inst_map when analyzing the body of a generic function instantiation. | |
| 1400 | /// Instead, the is_anytype knowledge is communicated via `anytype_args`. | |
| 1401 | comptime_args: ?[*]TypedValue, | |
| 1402 | /// When comptime_args is null, this is undefined. Otherwise, this flags each | |
| 1403 | /// parameter and tells whether it is anytype. | |
| 1404 | /// TODO apply the same enhancement for param_names below to this field. | |
| 1405 | anytype_args: [*]bool, | |
| 1398 | 1406 | /// The ZIR instruction that is a function instruction. Use this to find |
| 1399 | 1407 | /// the body. We store this rather than the body directly so that when ZIR |
| 1400 | 1408 | /// is regenerated on update(), we can map this to the new corresponding |
| ... | ... | @@ -4782,18 +4790,24 @@ pub fn analyzeFnBody(mod: *Module, decl: *Decl, func: *Fn, arena: Allocator) Sem |
| 4782 | 4790 | |
| 4783 | 4791 | else => continue, |
| 4784 | 4792 | }; |
| 4785 | if (func.comptime_args) |comptime_args| { | |
| 4793 | ||
| 4794 | const param_ty = if (func.comptime_args) |comptime_args| t: { | |
| 4786 | 4795 | const arg_tv = comptime_args[total_param_index]; |
| 4787 | if (arg_tv.val.tag() != .generic_poison) { | |
| 4788 | // We have a comptime value for this parameter. | |
| 4789 | const arg = try sema.addConstant(arg_tv.ty, arg_tv.val); | |
| 4790 | sema.inst_map.putAssumeCapacityNoClobber(inst, arg); | |
| 4791 | total_param_index += 1; | |
| 4792 | continue; | |
| 4793 | } | |
| 4794 | } | |
| 4795 | const param_type = fn_ty_info.param_types[runtime_param_index]; | |
| 4796 | const opt_opv = sema.typeHasOnePossibleValue(&inner_block, param.src, param_type) catch |err| switch (err) { | |
| 4796 | ||
| 4797 | const arg_val = if (arg_tv.val.tag() != .generic_poison) | |
| 4798 | arg_tv.val | |
| 4799 | else if (arg_tv.ty.onePossibleValue()) |opv| | |
| 4800 | opv | |
| 4801 | else | |
| 4802 | break :t arg_tv.ty; | |
| 4803 | ||
| 4804 | const arg = try sema.addConstant(arg_tv.ty, arg_val); | |
| 4805 | sema.inst_map.putAssumeCapacityNoClobber(inst, arg); | |
| 4806 | total_param_index += 1; | |
| 4807 | continue; | |
| 4808 | } else fn_ty_info.param_types[runtime_param_index]; | |
| 4809 | ||
| 4810 | const opt_opv = sema.typeHasOnePossibleValue(&inner_block, param.src, param_ty) catch |err| switch (err) { | |
| 4797 | 4811 | error.NeededSourceLocation => unreachable, |
| 4798 | 4812 | error.GenericPoison => unreachable, |
| 4799 | 4813 | error.ComptimeReturn => unreachable, |
| ... | ... | @@ -4801,7 +4815,7 @@ pub fn analyzeFnBody(mod: *Module, decl: *Decl, func: *Fn, arena: Allocator) Sem |
| 4801 | 4815 | else => |e| return e, |
| 4802 | 4816 | }; |
| 4803 | 4817 | if (opt_opv) |opv| { |
| 4804 | const arg = try sema.addConstant(param_type, opv); | |
| 4818 | const arg = try sema.addConstant(param_ty, opv); | |
| 4805 | 4819 | sema.inst_map.putAssumeCapacityNoClobber(inst, arg); |
| 4806 | 4820 | total_param_index += 1; |
| 4807 | 4821 | runtime_param_index += 1; |
| ... | ... | @@ -4811,7 +4825,7 @@ pub fn analyzeFnBody(mod: *Module, decl: *Decl, func: *Fn, arena: Allocator) Sem |
| 4811 | 4825 | inner_block.instructions.appendAssumeCapacity(arg_index); |
| 4812 | 4826 | sema.air_instructions.appendAssumeCapacity(.{ |
| 4813 | 4827 | .tag = .arg, |
| 4814 | .data = .{ .ty = param_type }, | |
| 4828 | .data = .{ .ty = param_ty }, | |
| 4815 | 4829 | }); |
| 4816 | 4830 | sema.inst_map.putAssumeCapacityNoClobber(inst, Air.indexToRef(arg_index)); |
| 4817 | 4831 | total_param_index += 1; |
src/Sema.zig+112-51
| ... | ... | @@ -4707,6 +4707,8 @@ const GenericCallAdapter = struct { |
| 4707 | 4707 | generic_fn: *Module.Fn, |
| 4708 | 4708 | precomputed_hash: u64, |
| 4709 | 4709 | func_ty_info: Type.Payload.Function.Data, |
| 4710 | /// Unlike comptime_args, the Type here is not always present. | |
| 4711 | /// .generic_poison is used to communicate non-anytype parameters. | |
| 4710 | 4712 | comptime_tvs: []const TypedValue, |
| 4711 | 4713 | target: std.Target, |
| 4712 | 4714 | |
| ... | ... | @@ -4719,20 +4721,29 @@ const GenericCallAdapter = struct { |
| 4719 | 4721 | |
| 4720 | 4722 | const other_comptime_args = other_key.comptime_args.?; |
| 4721 | 4723 | for (other_comptime_args[0..ctx.func_ty_info.param_types.len]) |other_arg, i| { |
| 4722 | if (other_arg.ty.tag() != .generic_poison) { | |
| 4723 | // anytype parameter | |
| 4724 | if (!other_arg.ty.eql(ctx.comptime_tvs[i].ty, ctx.target)) { | |
| 4724 | const this_arg = ctx.comptime_tvs[i]; | |
| 4725 | const this_is_comptime = this_arg.val.tag() != .generic_poison; | |
| 4726 | const other_is_comptime = other_arg.val.tag() != .generic_poison; | |
| 4727 | const this_is_anytype = this_arg.ty.tag() != .generic_poison; | |
| 4728 | const other_is_anytype = other_key.anytype_args[i]; | |
| 4729 | ||
| 4730 | if (other_is_anytype != this_is_anytype) return false; | |
| 4731 | if (other_is_comptime != this_is_comptime) return false; | |
| 4732 | ||
| 4733 | if (this_is_anytype) { | |
| 4734 | // Both are anytype parameters. | |
| 4735 | if (!this_arg.ty.eql(other_arg.ty, ctx.target)) { | |
| 4725 | 4736 | return false; |
| 4726 | 4737 | } |
| 4727 | } | |
| 4728 | if (other_arg.val.tag() != .generic_poison) { | |
| 4729 | // comptime parameter | |
| 4730 | if (ctx.comptime_tvs[i].val.tag() == .generic_poison) { | |
| 4731 | // No match because the instantiation has a comptime parameter | |
| 4732 | // but the callsite does not. | |
| 4733 | return false; | |
| 4738 | if (this_is_comptime) { | |
| 4739 | // Both are comptime and anytype parameters with matching types. | |
| 4740 | if (!this_arg.val.eql(other_arg.val, other_arg.ty, ctx.target)) { | |
| 4741 | return false; | |
| 4742 | } | |
| 4734 | 4743 | } |
| 4735 | if (!other_arg.val.eql(ctx.comptime_tvs[i].val, other_arg.ty, ctx.target)) { | |
| 4744 | } else if (this_is_comptime) { | |
| 4745 | // Both are comptime parameters but not anytype parameters. | |
| 4746 | if (!this_arg.val.eql(other_arg.val, other_arg.ty, ctx.target)) { | |
| 4736 | 4747 | return false; |
| 4737 | 4748 | } |
| 4738 | 4749 | } |
| ... | ... | @@ -5227,28 +5238,61 @@ fn instantiateGenericCall( |
| 5227 | 5238 | const comptime_tvs = try sema.arena.alloc(TypedValue, func_ty_info.param_types.len); |
| 5228 | 5239 | const target = sema.mod.getTarget(); |
| 5229 | 5240 | |
| 5230 | for (func_ty_info.param_types) |param_ty, i| { | |
| 5231 | const is_comptime = func_ty_info.paramIsComptime(i); | |
| 5232 | if (is_comptime) { | |
| 5233 | const arg_src = call_src; // TODO better source location | |
| 5234 | const casted_arg = try sema.coerce(block, param_ty, uncasted_args[i], arg_src); | |
| 5235 | if (try sema.resolveMaybeUndefVal(block, arg_src, casted_arg)) |arg_val| { | |
| 5236 | if (param_ty.tag() != .generic_poison) { | |
| 5237 | arg_val.hash(param_ty, &hasher, target); | |
| 5241 | { | |
| 5242 | var i: usize = 0; | |
| 5243 | for (fn_info.param_body) |inst| { | |
| 5244 | var is_comptime = false; | |
| 5245 | var is_anytype = false; | |
| 5246 | switch (zir_tags[inst]) { | |
| 5247 | .param => { | |
| 5248 | is_comptime = func_ty_info.paramIsComptime(i); | |
| 5249 | }, | |
| 5250 | .param_comptime => { | |
| 5251 | is_comptime = true; | |
| 5252 | }, | |
| 5253 | .param_anytype => { | |
| 5254 | is_anytype = true; | |
| 5255 | is_comptime = func_ty_info.paramIsComptime(i); | |
| 5256 | }, | |
| 5257 | .param_anytype_comptime => { | |
| 5258 | is_anytype = true; | |
| 5259 | is_comptime = true; | |
| 5260 | }, | |
| 5261 | else => continue, | |
| 5262 | } | |
| 5263 | ||
| 5264 | if (is_comptime) { | |
| 5265 | const arg_src = call_src; // TODO better source location | |
| 5266 | const arg_ty = sema.typeOf(uncasted_args[i]); | |
| 5267 | const arg_val = try sema.resolveValue(block, arg_src, uncasted_args[i]); | |
| 5268 | arg_val.hash(arg_ty, &hasher, target); | |
| 5269 | if (is_anytype) { | |
| 5270 | arg_ty.hashWithHasher(&hasher, target); | |
| 5271 | comptime_tvs[i] = .{ | |
| 5272 | .ty = arg_ty, | |
| 5273 | .val = arg_val, | |
| 5274 | }; | |
| 5275 | } else { | |
| 5276 | comptime_tvs[i] = .{ | |
| 5277 | .ty = Type.initTag(.generic_poison), | |
| 5278 | .val = arg_val, | |
| 5279 | }; | |
| 5238 | 5280 | } |
| 5281 | } else if (is_anytype) { | |
| 5282 | const arg_ty = sema.typeOf(uncasted_args[i]); | |
| 5283 | arg_ty.hashWithHasher(&hasher, target); | |
| 5239 | 5284 | comptime_tvs[i] = .{ |
| 5240 | // This will be different than `param_ty` in the case of `generic_poison`. | |
| 5241 | .ty = sema.typeOf(casted_arg), | |
| 5242 | .val = arg_val, | |
| 5285 | .ty = arg_ty, | |
| 5286 | .val = Value.initTag(.generic_poison), | |
| 5243 | 5287 | }; |
| 5244 | 5288 | } else { |
| 5245 | return sema.failWithNeededComptime(block, arg_src); | |
| 5289 | comptime_tvs[i] = .{ | |
| 5290 | .ty = Type.initTag(.generic_poison), | |
| 5291 | .val = Value.initTag(.generic_poison), | |
| 5292 | }; | |
| 5246 | 5293 | } |
| 5247 | } else { | |
| 5248 | comptime_tvs[i] = .{ | |
| 5249 | .ty = sema.typeOf(uncasted_args[i]), | |
| 5250 | .val = Value.initTag(.generic_poison), | |
| 5251 | }; | |
| 5294 | ||
| 5295 | i += 1; | |
| 5252 | 5296 | } |
| 5253 | 5297 | } |
| 5254 | 5298 | |
| ... | ... | @@ -5411,19 +5455,48 @@ fn instantiateGenericCall( |
| 5411 | 5455 | errdefer new_func.deinit(gpa); |
| 5412 | 5456 | assert(new_func == new_module_func); |
| 5413 | 5457 | |
| 5458 | const anytype_args = try new_decl_arena_allocator.alloc(bool, func_ty_info.param_types.len); | |
| 5459 | new_func.anytype_args = anytype_args.ptr; | |
| 5414 | 5460 | arg_i = 0; |
| 5415 | 5461 | for (fn_info.param_body) |inst| { |
| 5462 | var is_comptime = false; | |
| 5463 | var is_anytype = false; | |
| 5416 | 5464 | switch (zir_tags[inst]) { |
| 5417 | .param_comptime, .param_anytype_comptime, .param, .param_anytype => {}, | |
| 5465 | .param => { | |
| 5466 | is_comptime = func_ty_info.paramIsComptime(arg_i); | |
| 5467 | }, | |
| 5468 | .param_comptime => { | |
| 5469 | is_comptime = true; | |
| 5470 | }, | |
| 5471 | .param_anytype => { | |
| 5472 | is_anytype = true; | |
| 5473 | is_comptime = func_ty_info.paramIsComptime(arg_i); | |
| 5474 | }, | |
| 5475 | .param_anytype_comptime => { | |
| 5476 | is_anytype = true; | |
| 5477 | is_comptime = true; | |
| 5478 | }, | |
| 5418 | 5479 | else => continue, |
| 5419 | 5480 | } |
| 5481 | ||
| 5482 | // We populate the Type here regardless because it is needed by | |
| 5483 | // `GenericCallAdapter.eql` as well as function body analysis. | |
| 5484 | // Whether it is anytype is communicated by `anytype_args`. | |
| 5420 | 5485 | const arg = child_sema.inst_map.get(inst).?; |
| 5421 | 5486 | const copied_arg_ty = try child_sema.typeOf(arg).copy(new_decl_arena_allocator); |
| 5422 | if (child_sema.resolveMaybeUndefValAllowVariables( | |
| 5423 | &child_block, | |
| 5424 | .unneeded, | |
| 5425 | arg, | |
| 5426 | ) catch unreachable) |arg_val| { | |
| 5487 | anytype_args[arg_i] = is_anytype; | |
| 5488 | ||
| 5489 | const arg_src = call_src; // TODO: better source location | |
| 5490 | if (try sema.typeRequiresComptime(block, arg_src, copied_arg_ty)) { | |
| 5491 | is_comptime = true; | |
| 5492 | } | |
| 5493 | ||
| 5494 | if (is_comptime) { | |
| 5495 | const arg_val = (child_sema.resolveMaybeUndefValAllowVariables( | |
| 5496 | &child_block, | |
| 5497 | .unneeded, | |
| 5498 | arg, | |
| 5499 | ) catch unreachable).?; | |
| 5427 | 5500 | child_sema.comptime_args[arg_i] = .{ |
| 5428 | 5501 | .ty = copied_arg_ty, |
| 5429 | 5502 | .val = try arg_val.copy(new_decl_arena_allocator), |
| ... | ... | @@ -5480,22 +5553,7 @@ fn instantiateGenericCall( |
| 5480 | 5553 | |
| 5481 | 5554 | const comptime_args = callee.comptime_args.?; |
| 5482 | 5555 | const new_fn_info = callee.owner_decl.ty.fnInfo(); |
| 5483 | const runtime_args_len = count: { | |
| 5484 | var count: u32 = 0; | |
| 5485 | var arg_i: usize = 0; | |
| 5486 | for (fn_info.param_body) |inst| { | |
| 5487 | switch (zir_tags[inst]) { | |
| 5488 | .param_comptime, .param_anytype_comptime, .param, .param_anytype => { | |
| 5489 | if (comptime_args[arg_i].val.tag() == .generic_poison) { | |
| 5490 | count += 1; | |
| 5491 | } | |
| 5492 | arg_i += 1; | |
| 5493 | }, | |
| 5494 | else => continue, | |
| 5495 | } | |
| 5496 | } | |
| 5497 | break :count count; | |
| 5498 | }; | |
| 5556 | const runtime_args_len = @intCast(u32, new_fn_info.param_types.len); | |
| 5499 | 5557 | const runtime_args = try sema.arena.alloc(Air.Inst.Ref, runtime_args_len); |
| 5500 | 5558 | { |
| 5501 | 5559 | var runtime_i: u32 = 0; |
| ... | ... | @@ -5505,7 +5563,9 @@ fn instantiateGenericCall( |
| 5505 | 5563 | .param_comptime, .param_anytype_comptime, .param, .param_anytype => {}, |
| 5506 | 5564 | else => continue, |
| 5507 | 5565 | } |
| 5508 | const is_runtime = comptime_args[total_i].val.tag() == .generic_poison; | |
| 5566 | const is_runtime = comptime_args[total_i].val.tag() == .generic_poison and | |
| 5567 | comptime_args[total_i].ty.hasRuntimeBits() and | |
| 5568 | !comptime_args[total_i].ty.comptimeOnly(); | |
| 5509 | 5569 | if (is_runtime) { |
| 5510 | 5570 | const param_ty = new_fn_info.param_types[runtime_i]; |
| 5511 | 5571 | const arg_src = call_src; // TODO: better source location |
| ... | ... | @@ -6562,6 +6622,7 @@ fn funcCommon( |
| 6562 | 6622 | .zir_body_inst = func_inst, |
| 6563 | 6623 | .owner_decl = sema.owner_decl, |
| 6564 | 6624 | .comptime_args = comptime_args, |
| 6625 | .anytype_args = undefined, | |
| 6565 | 6626 | .hash = hash, |
| 6566 | 6627 | .lbrace_line = src_locs.lbrace_line, |
| 6567 | 6628 | .rbrace_line = src_locs.rbrace_line, |
src/value.zig+20-1
| ... | ... | @@ -954,6 +954,10 @@ pub const Value = extern union { |
| 954 | 954 | assert(ty.enumFieldCount() == 1); |
| 955 | 955 | break :blk 0; |
| 956 | 956 | }, |
| 957 | .enum_literal => i: { | |
| 958 | const name = val.castTag(.enum_literal).?.data; | |
| 959 | break :i ty.enumFieldIndex(name).?; | |
| 960 | }, | |
| 957 | 961 | // Assume it is already an integer and return it directly. |
| 958 | 962 | else => return val, |
| 959 | 963 | }; |
| ... | ... | @@ -2023,6 +2027,11 @@ pub const Value = extern union { |
| 2023 | 2027 | /// This function is used by hash maps and so treats floating-point NaNs as equal |
| 2024 | 2028 | /// to each other, and not equal to other floating-point values. |
| 2025 | 2029 | /// Similarly, it treats `undef` as a distinct value from all other values. |
| 2030 | /// This function has to be able to support implicit coercion of `a` to `ty`. That is, | |
| 2031 | /// `ty` will be an exactly correct Type for `b` but it may be a post-coerced Type | |
| 2032 | /// for `a`. This function must act *as if* `a` has been coerced to `ty`. This complication | |
| 2033 | /// is required in order to make generic function instantiation effecient - specifically | |
| 2034 | /// the insertion into the monomorphized function table. | |
| 2026 | 2035 | pub fn eql(a: Value, b: Value, ty: Type, target: Target) bool { |
| 2027 | 2036 | const a_tag = a.tag(); |
| 2028 | 2037 | const b_tag = b.tag(); |
| ... | ... | @@ -2200,8 +2209,18 @@ pub const Value = extern union { |
| 2200 | 2209 | } |
| 2201 | 2210 | return order(a, b, target).compare(.eq); |
| 2202 | 2211 | }, |
| 2203 | else => return order(a, b, target).compare(.eq), | |
| 2212 | .Optional => { | |
| 2213 | if (a.tag() != .opt_payload and b.tag() == .opt_payload) { | |
| 2214 | var buffer: Payload.SubValue = .{ | |
| 2215 | .base = .{ .tag = .opt_payload }, | |
| 2216 | .data = a, | |
| 2217 | }; | |
| 2218 | return eql(Value.initPayload(&buffer.base), b, ty, target); | |
| 2219 | } | |
| 2220 | }, | |
| 2221 | else => {}, | |
| 2204 | 2222 | } |
| 2223 | return order(a, b, target).compare(.eq); | |
| 2205 | 2224 | } |
| 2206 | 2225 | |
| 2207 | 2226 | /// This function is used by hash maps and so treats floating-point NaNs as equal |
test/behavior/generics.zig+18
| ... | ... | @@ -306,3 +306,21 @@ test "anonymous struct return type referencing comptime parameter" { |
| 306 | 306 | try expect(s.data == 1234); |
| 307 | 307 | try expect(s.end == 5678); |
| 308 | 308 | } |
| 309 | ||
| 310 | test "generic function instantiation non-duplicates" { | |
| 311 | if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO | |
| 312 | if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO | |
| 313 | if (builtin.os.tag == .wasi) return error.SkipZigTest; | |
| 314 | ||
| 315 | const S = struct { | |
| 316 | fn copy(comptime T: type, dest: []T, source: []const T) void { | |
| 317 | @export(foo, .{ .name = "test_generic_instantiation_non_dupe" }); | |
| 318 | for (source) |s, i| dest[i] = s; | |
| 319 | } | |
| 320 | ||
| 321 | fn foo() callconv(.C) void {} | |
| 322 | }; | |
| 323 | var buffer: [100]u8 = undefined; | |
| 324 | S.copy(u8, &buffer, "hello"); | |
| 325 | S.copy(u8, &buffer, "hello2"); | |
| 326 | } |