Update std.testing.expectEqual and friends to use peer type resolution

[castholm]

This PR is an attempt of improving the ergonomics of passing literals to std.testing.expectEqual by changing the type of its second parameter to anytype and coercing both arguments to a common type @TypeOf(expected, actual).

-pub fn expectEqual(expected: anytype, actual: @TypeOf(expected)) !void
+pub inline fn expectEqual(expected: anytype, actual: anytype) !void

The function is made inline to help ensure that certain coercions that can't happen at runtime happen at comptime, for example, from a value of the union's underlying enum type to an instance of the union itself.

This PR offers a partial solution to the problem described in the body of #4437. I invite everyone to provide feedback and suggest alternative solutions, and the core Zig team is encouraged to close this PR if they feel it does not align with their vision for the standard library.

In addition to expectEqual, this PR also applies similar changes to expectEqualDeep, expectApproxEqAbs and expectApproxEqRel.

Motivation

Currently, the second parameter actual is of the type @TypeOf(expected) which makes it difficult to pass literals like numbers or null as the first argument. Simple code that feels like it should "just work" results in compile errors due to the function trying to coerce the second argument either to a comptime-only type or a very conservative one such as @TypeOf(null). The reason for these compile errors is not always very obvious, especially for newcomers to the language, and every so often someone new will make a post in a Zig community confused why their assertions don't compile.

var my_int: i32 = 123;
try std.testing.expectEqual(246, my_int * 2);

// error: unable to resolve comptime value
// try std.testing.expectEqual(246, my_int * 2);
//                                  ~~~~~~~^~~
// note: value being casted to 'comptime_int' must be comptime-known

var my_optional: ?*const MyStruct = null;
try std.testing.expectEqual(null, my_optional);

// error: expected type '@TypeOf(null)', found '?*const main.MyStruct'
// note: parameter type declared here
// pub fn expectEqual(expected: anytype, actual: @TypeOf(expected)) !void {
//                                               ^~~~~~~~~~~~~~~~~

As a result of this, the user is often required to explicitly coerce the first argument to the expected type by using @as which makes assertions more cumbersome to type and more difficult to read. It might also encourage the user to swap the order of the arguments which will result in incorrect log messages from failed assertions.

var my_int: i32 = 123;
try std.testing.expectEqual(@as(i32, 246), my_int * 2);

var my_optional: ?*const MyStruct = null;
try std.testing.expectEqual(@as(?*const MyStruct, null), my_optional);

// '@as' is annoying, so let's swap the arguments

my_optional = &.{ .x = 1 };
try std.testing.expectEqual(my_optional, null);

// error: 'test_0' failed: expected main.MyStruct{ .x = 1 }, found null

Zig's test declarations is an excellet asset that makes writing tests a lot easier compared to other languages. In order to help encourage developers to write and maintain tests, test code should be ergonomic and easy to both write and read. The status quo of expectEqual being difficult to use conflicts with those goals.

The third commit demonstrates some cherry-picked cases where verbose @as was previously required but can now be removed.

Does this change break existing code?

If the Zig code base is anything to go by, the vast majority of existing uses of expectEqual should continue to work and only a small number of existing uses will result in compile errors that require fixing.

Because the previous signature was more strict and often required explicitly coercing the first argument to a specific type, I believe that this change should not silently break any existing uses of expectEqual. Assertions that pass prior to this change should continue to pass, and assertions that fail should continue to fail.

This change does however loudly break a minor number of specific uses that will now result in compile errors. These mostly involve passing anonymous struct/union/enum literals and the results of builtins that invoke result locations semantics such as @bitCast as the second argument, which no longer resolve to an appropriate type because the parameter type is now anytype.

Luckily, fixing these compile errors is as simple as specifying the struct/union/enum name or explicitly coercing the expressions by wrappping them in @as. The second commit in this PR fixes all the compile errors that were introduced to the Zig code base.

Because the function has been made inline, it could in theory also break function pointers and assignments to function body types (resulting in compile errors), but I strongly doubt anyone is using the assertion functions like that.

[castholm]

This type + IndexOfMinMaxResult were required to be added after the change to make it possible to instantiate them. You couldn't use const Result = @TypeOf(minMax(u8, "")) because each invocation of the old minMax returned a new and distinct struct.

[thejoshwolfe]

I generally think this change improves the usability of this library. Since the expected value is often a literal value, needing to type it explicitly is very annoying.

One potential downside of this is that peer type resolution might relax assertions of the return type of the function being tested, but that seems like a very minor downside. If such testing would still be meaningful, the explicit typing can return.

