Skip to content

Allow importing ZON without a result type (you can import build.zig.zon now) #22907

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 4 commits into from
Apr 2, 2025

Conversation

MasonRemaley
Copy link
Contributor

@MasonRemaley MasonRemaley commented Feb 16, 2025

Adds support for using import ZON without a result type.

@mlugg you should probably check over the logic for structs, as I imagine you were planning to anyway. In particular a few points I'm unsure about:

  • Is addTypeReferenceEntry necessary? (Maybe this is what you meant by I may need to register an incremental dependency.)
  • Imported types get a type name based on the first import location, this seems wrong they should probably be based on the ZON file (I think I can just write my own logic for this instead of using Sema.createTypeName but let me know your thoughts)
  • Side note: Zoir and LowerZon/std.zon are inconsistent in how they refer to tuples/arrays. Zoir calls them arrays, LowerZon/std.zon calls them tuples. Not a huge deal but I can resolve this one way or the other, let me know if you have an opinion either way.

For those who want to import build.zig.zon, remember to add it as a module. You can't access it via ../build.zig.zon since this is outside the module path. This is not necessary for ZON files in the module path.

build.zig

const build_zig_zon = b.createModule(.{
    .root_source_file = b.path("build.zig.zon"),
    .target = target,
    .optimize = optimize,
});
exe.root_module.addImport("build.zig.zon", build_zig_zon);

main.zig

const std = @import("std");
const build = @import("build.zig.zon");

pub fn main() !void {
    std.debug.print("Version: \"{s}\"\n", .{build.version});
}

@alexrp alexrp added this to the 0.14.0 milestone Feb 16, 2025
@cryptocode
Copy link
Contributor

In #22775 it was suggested that the build.zig.zon format should maybe be changed to be representable directly in the Zig type system.

With this PR, iterating through dependencies does work, though only through meta programming since the field is not of an indexable type:

    const deps = std.meta.fields(@TypeOf(zon.dependencies));
    inline for (deps) |d| {
        const dep = @field(zon.dependencies, d.name);
        ... etc
    }

Then again, accessing dependencies might have such rare usecases that maybe it's fine.

@MasonRemaley
Copy link
Contributor Author

This currently crashes on the output of #22939, looking into it.

@MasonRemaley
Copy link
Contributor Author

MasonRemaley commented Feb 19, 2025

Fixed. Struct field values are now allocated from the sema arena, similar to how it's done in sema for anonymous types. I didn't realize this was necessary, I thought the values from the struct init would still be around. Still not fully sure I understand the situation here and would be happy to have it explained to me!

Copy link
Member

@mlugg mlugg left a comment

Choose a reason for hiding this comment

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

Nearly there! There were one or two things here I said I'd do. I'll also write you up an incremental test case to throw in to test/incremental/.

.inits_resolved = true,
.any_aligned_fields = false,
.key = .{ .reified = .{
.zir_index = self.base_node_inst,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure if this is safe; might need some changes in places where we use this zir_index just to make sure that it's handling the ZON case. (I'll handle that.)

@mlugg
Copy link
Member

mlugg commented Feb 20, 2025

Just to explain this:

Fixed. Struct field values are now allocated from the sema arena, similar to how it's done in sema for anonymous types. I didn't realize this was necessary, I thought the values from the struct init would still be around. Still not fully sure I understand the situation here and would be happy to have it explained to me!

The slice you get from field_inits.get(ip) (and similar calls) is ephemeral; it's invalidated when the InternPool resizes. That can potentially happen whenever something is interned (InternPool.get / Zcu.PerThread.intern / Zcu.PerThread.xyzValue / etc). As such, what was probably happening before was that your recursive lowerExprAnonResTy was (since it itself interned some stuff) triggering an InternPool resize, meaning the rest of the values you wrote were in old memory (that old memory actually would still have been mapped/alloced, weird quirk of the thread-safe InternPool), so the rest of the code wouldn't see them (and would instead see the undefined/.none values which were originally there).

@MasonRemaley
Copy link
Contributor Author

MasonRemaley commented Feb 20, 2025

Ah that makes a lot of sense, thank you for walking me through it! That explains why I was only triggering it on a very large import (it was presumably putting enough stuff in intern pool to cause it to realloc.)

@MasonRemaley
Copy link
Contributor Author

I just noticed that I'm getting this log message while running the tests:

error: warning(zcu): unexpected EOF reading cached ZIR for zon/large_number.zon

I'm guessing this is related to the changes you already mentioned need to be made WRT the ZIR index?

I'll also write you up an incremental test case to throw in to test/incremental/.

Good idea, thanks!

@mlugg
Copy link
Member

mlugg commented Feb 20, 2025

I just noticed that I'm getting this log message while running the tests:

error: warning(zcu): unexpected EOF reading cached ZIR for zon/large_number.zon

Hm, that's concerning. I'll take a look at this locally when I can -- mind writing out the exact repro?

@MasonRemaley
Copy link
Contributor Author

I can reliably repro it like this:

  1. Build the compiler
build/stage3/bin/zig build -p build/stage4 -Denable-llvm -Dno-lib -Ddebug-extensions --prominent-compile-errors
  1. Run the ZON behvaior tests:
build/stage4/bin/zig test test/behavior/zon.zig
  1. Run the ZON behavior tests a second time emits the warning:
build/stage4/bin/zig test test/behavior/zon.zig

Running the tests a third time does not emit the warning.

@MasonRemaley
Copy link
Contributor Author

Alright, mlugg helped me fix the above warning. The math on the code writing the big int limbs for zoir was slightly off, and the file wasn't properly cleared on error--both issues are fixed now.

@andrewrk andrewrk removed this from the 0.14.0 milestone Mar 1, 2025
@mlugg mlugg force-pushed the import-zon-anon-type branch 2 times, most recently from 980c513 to b454171 Compare March 12, 2025 02:31
MasonRemaley and others added 4 commits April 2, 2025 05:53
In particular, this allows importing `build.zig.zon` at comptime.
* When saving bigint limbs, we gave the iovec the wrong length, meaning
  bigint data (and the following string and compile error data) was corrupted.
* When updating a stale ZOIR cache, we failed to truncate the file, so
  just wrote more bytes onto the end of the stale cache.
`wasm2c` uses an interesting mechanism to "fake" the existence of cache
directories. However, `wasi_snapshot_preview1_fd_seek` was not correctly
integrated with this system, so previously crashed when run on a file in
a cache directory due to trying to call `fseek` on a `FILE *` which was
`NULL`.
@mlugg mlugg force-pushed the import-zon-anon-type branch from b454171 to dd3f01e Compare April 2, 2025 04:58
@mlugg
Copy link
Member

mlugg commented Apr 2, 2025

Sorry for the delay in getting to this! I've fixed a minor bug related to the zir_index on generated structs, added an incremental case, and squashed most of the commits together. I'll set this to auto-merge now; thanks!

@alexrp, since you've been doing a bunch of 0.14.1 cherry-picking and I'll probably forget... 8720995 and d8ac37f should be cherry-picked into 0.14.1, since they fix a kinda-significant bug. The other commits here shouldn't; they introduce a new feature.

@mlugg mlugg enabled auto-merge April 2, 2025 05:00
@alexrp alexrp added this to the 0.14.1 milestone Apr 2, 2025
@mlugg mlugg merged commit 4303b43 into ziglang:master Apr 2, 2025
17 of 18 checks passed
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.

5 participants