Skip to content

Add json_rename support for enums.#2

Open
blazingzephyr wants to merge 5 commits into
sakakibara:mainfrom
blazingzephyr:enum_json_rename
Open

Add json_rename support for enums.#2
blazingzephyr wants to merge 5 commits into
sakakibara:mainfrom
blazingzephyr:enum_json_rename

Conversation

@blazingzephyr

Copy link
Copy Markdown

Self-explainatory, had to settle on some runtime checks for this, but other than that, this is a small change.

            // Build comptime table for renames which will then be checked runtime.
            const lookup_table = comptime blk: {
                if (TAnnotation.getOrEmpty(T)) |a| {
                    if (a.json_rename) |r| break :blk r;
                }
                if (@hasDecl(T, "json_rename")) {
                    const renames = T.json_rename;
                    var entries: []const annotation_mod.TypeAnnotationRename = &.{};

                    for (@typeInfo(@TypeOf(renames)).@"struct".decls) |d| {
                        entries = entries ++ &[_]annotation_mod.TypeAnnotationRename{
                            .{ .from = @field(renames, d.name), .to = d.name },
                        };
                    }
                    break :blk entries;
                }
                break :blk &.{};
            };

            const entry = for (lookup_table) |rf| {
                if (std.mem.eql(u8, rf.from, s)) break rf.to;
            } else s;

            if (std.meta.stringToEnum(T, entry)) |v| return v;

Depends on annotation provider, so please merge it first.

@blazingzephyr

Copy link
Copy Markdown
Author

Just wanted to mention this shortly, I don't like how this is implemented and this is more-so a feature request than a pull request, because as of now runtime evaluation of the fields greatly worsens the performance.

@sakakibara sakakibara left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for following up with enum support. Keeping json_rename consistent across structs, tagged unions, and enums makes sense.

I reviewed the enum-specific commit on top of #1 and ran focused decode/encode probes. Please update this PR after the changes requested in #1 land so the final diff uses the revised annotation API. The current head also inherits #1's zig build test compile failure.

Before merging, please also:

  • Add matching encodeTyped support so enum annotations remain symmetric between decoding and encoding.
  • Use replacement semantics for renamed enum values, consistent with the existing json_rename behavior. A renamed value should not also accept its original Zig tag.
  • Apply the same precedence contract as #1: a non-null provider rename list overrides type-local renames, while null falls back to them.
  • Add tests for type-local and provider-based renames, their precedence and fallback, streaming and tree decoding, typed encoding, and round trips.
  • Validate invalid local and provider enum entries at comptime, including duplicate or ambiguous JSON names.
  • Keep invalid-value diagnostics tied to the received JSON string rather than replacing it with an internal Zig tag name.
  • Name the computed table renames, name the resolved value tag_name, and remove the comment that narrates how the lookup is performed.
  • Document enum renaming and add an Unreleased changelog entry.
  • After #1 merges, rebase this branch onto the updated main before final review.

I left inline comments on the type-local lookup and replacement semantics.

Comment thread src/decode.zig
const renames = T.json_rename;
var entries: []const annotation_mod.TypeAnnotationRename = &.{};

for (@typeInfo(@TypeOf(renames)).@"struct".decls) |d| {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This lookup does not see entries in a type-local annotation such as .{ .slow = "S" }. Those entries are anonymous-struct fields; struct.decls is empty.

Please build this table from struct.fields and run the same comptime annotation validation used by the other annotated types. A focused type-local enum test should cover both the mapping and invalid-entry validation.

Comment thread src/decode.zig

const entry = for (lookup_table) |rf| {
if (std.mem.eql(u8, rf.from, s)) break rf.to;
} else s;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Falling back to s here means that a renamed enum value still accepts its original Zig tag, so json_rename becomes an alias rather than a replacement.

Please keep this consistent with the existing rename behavior: use the renamed wire value when one is declared, and reject the original tag. Add a test that distinguishes those two inputs.

@sakakibara

Copy link
Copy Markdown
Owner

Sorry, I missed your note above that you view this as more of a feature request than a merge-ready pull request, and that you are not satisfied with the runtime cost of the current lookup.

Please do not treat my review as an expectation that you rewrite the implementation. It can serve as acceptance criteria for the eventual feature.

I would be happy to take this on after #1 is settled. If you would prefer to continue working on it yourself, please let me know; otherwise, no response is needed.

The likely direction is to build the complete effective JSON-name-to-enum-value mapping at comptime and perform a single effective-name lookup at runtime. This can follow the same strategy as std.meta.stringToEnum: StaticStringMap for ordinary enum sizes and an inline-comparison fallback for larger enums.

If I pick it up, I will benchmark annotated decoding against the unannotated path before settling the implementation.

Thank you for raising the use case and providing the initial exploration.

@blazingzephyr

Copy link
Copy Markdown
Author

I could look further into this but I will have to finish #1 first and this is going to take some time to get into, but I would love to cooperate and work on this with you. I will share updates when there will be any. Thank you for your patience

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.

2 participants