Has anyone explored using swapped typing, where instead of the second parameter taking the type of the first, the first parameter takes the type of the second? That's not possible in one line of zig, but with the two function approach here, it could be done. I'm not sure if that would make literal expressions work better, such as .{} struct constructions or .name enum literals.

Some of the changes in this PR seem like regressions, and I'd like further research to be done before giving my approval on this PR, but i wouldn't be opposed to this getting merged as-is.

[thejoshwolfe]

Does this work?

Suggested change

	try std.testing.expectEqual(Foo{ .x = 101, .y = 201 }, foo);
	try std.testing.expectEqual(.{ .x = 101, .y = 201 }, foo);

[castholm]

Fails with

lib\std\testing.zig:57:15: error: incompatible types: 'struct{comptime x: comptime_int = 101, comptime y: comptime_int = 201}' and 'cast.test.cast function with an opaque parameter.Foo'
    const T = @TypeOf(expected, actual);
              ^~~~~~~~~~~~~~~~~~~~~~~~~
lib\std\testing.zig:57:23: note: type 'struct{comptime x: comptime_int = 101, comptime y: comptime_int = 201}' here
    const T = @TypeOf(expected, actual);
                      ^~~~~~~~
lib\std\testing.zig:57:33: note: type 'cast.test.cast function with an opaque parameter.Foo' here
    const T = @TypeOf(expected, actual);
                                ^~~~~~
test\behavior\cast.zig:1159:32: note: called from here
    try std.testing.expectEqual(.{ .x = 101, .y = 201 }, foo);
        ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[thejoshwolfe]

That's pretty surprising. I wonder if peer type resolution is supposed to work for this, and this is a limitation of the language that is planned to be fixed someday. 🤔

Since the status quo uses the anonymous struct construction, I assume that expectEqualInner(@as(@TypeOf(actual), expected), actual) or something like that would work, where the type of the actual parameter is the authoritative one.

[castholm]

Related accepted proposal? #16865

I don't know much about the compiler internals but I suspect that the literal is being demoted from an "anonymous struct type" to a "tuple with comptime fields" once it enters the function, which is why the coercion no longer can happen.

[mlugg]

This one is actually currently supposed to work - this is a PTR bug/limitation. However, it wouldn't work once anonymous struct types are removed from Zig, which is an accepted proposal.

[thejoshwolfe]

This is a surprising change. Does peer type resolution not work for this? I would have thought this implicit case would be unnecessary if peer type resolution would work.

[castholm]

Fails with

lib\std\testing.zig:58:32: error: coercion from enum 'type.test.Type.Union.Tag' to union 'type.test.Type.Union.Tagged' must initialize 'i32' field 'signed'
    return expectEqualInner(T, expected, actual);
                               ^~~~~~~~
