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

std: json.parseFromValue() #15981

Merged
merged 9 commits into from
Jun 20, 2023
Merged

std: json.parseFromValue() #15981

merged 9 commits into from
Jun 20, 2023

Conversation

thejoshwolfe
Copy link
Contributor

To support this case: #15705 (comment)

Adding a parseFromValue() including a custom parse function means that we can parse complex json documents with polymorphic objects by using custom code to determine which union item things should be. See test "polymorphic parsing" for an example.

@andrewrk
Copy link
Member

@matu3ba are you game to add these to compiler-rt?

error: ld.lld: undefined symbol: __fixkfti
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_338634)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_338637)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_408252)
    note: referenced 4 more times
    note: did you mean: __fixdfti
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/ef3f74ff72425e6d13da4960cdf5bcea/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/ef3f74ff72425e6d13da4960cdf5bcea/libcompiler_rt.a.o)
error: ld.lld: undefined symbol: __floattikf
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_338634)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_338637)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0bbda7e5a922a3ea118795e1c05547cb/test.o:(json.static.sliceToInt__anon_408252)
    note: referenced 4 more times
    note: did you mean: __floatdikf
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/ef3f74ff72425e6d13da4960cdf5bcea/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/ef3f74ff72425e6d13da4960cdf5bcea/libcompiler_rt.a.o)
error: ld.lld: undefined symbol: __fixkfti
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_339383)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_339386)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_409855)
    note: referenced 4 more times
    note: did you mean: __fixdfti
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/fad0680e4c1f1f4f99e0d70bfdb69649/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/fad0680e4c1f1f4f99e0d70bfdb69649/libcompiler_rt.a.o)
error: ld.lld: undefined symbol: __floattikf
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_339383)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_339386)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/48ca3e919f051135866e6be20173b64a/test.o:(json.static.sliceToInt__anon_409855)
    note: referenced 4 more times
    note: did you mean: __floatdikf
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/fad0680e4c1f1f4f99e0d70bfdb69649/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/fad0680e4c1f1f4f99e0d70bfdb69649/libcompiler_rt.a.o)
error: ld.lld: undefined symbol: __fixkfti
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_339431)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_339434)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_409755)
    note: referenced 4 more times
    note: did you mean: __fixdfti
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/d5aa95aa68752008508c1fa05de1ed32/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/d5aa95aa68752008508c1fa05de1ed32/libcompiler_rt.a.o)
error: ld.lld: undefined symbol: __floattikf
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_339431)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_339434)
    note: referenced by static.zig:746 (/home/ci/actions-runner4/_work/zig/zig/lib/std/json/static.zig:746)
    note:               /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-local-cache/o/0cf188cbe557c55227e6d700651ae764/test.o:(json.static.sliceToInt__anon_409755)
    note: referenced 4 more times
    note: did you mean: __floatdikf
    note: defined in: /home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/d5aa95aa68752008508c1fa05de1ed32/libcompiler_rt.a(/home/ci/actions-runner4/_work/zig/zig/build-debug/zig-global-cache/o/d5aa95aa68752008508c1fa05de1ed32/libcompiler_rt.a.o)

@matu3ba
Copy link
Contributor

matu3ba commented Jun 15, 2023

are you game to add these to compiler-rt?

Can do. I wonder though, why I am unable to find search results to the ppc runtime abi functions and the only results are inside libgcc __fixkfti. This looks like libgcc doing their own abi thing without documenting it or PPC(64) docs being aweful to search.

Those two are used as PPC64 aliases: __fixkfti, __floattikf here.

TODO for docs: add assumptions about long double being 128IEEE (-mabi=ibmlongdouble -mlong-double-128) instead of spaghetti code https://developers.redhat.com/articles/2023/05/16/benefits-fedora-38-long-double-transition-ppc64le#how_to_transition_to_ieee_128_bit_long_double

UPDATE: Tracking issue for PPC is #16057. I'll fill status and make the tracking issue more readable in the next days.

@matu3ba
Copy link
Contributor

matu3ba commented Jun 16, 2023

@thejoshwolfe Can you rebase on top of #16054 to check, if the fix works correctly master to get a green CI?

Otherwise, I'll try to poke it with valgrind later today.

@andrewrk
Copy link
Member

@matu3ba you can test locally. Run the std lib tests with the appropriate -target parameter and then run with QEMU.

@thejoshwolfe thejoshwolfe force-pushed the json-parse-from-value branch from 0499df2 to e9f0195 Compare June 17, 2023 13:32
@andrewrk
Copy link
Member

