Skip to content

Commit 7323240

Browse files
i-am-the-slimethesamet
i-am-the-slime
authored and
thesamet
committed
resolve oneof field name conflicts with imported packages (#1947)
Include oneof field names in conflict detection to properly generate _root_ prefixes when oneof fields conflict with imported package names. This fixes issue #1911 where ScalaPB would generate incorrect import paths for oneof fields that conflict with imported package names. The fix includes: - Enhanced conflict detection in DescriptorImplicits.scala to include oneof field names - Updated ProtobufGenerator.scala to properly handle conflicts when generating imports - Added comprehensive tests to verify the fix works correctly - Fixed test file to use correct generated class name (OneofImportConflictTest) Closes #1911
1 parent 55b6847 commit 7323240

File tree

10 files changed

+2296
-10
lines changed

10 files changed

+2296
-10
lines changed

‎.claude/settings.local.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"permissions": {
3+
"allow": [
4+
"Bash(gh pr view:*)",
5+
"Bash(gh pr checkout:*)",
6+
"Bash(git rev-parse:*)",
7+
"Bash(grep:*)"
8+
],
9+
"deny": []
10+
}
11+
}

‎.direnv/bin/nix-direnv-reload‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#!/usr/bin/env bash
2+
set -e
3+
if [[ ! -d "/home/thesamet/dev/ScalaPB" ]]; then
4+
echo "Cannot find source directory; Did you move it?"
5+
echo "(Looking for "/home/thesamet/dev/ScalaPB")"
6+
echo 'Cannot force reload with this script - use "direnv reload" manually and then try again'
7+
exit 1
8+
fi
9+
10+
# rebuild the cache forcefully
11+
_nix_direnv_force_reload=1 direnv exec "/home/thesamet/dev/ScalaPB" true
12+
13+
# Update the mtime for .envrc.
14+
# This will cause direnv to reload again - but without re-building.
15+
touch "/home/thesamet/dev/ScalaPB/.envrc"
16+
17+
# Also update the timestamp of whatever profile_rc we have.
18+
# This makes sure that we know we are up to date.
19+
touch -r "/home/thesamet/dev/ScalaPB/.envrc" "/home/thesamet/dev/ScalaPB/.direnv"/*.rc

‎.direnv/nix-profile-25.05-vj980b72z6zb0yg6‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
/nix/store/4s2y40nivl9xxjl0ibky63z4zp8yb6fs-nix-shell-env

‎.direnv/nix-profile-25.05-vj980b72z6zb0yg6.rc‎

Lines changed: 2147 additions & 0 deletions
Large diffs are not rendered by default.

‎compiler-plugin/src/main/scala/scalapb/compiler/DescriptorImplicits.scala‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -390,10 +390,13 @@ class DescriptorImplicits private[compiler] (
390390
case FieldDescriptor.JavaType.BYTE_STRING => "_root_.com.google.protobuf.ByteString"
391391
case FieldDescriptor.JavaType.STRING => "_root_.scala.Predef.String"
392392
case FieldDescriptor.JavaType.MESSAGE =>
393-
fd.getMessageType.scalaType
394-
.fullNameWithMaybeRoot(fd.getContainingType.fields.map(_.scalaName))
393+
val contextNames = fd.getContainingType.fields.map(_.scalaName) ++
394+
fd.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
395+
fd.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames)
395396
case FieldDescriptor.JavaType.ENUM =>
396-
fd.getEnumType.scalaType.fullNameWithMaybeRoot(fd.getContainingType.fields.map(_.scalaName))
397+
val contextNames = fd.getContainingType.fields.map(_.scalaName) ++
398+
fd.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
399+
fd.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames)
397400
}
398401

399402
def singleScalaTypeName = customSingleScalaTypeName.getOrElse(baseSingleScalaTypeName)

‎compiler-plugin/src/main/scala/scalapb/compiler/ProtobufGenerator.scala‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -186,11 +186,15 @@ class ProtobufGenerator(
186186
.mkString("_root_.com.google.protobuf.ByteString.copyFrom(Array[Byte](", ", ", "))")
187187
case FieldDescriptor.JavaType.STRING => escapeScalaString(defaultValue.asInstanceOf[String])
188188
case FieldDescriptor.JavaType.MESSAGE =>
189+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
190+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
189191
field.getMessageType.scalaType
190-
.fullNameWithMaybeRoot(field.getContainingType) + ".defaultInstance"
192+
.fullNameWithMaybeRoot(contextNames) + ".defaultInstance"
191193
case FieldDescriptor.JavaType.ENUM =>
194+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
195+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
192196
field.getEnumType.scalaType
193-
.fullNameWithMaybeRoot(field.getContainingType) + "." + defaultValue
197+
.fullNameWithMaybeRoot(contextNames) + "." + defaultValue
194198
.asInstanceOf[EnumValueDescriptor]
195199
.scalaName
196200
.asSymbol
@@ -215,13 +219,22 @@ class ProtobufGenerator(
215219
case FieldDescriptor.JavaType.BYTE_STRING => Identity
216220
case FieldDescriptor.JavaType.STRING => Identity
217221
case FieldDescriptor.JavaType.MESSAGE =>
218-
FunctionApplication(field.getMessageType.scalaType.fullName + ".fromJavaProto")
222+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
223+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
224+
FunctionApplication(
225+
field.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromJavaProto"
226+
)
219227
case FieldDescriptor.JavaType.ENUM =>
228+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
229+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
220230
if (field.getFile.isProto3)
221231
MethodApplication("intValue") andThen FunctionApplication(
222-
field.getEnumType.scalaType.fullName + ".fromValue"
232+
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromValue"
233+
)
234+
else
235+
FunctionApplication(
236+
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromJavaValue"
223237
)
224-
else FunctionApplication(field.getEnumType.scalaType.fullName + ".fromJavaValue")
225238
}
226239
baseValueConversion andThen toCustomTypeExpr(field)
227240
}
@@ -285,12 +298,20 @@ class ProtobufGenerator(
285298
case FieldDescriptor.JavaType.BYTE_STRING => Identity
286299
case FieldDescriptor.JavaType.STRING => Identity
287300
case FieldDescriptor.JavaType.MESSAGE =>
288-
FunctionApplication(field.getMessageType.scalaType.fullName + ".toJavaProto")
301+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
302+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
303+
FunctionApplication(
304+
field.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames) + ".toJavaProto"
305+
)
289306
case FieldDescriptor.JavaType.ENUM =>
307+
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
308+
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
290309
if (field.getFile.isProto3)
291310
(MethodApplication("value") andThen maybeBox("_root_.scala.Int.box"))
292311
else
293-
FunctionApplication(field.getEnumType.scalaType.fullName + ".toJavaValue")
312+
FunctionApplication(
313+
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".toJavaValue"
314+
)
294315
}
295316
}
296317

‎e2e/src/main/protobuf/location/v1/location.proto‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
syntax = "proto3";
2+
3+
package location.v1;
4+
5+
message Coordinate {
6+
double latitude = 1;
7+
double longitude = 2;
8+
}

‎e2e/src/main/protobuf/oneof_import_conflict.proto‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
syntax = "proto3";
2+
3+
package com.thesamet.proto.e2e;
4+
5+
import "location/v1/location.proto";
6+
7+
// Test case for oneof field name conflicting with imported package name.
8+
// This ensures ScalaPB correctly adds _root_ prefixes to avoid naming conflicts.
9+
message OneofImportConflictTest {
10+
oneof location { // This field name conflicts with the imported package "location"
11+
location.v1.Coordinate location_coordinate = 2;
12+
}
13+
}
14+
15+
// Additional test with multiple conflicts
16+
message MultipleConflictTest {
17+
oneof location {
18+
location.v1.Coordinate coord = 1;
19+
}
20+
21+
// Regular field with same type should also get _root_ prefix due to oneof conflict
22+
location.v1.Coordinate regular_coord = 2;
23+
}

‎e2e/src/test/scala/OneofImportConflictSpec.scala‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import org.scalatest.flatspec.AnyFlatSpec
2+
import org.scalatest.matchers.should.Matchers
3+
import com.thesamet.proto.e2e.oneof_import_conflict.{OneofImportConflictTest, MultipleConflictTest}
4+
5+
class OneofImportConflictSpec extends AnyFlatSpec with Matchers {
6+
7+
"oneof field conflicting with imported package name" should "generate correct code with _root_ prefixes" in {
8+
// Test basic oneof conflict - the fact this compiles means _root_ prefixes work
9+
val coord = _root_.location.v1.location.Coordinate(latitude = 1.0, longitude = 2.0)
10+
11+
val message = OneofImportConflictTest(
12+
location = OneofImportConflictTest.Location.LocationCoordinate(coord)
13+
)
14+
15+
// Test functionality works correctly
16+
message.getLocationCoordinate shouldBe coord
17+
message.location.isLocationCoordinate shouldBe true
18+
message.location.locationCoordinate shouldBe Some(coord)
19+
20+
// Test serialization round-trip
21+
val bytes = message.toByteArray
22+
val parsed = OneofImportConflictTest.parseFrom(bytes)
23+
parsed shouldBe message
24+
}
25+
26+
"multiple field types with import conflicts" should "all use _root_ prefixes consistently" in {
27+
val coord1 = _root_.location.v1.location.Coordinate(latitude = 3.0, longitude = 4.0)
28+
val coord2 = _root_.location.v1.location.Coordinate(latitude = 5.0, longitude = 6.0)
29+
30+
val message = MultipleConflictTest(
31+
location = MultipleConflictTest.Location.Coord(coord1),
32+
regularCoord = Some(coord2)
33+
)
34+
35+
// Test both oneof and regular fields work correctly
36+
message.location.isCoord shouldBe true
37+
message.location.coord shouldBe Some(coord1)
38+
message.regularCoord shouldBe Some(coord2)
39+
40+
// Test serialization round-trip
41+
val bytes = message.toByteArray
42+
val parsed = MultipleConflictTest.parseFrom(bytes)
43+
parsed shouldBe message
44+
}
45+
}

‎third_party/google/protobuf/.claude/settings.local.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"permissions": {
3+
"allow": [
4+
"Bash(cp:*)"
5+
],
6+
"deny": []
7+
}
8+
}

0 commit comments

Comments
 (0)