Skip to content

resolve oneof field name conflicts with imported packages #1912

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,13 @@ class DescriptorImplicits private[compiler] (
case FieldDescriptor.JavaType.BYTE_STRING => "_root_.com.google.protobuf.ByteString"
case FieldDescriptor.JavaType.STRING => "_root_.scala.Predef.String"
case FieldDescriptor.JavaType.MESSAGE =>
fd.getMessageType.scalaType
.fullNameWithMaybeRoot(fd.getContainingType.fields.map(_.scalaName))
val contextNames = fd.getContainingType.fields.map(_.scalaName) ++
fd.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
fd.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames)
case FieldDescriptor.JavaType.ENUM =>
fd.getEnumType.scalaType.fullNameWithMaybeRoot(fd.getContainingType.fields.map(_.scalaName))
val contextNames = fd.getContainingType.fields.map(_.scalaName) ++
fd.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
fd.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames)
}

def singleScalaTypeName = customSingleScalaTypeName.getOrElse(baseSingleScalaTypeName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,15 @@ class ProtobufGenerator(
.mkString("_root_.com.google.protobuf.ByteString.copyFrom(Array[Byte](", ", ", "))")
case FieldDescriptor.JavaType.STRING => escapeScalaString(defaultValue.asInstanceOf[String])
case FieldDescriptor.JavaType.MESSAGE =>
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
field.getMessageType.scalaType
.fullNameWithMaybeRoot(field.getContainingType) + ".defaultInstance"
.fullNameWithMaybeRoot(contextNames) + ".defaultInstance"
case FieldDescriptor.JavaType.ENUM =>
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
field.getEnumType.scalaType
.fullNameWithMaybeRoot(field.getContainingType) + "." + defaultValue
.fullNameWithMaybeRoot(contextNames) + "." + defaultValue
.asInstanceOf[EnumValueDescriptor]
.scalaName
.asSymbol
Expand All @@ -217,13 +221,22 @@ class ProtobufGenerator(
case FieldDescriptor.JavaType.BYTE_STRING => Identity
case FieldDescriptor.JavaType.STRING => Identity
case FieldDescriptor.JavaType.MESSAGE =>
FunctionApplication(field.getMessageType.scalaType.fullName + ".fromJavaProto")
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
FunctionApplication(
field.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromJavaProto"
)
case FieldDescriptor.JavaType.ENUM =>
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
if (!field.legacyEnumFieldTreatedAsClosed())
MethodApplication("intValue") andThen FunctionApplication(
field.getEnumType.scalaType.fullName + ".fromValue"
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromValue"
)
else
FunctionApplication(
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".fromJavaValue"
)
else FunctionApplication(field.getEnumType.scalaType.fullName + ".fromJavaValue")
}
baseValueConversion andThen toCustomTypeExpr(field)
}
Expand Down Expand Up @@ -289,12 +302,20 @@ class ProtobufGenerator(
case FieldDescriptor.JavaType.BYTE_STRING => Identity
case FieldDescriptor.JavaType.STRING => Identity
case FieldDescriptor.JavaType.MESSAGE =>
FunctionApplication(field.getMessageType.scalaType.fullName + ".toJavaProto")
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
FunctionApplication(
field.getMessageType.scalaType.fullNameWithMaybeRoot(contextNames) + ".toJavaProto"
)
case FieldDescriptor.JavaType.ENUM =>
val contextNames = field.getContainingType.fields.map(_.scalaName) ++
field.getContainingType.getRealOneofs.asScala.map(_.scalaName.nameSymbol)
if (!field.legacyEnumFieldTreatedAsClosed())
(MethodApplication("value") andThen maybeBox("_root_.scala.Int.box"))
else
FunctionApplication(field.getEnumType.scalaType.fullName + ".toJavaValue")
FunctionApplication(
field.getEnumType.scalaType.fullNameWithMaybeRoot(contextNames) + ".toJavaValue"
)
}
}

Expand Down
8 changes: 8 additions & 0 deletions e2e/src/main/protobuf/location/v1/location.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto3";

package location.v1;

message Coordinate {
double latitude = 1;
double longitude = 2;
}
23 changes: 23 additions & 0 deletions e2e/src/main/protobuf/oneof_import_conflict.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
syntax = "proto3";

package com.thesamet.proto.e2e;

import "location/v1/location.proto";

// Test case for oneof field name conflicting with imported package name.
// This ensures ScalaPB correctly adds _root_ prefixes to avoid naming conflicts.
message OneofImportConflictTest {
oneof location { // This field name conflicts with the imported package "location"
location.v1.Coordinate location_coordinate = 2;
}
}

// Additional test with multiple conflicts
message MultipleConflictTest {
oneof location {
location.v1.Coordinate coord = 1;
}

// Regular field with same type should also get _root_ prefix due to oneof conflict
location.v1.Coordinate regular_coord = 2;
}
45 changes: 45 additions & 0 deletions e2e/src/test/scala/OneofImportConflictSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import com.thesamet.proto.e2e.oneof_import_conflict.{OneofImportConflict, MultipleConflictTest}

class OneofImportConflictSpec extends AnyFlatSpec with Matchers {

"oneof field conflicting with imported package name" should "generate correct code with _root_ prefixes" in {
// Test basic oneof conflict - the fact this compiles means _root_ prefixes work
val coord = _root_.location.v1.location.Coordinate(latitude = 1.0, longitude = 2.0)

val message = OneofImportConflict(
location = OneofImportConflict.Location.LocationCoordinate(coord)
)

// Test functionality works correctly
message.getLocationCoordinate shouldBe coord
message.location.isLocationCoordinate shouldBe true
message.location.locationCoordinate shouldBe Some(coord)

// Test serialization round-trip
val bytes = message.toByteArray
val parsed = OneofImportConflict.parseFrom(bytes)
parsed shouldBe message
}

"multiple field types with import conflicts" should "all use _root_ prefixes consistently" in {
val coord1 = _root_.location.v1.location.Coordinate(latitude = 3.0, longitude = 4.0)
val coord2 = _root_.location.v1.location.Coordinate(latitude = 5.0, longitude = 6.0)

val message = MultipleConflictTest(
location = MultipleConflictTest.Location.Coord(coord1),
regularCoord = Some(coord2)
)

// Test both oneof and regular fields work correctly
message.location.isCoord shouldBe true
message.location.coord shouldBe Some(coord1)
message.regularCoord shouldBe Some(coord2)

// Test serialization round-trip
val bytes = message.toByteArray
val parsed = MultipleConflictTest.parseFrom(bytes)
parsed shouldBe message
}
}