Skip to content

std.zig.fmtId: conditionally escape primitives/_ (breaking) #18920

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 3 commits into from
Apr 8, 2024

Conversation

castholm
Copy link
Contributor

This updates std.zig.fmtId to support conditionally @"" escaping primitives and the reserved _ identifier via format specifiers:

  • {}: escape invalid identifiers, identifiers that shadow primitives and the reserved _ identifier.
  • {p}: same as {}, but don't escape identifiers that shadow primitives.
  • {_}: same as {}, but don't escape the reserved _ identifier.
  • {p_} or {_p}: only escape invalid identifiers.

In other words, the default empty {} specifier is as conservative as possible and you add p and/or _ to add primitives/_ to the set of identifiers that can be rendered unescaped.

Any other format specifier is a compile error.

Motivation

Correctly formatted and escaped code generation. Status quo fmtId is problematic because it doesn't escape primitives, which means programs that generate Zig code currently need to implement their own formatters that also check std.zig.primitives.isPrimitive for rendering declaration identifiers.

Breaking changes and mitigations

  • print("{}", .{std.zig.fmtId(foo)}) did not @"" escape primitives before, but now does. Use {p} for the old behavior.
  • print("{s}", .{std.zig.fmtId(foo)}) (or any other unrecognized specifier) is now a compile error. Again, use {p} for the old behavior.
  • isValidId now considers _ to be a valid identifier (which it is, in certain contexts). If this distinction is important, consider combining existing uses of this function with the new isUnderscore function; std.zig.isValidId(foo) should be replaced with std.zig.isValidId(foo) and !std.zig.isUnderscore(foo).

Additional considerations

fmtId can currently render illegal identifiers like @"" and @"\x00". It could be updated to return an error for these cases, but I would suggest accepting #14534 instead since it would ease the burden of code generators and mean that every possible identifier can be represented in Zig in some form, either escaped or unescaped.

castholm added 2 commits April 7, 2024 14:47
This is a breaking change.

This updates `std.zig.fmtId` to support conditionally escaping
primitives and the reserved `_` identifier via format specifiers:

- `{}`: escape invalid identifiers, identifiers that shadow primitives
  and the reserved `_` identifier.
- `{p}`: same as `{}`, but don't escape identifiers that
  shadow primitives.
- `{_}`: same as `{}`, but don't escape the reserved `_` identifier.
- `{p_}` or `{_p}`: only escape invalid identifiers.

(The idea is that `p`/`_` mean "allow primitives/underscores".)

Any other format specifiers will result in compile errors.

Additionally, `isValidId` now considers `_` a valid identifier. If this
distinction is important, consider combining existing uses of this
function with the new `isUnderscore` function.
`{}` for decls
`{p}` for enum fields
`{p_}` for struct fields and in contexts following a `.`

Elsewhere, `{p}` was used since it's equivalent to the old behavior.
This might fix a CI failure for powerpc64le-linux-musl.
@castholm
Copy link
Contributor Author

castholm commented Apr 7, 2024

test
+- test-c-abi
   +- run test-c-abi-powerpc64le-linux.4.19...6.5.7-musl-ppc64le-ReleaseSafe
      +- zig test test-c-abi-powerpc64le-linux.4.19...6.5.7-musl-ppc64le-ReleaseSafe ReleaseSafe powerpc64le-linux-musl 1 errors
/home/ci/actions-runner10/_work/zig/zig/lib/compiler/test_runner.zig:111:30: error: no field named 'log_err_count' in enum 'meta.FieldEnum(zig.Server.Message.TestResults.Flags)'
/home/ci/actions-runner10/_work/zig/zig/lib/std/meta.zig:563:12: note: enum declared here
error: the following command failed with 1 compilation errors:
/home/ci/actions-runner10/_work/zig/zig/build-debug/stage3-debug/bin/zig test -cflags -std=c99 -- /home/ci/actions-runner10/_work/zig/zig/test/c_abi/cfuncs.c -OReleaseSafe -target powerpc64le-linux-musl -mcpu baseline -Mroot=/home/ci/actions-runner10/_work/zig/zig/test/c_abi/main.zig -lc --cache-dir /home/ci/actions-runner10/_work/zig/zig/build-debug/zig-local-cache --global-cache-dir /home/ci/actions-runner10/_work/zig/zig/build-debug/zig-global-cache --name test-c-abi-powerpc64le-linux.4.19...6.5.7-musl-ppc64le-ReleaseSafe -L /home/ci/deps/zig+llvm+lld+clang-aarch64-linux-musl-0.12.0-dev.203+d3bc1cfc4/lib -I /home/ci/deps/zig+llvm+lld+clang-aarch64-linux-musl-0.12.0-dev.203+d3bc1cfc4/include --zig-lib-dir /home/ci/actions-runner10/_work/zig/zig/lib -fno-lto --listen=- 
Build Summary: 4581/4899 steps succeeded; 314 skipped; 1 failed; 93047/[97](https://github.com/ziglang/zig/actions/runs/8588945925/job/23534425359#step:3:98)252 tests passed; 4205 skipped (disable with --summary none)

I have absolutely no idea why this failed on the Linux runners, I don't see how the changes in this PR are related to std.meta reflection in test_runner.zig and I'm pretty sure I've audited all usages of fmtId at least three times over now.

I replaced it with @TypeOf(@as(std.zig.Server.Message.TestResults.Flags, undefined).log_err_count), let's see if CI passes now.

@andrewrk
Copy link
Member

andrewrk commented Apr 7, 2024

I have absolutely no idea why this failed on the Linux runners, I don't see how the changes in this PR are related to std.meta reflection in test_runner.zig and I'm pretty sure I've audited all usages of fmtId at least three times over now.

I replaced it with @TypeOf(@as(std.zig.Server.Message.TestResults.Flags, undefined).log_err_count), let's see if CI passes now.

Related:

@andrewrk andrewrk enabled auto-merge April 7, 2024 21:37
@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Apr 7, 2024
@andrewrk andrewrk merged commit 355ccee into ziglang:master Apr 8, 2024
10 checks passed
@castholm castholm deleted the fmtId branch April 8, 2024 14:37
ehaas added a commit to ehaas/arocc that referenced this pull request Apr 9, 2024
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. 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.

3 participants