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

Jump to bottom

C# identifier deduplification fails in corner case #309

Closed
jskeet opened this issue Apr 28, 2015 · 3 comments
Closed

C# identifier deduplification fails in corner case #309

jskeet opened this issue Apr 28, 2015 · 3 comments
Assignees
@jskeet
Labels
c#

Comments

@jskeet
Copy link
Contributor

jskeet commented Apr 28, 2015 •
edited

Sample proto:

syntax = "proto3";

package foo;

message A {
    int32 _A = 1;
}

Note that A and _A are the same after underscore stripping, so the deduplification code adds an underscore to the property name - which then collides with the field name:

private int A_;
public bool HasA_ {
  get { return hasA_; }
}
public int A_ {
  get { return A_; }
}

This was originally a problem in the previous C# code gen too:
jskeet/protobuf-csharp-port#13

Looks like it was fixed by this change:
jskeet/protobuf-csharp-port@c7b23c1

(That may not be directly applicable in the C++ codebase, but it's a good starting point.)
jskeet added a commit to jskeet/protobuf that referenced this issue Apr 28, 2015
@jskeet
Updated set of unit tests and unit test protos.
ce66c5f 
This commit includes changes to the C#-specific protos, and rebuilt versions of the "stock" protos.
The stock protos have been locally updated to have a specific C# namespace, but this is expected to change soon, so hasn't been committed.
Four areas are currently not tested:
1) Serialization - we may restore this at some point, possibly optionally.
2) Services - currently nothing is generated for this; will need to see how it interacts with GRPC
3) Fields beginning with _{digit} - see protocolbuffers#308
4) Fields with names which conflict with the declaring type in nasty ways - see protocolbuffers#309
@anandolee anandolee added the c# label Apr 28, 2015
@jskeet jskeet added c# and removed c# labels Apr 29, 2015
@jskeet jskeet self-assigned this Aug 3, 2015
@anandolee
Copy link
Contributor

@jskeet, we are cleaning up issues. Can you help to verify if this is fixed?

@jskeet
Copy link
Contributor Author

jskeet commented Mar 6, 2017 •
edited

No, this is still broken. I've updated the sample .proto file, but we still end up with:

private int A_;
public int A_ {
    get { return A_; }
    set {
        A_ = value;
    }
}

It might be okay to close it as "won't fix" though... (Trying to make every single possible field and name combination work is a fool's errand, IMO... by the time you bear in mind all possible keywords, the autogenerated names etc. It would end up with very complicated code for relatively little benefit.)

@anandolee
Copy link
Contributor

Will not fix it.

xykong pushed a commit to xykong/protobuf-for-unity that referenced this issue Feb 24, 2018
@jskeet
Updated set of unit tests and unit test protos.
efb713a 
This commit includes changes to the C#-specific protos, and rebuilt versions of the "stock" protos.
The stock protos have been locally updated to have a specific C# namespace, but this is expected to change soon, so hasn't been committed.
Four areas are currently not tested:
1) Serialization - we may restore this at some point, possibly optionally.
2) Services - currently nothing is generated for this; will need to see how it interacts with GRPC
3) Fields beginning with _{digit} - see protocolbuffers/protobuf#308
4) Fields with names which conflict with the declaring type in nasty ways - see protocolbuffers/protobuf#309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jskeet jskeet

Labels
c#
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jskeet @anandolee