Name clashes about protoc-gen-elm HOT 21 CLOSED

I just did a small test of the gRPC generation in a test project. It works well, and it is really easy to get going. I don't think I have played with it enough to come with any sort of constructive feedback, but first impression is great.

What I plan to do is to make a branch in one of my bigger projects and migrate to this version, and then replace my own gRPC code with this, and then I might have some more thoughts.

from protoc-gen-elm.

What's your use case?

For one piece of response data, I just want to fall back to some sane defaults if it can't be transformed according to a set of criteria. I figured the generated default functions provided those sane defaults for my use case. I'm sure there are other use cases.

from protoc-gen-elm.

[email protected] should generate re-exported "default..." functions

from protoc-gen-elm.

I just released 3.0.0 so I'm going to close this.

from protoc-gen-elm.

Your idea of using files as a replacement of (otherwise not available) namespaces for types is pretty cool.

Let's take this a bit further.
Protoc complains if there any top level messages that have a name collision.
So instead of generating a file Proto.Messages for a file messages.proto, we could instead generate a file per message, i.e. Proto.OrderStatus. Nested types could then go into a nested directory - Proto.OrderStatus.Processing.

I would tend to not add a new module for oneofs and instead use "_" (just like currently) to make name collisions unlikely.

What do you think?

from protoc-gen-elm.

I think underscores are fine if the goal is to keep the module tree flat. Although very long identifiers might be annoying to refer to in code.

The structure I had in mind with generating more modules was something like: keep the "one Elm module per protofile" structure we have today, and in addition generate a submodule for each message type that has nested data types. So if OrderStatus is in messages.proto, generate a Proto.Messages module where OrderStatus lives, and also generate a Proto.Messages.OrderStatus module in which Processing etc. live. Thinking about it, it seems that can still cause name clashes with protobuf packages. I wonder how Prost handles that. Let me check that out.

I'm not sure what's best, honestly.

from protoc-gen-elm.

I used some Prost and Tonic recently and quite like the way they handle the scoping. I'll try to implement equivalent code generation for Elm:

• Generate one file per package
• Generate one file for each message with nested types inside it
• Use Underscores to namespace enums