test\behavior\type.zig:429:20: note: field 'signed' declared here
    const Tagged = @Type(.{
                   ^~~~~
test\behavior\type.zig:429:20: note: union declared here
    const Tagged = @Type(.{
                   ^~~~~
test\behavior\type.zig:441:28: note: called from here
    try testing.expectEqual(Tag.signed, tagged);
        ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

[mlugg]

Whilst unions are able to coerce to their tag type, it is not desirable for PTR to be able to trigger such a coercion. Consider this code:

const U = union(enum) {
    a: u32,
    b: u32,
};
const x = if (foo) U{ .a = 1 } else .b;

It would be very unexpected if the type of x here was the tag type of U, silently dropping the payload!

[vadim-za]

Whilst unions are able to coerce to their tag type, it is not desirable for PTR to be able to trigger such a coercion. Consider this code:

const U = union(enum) {
    a: u32,
    b: u32,
};
const x = if (foo) U{ .a = 1 } else .b;

It would be very unexpected if the type of x here was the tag type of U, silently dropping the payload!

I would actually expect it to drop the payload: since the else branch doesn't have one, there is no way the final result can contain the payload. Alternatively I could also understand if PTR yields an error in this case (although I'm not immediately convinced this is a good idea: intuitively if a==b performs a comparison in terms of type T, I'd expect this type to be the result of @TypeOf(a,b)). But not yielding a union, I don't see how this makes sense.

[thejoshwolfe]

This seems suspiciously like a regression. Does .{ .index_min = 0, .index_max = 6 } not work for the test cases?

[castholm]

Doesn't work for the same reason as test/behavior/cast.zig. You have to use reflection and get the type of the actual value and explicitly coerce to it, which is very inconvenient.

pub fn indexOfMinMax(comptime T: type, slice: []const T) struct { index_min: usize, index_max: usize } {
    // ...
}

test "indexOfMinMax" {
    const actual = indexOfMinMax(u8, "abcdefg");
    try testing.expectEqual(@as(@TypeOf(actual), .{ .index_min = 0, .index_max = 6 }), actual);
}

[castholm]

One potential downside of this is that peer type resolution might relax assertions of the return type of the function being tested, but that seems like a very minor downside. If such testing would still be meaningful, the explicit typing can return.

Similar concerns were brought up in Discord a few weeks ago, and I pointed out that even the status quo expectEqual doesn't fully guard against type coercion pitfalls and can result in API breakages going unnoticed. I'll repost my thoughts for visibility:

I only just now saw this discussion regarding my expectEqual PR and that coercing to actual might be wrong because it might be the wrong type and I want to point out that a limited version of this problem already exists with the current signature.

Imagine you export the function fn getSecret() ?i32. The fact that it returns an optional ?i32 is very important and changing this is a breaking change of your API, so you write the test try expectEqual(@as(?i32, 123), getSecret()) believing that this will verify that the function returns the correct value of the correct type.

After a while, someone refactors a bunch of stuff in your codebase and inadvertently changes the function signature to fn getSecret() i32. Does this cause the test to fail? No, it continues to pass, because i32 is implicitly coerced to ?i32. So unless you detect it through some other mechanism, you've just shipped a silently breaking change to your users which will cause code like getSecret() orelse 456 to now result in compile errors.

The only 100% always safe way of testing that a value is of an exact type is to test the actual type, as in try expectEqual(?i32, @TypeOf(getSecret())).

So if testing the exact types is important, you have to compare the actual types.

[thejoshwolfe]

it looks like limitations with peer type resolution and the upcoming changes to anonymous structs make the direction of this PR not strictly an improvement. I would like to suggest instead of the first parameter determining the type (master branch) and instead of both parameters determining the type (this PR), we should have the second parameter determine the type.

Am I wrong to believe that it would be the best of all cases in these stdlib test changes? (except for the Tag/Tagged case in test/behavior/type.zig.)

[castholm]

@thejoshwolfe

I would like to suggest instead of the first parameter determining the type (master branch) and instead of both parameters determining the type (this PR), we should have the second parameter determine the type.

Changing the implementation to

 pub inline fn expectEqual(expected: anytype, actual: anytype) !void {
-    const T = @TypeOf(expected, actual);
+    const T = @TypeOf(actual);
     return expectEqualInner(T, expected, actual);
 }

makes cases like this compile:

const Foo = struct { x: usize, y: usize };
var actual: Foo = .{ .x = 10, .y = 20 };
_ = &actual;
try expectEqual(.{ .x = 10, .y = 20 }, actual);

But I suspect that this will only fix this particular issue short-term and will stop working once #16865 is implemented. It also doesn't fix the issue with builtins invoking RLS so cases like these will still need @as:

const inf_u32: u32 = 0x7F800000;
try expectEqual(inf_u32, @as(u32, @bitCast(std.math.inf(f32))));

Changing the signature to fn expectEqual(expected: @TypeOf(actual), actual: anytype) !void is not possible without a language-level change (#3226).

You could flip the parameters from expected, actual to actual, expected (which is what #4437 originally suggested) but this would be a rather intrusive change that would force everyone to go and update every single assertion if they want correct error messages.

Neither of these two fix the builtin-with-RLS case for the anytype parameter, you will still need an explicit type at the call site for that.

it looks like limitations with peer type resolution and the upcoming changes to anonymous structs make the direction of this PR not strictly an improvement.

I respectfully disagree. If you check the second commit in this PR, there were fewer than 40 assertions that needed to be changed due to compile errors. If I grep the Zig compiler codebase for expectEqual(@as( I get over 1600 results, most of which could be updated have the @as removed after this change. I think that ratio of regressions to improvements is acceptable. Comparisons between simple scalars are much more common than comparisons between structs/unions and this should probably hold true even in consumer codebases.

[thejoshwolfe]

@castholm thanks for your patience. after catching up on the discussions in #4437 and #16865 , i see people have already put a lot of thought into the stuff that i'm coming up with just now. given the proposals that have been accepted, i think you're right, and this PR is the best improvement way forward that we have. (sorry it took so long for me to get there.)

thanks for your contributions! (I did not check what was failing in the CI.)

[andrewrk]

Thank you @castholm. It's a nice solution and a nice writeup.