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: TypeInfo.StructField.default_value cannot represent undefined. #7091

Open
SpexGuy opened this issue Nov 12, 2020 · 5 comments
Open
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@SpexGuy
Copy link
Contributor

SpexGuy commented Nov 12, 2020

This currently manifests as a compiler crash:

export fn foo() void {
    const S = struct { f: *u32 = undefined };
    var x = @typeInfo(S).Struct.fields[0].default_value.?;
}
Unreachable at /deps/zig/src/stage1/codegen.cpp:7140 in gen_const_val_ptr. This is a bug in the Zig compiler.
Unable to dump stack trace: debug info stripped
Compiler returned: 255

But the problem is in the design. The decision in #1831 means that there is no way to construct an optional with an undefined value but defined nullity. So the default_value field of StructField, which normally has the type ?field_type, cannot represent a default value of undefined. I'm not really sure what the right answer is in this situation. One option would be to say that undefined is a special defined value at comptime, and therefore @as(?T, @as(T, undefined)) produces a defined nonnull value at comptime but not at runtime. This is pretty weird though, I don't think it's a good solution. undefined could generate a sentinel value in StructField.default_value, but that would make it harder to use. We could separate default_value into a flag and a value, but that's also pretty ugly.

@SpexGuy
Copy link
Contributor Author

SpexGuy commented Nov 12, 2020

As terrible as it is, I'm actually leaning towards the thing I called out as "not a good solution". I'm going to use the name "uninitialized" instead of "undefined" in my explanation, because I think that name makes the decision clearer. The "uninitialized" value at comptime is like javascript undefined - a well-defined sentinel value that can be stored in any non-zero-sized type, can be safely stored in an optional, and can be checked. Runtime variables that have been initialized from the comptime value "uninitialized" are the scary UB beast. It has to be this way because the compiler needs to know if a comptime variable has the special value "uninitialized". If you make a comptime const x that contains a typed "uninitialized", and then you use x as the default value for a field, you are telling the compiler that those bytes don't need to be initialized (and are therefore undefined). In order for this to work, the compiler has to know that the value is "uninitialized". So, as weird as this is, checking if a value contains "uninitialized" is a totally normal and valid thing to do at compile time. This would also mean that we could add @isUndefined(comptime x: anytype) bool.

@rohlem
Copy link
Contributor

rohlem commented Nov 13, 2020

I'm not sure where else you would employ these semantics, but just for the use case of reflecting on default values, we could just change the @typeInfo field's type to ??T, right?
Or, more readably, give it a type const DefaultValue = union(enum) {no_default: void, default_undefined: void, default_defined: T, };. EDIT: uhh, tagged obviously

Zig explicitly wants you to write = undefined for not-initializing variables; afaik this applies to constants and comptime values just the same. EDIT: Having struct fields without default values just means they have to be specified (potentially as undefined) at construction. I don't see in what scenario this new semantic difference would come into play.

@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Nov 16, 2020
@andrewrk andrewrk added this to the 0.8.0 milestone Nov 16, 2020
@andrewrk
Copy link
Member

This would also mean that we could add @isUndefined(comptime x: anytype) bool.

We could make this support both comptime and runtime values. For comptime values, it would always be accurate. For runtime values, it would have the possibility of false negatives but no false positives. The use case for runtime value support is safety checking. For example to solve #3568 it will do this under the covers:

Original source:

if (condition) {
    foo();
}

Under the covers:

if (@isUndefined(condition)) {
    @panic("branch on undefined condition");
}
if (condition) {
    foo();
}

@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend. labels Nov 16, 2020
@thejoshwolfe
Copy link
Sponsor Contributor

what about making the default_value field a tagged union with { none, @"undefined", value: T}?

@SpexGuy
Copy link
Contributor Author

SpexGuy commented Nov 19, 2020

what about making the default_value field a tagged union with { none, @"undefined", value: T}?

I think that's equivalent, assuming undefined is transitive at compile time:

const x: *u32 = undefined;
const T = struct {
    value: *u32 = x, // is this recognized as undefined?
};
// if so, we can implement the check in user code:
pub fn isUndefined(comptime x: anytype) {
    const S = struct { field: @TypeOf(x) = x };
    return @TypeOf(S).Struct.fields[0].default_value == .undefined;
}

If they are equivalent, I think I prefer the optional that can contain undefined. It's easier to use when processing type info.

#7115 may also be relevant here.

@andrewrk andrewrk modified the milestones: 0.8.0, 0.8.1 Jun 4, 2021
@andrewrk andrewrk modified the milestones: 0.8.1, 0.9.1 Aug 31, 2021
@andrewrk andrewrk modified the milestones: 0.9.1, 0.9.0, 0.11.0 Nov 20, 2021
@andrewrk andrewrk added proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. and removed bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend. labels Nov 22, 2021
@andrewrk andrewrk modified the milestones: 0.11.0, 0.10.0 Nov 22, 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
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. 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