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

when multiple union fields share the same type, allow them to share a body in a switch prong #1107

Closed
andrewrk opened this issue Jun 13, 2018 · 14 comments · Fixed by #2799
Closed
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

const Foo = union(enum) {
    One: i32,
    Two: bool,
    Three: i32,
};
fn bar(f: Foo) void {
    switch (f) {
        Foo.One, Foo.Three => |x| { },
        Foo.Two => |b| { },
    }
}
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jun 13, 2018
@andrewrk andrewrk added this to the 0.4.0 milestone Jun 13, 2018
@PavelVozenilek
Copy link

It should be possible to omit the Foo prefix. There's no ambiguity.


And if the unioned types are unique, I may prefer to use them:

const Foo = union(enum) {
    One: i32,
    Two: bool,
    Three: u32,
};
fn bar(f: Foo) void {
    switch (f) {
        u32, i32=> |x| { },
        bool => |b| { },
    }
}

@andrewrk
Copy link
Member Author

It should be possible to omit the Foo prefix. There's no ambiguity.

#683

@andrewrk andrewrk added the accepted This proposal is planned. label Nov 21, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Mar 20, 2019
@shritesh
Copy link
Contributor

The above behavior works now but there might be a soundness issue here as the following works too:

const Foo = union(enum) {
    One: i32,
    Two: bool,
    Three: i32,
};

fn bar(f: Foo) void {
    switch (f) {
        .One, .Two, .Three => |x| {
            @import("std").debug.warn("{}\n", x);
        },
    }
}

test "" {
    bar(Foo{ .One = 21 });
    bar(Foo{ .Two = false });
    bar(Foo{ .Three = -1 });
}

@andrewrk
Copy link
Member Author

Hmm yes that looks like a missing compile error.

@LemonBoy
Copy link
Contributor

The above behavior works now but there might be a soundness issue here as the following works too:

It's not unsound, x is not the payload but f, it's just...weird?

@shritesh
Copy link
Contributor

Yeah. The type of x becomes Foo when more than one cases are specified.

@hryx
Copy link
Contributor

hryx commented Jun 9, 2019

x is not the payload but f

This has bitten me before. When I see |x|, I always expect it to unwrap something. So in this case I would not expect x to be the same type as f. Not to mention that the behavior of "|x| unwraps, unless the case's tag types don't match" is pretty gnarly.

@emekoi
Copy link
Contributor

emekoi commented Jun 30, 2019

then would this not be allowed?

fn bar(f: Foo) void {
    switch (f) {
        .One => |x| {
            @import("std").debug.warn("{}\n", x);
        },
		// tag types don't match
        else => |x| {
            @import("std").debug.warn("{}\n", x);
        },
    }
}

@hryx
Copy link
Contributor

hryx commented Jun 30, 2019

then would this not be allowed?

I would say it should not be allowed, because the else => should have the same effective behavior as .Two, .Three =>.

My intuition is that |x| always unwraps, and to get the original Foo you must use f.

@emekoi
Copy link
Contributor

emekoi commented Jun 30, 2019

would that be a compiler error then?

@hryx
Copy link
Contributor

hryx commented Jun 30, 2019

Yep, a compiler error would make sense. Something along these lines

error: cannot unwrap union when tag types don't match

note: mismatched types were Foo.two (bool), Foo.three (i32)

@emekoi emekoi mentioned this issue Jul 1, 2019
@emekoi
Copy link
Contributor

emekoi commented Jul 1, 2019

on second thought i think the existing behavior is correct for else prongs. it allows for less verbose code like this in the standard library:

switch (kernel32.GetLastError()) {
    ERROR.SHARING_VIOLATION => return error.SharingViolation,
    ERROR.ALREADY_EXISTS => return error.PathAlreadyExists,
    ERROR.FILE_EXISTS => return error.PathAlreadyExists,
    ERROR.FILE_NOT_FOUND => return error.FileNotFound,
    ERROR.PATH_NOT_FOUND => return error.FileNotFound,
    ERROR.ACCESS_DENIED => return error.AccessDenied,
    ERROR.PIPE_BUSY => return error.PipeBusy,
    ERROR.FILENAME_EXCED_RANGE => return error.NameTooLong,
    else => |err| return unexpectedError(err),
}

@hryx
Copy link
Contributor

hryx commented Jul 2, 2019

An alternative, if capturing an else were to be an error:

const err = kernel32.GetLastError(); // store in `e` rather than using `|err|`
switch (err) {
    ERROR.SHARING_VIOLATION => return error.SharingViolation,
    // ...
    else => return unexpectedError(err),
}

It would still be confusing to me if |err| sometimes unwraps and sometimes doesn't, depending on context. Though maybe it's a little better if that inconsistency were isolated only to else conditions.

@andrewrk
Copy link
Member Author

andrewrk commented Jul 2, 2019

The fact that else gives a payload which is the same thing (and type) as the target expression is intentional, and @emekoi pointed out above where I used it in the standard library. I'm open to the proposal to reverse this decision, but it's very much intentional in status quo.

I do think it should be a compile error on other switch prongs, however, when they do not share the same type.

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

Successfully merging a pull request may close this issue.

6 participants