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 a body in a switch prong, use a peer result type cast, rather than requiring identical types #2812

Closed
andrewrk opened this issue Jul 4, 2019 · 6 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

@andrewrk
Copy link
Member

andrewrk commented Jul 4, 2019

Followup from #1107.

This test passes now:

test "switch prongs with cases with identical payload types" {
    const Union = union(enum) {
        A: usize,
        B: isize,
        C: usize,
    };
    const S = struct {
        fn doTheTest() void {
            doTheSwitch1(Union{ .A = 8 });
            doTheSwitch2(Union{ .B = -8 });
        }
        fn doTheSwitch1(u: Union) void {
            switch (u) {
                .A, .C => |e| {
                    expect(@typeOf(e) == usize);
                    expect(e == 8);
                },
                .B => |e| @panic("fail"),
            }
        }
        fn doTheSwitch2(u: Union) void {
            switch (u) {
                .A, .C => |e| @panic("fail"),
                .B => |e| {
                    expect(@typeOf(e) == isize);
                    expect(e == -8);
                },
            }
        }
    };
    S.doTheTest();
    comptime S.doTheTest();
}

But this diff:

-        A: usize,
+        A: u8,
        B: isize,
-        C: usize,
+        C: u16,

Gives this error:

/home/andy/downloads/zig/build/test.zig:18:28: error: capture group with incompatible types
                .A, .C => |e| {
                           ^
/home/andy/downloads/zig/build/test.zig:18:17: note: type 'u8' here
                .A, .C => |e| {
                ^
/home/andy/downloads/zig/build/test.zig:18:21: note: type 'u16' here
                .A, .C => |e| {
                    ^

However, I propose this should work. peer type resolution would make the type of e be u16 and everything is fine. Likewise a T could be grouped with a ?T.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jul 4, 2019
@andrewrk andrewrk added this to the 0.6.0 milestone Jul 4, 2019
@fengb
Copy link
Contributor

fengb commented Jul 5, 2019

How about explicitly typing the coercion to prevent accidental rogue casts?

.A, .C => |e: u16|

@andrewrk
Copy link
Member Author

andrewrk commented Jul 5, 2019

Can you give an example of an accidental rogue cast?

@fengb
Copy link
Contributor

fengb commented Jul 5, 2019

I'm afraid of situations where changing the field type should trigger an error. Example:

const Union = union(enum) {
  .A: u8,
  .B: u8,
//  .C: u8,
  .C: u16,
  .D: u16,
};

In the current proposal, any switches that previously had A+B+C will automatically be "upgraded" to u16 without any warning. I'm not sure if this is an actual problem in practice, but I feel a little safer if it was opt-in.

@andrewrk
Copy link
Member Author

andrewrk commented Jul 6, 2019

I don't think this is an example of a rogue cast, so far everything is fine in this example.

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Feb 10, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 27, 2020
@andrewrk andrewrk added the accepted This proposal is planned. label Jan 7, 2021
@ifreund
Copy link
Member

ifreund commented Jan 11, 2021

I don't like this as it is not obvious that coercion is happening and there is no language-level way to annotate the type of the capture. I'd much rather see #7224 accepted which would remove the need for this if I understand correctly while bringing considerable other benefits.

@SpexGuy
Copy link
Contributor

SpexGuy commented Jan 11, 2021

I think those are separate use cases. #7224 requires separate code gen for each case, which could cause unnecessary code bloat. Especially on embedded platforms, the variant in this issue is preferred. The rules of coercion are designed so that it should not cause problems. I haven't seen any examples brought forth where the coercion would cause unintended issues, and I can't think of any.

@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 20, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
mlugg added a commit to mlugg/zig that referenced this issue May 28, 2023
This is a bit harder than it seems at first glance. Actually resolving
the type is the easy part: the interesting thing is actually getting the
capture value. We split this into three cases:

* If all payload types are the same (as is required in status quo), we
  can just do what we already do: get the first field value.
* If all payloads are in-memory coercible to the resolved type, we still
  fetch the first field, but we also emit a `bitcast` to convert to the
  resolved type.
* Otherwise, we need to handle each case separately. We emit a nested
  `switch_br` which, for each possible case, gets the corresponding
  union field, and coerces it to the resolved type. As an optimization,
  the inner switch's 'else' prong is used for any peer which is
  in-memory coercible to the target type, and the bitcast approach
  described above is used.

Pointer captures have the additional constraint that all payload types
must be in-memory coercible to the resolved type.

Resolves: ziglang#2812
mlugg added a commit to mlugg/zig that referenced this issue May 29, 2023
This is a bit harder than it seems at first glance. Actually resolving
the type is the easy part: the interesting thing is actually getting the
capture value. We split this into three cases:

* If all payload types are the same (as is required in status quo), we
  can just do what we already do: get the first field value.
* If all payloads are in-memory coercible to the resolved type, we still
  fetch the first field, but we also emit a `bitcast` to convert to the
  resolved type.
* Otherwise, we need to handle each case separately. We emit a nested
  `switch_br` which, for each possible case, gets the corresponding
  union field, and coerces it to the resolved type. As an optimization,
  the inner switch's 'else' prong is used for any peer which is
  in-memory coercible to the target type, and the bitcast approach
  described above is used.

Pointer captures have the additional constraint that all payload types
must be in-memory coercible to the resolved type.

Resolves: ziglang#2812
mlugg added a commit to mlugg/zig that referenced this issue May 30, 2023
This is a bit harder than it seems at first glance. Actually resolving
the type is the easy part: the interesting thing is actually getting the
capture value. We split this into three cases:

* If all payload types are the same (as is required in status quo), we
  can just do what we already do: get the first field value.
* If all payloads are in-memory coercible to the resolved type, we still
  fetch the first field, but we also emit a `bitcast` to convert to the
  resolved type.
* Otherwise, we need to handle each case separately. We emit a nested
  `switch_br` which, for each possible case, gets the corresponding
  union field, and coerces it to the resolved type. As an optimization,
  the inner switch's 'else' prong is used for any peer which is
  in-memory coercible to the target type, and the bitcast approach
  described above is used.

Pointer captures have the additional constraint that all payload types
must be in-memory coercible to the resolved type.

Resolves: ziglang#2812
@andrewrk andrewrk modified the milestones: 0.12.0, 0.11.0 Jun 13, 2023
TUSF pushed a commit to TUSF/zig that referenced this issue May 9, 2024
This is a bit harder than it seems at first glance. Actually resolving
the type is the easy part: the interesting thing is actually getting the
capture value. We split this into three cases:

* If all payload types are the same (as is required in status quo), we
  can just do what we already do: get the first field value.
* If all payloads are in-memory coercible to the resolved type, we still
  fetch the first field, but we also emit a `bitcast` to convert to the
  resolved type.
* Otherwise, we need to handle each case separately. We emit a nested
  `switch_br` which, for each possible case, gets the corresponding
  union field, and coerces it to the resolved type. As an optimization,
  the inner switch's 'else' prong is used for any peer which is
  in-memory coercible to the target type, and the bitcast approach
  described above is used.

Pointer captures have the additional constraint that all payload types
must be in-memory coercible to the resolved type.

Resolves: ziglang#2812
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

4 participants