@matu3ba the names are still not correct on powerpc64le-linux-musl and powerpc-linux-musl.

undefined symbol: __fixtfti
undefined symbol: __floattitf
undefined symbol: __fixtfti
undefined symbol: __floattitf

@thejoshwolfe thejoshwolfe force-pushed the json-parse-from-value branch from e9f0195 to 27a0fbb Compare June 17, 2023 19:45
@matu3ba
Copy link
Contributor

matu3ba commented Jun 17, 2023

the names are still not correct on powerpc64le-linux-musl and powerpc-linux-musl.

First of all, sorry for the inconvenience. I should have waited the full libstd test suite. It would be very nice, if there would be a target filter to speed things up (prompt also showed up arm stuff). But I should have used -Dtest-filter=[string].

Also, reminder to myself: Dont do PRs at 2am and wait for CI/test suite.

I suspect LLVM does not use kf functions for those 2 targets and uses the regular ones instead.

UPDATE:
Works now at least for the 2 use cases:

./deb/bin/zig test lib/std/json/static_test.zig -target powerpc-linux-musl --test-cmd qemu-ppc --test-cmd-bin
./deb/bin/zig test lib/std/json/dynamic_test.zig -target powerpc-linux-musl --test-cmd qemu-ppc --test-cmd-bin
./deb/bin/zig test lib/std/json/static_test.zig -target powerpc64le-linux-musl --test-cmd qemu-ppc64le --test-cmd-bin
./deb/bin/zig test lib/std/json/dynamic_test.zig -target powerpc64le-linux-musl --test-cmd qemu-ppc64le --test-cmd-bin

I'll run the complete test suite run over night locally.

@thejoshwolfe thejoshwolfe force-pushed the json-parse-from-value branch from 27a0fbb to 3cc2297 Compare June 18, 2023 16:20
@thejoshwolfe
Copy link
Contributor Author

I'm going to see if I can get the CI to pass even if I have to leave the one test behind. Enabling the test can turn into its own PR, but I'd like to get the features in this PR merged in time for 0.11.

jacobly0 added a commit to jacobly0/zig that referenced this pull request Jun 19, 2023
Base automatically changed from json-dynamic-parse to master June 19, 2023 15:21
@thejoshwolfe thejoshwolfe force-pushed the json-parse-from-value branch from c1815dc to d1767e7 Compare June 19, 2023 15:23
try testing.expect(tree.div.color == .blue);
try testing.expectEqualStrings("Cancel", tree.div.children[1].button.caption);
}
// TODO: getting a strange linker error with this test.
Copy link
Member

@andrewrk andrewrk Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In master branch, all disabled tests must have a corresponding open issue. If you wish to commit this disabled test, please open a corresponding issue for it and omit the word "TODO" from the comment (#363).

Also, there is a preferred way to disable tests which is to return error.SkipZigTest inside the test, rather than commenting out the code.

Also, I believe this issue is actually fixed in master branch. I suggest to wait until #16089 lands and then rebase your PR with the tests enabled.

Final request, after tests are passing, please clean up the commits in this PR that enable/disable tests in ways that no longer matter since master branch has fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@thejoshwolfe thejoshwolfe force-pushed the json-parse-from-value branch from 6cf6458 to ef0b419 Compare June 20, 2023 13:43
@thejoshwolfe thejoshwolfe merged commit 0f2339f into master Jun 20, 2023
@thejoshwolfe thejoshwolfe deleted the json-parse-from-value branch June 20, 2023 23:01
@llogick
Copy link
Contributor

llogick commented Jun 23, 2023

The function doesn't seem to be reachable via std.json.parseFromValue(),

zig/lib/std/json.zig

Lines 85 to 91 in 9d66481

pub const ParseOptions = @import("json/static.zig").ParseOptions;
pub const parseFromSlice = @import("json/static.zig").parseFromSlice;
pub const parseFromSliceLeaky = @import("json/static.zig").parseFromSliceLeaky;
pub const parseFromTokenSource = @import("json/static.zig").parseFromTokenSource;
pub const parseFromTokenSourceLeaky = @import("json/static.zig").parseFromTokenSourceLeaky;
pub const ParseError = @import("json/static.zig").ParseError;
pub const Parsed = @import("json/static.zig").Parsed;

@thejoshwolfe
Copy link
Contributor Author

The function doesn't seem to be reachable via std.json.parseFromValue(),

Oops! Good catch. Fixed in #16194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants