Skip to content

Update Kotlin compiler to escape package names#13310

Closed
pkwarren wants to merge 1 commit intoprotocolbuffers:mainfrom
pkwarren:pkw/issue-13182
Closed

Update Kotlin compiler to escape package names#13310
pkwarren wants to merge 1 commit intoprotocolbuffers:mainfrom
pkwarren:pkw/issue-13182

Conversation

@pkwarren
Copy link
Contributor

The current Kotlin code generator creates code which fails to compile with imports like:

syntax = "proto3";
package net.example.v1;
message Sample {
  string net = 1;
}

This results in a condition where 'net' is ambiguous in some cases (could refer to the field or the package). To avoid these issues, the code generator should enclose package paths in backticks:

net.example.v1.SampleKt => `net.example.v1`.SampleKt

Fixes #13182.

@pkwarren pkwarren requested a review from a team as a code owner July 13, 2023 19:36
@pkwarren pkwarren requested review from googleberg and removed request for a team July 13, 2023 19:36
@google-cla
Copy link

google-cla bot commented Jul 13, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 17, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 17, 2023
@pkwarren
Copy link
Contributor Author

pkwarren commented Jul 18, 2023

@acozzette - Thanks for letting the tests run - they uncovered an issue with the original implementation with nested classes. I've updated the code to handle this case.

@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 20, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 20, 2023
pkwarren added a commit to bufbuild/plugins that referenced this pull request Jul 24, 2023
Update the kotlin v23.4 plugin to include the fix for
protocolbuffers/protobuf#13310, which is
important to fix compilation issues with the generated code when the
package name contains a potential name clash with one of the protobuf
field names.
pkwarren added a commit to bufbuild/plugins that referenced this pull request Jul 24, 2023
Update the kotlin v23.4 plugin to include the fix for
protocolbuffers/protobuf#13310, which is
important to fix compilation issues with the generated code when the
package name contains a potential name clash with one of the protobuf
field names.
@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 24, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 24, 2023
@acozzette
Copy link

@pkwarren Could you merge the current main into your branch (or rebase if you prefer)? I think the test failures are things that were fixed recently on main but those fixes might not take effect until you update your branch.

@pkwarren
Copy link
Contributor Author

@pkwarren Could you merge the current main into your branch (or rebase if you prefer)? I think the test failures are things that were fixed recently on main but those fixes might not take effect until you update your branch.

Updated.

@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 24, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 24, 2023
@jhump
Copy link
Contributor

jhump commented Aug 2, 2023

@zhangsk, @fowles, anything else needed before this PR can be merged?

@zhangskz
Copy link
Member

zhangskz commented Aug 2, 2023

Thanks for checking in -- looks like this got stuck when going through the internal submit process due to some failing tests that seem to be unrelated.

Giving it a kick but please feel free to ping again if you don't see this submitted soon. Sorry for the delay!

@pkwarren
Copy link
Contributor Author

@zhangskz, @fowles, @acozzette - Anything remaining I need to do on my end to get this PR merged? Happy to merge latest main/rebase+squash if needed.

@zhangskz
Copy link
Member

Looks like the change for this somehow got garbage collected internally after getting stuck on already failing tests -- we gave it another kick and hopefully it should go through properly this time.

@jhump
Copy link
Contributor

jhump commented Aug 22, 2023

@zhangskz, any updates? Can this be merged yet?

The current Kotlin code generator creates code which fails to compile
with imports like:

```
syntax = "proto3";
package net.example.v1;
message Sample {
  string net = 1;
}
```

This results in a condition where 'net' is ambiguous in some cases
(could refer to the field or the package). To avoid these issues, the
code generator should enclose package paths in backticks:

```
net.example.v1.SampleKt => `net.example.v1`.SampleKt
```

Fixes protocolbuffers#13182.
@pkwarren
Copy link
Contributor Author

pkwarren commented Sep 7, 2023

I've gone ahead and rebased against latest main and squashed down to a single commit (in case that was preventing this from being automatically merged).

@pkwarren pkwarren requested a review from zhangskz September 7, 2023 16:19
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 8, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 8, 2023
dawidcha pushed a commit to dawidcha/protobuf that referenced this pull request Sep 27, 2023
The current Kotlin code generator creates code which fails to compile with imports like:

```
syntax = "proto3";
package net.example.v1;
message Sample {
  string net = 1;
}
```

This results in a condition where 'net' is ambiguous in some cases (could refer to the field or the package). To avoid these issues, the code generator should enclose package paths in backticks:

```
net.example.v1.SampleKt => `net.example.v1`.SampleKt
```

Fixes protocolbuffers#13182.

Closes protocolbuffers#13310

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#13310 from pkwarren:pkw/issue-13182 3f72b2f
PiperOrigin-RevId: 564481691
@pkwarren pkwarren deleted the pkw/issue-13182 branch October 5, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error compiling Kotlin code when field names conflict with top-level package names

6 participants