-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update std.testing.expectEqual
and friends to use peer type resolution
#17431
Changes from all commits
e5994f5
4c1da09
d7b3650
85869f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1156,7 +1156,7 @@ test "cast function with an opaque parameter" { | |||||
.func = @ptrCast(&Foo.funcImpl), | ||||||
}; | ||||||
c.func(c.ctx); | ||||||
try std.testing.expectEqual(foo, .{ .x = 101, .y = 201 }); | ||||||
try std.testing.expectEqual(Foo{ .x = 101, .y = 201 }, foo); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fails with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
} | ||||||
|
||||||
test "implicit ptr to *anyopaque" { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -438,9 +438,9 @@ test "Type.Union" { | |
}, | ||
}); | ||
var tagged = Tagged{ .signed = -1 }; | ||
try testing.expectEqual(Tag.signed, tagged); | ||
try testing.expectEqual(Tag.signed, @as(Tag, tagged)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fails with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
It would be very unexpected if the type of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
tagged = .{ .unsigned = 1 }; | ||
try testing.expectEqual(Tag.unsigned, tagged); | ||
try testing.expectEqual(Tag.unsigned, @as(Tag, tagged)); | ||
} | ||
|
||
test "Type.Union from Type.Enum" { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems suspiciously like a regression. Does
.{ .index_min = 0, .index_max = 6 }
not work for the test cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.