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

proposal: error set equality behavior #11022

Closed
mitchellh opened this issue Mar 1, 2022 · 8 comments
Closed

proposal: error set equality behavior #11022

mitchellh opened this issue Mar 1, 2022 · 8 comments
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@mitchellh
Copy link
Contributor

mitchellh commented Mar 1, 2022

Tested in Zig versions: 0.9.0 released, 0.10.0-dev.1037+331cc810d

I know you said no new language proposals. I'm only bringing this up because there is an explicit TODO in the stage2 codebase to discuss this...

I was exploring how error set type equality works in stage1, so that I could implement it in stage2, and I noticed the behavior is perhaps not consistent with other type comparisons and I would argue not generally useful in its current form. I am proposing changing it for stage2.

Current Stage1 Behavior

The test below demonstrates:

// This is the current stage1 behavior
test "error set equality" {
	const a = error{One};
	const b = error{One};
	
	try expect(a == a);
	try expect(a != b);
	try expect(a != error{One});
}

The error must be exactly the error declaration, it doesn't match structurally.

Inconsistent?

This is inconsistent with anyerror, which is itself an error set. (Edit: as noted in comments, this is not actually inconsistent if you consider anyerror is just a declaration to the "global error set.")

// But...
test "anyerror" {
  const a = anyerror;
  const b = anyerror;
  
  try expect(a == anyerror);
  try expect(a == a);
  try expect(a == b);
}

Further, this is inconsistent with other types:

// But...
test "alias u8" {
  const a = u8;
  const b = u8;
  
  try expect(a == u8);
  try expect(a == a);
  try expect(a == b);
}

Notice that as long as the type values match, it is true. It doesn't matter where it was declared.

But, it is consistent with structs. If we consider an error a structural type, in spirit.

Proposed Behavior

I propose that the following test should be the working behavior.

However, Zig doesn't structurally match struct types. That is a good argument against.

// Suggestion, does NOT work in stage1 or stage2 right now
test "error set equality" {
	const a = error{One};
	const b = error{One};
	
	try expect(a == a);
	try expect(a == b);
	try expect(a == error{One});
	
	// should treat as a set
	const c = error{One,Two};
	const d = error{Two,One};
	
	try expect(c == d);
}
@ikskuh
Copy link
Contributor

ikskuh commented Mar 1, 2022

Related issues:

@Hejsil
Copy link
Contributor

Hejsil commented Mar 1, 2022

Just want to clarify that currently the error set declaration works very much like an enum declaration here. So if we replace error with enum we can see that the behavior is the same:

test "enum set equality" {
	const a = enum{One};
	const b = enum{One};
	
	try expect(a == a);
	try expect(a != b);
	try expect(a != enum{One});
}

And for the anyerror case, I would not really consider it inconstant. anyerror is not declaring a new global error set, but just aliasing the "one global error set".

I do like the proposal though, because unlike enums error set values implicitly cast between each other, so I can understand that one would think that error{One} == error{One} should be true when error.One == error.One is true.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Mar 1, 2022
@andrewrk andrewrk added this to the 0.10.0 milestone Mar 1, 2022
@SpexGuy
Copy link
Contributor

SpexGuy commented Mar 1, 2022

I agree with the OP, it doesn't really make sense for error sets to be distinct. As an addendum though, this also will affect type info. The order of error tags in the type info for the error set should be sorted alphabetically (in memcmp order).

@SpexGuy
Copy link
Contributor

SpexGuy commented Mar 1, 2022

We will also need to be careful about comparison of lazy error sets. When two are compared, we need to know enough about them to figure out whether they will resolve to the same error set eventually.

@mitchellh
Copy link
Contributor Author

mitchellh commented Mar 1, 2022

We will also need to be careful about comparison of lazy error sets. When two are compared, we need to know enough about them to figure out whether they will resolve to the same error set eventually.

I have a working implementation of this proposal for stage2 in a branch already. Error type comparison forces (or can force) lazy error set resolution. If the comparison is trivially false, I avoid it (i.e. comparing an error set with a non error set). But, at a certain point, I don't think there is a way around it.

mitchellh added a commit to mitchellh/zig that referenced this issue Mar 9, 2022
This implements type equality for merged error types. This is done
through element-wise error set comparison. This is a bit odd since
no other error type is structurally compared (see ziglang#11022) but it appears
to be the same behavior that stage1 also expects based on tests.
@andrewrk andrewrk added the accepted This proposal is planned. label Mar 9, 2022
@andrewrk
Copy link
Member

andrewrk commented Mar 9, 2022

We're experimenting with an amendment to this proposal which is to treat inferred error sets as always distinct types:

test {
    const FooError = @typeInfo(@typeInfo(foo).Fn.return_type.?).ErrorUnion.error_set;
    const BarError = @typeInfo(@typeInfo(bar).Fn.return_type.?).ErrorUnion.error_set;
    const BazError = @typeInfo(@typeInfo(baz).Fn.return_type.?).ErrorUnion.error_set;

    try expect(BarError != error{Bad});

    try expect(FooError != anyerror);
    try expect(BarError != anyerror);
    try expect(BazError != anyerror);

    try expect(FooError != BarError);
    try expect(FooError != BazError);
    try expect(BarError != BazError);

    try expect(FooError == FooError);
    try expect(BarError == BarError);
    try expect(BazError == BazError);
}

fn foo() !void {
    return bar();
}

fn bar() !void {
    return error.Bad;
}

fn baz() !void {
    return quux();
}

fn quux() anyerror!void {}

The only reason for this is to ease the implementation of the language specification. It's problematic to make type equality force error set resolution.

@SpexGuy
Copy link
Contributor

SpexGuy commented Mar 10, 2022

That's fine as long as the type continues to be distinct after it has been resolved. If resolving the type can change the result of ==, then the language needs to specify under what conditions that resolution happens, and we need to make sure that that doesn't leak information about comptime execution order.

Edit: And actually, if we're doing that, there's not really much benefit to deduplicating error sets, because it will basically never happen in practice. So we should probably compare them for equality based on declaration in that case, like enums.

mitchellh added a commit to mitchellh/zig that referenced this issue Mar 10, 2022
This implements type equality for merged error types. This is done
through element-wise error set comparison. This is a bit odd since
no other error type is structurally compared (see ziglang#11022) but it appears
to be the same behavior that stage1 also expects based on tests.
andrewrk pushed a commit to mitchellh/zig that referenced this issue Mar 10, 2022
This implements type equality for error sets. This is done
through element-wise error set comparison.

Inferred error sets are always distinct types and other error sets are
always sorted. See ziglang#11022.
@mitchellh
Copy link
Contributor Author

This proposal is now implemented in stage2 via #11098

tiehuis pushed a commit to tiehuis/zig that referenced this issue Mar 19, 2022
This implements type equality for error sets. This is done
through element-wise error set comparison.

Inferred error sets are always distinct types and other error sets are
always sorted. See ziglang#11022.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

5 participants