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

Doubly-nested types generate invalid C# code #307

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

Doubly-nested types generate invalid C# code #307

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

Comments

@jskeet
Copy link
Contributor

jskeet commented Apr 28, 2015

Consider a proto of:

syntax = "proto2";

package foo;

message Outer {
  message Middle {
    message Inner {
    }
  }
}

The generated C# code contains:

public override pbd::MessageDescriptor DescriptorForType {
  get { return global::foo.Outer.Types.Middle.Inner.Descriptor; }
}

This should be:

public override pbd::MessageDescriptor DescriptorForType {
  get { return global::foo.Outer.Types.Middle.Types.Inner.Descriptor; }
}

Note the extra Types between Middle and Inner.

(This currently makes the generated code from unittest.proto fail to compile, so once that does compile, that's enough of a test for the fix...)
@jskeet jskeet added c# and removed c# labels Apr 29, 2015
jskeet added a commit to jskeet/protobuf that referenced this issue May 15, 2015
@jskeet
Generate *all* protos in the script, applying fixups.
059c32c 
We still have some protos which aren't generated how we want them to be:

- Until we have an option to specify the "umbrella" class, DescriptorProtoFile
  will be broken. (The change of name here affects the reflection descriptor,
  which accounts for most of the change. That's easier than trying to work out
  exactly which occurrences of Descriptor need changing though.)
- That change affects UnittestCustomOptions
- Issue protocolbuffers#307 breaks Unittest.cs

After this commit, we don't have the record of the fixups in the files themselves
any more, but one centralized record in the shell script.
@jskeet jskeet mentioned this issue May 15, 2015
Closed
jskeet added a commit to jskeet/protobuf that referenced this issue May 16, 2015
@jskeet
Generate *all* protos in the script, applying fixups.
b1a395c 
We still have some protos which aren't generated how we want them to be:

- Until we have an option to specify the "umbrella" class, DescriptorProtoFile
  will be broken. (The change of name here affects the reflection descriptor,
  which accounts for most of the change. That's easier than trying to work out
  exactly which occurrences of Descriptor need changing though.)
- That change affects UnittestCustomOptions
- Issue protocolbuffers#307 breaks Unittest.cs

After this commit, we don't have the record of the fixups in the files themselves
any more, but one centralized record in the shell script.
@jskeet jskeet mentioned this issue May 16, 2015
Merged
@ghost ghost mentioned this issue Jun 1, 2015
Closed
@jtattermusch
Copy link
Contributor

Is this still an issue in the new codegen?

@jskeet
Copy link
Contributor Author

jskeet commented Jun 20, 2015

Yes, it is. (Probably because I haven't touched any of the code that works out type names.)

@jskeet
Copy link
Contributor Author

jskeet commented Jun 23, 2015

Looks like the problem is in csharp_helpers.cc, in ToCSharpName:

result += StringReplace(classname, ".", ".Types.", false);

It may be as simple as changing "false" to "true". Will need to investigate.

jskeet added a commit to jskeet/protobuf that referenced this issue Jun 29, 2015
@jskeet
Fix for doubly-nested types - issue protocolbuffers#307.
8d83f8d 
No specific test case - if the generated code compiles, the issue is fixed :)
jskeet added a commit that referenced this issue Jun 29, 2015
@jskeet
Merge pull request #550 from jskeet/issue307
241e17b 
Fix for doubly-nested types - issue #307.
@jskeet jskeet mentioned this issue Jul 3, 2015
Closed
@jskeet jskeet closed this as completed Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c#
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jskeet @jtattermusch