-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
ZON #20271
base: master
Are you sure you want to change the base?
ZON #20271
Conversation
@@ -88,6 +88,11 @@ pub const Case = struct { | |||
expect_exact: bool = false, | |||
backend: Backend = .stage2, | |||
link_libc: bool = false, | |||
/// A list of imports to cache alongside the source file. | |||
imports: []const []const u8 = &.{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added support for importing files into test cases so I could test importing ZON. I'm sure there's a better way, but this seems to work fine as a stop gap. IIUC there are plans to rework this eventually anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that the existing compiler error tests pass if there is no error when one was expected. It's possible this has been resolved since I noticed it I have not checked recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that the existing compiler error tests pass if there is no error when one was expected.
Oh, that sounds like a problem! Any chance you could check if that's been fixed, and if not try to fix it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I phrased this poorly--I meant that it was a preexisting bug, not an issue with my changes to the test runner.
I'll check if it's still an issue before merging and file an issue to track it if it is (I don't want to fix it in this PR because it'll likely result in updates to a bunch of unrelated tests.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding &
in ZON...
You're correct that ZON definitely shouldn't include this. Our main use of ZON today (build.zig.zon
) already creates variable-length structures without this syntax. To avoid a divergence between ZON you can @import
and build.zig.zon
, I would consider this a merge blocker, but that's Andrew's call of course.
Implementation-wise, the ZIR instruction corresponding to @import
should be modified to provide a result type if one is present (you can use .none
to represent the case where there isn't one). Then, the ZON analysis logic should traverse into this result type as needed. This has the additional advantage of providing better source locations if the structure of an imported file doesn't match what's expected, since the type error will occur whilst parsing the relevant ZON expression.
Hmm I think I agree, better to knock it out now than to release it as is then immediately change the syntax. I would've done this up front but incorrectly thought it required major changes to the compiler. I'll take a look at making this change. If no result type is specified, I'll have it default to a tuple rather than a slice unless there are any objections to that default. |
Yup. I agree that it makes sense to use a tuple when there's no result type. By the way, in case you're unfamiliar with the |
There were two primary issues at play here: 1. The hex float prefix was not handled correctly when the stream was reset for the fallback parsing path, which occured when the mantissa was longer max mantissa digits. 2. The implied exponent was not adjusted for hex-floats in this branch. Additionally, some of the float parsing routines have been condensed, making use of comptime. closes ziglang#20271
There were two primary issues at play here: 1. The hex float prefix was not handled correctly when the stream was reset for the fallback parsing path, which occured when the mantissa was longer max mantissa digits. 2. The implied exponent was not adjusted for hex-floats in this branch. Additionally, some of the float parsing routines have been condensed, making use of comptime. closes ziglang#20271
In case you haven't found why this is happening, it's here: zig/ci/x86_64-linux-release.sh Lines 60 to 66 in 17f14e1
Edit: I'll address this in a PR shortly. Sit tight for a bit. |
After #20321 you should see the same failures locally when running Edit: I think if you rebase and resolve that build.zig conflict, those CI failures will disappear. |
Nice thanks! I'll do the rebase and get started on removing & from the syntax when I'm back in town next week. |
0358ab4
to
ce32047
Compare
Thanks, I'm not familiar with this code so this is helpful! I started implementing storing the result type on imports, but got a bit stuck. AstGen used to use // AstGen.builtinCall
const operand_node = params[0];
const rhs = try expr(gz, scope, .{ .rl = .none }, operand_node);
try gz.addPlNode(.import, node, Zir.Inst.Bin{
.lhs = result_type,
.rhs = rhs,
}); Zir used to use .str_tok to get the import path, that's also been updated: // Zir.zirImport
const inst_data = sema.code.instructions.items(.data)[@intFromEnum(inst)].pl_node;
const extra = sema.code.extraData(Zir.Inst.Bin, inst_data.payload_index).data;
const src = block.nodeOffset(inst_data.src_node);
const operand = try sema.resolveConstString(
block,
src,
extra.rhs,
.{ .needed_comptime_reason = "import path must be comptime-known" },
); When this code runs,
It appears that the value I get back in Sema for Any pointers would be much appreciated--I can also push the WIP commit if that would be helpful, but I imagine that I'm probably missing something that will be obvious to people more familiar with this part of the codebase. |
You probably just need to clear your cache directory -- ZIR is cached based on However, your lowering strategy for |
Ha that was it, thanks! I made the same mistake yesterday and didn't quite understand how I fixed it--that had been bugging me glad to have it resolved.
Makes sense, will do! |
@andrewrk Thanks for the feedback! I'll work through all the review comments, and then ping here again when I'm done. |
I hope this PR isn't abandoned! Excited to finally have a ZON parser in the stdlib |
@VisenDev it's not abandoned--this feature is very important to my work. I'm working on some other stuff w/ tight deadlines right now, but I'm planning on getting back to this in about a week and a half. |
* Gets all tests passing after rebase * Resolves a bunch of TODOs * WIP commit while I build release version of main * Errors on embedded nulls in identifiers * Parsing identifiers properly without allocating * Properly frees parsed idents * Fixes integer negation logic * Uses existing big int parser * Cleans up runtime int parsing * Some investigation into float issue * More float investigation * Adds missing test * Resolves TODOs * Fixes some error tokens * Cleans up remaining parsing todos * Removes TODOs * Excludes tests format formatting * Formats * Parses directly as desired float type * Moves zon lowering into own file Records result type for imports, does not yet use Fixes print zir for imports, better encoding of instruction Uses the result type to add indirection where necessary Does not yet remove & from syntax Removes & from comptime ZON syntax Yet to be removed from runtime syntax Fixes after rebase Allow coercion to slices, not to any other pointer type, adds tests No longer allows `&` in the runtime parser Also fixes merge Fixes bug where you could parse structs as tuples and vice versa Also cleans up error handling Reworks runtime error handling Explains usage of unreachable Fixes CI failure, and exposes notes in API Shows notes on integer literal errors when parsing ZON Fixes free of undefined value if struct with allocated fields is missing a field Skips tests failing due to C backend issue Notes why tests are skipped, renames to make easier to use test filter
…eds to be reworked anyway
Need to do ints next
…ump around in the code
Coercing enum literals to unions isn't yet implemented, and some of the union tests are still skipped due to a bug with explicit tags I still need to fix
FYI the last several CI runs have all failed the same way: compile errors when targeting a 32-bit system. You can easily reproduce with |
Thanks for the explanation! I don't expect the CI to succeed right now as I'm partway through refactoring to parse imports to a known result type. (Locally, the tests that I expect to pass at this point are all passing, and I expect all of them will on CI too once I'm done--if not I'll try this.) [EDIT] Totally missed that you said compile error. You explained in person, my last commit likely fixes the issue, if it doesn't I'll test it locally to sort it out. |
Also fixes compile error on 32 bit systems. All zon non compile error behavior tests now pass locally.
ZON
This PR implements ZON, or Zig Object Notation (think JSON, but with Zig syntax instead of Javascript.)
In particular, it implements:
A lot of files are added since I added a lot of test cases, the bulk of the work is in
src/zon.zig
andlib/std/zon
. If there's anything I can do to make this easier to review, let me know.Tests cover all new features.
Runtime
The runtime code can be found at
lib/std/zon
.Parsing
lib/std/zon/parse.zig
Detailed doc comments are provided in the source. At a high level, this module provides the following functions and some configuration options:
parseFromSlice
parseFromAst
parseFromAstNoAlloc
parseFromAstNode
parseFromAstNodeNoAlloc
parseFree
Most people will just want
parseFromSlice
and maybeparseFree
, but the more fine grained functions are available for those who needthem.
Stringify
lib/std/zon/stringify.zig
Detailed doc comments are provided in the source. At a high level, this module provides the following functions and some configuration options:
stringify
stringifyMaxDepth
stringifyArbitraryDepth
stringifier
Most users will just need stringify, or one of its variants.
However,
stringifier
returns a much more fine grained interface that is used in the implementation of the other functions. It may be used directly if you need to serialize something piece by piece--perhaps to apply different configuration to different parts of the value, or because the value does not actually exist laid out in memory in the same form in which you want to stringify it.Compile Time
Almost all of the logic for this is in
src/zon.zig
.This PR also implements importing ZON at compile time:
Things that may change later...
&
In ZON syntaxRight now, ZON slices need to be prefixed with&
to distinguish them from tuples. This is necessary because we don't know the result type when importing. In the future I think ZON should not distinguish between tuples and slices, it should just coerce to whatever the destination type is.Update:
&
is not allowed in ZON, ZON will automatically coerce tuples to arrays or slices when given a destination type.Untagged nonexhaustive enum values not yet supported
Zig does not currently have a way to represent an untagged enum value as a literal, and as such there's no way to represent one in ZON.
We could resolve this by adding a syntax for untagged enum literals to Zig, or by allowing ZON to coerce integers to untagged enum literals (provided the destination type is known.) Personally I prefer the former solution.
Type mismatches do not always point at helpful location
This is an existing issue, but it can make type errors in ZON imports annoying.
Consider this incorrect code:
It produces the following error:
This isn't super helpful. The location information should be pointed to the struct literal, or a note should be added that points to the struct literal.
This comes up all the time with ZON, e.g. any time you do something like this and make a mistake:
Allowing eliding outer braces may make sense
There's case to be made to allow eliding braces on an outer struct in ZON, like this:
Divergences from Zig
Zig has no NaN or inf literals right now. Zon does, since otherwise it would have no way to represent these values.
IMO we should add the same literals to Zig.
See Also
@import
.zon files #14531\x00
(null bytes) and empty string in@""
identifier syntax in the language specification #14534.
in tuples & anonymous struct literal syntax #5039