Peer type resolution in expectEqual broke functionality

[ikskuh]

Zig Version

zig-0.12.0-dev.3438+5c628312b

Steps to Reproduce and Observed Behavior

const std = @import("std");
test {
    const E = enum { a, b, c };
    const U = union(E) { a: u32, b: u32, c: u32 };

    const u: U = .{ .a = 10 };
    try std.testing.expectEqual(E.a, u);
}

yields

lib/std/testing.zig:58:32: error: coercion from enum 'test.test_0.E' to union 'test.test_0.U' must initialize 'u32' field 'a'
    return expectEqualInner(T, expected, actual);
                               ^~~~~~~~
/tmp/test.zig:4:26: note: field 'a' declared here
    const U = union(E) { a: u32, b: u32, c: u32 };
                         ^~~~~~
/tmp/test.zig:4:15: note: union declared here
    const U = union(E) { a: u32, b: u32, c: u32 };
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/test.zig:7:32: note: called from here
    try std.testing.expectEqual(E.a, u);
        ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

Expected Behavior

The test must fail, but compile.

The problem was introduced in e5994f5 where expectEqual was changed to:

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

which now breaks a bunch of code that checked for expected return types and such:

try std.testing.expectEqual(@as(u8, 10), @as(i32, 10)); // won't compile before, does pass now

as well as the use case with the enum tag checking, which is now broken by the peer-type resolved type checking

[castholm]

These regressions were discussed in #17431 which introduced the change, in particular #17431 (comment) (union tags) and #17431 (comment) (type equality).

Using expectEqual to implicitly test the exact type of a value was always flawed due to coercions (for example, T to ?T, or narrower int types to wider ones). The only reliable way to test the exact type of a value is by actually comparing the actual type using expectEqual(Expected, @TypeOf(actual)).

The case with union tags is unfortunate, but given that it was discussed in the PR and still merged, it seems that the conclusion was that this was an acceptable tradeoff to enable the much more common use case of having null or numeric literals on the left-hand side. Either way, this should probably not be tagged as a bug.

[castholm]

It would probably be possible to update expectEqual to include a special case for when expected is the tag type of actual (or vice versa) without invoking PTR. The question is whether this would result in any new problems or footguns.

[ikskuh]

Using expectEqual to implicitly test the exact type of a value was always flawed due to coercions (for example, T to ?T, or narrower int types to wider ones).

Yeah, that's not what my use case was. The use case was:
The interface is expected to return a type that is coercible to the provided expected type, so returning anything between u0 and u8 was valid, but returning i1 wouldn't be valid then.
This requires to have another function then, as we cannot model that test case with what we have now

Either way, this should probably not be tagged as a bug.

This change shouldn't silently break existing code

I think it does silently break assumptions about how expectEquals work because i introduced some bugs into my code which would've been caught before, but were silently swallowed now due to PTR, which isn't really a nice experience.

So i guess it's either a regression, an explicit breaking change or something we have to discuss again, thus i created that issue. For me, the new behaviour is completly unexpected and as it was a silent change, will also silently break code that will be ported from 0.11 to 0.12

If the goal was to support literals, i guess special-casing comptime_int and comptime_float might be acceptible, or split the expectEqual function into expectEqualValue and expectEqualCoercible

[castholm]

Yeah, that's not what my use case was. The use case was:
The interface is expected to return a type that is coercible to the provided expected type, so returning anything between u0 and u8 was valid, but returning i1 wouldn't be valid then.
This requires to have another function then, as we cannot model that test case with what we have now

Hmm. When I made that change, I did not consider that assertions made with the specific intent of testing that actual can coerce to expected would no longer hold, but you're correct that this does break those kinds of assumptions. The fix is to use @as(@TypeOf(expected), actual) in cases where this distinction matters. It's probably a detail that should be mentioned in the 0.12.0 release notes.

I would still make the argument that expectEqual should not be relied upon for testing type equality or coercibility of the arguments, regardless of which signature expectEqual ends up with. It's not clear to a reader that code like expectEqual(@as(u8, 0), result) is testing the details of coercion. If you instead wrap result in @as(u8, result), it's easier for a reader to tell that result must be coercible to u8 and that this is an invariant that needs to hold at compile time.

In other words, for generic code where testing the properties of a type is important, @as(T, t) or explicitly typed variables should be used to test coercibility and comptime try expectEqual(T, U) (or a plain T == U) to compare exact types.

[ikskuh]

I would still make the argument that expectEqual should not be relied upon for testing type equality or coercibility of the arguments, regardless of which signature expectEqual ends up with.

That's why i propose to split it up and make it explicit about the expected behaviour, either just expect equal values or expect the coercion of values