zig-protobuf icon indicating copy to clipboard operation
zig-protobuf copied to clipboard

make UnionDecodeError pub, replace usingnamespace

Open nat3Github opened this issue 8 months ago • 5 comments

wanting to try -fincremental with zig i stumbled across the use of usingnamespace with the message mixins in the autogenerated pb file.

this merge request changes:

  • makes UnionDecodeError pub that it can be used when replacing usingnamespace with the actual fn's
  • replaces the usingnamespace message mixins with hardcoded fn's
  • refactors jsonParse(...) to pub fn jsonParseT(T:type ...) for less code duplication
  • refactors jsonStringify(...) to pub fn jsonStringifyT(T:type ...) for less code duplication

i reccon that way it's a bit more work keeping the fn's in sync if anything changes in message mixins... this is up to you to decide.

i would be happy to see UnionDecodeError made pub on master regardless of the other changes, because then one could at least replace the usingnamespace's manually.

nat3Github avatar Apr 17 '25 07:04 nat3Github

what is the motivation to remove "usingnamespace"? it keeps the generated code minimal. small enpugh to be edited by hand and to describe structures only

menduz avatar Apr 17 '25 11:04 menduz

what is the motivation to remove "usingnamespace"? it keeps the generated code minimal. small enpugh to be edited by hand and to describe structures only

the motivation is that incremental compilation does not support "usingnamespace". so every library or project depending on this would not be able to use incremental compilation.

nat3Github avatar Apr 17 '25 12:04 nat3Github

Ok, got it. What do you think about the mixin approach described at https://github.com/ziglang/zig/issues/20663 instead of hardcoding all functions in each message?

menduz avatar Apr 17 '25 13:04 menduz

sure you could also do it like that. it depends which call semantics you want. changing that would change the api a bit. you could in fact just remove the mixin/usingnamespace alltogether and if someone needs a function like jsonParse/stringify/decode/etc. one just calls protobuf.jsonStringifyT(T,...) or protobuf.mixins(T).jsonStringify(...) or something like that.

that was just the most practical approach without changes to the api. however, in regards to the future adoption of incremental i think there is value in removing the usingnamespace one way or another.

nat3Github avatar Apr 17 '25 15:04 nat3Github

Sorry for being late to the party, it took me some time to look into it.

Ok, got it. What do you think about the mixin approach described at https://github.com/ziglang/zig/issues/20663 instead of hardcoding all functions in each message?

I personnally do not like the approach proposed here, and am not even sure it will be compatible with our code. In our mixins, we have functions that take an instance of the type (encoding) and some who doesn't (decoding). How are we supposed to handle this in a separate field? Also, this field will lead to stupid considerations such as us filtering it when analyzing the type in our various logic, and choosing a name that doesn't collide with a potential protobuf-defined field.

If we really have to get rid of using namespace, we sadly will have to move it to the generator if we want to keep the current interface as-is. It's not dramatic, but i'm a bit sad that this feature from zig is on the chopping block.

What is the status of the incremental compiler officially? I can't find a solid information of this so far. Considering it is optional, i'll see it as a WIP/Experimental feature for now, which would mean that this proposed change should hit the zig-master branch for now.

edit: Oh and @nat3Github, thanks a lot for your contribution, any feedback and PR is truly appreciated.

Arwalk avatar Apr 18 '25 06:04 Arwalk

@Arwalk usingnamespace has been removed in latest Zig master, necessitating changes. Should the moving-to-generator approach you mentioned be considered the canonical way forward?

mochalins avatar Jul 11 '25 00:07 mochalins

@Arwalk usingnamespace has been removed in latest Zig master, necessitating changes. Should the moving-to-generator approach you mentioned be considered the canonical way forward?

Yes. If you are motivated to make a PR for zig-master, I'd appreciate it. It shouldn't be that hard. Thanks for the heads up.

To anyone reading this, I'm very sorry for not being very active lately.

Arwalk avatar Jul 11 '25 05:07 Arwalk

@nat3Github i'm closing this because i'm handling it in #103 but i would like to thank you again for your contribution, and apologize for not handling it in a timely manner.

Arwalk avatar Aug 01 '25 19:08 Arwalk