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

name collisions #8

Open
robx opened this issue Jan 17, 2018 · 3 comments
Open

name collisions #8

robx opened this issue Jan 17, 2018 · 3 comments

Comments

@robx
Copy link
Contributor

robx commented Jan 17, 2018

Here's some protobuf for which the elm protobuf generator results in elm code that fails to compile. For one, the MATCH enum value and the match oneof field collide. And after fixing that, I get collisions between for example the Change message type alias and the change oneof field.

I can get around this for now by renaming things (and I realize the README checklist lists oneof as unsupported), but seems worth addressing at some point? And I hope a "real-world" test case might be good to have around anyway.

syntax = "proto3";

enum ClaimType {
  MATCH = 0;
  NOMATCH = 1;
}

message Claim {
  ClaimType type = 1;
  repeated uint32 cards = 2;
}

message Event {
  message Join {
    string name = 1;
  }
  message Claimed {
    string name = 1;
    ClaimType type = 2;
    enum Result {
      CORRECT = 0;
      WRONG = 1;
      LATE = 2;
    }
    Result result = 3;
  }   
  oneof event_oneof {
    Join join = 1;
    Claimed claimed = 2;
  }
}

message Change {
  message Position {
    uint32 x = 1;
    uint32 y = 2;
  }
  message Deal {
    message Place {
      Position position = 1;
      uint32 card = 2;
    }
    repeated Place places = 1;
  }
  message Match {
    repeated Position positions = 1;
  }
  message Move {
    message MoveOne {
      Position from = 1;
      Position to = 2;
    }
    repeated MoveOne moves = 1;
  }
  oneof change_oneof {
    Deal deal = 1;
    Match match = 2;
    Move move = 3;
  }
}

message Update {
  oneof update_oneof {
    Change change = 1;
    Event event = 2;
  }
}
@robx
Copy link
Contributor Author

robx commented Jan 17, 2018

The compiler errors:

-- DUPLICATE DEFINITION -------------------------------- ./src/Proto/Triples.elm

Naming multiple top-level values `Change` makes things ambiguous. When you say
`Change` which one do you want?

225|>type alias Change =
226|>    { changeOneof : ChangeOneof
227|>    }

Find all the top-level values named `Change` and do some renaming. Make sure the
names are distinct!

-- DUPLICATE DEFINITION -------------------------------- ./src/Proto/Triples.elm

Naming multiple top-level values `Event` makes things ambiguous. When you say
`Event` which one do you want?

78|>type alias Event =
79|>    { eventOneof : EventOneof
80|>    }

Find all the top-level values named `Event` and do some renaming. Make sure the
names are distinct!

-- DUPLICATE DEFINITION -------------------------------- ./src/Proto/Triples.elm

Naming multiple top-level values `Match` makes things ambiguous. When you say
`Match` which one do you want?

13|>type ClaimType
14|>    = Match -- 0
15|>    | Nomatch 

Find all the top-level values named `Match` and do some renaming. Make sure the
names are distinct!

Detected errors in 1 module.                                        

@ftc
Copy link

ftc commented Aug 15, 2018

This is an issue for me as well. I created a simplified proto file that exhibits this problem:

syntax = "proto3";

package com.example;

message FirstMessage {
        oneof same_fieldname{
                string foo = 1;
                int32 bar = 2;
        }

}
message SecondMessage{
        oneof same_fieldname{
                int64 baz = 1;
                string bip = 2;
        }
}

All of the files to reproduce this bug are uploaded here:
ElmProtoBug.zip

Running elm-reactor and navigating to http://localhost:8000/src/Bad.elm reproduces the problem.

Error output

Starting downloads...

  ● elm-lang/virtual-dom 2.0.4
  ● tiziano88/elm-protobuf 2.1.0
  ● elm-lang/core 5.1.1
  ● elm-lang/html 2.0.0
  ● jweir/elm-iso8601 
3.0.2
Packages configured successfully!
Detected errors in 1 module.


-- DUPLICATE DEFINITION -------------------------------------------- src/Bad.elm

Naming multiple top-level values `sameFieldnameDecoder` makes things ambiguous.
When you say `sameFieldnameDecoder` which one do you want?

70| sameFieldnameDecoder =
    ^^^^^^^^^^^^^^^^^^^^
Find all the top-level values named `sameFieldnameDecoder` and do some renaming.
Make sure the names are distinct!



-- DUPLICATE DEFINITION -------------------------------------------- src/Bad.elm

Naming multiple top-level values `sameFieldnameEncoder` makes things ambiguous.
When you say `sameFieldnameEncoder` which one do you want?

79| sameFieldnameEncoder v =
    ^^^^^^^^^^^^^^^^^^^^
Find all the top-level values named `sameFieldnameEncoder` and do some renaming.
Make sure the names are distinct!



-- DUPLICATE DEFINITION -------------------------------------------- src/Bad.elm

Naming multiple top-level values `SameFieldnameUnspecified` makes things
ambiguous. When you say `SameFieldnameUnspecified` which one do you want?

19|>type SameFieldname
20|>    = SameFieldnameUnspecified
21|>    | Foo String
22|>    | Bar Int

Find all the top-level values named `SameFieldnameUnspecified` and do some
renaming. Make sure the names are distinct!



-- DUPLICATE DEFINITION -------------------------------------------- src/Bad.elm

There are multiple types named `SameFieldname` in this module.

19|>type SameFieldname
20|>    = SameFieldnameUnspecified
21|>    | Foo String
22|>    | Bar Int

Search through this module, find all the types named `SameFieldname`, and give
each of them a unique name.

Workaround: Changing the proto file to the following fixes the problem

syntax = "proto3";

package com.example;

message FirstMessage {
        oneof same_fieldname{
                string foo = 1;
                int32 bar = 2;
        }

}
message SecondMessage{
        oneof different_fieldname{
                int64 baz = 1;
                string bip = 2;
        }
}

@tiziano88
Copy link
Owner

Thanks for the comments! I think the correct thing to do here would be to rename the generated types to contain the parent message name. I will give it a try and post updates here. Thanks 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

No branches or pull requests

3 participants