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 collision of proto messages and Kotlin built-in classes #237

Closed
geoffliu opened this issue Oct 25, 2022 · 6 comments · Fixed by #241
Closed

Name collision of proto messages and Kotlin built-in classes #237

geoffliu opened this issue Oct 25, 2022 · 6 comments · Fixed by #241
Labels
bug Something isn't working component-codegen question Further information is requested
Milestone

Comments

@geoffliu
Copy link

geoffliu commented Oct 25, 2022

Hi all,

We're hitting an issue when trying to process a proto message named Sequence. In the generated file, the kotlin built-in Sequence is needed, but the name is shadowed by the generated message. Should we prefix all Kotlin classes with their proper packages, e.g. kotlin.sequences.Sequence? Not sure what other Kotlin built-ins are used, but I suspect we'll have similar issues.

cheers,
Geoff

@garyp garyp added bug Something isn't working component-codegen labels Oct 25, 2022
@garyp garyp added this to the 0.15.0 milestone Oct 25, 2022
@garyp
Copy link
Collaborator

garyp commented Oct 25, 2022

Yeah good catch. We already prefix most of the references to generated code with their full package name so that they don't conflict with built-ins. We must've missed a spot. Though maybe prefixing all of the Kotlin built-ins with their full path would be a good idea as well. It just makes the generated code harder to read 😞

I'll add a unit test with a message named Sequence (and List and Map too while we're at it) to ensure we catch all of the places where this can cause problems.

@garyp garyp modified the milestones: 0.15.0, 0.14.2 Oct 25, 2022
@garyp
Copy link
Collaborator

garyp commented Oct 25, 2022

@geoffliu Actually, can you provide a proto file that can reproduce this issue? I can't reproduce an error with a protobuf message named Sequence. I tried defining a message like this:

syntax = "proto3";
package foobar;

message Sequence {
    int32 sequence = 1;
}

But it compiled just fine.

@garyp garyp added the question Further information is requested label Oct 25, 2022
@geoffliu
Copy link
Author

@garyp Try one with a repeated field?

@Dogacel
Copy link
Contributor

Dogacel commented Nov 4, 2022

I think relates to #230, I suggest adding a package mapping or prefixing option.

@garyp
Copy link
Collaborator

garyp commented Nov 13, 2022

@geoffliu Thanks for the suggestion. I was able to reproduce the error. Fix coming shortly...

@geoffliu
Copy link
Author

Nice, thank you!

garyp added a commit that referenced this issue Nov 14, 2022
Fix compiler error when a protobuf type is named `Sequence`

Fixes #237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component-codegen question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants