Skip to content
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

langref: add example for errdefer |err| sytnax #20441

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Jun 28, 2024

No description provided.

var captured: ?anyerror = null;

if (captureError(&captured)) unreachable else |err| {
try std.testing.expectEqual(error.GeneralFailure, captured.?);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ if I remove .? and replace captured.? with just captured the test fails with expected error.GeneralFailure, found error.GeneralFailure. Seems like a bug in the language: either this code should not compile without ?, or it should return true.

Copy link
Contributor

@rohlem rohlem Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just be that std.testing.expectEqual doesn't look at the payload of optionals, but when printing the output is the same.
EDIT: Nope, looks like @TypeOf(a, b) should make both into optionals, which should be unwrapped and fail comparison here. Maybe some conversion from single-member error set to anyerror is screwy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the issue: @TypeOf of an optional error type with a non-optional error type leads to an error union with optional error payload,
in this case @TypeOf(error.GeneralError, ?anyerror) == error{GeneralError}!?anyerror.
Here the error.GeneralError value inhabits the error, while the ?anyerror inhabits the payload, which makes them unequal in the ErrorUnion case of expectEqual.
IMO this isn't a language defect but a deficiency in std.testing.expectEqual, filing an issue for discussion.

Copy link
Contributor

@Rexicon226 Rexicon226 Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening is that the "found" error.GeneralFailure, is an instant of the anyerror, whereas the "expected" error.GeneralFailure is part of the IES. There is no well defined behaviour for coercing an error into an error{X}!?anyerror. Does the error go into the error set or into the anyerror? Here it just become two different things that doesn't equal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rexicon226 I do think that there is well-defined behavior: Anything that isn't an undecorated error is a payload; that's the way type resolution for return works (which is the same mechanism behind multi-argument @TypeOf).
The reason one ?anyerror is a payload I think is that it's decorated by ?, I don't think it being part of an inferred error set matters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created #20442 for discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it! Just to restate it, the problem here is that expectEquals tries to find a common type between its arguments. The lhs type here is error{GeneralFailure}, the rhs type is ?anyerror. And there are three "reasonable" answers as to what's the common type here:

  1. common type is ?anyerror -> convert singleton error set to anyerror, that convert that to an optional.
  2. common type is an error union error{GeneralFailure} ! ?anyerror -> using the general rule that the common type for error{...} and T is error{...}!T
  3. no common type, as this is ambigious for the human.

Currently, Zig uses the option 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohlem I see, you’re right. Didn’t fully consider how it was working. Thanks for the issue :)

@matklad matklad force-pushed the matklad/errdefercapture branch from e1e0e9c to d6c43d6 Compare July 10, 2024 10:44
@andrewrk andrewrk merged commit 8267929 into ziglang:master Jul 21, 2024
10 checks passed
@andrewrk
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants