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: support field aliases #10193

Closed

Conversation

travisstaloch
Copy link
Contributor

  • this is a rework of std.json: support field aliases #8987. rebasing that old pr proved difficult so it
    was easier to just start over.
  • this patch has the same api and tests as std.json: support field aliases #8987 but uses an enums.EnumMap
    rather than StringArrayHashMap. this allows field_alias_map to be
    made entirely on the stack and requires no allocator (not even a
    FixedBufferAllocator which the previous used for re-indexing its map).

- this is a rework of ziglang#8987.  rebasing that old pr proved difficult so it
  was easier to just start over.
- this patch has the same api and tests as ziglang#8987 but uses an enums.EnumMap
  rather than StringArrayHashMap.  this allows `field_alias_map` to be
  made entirely on the stack and requires no allocator (not even a
  FixedBufferAllocator which the previous used for re-indexing its map).
@travisstaloch
Copy link
Contributor Author

Note: i wanted to create field_alias_map entirely at comptime but options.field_aliases is a runtime value which prevented this.

@daurnimator
Copy link
Contributor

I think any implementation of this needs to be 0 overhead at runtime if it's not used.

Otherwise, are there really situations where you want to rename a key at all depths? Wouldn't you usually only want to do so at specific points in the json tree/for certain types?

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Nov 21, 2021
@travisstaloch
Copy link
Contributor Author

I agree that this design doesn't account for fields w/ same names at different depths. One way to be entirely comptime and be specific to a single struct would be to move back to something closer the original __tags design such as:

    const S = struct {
        value: u8,
        pub const __field_aliases = &.{
            .{ .field = "value", .alias = "__value" },
        };
    };

We might run into comptime OOM again. If so, we can just delay until stage 2 comptime memory optimizations happen.

@travisstaloch
Copy link
Contributor Author

travisstaloch commented Nov 21, 2021

If this sounds workable, I would prefer this form as i find it a little more readable and may improve comptime perf by allowing to skip a meta.stringToEnum() call

    const S = struct {
        value: u8,
        pub const __field_aliases = .{
            .value = "__value",
        };
    };

@travisstaloch
Copy link
Contributor Author

@daurnimator this commit attempts to address your previous comment. I've attempted to move all the work to comptime and prevent the aliases from applying to sub-objects.

- this patch moves field_aliases from an `ParseOptions` field to a
  public decl of T.  this makes the aliases available at comptime and
  only apply to T.  now the aliases won't apply to matching fields in
  child json objects.
- interpret `pub const __field_aliases` decl as a `std.enums.EnumFieldStruct`
- this shrinks each of the the fields from 24 to 8 bytes
- remove extra if condition logic
@travisstaloch
Copy link
Contributor Author

@daurnimator or anyone else, did you notice this? any thoughts? i think it meets the requirements of happening entirely at comptime and not adding any runtime performance cost. also i've been holding off on documenting this until getting some feedback as to wether it might be accepted.

@andrewrk
Copy link
Member

andrewrk commented Dec 1, 2021

Looks fine to me. Question though, what's the point of field aliases? Couldn't you just name your fields @"anything you want"?

@travisstaloch
Copy link
Contributor Author

Using field aliases is nice when you want to de-serialize some arbitrarily named json data into existing structures. It promotes re-use. afaik, without this de-serialization into existing an struct requires creating a new struct with json-matching names along with some adapter to map between them.

@andrewrk
Copy link
Member

andrewrk commented Dec 2, 2021

In this case, wouldn't you want to pass in the aliases as a separate struct, rather than having it embedded into the struct? Otherwise you wouldn't be able to use a third party struct.

@travisstaloch
Copy link
Contributor Author

True. I hadn't thought about this. Perhaps we would move field_aliases back to ParseOptions then and don't pass them down recursively (in order to prevent them from applying to nested structs)?

@daurnimator
Copy link
Contributor

Perhaps we would move field_aliases back to ParseOptions then and don't pass them down recursively (in order to prevent them from applying to nested structs)?

I think we should be able to specify a "path" for when the substitution applies. When you go into a nested structure then you would trim off the path prefix as you pass the options down.

@andrewrk andrewrk closed this May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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