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

design flaw with equality comparison of tagged unions #2251

Open
shawnl opened this issue Apr 11, 2019 · 12 comments
Open

design flaw with equality comparison of tagged unions #2251

shawnl opened this issue Apr 11, 2019 · 12 comments
Labels
use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone

Comments

@shawnl
Copy link
Contributor

shawnl commented Apr 11, 2019

see #2251 (comment)


This is currently breaking the tests on Arm64 because builtin.arch is a union(enum) and the tests try to do an equality comparison on it.

/home/shawn/build-refs/heads/builtins/test/tests.zig:171:78: error: operator not allowed for type 'builtin.Arch'
        const is_native = (test_target.os == builtin.os and test_target.arch == builtin.arch);
                                                                             ^

It is slightly weird to do a comparison on a union as it would always bit a bit-wise comparison, and not a float comparison.

Perhaps there is another way to fix this.

@tiehuis
Copy link
Member

tiehuis commented Apr 11, 2019

You can compare tags of union(enum) by using @enumToInt.

const Foo = union(enum) {
    A: usize,
    B: []const u8,
    C,
};

test "example" {
    var a = Foo{ .A = 0 };
    var b = Foo{ .B = "here" };

    @import("std").debug.assert(@enumToInt(a) != @enumToInt(b));
}

@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Apr 11, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Apr 11, 2019
@andrewrk andrewrk changed the title allow == for union(enum) compile error on Arm64 Apr 11, 2019
@andrewrk
Copy link
Member

This is tested and working in master branch now.

@andrewrk
Copy link
Member

Apologies - spoke too soon. Not sure what I was thinking of here.

@andrewrk andrewrk reopened this Sep 26, 2019
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Sep 26, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 26, 2019
@andrewrk
Copy link
Member

Anyway aarch64 has test coverage in the CI now; this is a different issue. I'll retitle it.

const std = @import("std");
const builtin = @import("builtin");

test "" {
    var arch: builtin.Arch = undefined;
    var os: builtin.Os = undefined;
    const is_native = (os == builtin.os and arch == builtin.arch);
    std.debug.warn("is_native = {}\n", is_native);
}

The issue is that it's weird that this test passes for -target x86_64-linux but it fails for -target aarch64-linux:

[nix-shell:~/Downloads/zig/build]$ ./zig test test.zig 
1/1 test "oaeu"...is_native = false
OK
All tests passed.

[nix-shell:~/Downloads/zig/build]$ ./zig test test.zig -target aarch64v8-linux
/home/andy/Downloads/zig/build/test.zig:7:50: error: operator not allowed for type 'builtin.Arch'
    const is_native = (os == builtin.os and arch == builtin.arch);
                                                 ^

I think this is a flaw in the language.

@andrewrk andrewrk changed the title compile error on Arm64 design flaw with equality comparison of tagged unions Sep 26, 2019
@shawnl
Copy link
Contributor Author

shawnl commented Oct 2, 2019

(not relevant, see below)

@andrewrk
Copy link
Member

andrewrk commented Oct 2, 2019

That's #3330

@shawnl
Copy link
Contributor Author

shawnl commented Oct 3, 2019

x86_64 actually has three abis when it comes to vectors: sse2 (128-bit), avx2 (256-bit), and avx512f (512-bit): %xmm, %ymm, %zmm, and I'd like to extend the ABI enum so that we can forbid exporting vector widths that the current ABI can't handle.

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Jan 7, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 30, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@SpexGuy SpexGuy added the use case Describes a real use case that is difficult or impossible, but does not propose a solution. label Mar 20, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
@ghost
Copy link

ghost commented Oct 3, 2022

This example posted by Andrew no longer works:

const std = @import("std");
const builtin = @import("builtin");

test "" {
    var arch: builtin.Arch = undefined;
    var os: builtin.Os = undefined;
    const is_native = (os == builtin.os and arch == builtin.arch);
    std.debug.warn("is_native = {}\n", is_native);
}

First an error saying that the test can't have an empty name, then an error saying that builtin.Arch doesn't exist

But I'm wondering about the ability to compare tagged unions in general, because this example seems to show that in the past you could compare tagged unions, but now you can't?

const U = union(enum) {
    a: u8,
    b: i8,
};
pub fn main() void {
    const a = U{ .a = 4 };
    const b = U{ .b = -4 };
    _ = a == b;
}

This fails for me with operator == not allowed for type U.

Was the ability to compare tagged unions removed? If so, why?!?

@nurpax
Copy link
Contributor

nurpax commented Mar 20, 2023

It would really be useful to be able to compare tagged unions. For example, I worked around this limitation just now with code like this:

pub const RenderModeTag = enum(u32) { image, constant };
pub const RenderMode = union(RenderModeTag) {
    image: struct { image_id: u32 },
    constant: u32,

    pub fn eq(a: RenderMode, b: RenderMode) bool {
        if (a == .image and b == .image) {
            return a.image.image_id == b.image.image_id;
        } else if (a == .constant and b == .constant) {
            return a.constant == b.constant;
        }
        return false;
    }
};

@kcbanner
Copy link
Contributor

std.meta.eql might work for that case - it compares first the tag, then recurses to compare the values if the tag was equal.

@ghost
Copy link

ghost commented May 21, 2023

So what is the status of this issue? If not being able to compare tagged unions is intended (it shouldn't be), what's the bug?

@Vexu
Copy link
Member

Vexu commented May 22, 2023

Looking at 0.4.0 codegen.cpp builtin.arch used to be defined as Arch.%s for arches with no subarches and as Arch{ .%s = Arch.%s.%s } for ones with subarches. As a result test_target.arch == builtin.arch worked for x86_64 but not arm since builtin.arch was an enum for x86_64 and a union on arm:

const U = union(enum) {
    a: u8,
    b,
};
const b = U.b; // for x86_64
const b = U{ .b =  {} }; // for arm
pub fn main() void {
    var a = U{ .a = 4 };
    _ = a == b;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Projects
None yet
Development

No branches or pull requests

7 participants