For your example (assuming package order, protoc-gen-elm would generate:

• A file Proto/Order.elm:
  • A type alias OrderStatus with a field status:
    type alias OrderStatus = { status: Maybe OrderStatus_Status }
  • A custom type OrderStatus_Status:

    type OrderStatus_Status = 
        OrderStatus_Processing Proto.Order.OrderStatus.Processing
        | OrderStatus_Succeeded Proto.Order.OrderStatus.Succeeded
        | OrderStatus_Failed Proto.Order.OrderStatus.Failed
    

• A file Proto/Order/OrderStatus.elm
  • A type alias for each the inner messages: Processing, Succeeded and Failed.

from protoc-gen-elm.

I ran into a tricky recursive modules problem:

In protobuf, the following file is completely fine:

message Message {
  bool idk = 1;
}

message Oneof {
  oneof msg {
    Message opt1 = 1;
    Message opt2 = 2;
  }
}

but in prost's generated code (and what I wanted to do as well),
this generates two modules: A parent module, and a child module for the OneOf message.
The child module then defines a union type and en/decoders for the oneof msg part.

This seems fine, but since a type from the outer scope is used, we have to import the parent module from the child module, which results in a recursive import.
In this particular case, the issue can be solved by just using a type alias, but once a union type comes into play, the types are no longer structurally equivalent and we cannot make the code compile without importing.

So it seems like we can only solve this by using handcrafted namespacing via "_" seperators in the parent module and then export more readable aliased versions in the child module. We could also use some kind of .Internal module to prevent the ugly names from making the IDE experience worse.

from protoc-gen-elm.

Right! That's an interesting case.

Could this be solved by not generating a new module for oneof fields? Or in general, not for fields at all. In other words, only generate a new module for messages that have other messages or enums declared within them?

So in the above case, we'd still end up with only one module, as there is no nested message in there, and the en/decoders for oneof msgs would also reside in this one module...

Actually, no that still wouldn't work in the case where a nested message refers to an outer message in the same package. I guess this is fine in Prost, because cyclic dependency graphs of modules are not illegal in Rust. But the same is not true of Elm.

The internal modules approach could be interesting to explore (and would be even better if Elm allowed us to expose a module to only a particular set of modules without wrapping it all in a library, or if you could have local libraries). Do you have a particular approach in mind? I'm guessing you mean having a module where the generator just dumps all type definitions, namespaced with underscores, and then expose type aliases that refer to them in the more structured modules?

from protoc-gen-elm.

Yeah exactly, I'll just dump everything in one file. So far this approach seems to work out. The code is extremely ugly but since it's generated and not the "public API" anyways, it should be fine.

from protoc-gen-elm.

Hmm while messages work fine, enums are problematic with this approach.

If we for example define an enum

package Test;

enum E {
  A = 0;
  B = 1;
}

we can generate code in the internals module like

type Proto__Test__E = Proto__Test__E_A | Proto__Test__E_B

but we lose the ability to pattern match when we define an alias.

Do you have an idea how we could solve this?

from protoc-gen-elm.

That's a shame. And it's not only enums, then, but also oneof variants. Honestly, I can't think of a good solution off the top of my head. Maybe it isn't meant to be...

Maybe the best thing we can hope for really is to use prefixes as namespaces within a package. Perhaps the best we can do to avoid name clashes is my first suggestion in the OP:

Prefix each variant with the custom type name as well (type OrderStatus_Status = OrderStatus_Status_Failed OrderStatus_Failed | ...)

If nothing else it's a first step, and as far as I can see, a workable - if not quite ideal - solution to the name clash problem.

from protoc-gen-elm.

I'll try experimenting with remapping the union types via a generated function.
In that case, as a user you will have to remember to call the generated conversion functions but the code should still look nicer:

Instead of

import Proto.PackageName exposing (MsgName_OneofName(..))

case oneOf of
  MsgName_OneofName_OptionA _ -> ...
  MsgName_OneofName_OptionB _ -> ...

import Proto.PackageName.OneofName exposing (convertOneOfName, OneOfName(..))

case convertOneOfName oneOf of
  OptionA _ -> ...
  OptionB _ -> ...

What do you think? Can you think of good names for the converters in the two directions?
The generated documentation for the types should probably mention the two generated functions.

from protoc-gen-elm.

I have published [email protected] which implements a more advanced module system which is package/message scope based instead of file based. You can try it out and see if it fixes the issue for you.
I'll need to add some tests and documentation, but so far it looks pretty good.
Side note if you are using tonic as well: grpc generation should work as well with the env variable EXPERIMENTAL_GRPC=true

from protoc-gen-elm.

Alright, thank you. I'll check it out and give you some feedback over the weekend.

from protoc-gen-elm.

I didn't know this had gRPC support. I've just rolled my own implementation of the gRPC-Web protocol in Elm so far, using this library as support for encoding/decoding messages.

from protoc-gen-elm.

I did a small amount of testing, and have some thoughts.

1. I discovered another edge case resulting in name clashes. Consider the following message:

   message TestRequest {
     message Inner {
       message Inner {}
     }
   
     oneof inner {
       string s = 1;
       uint32 n = 2;
     }
   }

   The type generated by the oneof (TestRequest.Inner.Inner) competes with the type generated by the inner inner message (also TestRequest.Inner.Inner). I thought some sort of a prefixing scheme for oneofs might help, but it seems like there would always be the possibility of a name clash, and it would make the resulting API worse, which is what we were trying to avoid in the first place. That said, I suppose one way to look at it is that we want to eliminate the vast majority of name clash cases for the majority of reasonably sane protobuf code, rather than to guarantee no name clashes 100% of the time.

2. The inconsistencies in how oneofs are generated compared to inner messages irks me a bit. I realise this is the original solution we were discussing in this thread, but it does feel a bit strange, doesn't it? That oneofs always generate a new module, whereas inner messages don't necessarily. I wonder if this is a case where we should just say "good enough", and get on with our lives.

I suppose, number 1 here is also technically a manifestation of the original problem in this thread, in a way, although certainly more obscure. It only happens when you have a message inside a message inside a message where the two innermost have the same name, and you have a oneof in the outermost message with the same name as the other two messages. How often does that happen? (famous last words)

All that said, I really like how the recent changes have lead to a nicer API. It looks so much cleaner, and I think it addresses the most common of name clash cases. Thank you for doing this!

from protoc-gen-elm.

Regarding the new name clash case, I just noticed prost/tonic has a similar issue, although there it's a problem even without the crazy nesting. I'd say it's not worth it to spend too much time on this.

from protoc-gen-elm.

Yeah, it's pretty much impossible to avoid all name clashes and generate a reasonable module graph. I think it's fine though, as you said, the standard use cases are clash-free and you kind of have to try hard to break it.

As long as there isn't a good use case for stuff like doubly nesting messages with the same name as a oneof or using the same name with different casing (innerMsg vs InnerMsg vs inner_msg...), I'm fine with what we have right now.

Regarding 2) I would tend to live with the inconsistency.

Did you try the Grpc generation? I put it behind a flag because I wanted to experiment with the API with my own app. If you have thoughts on how to improve it feel free to open a new issue :)

I love the feedback :D

from protoc-gen-elm.

Hey. gRPC works well. I ported my entire project and haven't run into any problems.

One comment unrelated to gRPC: I'm missing the functions to get default values for messages. They are only in internal now. It would be nice to have them available outside of internal, as well.

from protoc-gen-elm.

Hey. gRPC works well. I ported my entire project and haven't run into any problems.

That's great to hear!

One comment unrelated to gRPC: I'm missing the functions to get default values for messages. They are only in internal now. It would be nice to have them available outside of internal, as well.

Ah interesting, I didn't need the default values so far, so I didn't bother to re-export them.
What's your use case?

from protoc-gen-elm.