Skip to content

std.json: fix compile error for comptime fields #10849

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

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

sharpobject
Copy link
Contributor

This is covered by an existing test which was already failing.

@daurnimator
Copy link
Contributor

I don't understand why this would be needed..... is this working around a compiler bug?

@sharpobject
Copy link
Contributor Author

sharpobject commented Feb 10, 2022

I don't know what the expected behavior is when you pass a *const anyopaque to a function that takes a f64. If that is expected to work, then it's working around a compiler bug. If not, then it's not.

The stdlib tests for this file are currently failing because of this issue. Is that not what you see if you run them?

@sharpobject
Copy link
Contributor Author

Here's what I get over here on master

$ ./build/zig test lib/std/json.zig --zig-lib-dir lib --main-pkg-path lib/std
./lib/std/json.zig:1769:92: error: expected type 'i32', found '*const anyopaque'
                                    if (!try parsesTo(field.field_type, field.default_value.?, tokens, child_options)) {
                                                                                           ^
./lib/std/json.zig:1906:32: note: called from here
    const r = try parseInternal(T, token, tokens, options);
                               ^
./lib/std/json.zig:2106:66: note: called from here
        try testing.expectEqual(T{ .a = 0, .b = true }, try parse(T, &TokenStream.init(
                                                                 ^
./lib/std/json.zig:2100:34: note: called from here
test "parse with comptime field" {
                                 ^
./lib/std/json.zig:1769:92: error: expected type 'f64', found '*const anyopaque'
                                    if (!try parsesTo(field.field_type, field.default_value.?, tokens, child_options)) {
                                                                                           ^
./lib/std/json.zig:1906:32: note: called from here
    const r = try parseInternal(T, token, tokens, options);
                               ^
./lib/std/json.zig:2246:42: note: called from here
    try testing.expectEqual(t3, try parse(T3, &TokenStream.init(str), options_first));
                                         ^
./lib/std/json.zig:2220:47: note: called from here
test "parse into struct with duplicate field" {
                                              ^
./lib/std/json.zig:1769:92: error: expected type '[]const u8', found '*const anyopaque'
                                    if (!try parsesTo(field.field_type, field.default_value.?, tokens, child_options)) {
                                                                                           ^
./lib/std/json.zig:1700:38: note: called from here
                    if (parseInternal(u_field.field_type, token, &tokens_copy, options)) |value| {
                                     ^
./lib/std/json.zig:1906:32: note: called from here
    const r = try parseInternal(T, token, tokens, options);
                               ^
./lib/std/json.zig:2130:28: note: called from here
        const r = try parse(T, &TokenStream.init(
                           ^
./lib/std/json.zig:2100:34: note: called from here
test "parse with comptime field" {
                                 ^
./lib/std/json.zig:1769:92: error: expected type '[]const u8', found '*const anyopaque'
                                    if (!try parsesTo(field.field_type, field.default_value.?, tokens, child_options)) {
                                                                                           ^
./lib/std/json.zig:1700:38: note: called from here
                    if (parseInternal(u_field.field_type, token, &tokens_copy, options)) |value| {
                                     ^
./lib/std/json.zig:1906:32: note: called from here
    const r = try parseInternal(T, token, tokens, options);
                               ^
./lib/std/json.zig:2130:28: note: called from here
        const r = try parse(T, &TokenStream.init(
                           ^
./lib/std/json.zig:2100:34: note: called from here
test "parse with comptime field" {
                                 ^

@matu3ba
Copy link
Contributor

matu3ba commented Feb 10, 2022

working around a compiler bug

Please be descriptive on what semantics your code change has. Before the change the compiler (I do assume stage1) crashed on the comptime field.
To fix this you are suggesting to use the default value.

This is covered by an existing test which was already failing.

Which test? I dont see any disabled one inside json.zig and json/test.zig

Finally and most important: Can you provide a use case that justify this hack to derive behavior from a opaque pointer meant to only be used on the external interfacing code ?

@daurnimator
Copy link
Contributor

The stdlib tests for this file are currently failing because of this issue. Is that not what you see if you run them?

tests currently pass for me; and on every CI run for the repository.
Are you on some esoteric platform?
Otherwise you likely have corrupted your zig installation/checkout somehow.

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Feb 11, 2022
@sharpobject
Copy link
Contributor Author

sharpobject commented Feb 11, 2022

Please be descriptive on what semantics your code change has. Before the change the compiler (I do assume stage1) crashed on the comptime field.
To fix this you are suggesting to use the default value.

Prior to the change in which anytype fields were removed from the language, this line already used the default value. This change attempts to restore that behavior, by passing the default value to parsesTo, which is a function that takes a type, a value of that type, and some tokens, and checks whether the tokens parse to the value. The intent of this line is to check that the value of the field in the json is equal to the value of the comptime field.

If there is some philosophical objection to the idea of accessing the default value (which is where the value of the comptime field is stored) ever for any reason, I guess we will have to give up on the idea of having a generic json parser that can verify that values in json objects correspond to comptime fields. In that case, I can submit a different patch that completely removes the feature. I would also need to remove the current feature where the json parser is willing to parse inputs that have missing fields and use the default value for those fields, which involves this existing code:

            inline for (structInfo.fields) |field, i| {
                if (!fields_seen[i]) {
                    if (field.default_value) |default_ptr| {
                        if (!field.is_comptime) {
                            const default = @ptrCast(*const field.field_type, default_ptr).*;
                            @field(r, field.name) = default;
                        }
                    } else {
                        return error.MissingField;
                    }
                }
            }

Which test? I dont see any disabled one inside json.zig and json/test.zig

It's not disabled. I posted the output and the command used to generate the output in the comment immediately preceding yours.

Are you on some esoteric platform?

The above is the failure I observe when I run precisely these commands on my M1 mac, starting with no zig2 folder:

git clone [email protected]:ziglang/zig.git zig2
cd zig2
mkdir build
cd build
cmake .. -DCMAKE_PREFIX_PATH=$(brew --prefix llvm)
make
cd ..
./build/zig test lib/std/json.zig --zig-lib-dir lib --main-pkg-path lib/std

I haven't succeeded in building llvm 13 on linux on x64, but I am also able to reproduce this test failure on linux on x64 by running the test using a zig binary obtained from snap against a fresh clone of the repository. the zig binary obtained from snap reports its version as 0.10.0-dev.662+e139c41fd. That is, after acquiring the zig from snap, I can run these commands on linux on x64 to also produce the output I posted above:

git clone [email protected]:ziglang/zig.git zig2
cd zig2
zig test lib/std/json.zig --zig-lib-dir lib --main-pkg-path lib/std

I am attempting to follow the instructions given at https://github.com/ziglang/zig/wiki/Building-Zig-From-Source and https://github.com/ziglang/zig/blob/master/CONTRIBUTING.md about building zig (in the case of the M1 mac) and running stdlib tests. Have I done so incorrectly?

@daurnimator
Copy link
Contributor

Have I done so incorrectly?

Ah it seems I tested against non-latest zig. updating to master I indeed see the same failure.

anytype fields were removed from the language

Ah ha! I now understand where the change comes from.

sharpobject and others added 2 commits February 13, 2022 14:33
This is covered by an existing test which was already failing.
Only about half of the tests in std were actually being run (918 vs 2144).
@Vexu Vexu force-pushed the sharpobject_fix_json_comptime_fields branch from c12ffb0 to 8937f18 Compare February 13, 2022 12:37
@Vexu Vexu merged commit 3eb29f1 into ziglang:master Feb 14, 2022
@daurnimator
Copy link
Contributor

I wonder if this also needs an @alignCast?
#10705 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants