scalapb / ScalaPB

Protocol buffer compiler for Scala.
https://scalapb.github.io/
Apache License 2.0
1.3k stars 280 forks source link

issue where generated code has a ref to `Option` as opposed to `_root_.scala.Option` #1746

Open pjfanning opened 1 day ago

pjfanning commented 1 day ago

I'm having an issue with getting Pekko gRPC to work with ScalaPB 1.0.0.alpha1. The generated class at bottom of description has _root_.scala.Option in most places but has plain Option in 2 places. This is an issue because there is a class com.google.protobuf.Option,

You can see the issue at: https://github.com/scalapb/ScalaPB/blob/c4a3b85df27ecc926b4d102a0f9a593b4676e68d/docs/src/main/scala/generated/com/thesamet/docs/json/MyContainer.scala#L42

One instance in my class is def withLegacyClosedEnum(__v: _root_.scala.Boolean): JavaFeatures = copy(legacyClosedEnum = Option(__v)). Note the = Option(__v). The other instance is the same issue (just 3 lines later in the generated code).

// Generated by the Scala Plugin for the Protocol Buffer Compiler.
// Do not edit!

package com.google.protobuf

/** @param legacyClosedEnum
  *   Whether or not to treat an enum field as closed.  This option is only
  *   applicable to enum fields, and will be removed in the future.  It is
  *   consistent with the legacy behavior of using proto3 enum types for proto2
  *   fields.
  */
@SerialVersionUID(0L)
final case class JavaFeatures(
    legacyClosedEnum: _root_.scala.Option[_root_.scala.Boolean] = _root_.scala.None,
    utf8Validation: _root_.scala.Option[com.google.protobuf.JavaFeatures.Utf8Validation] = _root_.scala.None,
    unknownFields: _root_.scalapb.UnknownFieldSet = _root_.scalapb.UnknownFieldSet.empty
    ) extends scalapb.GeneratedMessage with scalapb.lenses.Updatable[JavaFeatures] {
    @transient
    private[this] var __serializedSizeMemoized: _root_.scala.Int = 0
    private[this] def __computeSerializedSize(): _root_.scala.Int = {
      var __size = 0
      if (legacyClosedEnum.isDefined) {
        val __value = legacyClosedEnum.get
        __size += _root_.com.google.protobuf.CodedOutputStream.computeBoolSize(1, __value)
      };
      if (utf8Validation.isDefined) {
        val __value = utf8Validation.get.value
        __size += _root_.com.google.protobuf.CodedOutputStream.computeEnumSize(2, __value)
      };
      __size += unknownFields.serializedSize
      __size
    }
    override def serializedSize: _root_.scala.Int = {
      var __size = __serializedSizeMemoized
      if (__size == 0) {
        __size = __computeSerializedSize() + 1
        __serializedSizeMemoized = __size
      }
      __size - 1

    }
    def writeTo(`_output__`: _root_.com.google.protobuf.CodedOutputStream): _root_.scala.Unit = {
      legacyClosedEnum.foreach { __v =>
        val __m = __v
        _output__.writeBool(1, __m)
      };
      utf8Validation.foreach { __v =>
        val __m = __v.value
        _output__.writeEnum(2, __m)
      };
      unknownFields.writeTo(_output__)
    }
    def getLegacyClosedEnum: _root_.scala.Boolean = legacyClosedEnum.getOrElse(false)
    def clearLegacyClosedEnum: JavaFeatures = copy(legacyClosedEnum = _root_.scala.None)
    def withLegacyClosedEnum(__v: _root_.scala.Boolean): JavaFeatures = copy(legacyClosedEnum = Option(__v))
    def getUtf8Validation: com.google.protobuf.JavaFeatures.Utf8Validation = utf8Validation.getOrElse(com.google.protobuf.JavaFeatures.Utf8Validation.UTF8_VALIDATION_UNKNOWN)
    def clearUtf8Validation: JavaFeatures = copy(utf8Validation = _root_.scala.None)
    def withUtf8Validation(__v: com.google.protobuf.JavaFeatures.Utf8Validation): JavaFeatures = copy(utf8Validation = Option(__v))
    def withUnknownFields(__v: _root_.scalapb.UnknownFieldSet) = copy(unknownFields = __v)
    def discardUnknownFields = copy(unknownFields = _root_.scalapb.UnknownFieldSet.empty)
    def getFieldByNumber(__fieldNumber: _root_.scala.Int): _root_.scala.Any = {
      (__fieldNumber: @_root_.scala.unchecked) match {
        case 1 => legacyClosedEnum.orNull
        case 2 => utf8Validation.map(_.javaValueDescriptor).orNull
      }
    }
    def getField(__field: _root_.scalapb.descriptors.FieldDescriptor): _root_.scalapb.descriptors.PValue = {
      _root_.scala.Predef.require(__field.containingMessage eq companion.scalaDescriptor)
      (__field.number: @_root_.scala.unchecked) match {
        case 1 => legacyClosedEnum.map(_root_.scalapb.descriptors.PBoolean(_)).getOrElse(_root_.scalapb.descriptors.PEmpty)
        case 2 => utf8Validation.map(__e => _root_.scalapb.descriptors.PEnum(__e.scalaValueDescriptor)).getOrElse(_root_.scalapb.descriptors.PEmpty)
      }
    }
    def toProtoString: _root_.scala.Predef.String = _root_.scalapb.TextFormat.printToUnicodeString(this)
    def companion: com.google.protobuf.JavaFeatures.type = com.google.protobuf.JavaFeatures
    // @@protoc_insertion_point(GeneratedMessage[pb.JavaFeatures])
}

object JavaFeatures extends scalapb.GeneratedMessageCompanion[com.google.protobuf.JavaFeatures] {
  implicit def messageCompanion: scalapb.GeneratedMessageCompanion[com.google.protobuf.JavaFeatures] = this
  def parseFrom(`_input__`: _root_.com.google.protobuf.CodedInputStream): com.google.protobuf.JavaFeatures = {
    var __legacyClosedEnum: _root_.scala.Option[_root_.scala.Boolean] = _root_.scala.None
    var __utf8Validation: _root_.scala.Option[com.google.protobuf.JavaFeatures.Utf8Validation] = _root_.scala.None
    var `_unknownFields__`: _root_.scalapb.UnknownFieldSet.Builder = null
    var _done__ = false
    while (!_done__) {
      val _tag__ = _input__.readTag()
      _tag__ match {
        case 0 => _done__ = true
        case 8 =>
          __legacyClosedEnum = _root_.scala.Option(_input__.readBool())
        case 16 =>
          __utf8Validation = _root_.scala.Option(com.google.protobuf.JavaFeatures.Utf8Validation.fromValue(_input__.readEnum()))
        case tag =>
          if (_unknownFields__ == null) {
            _unknownFields__ = new _root_.scalapb.UnknownFieldSet.Builder()
          }
          _unknownFields__.parseField(tag, _input__)
      }
    }
    com.google.protobuf.JavaFeatures(
        legacyClosedEnum = __legacyClosedEnum,
        utf8Validation = __utf8Validation,
        unknownFields = if (_unknownFields__ == null) _root_.scalapb.UnknownFieldSet.empty else _unknownFields__.result()
    )
  }
  implicit def messageReads: _root_.scalapb.descriptors.Reads[com.google.protobuf.JavaFeatures] = _root_.scalapb.descriptors.Reads{
    case _root_.scalapb.descriptors.PMessage(__fieldsMap) =>
      _root_.scala.Predef.require(__fieldsMap.keys.forall(_.containingMessage eq scalaDescriptor), "FieldDescriptor does not match message type.")
      com.google.protobuf.JavaFeatures(
        legacyClosedEnum = __fieldsMap.get(scalaDescriptor.findFieldByNumber(1).get).flatMap(_.as[_root_.scala.Option[_root_.scala.Boolean]]),
        utf8Validation = __fieldsMap.get(scalaDescriptor.findFieldByNumber(2).get).flatMap(_.as[_root_.scala.Option[_root_.scalapb.descriptors.EnumValueDescriptor]]).map(__e => com.google.protobuf.JavaFeatures.Utf8Validation.fromValue(__e.number))
      )
    case _ => throw new RuntimeException("Expected PMessage")
  }
  def javaDescriptor: _root_.com.google.protobuf.Descriptors.Descriptor = com.google.protobuf.JavaFeaturesProto.javaDescriptor.getMessageTypes().get(0)
  def scalaDescriptor: _root_.scalapb.descriptors.Descriptor = com.google.protobuf.JavaFeaturesProto.scalaDescriptor.messages(0)
  def messageCompanionForFieldNumber(__number: _root_.scala.Int): _root_.scalapb.GeneratedMessageCompanion[_] = throw new MatchError(__number)
  lazy val nestedMessagesCompanions: Seq[_root_.scalapb.GeneratedMessageCompanion[_ <: _root_.scalapb.GeneratedMessage]] = Seq.empty
  def enumCompanionForFieldNumber(__fieldNumber: _root_.scala.Int): _root_.scalapb.GeneratedEnumCompanion[_] = {
    (__fieldNumber: @_root_.scala.unchecked) match {
      case 2 => com.google.protobuf.JavaFeatures.Utf8Validation
    }
  }
  lazy val defaultInstance = com.google.protobuf.JavaFeatures(
    legacyClosedEnum = _root_.scala.None,
    utf8Validation = _root_.scala.None
  )
  /** The UTF8 validation strategy to use.  See go/editions-utf8-validation for
    * more information on this feature.
    */
  sealed abstract class Utf8Validation(val value: _root_.scala.Int) extends _root_.scalapb.GeneratedEnum {
    type EnumType = com.google.protobuf.JavaFeatures.Utf8Validation
    type RecognizedType = com.google.protobuf.JavaFeatures.Utf8Validation.Recognized
    def isUtf8ValidationUnknown: _root_.scala.Boolean = false
    def isDefault: _root_.scala.Boolean = false
    def isVerify: _root_.scala.Boolean = false
    def companion: _root_.scalapb.GeneratedEnumCompanion[Utf8Validation] = com.google.protobuf.JavaFeatures.Utf8Validation
    final def asRecognized: _root_.scala.Option[com.google.protobuf.JavaFeatures.Utf8Validation.Recognized] = if (isUnrecognized) _root_.scala.None else _root_.scala.Some(this.asInstanceOf[com.google.protobuf.JavaFeatures.Utf8Validation.Recognized])
  }

  object Utf8Validation extends _root_.scalapb.GeneratedEnumCompanion[Utf8Validation] {
    sealed trait Recognized extends Utf8Validation
    implicit def enumCompanion: _root_.scalapb.GeneratedEnumCompanion[Utf8Validation] = this

    /** Invalid default, which should never be used.
      */
    @SerialVersionUID(0L)
    case object UTF8_VALIDATION_UNKNOWN extends Utf8Validation(0) with Utf8Validation.Recognized {
      val index = 0
      val name = "UTF8_VALIDATION_UNKNOWN"
      override def isUtf8ValidationUnknown: _root_.scala.Boolean = true
    }

    /** Respect the UTF8 validation behavior specified by the global
      * utf8_validation feature.
      */
    @SerialVersionUID(0L)
    case object DEFAULT extends Utf8Validation(1) with Utf8Validation.Recognized {
      val index = 1
      val name = "DEFAULT"
      override def isDefault: _root_.scala.Boolean = true
    }

    /** Verifies UTF8 validity overriding the global utf8_validation
      * feature. This represents the legacy java_string_check_utf8 option.
      */
    @SerialVersionUID(0L)
    case object VERIFY extends Utf8Validation(2) with Utf8Validation.Recognized {
      val index = 2
      val name = "VERIFY"
      override def isVerify: _root_.scala.Boolean = true
    }

    @SerialVersionUID(0L)
    final case class Unrecognized(unrecognizedValue: _root_.scala.Int) extends Utf8Validation(unrecognizedValue) with _root_.scalapb.UnrecognizedEnum
    lazy val values: scala.collection.immutable.Seq[ValueType] = scala.collection.immutable.Seq(UTF8_VALIDATION_UNKNOWN, DEFAULT, VERIFY)
    def fromValue(__value: _root_.scala.Int): Utf8Validation = __value match {
      case 0 => UTF8_VALIDATION_UNKNOWN
      case 1 => DEFAULT
      case 2 => VERIFY
      case __other => Unrecognized(__other)
    }
    def javaDescriptor: _root_.com.google.protobuf.Descriptors.EnumDescriptor = com.google.protobuf.JavaFeatures.javaDescriptor.getEnumTypes().get(0)
    def scalaDescriptor: _root_.scalapb.descriptors.EnumDescriptor = com.google.protobuf.JavaFeatures.scalaDescriptor.enums(0)
  }
  implicit class JavaFeaturesLens[UpperPB](_l: _root_.scalapb.lenses.Lens[UpperPB, com.google.protobuf.JavaFeatures]) extends _root_.scalapb.lenses.ObjectLens[UpperPB, com.google.protobuf.JavaFeatures](_l) {
    def legacyClosedEnum: _root_.scalapb.lenses.Lens[UpperPB, _root_.scala.Boolean] = field(_.getLegacyClosedEnum)((c_, f_) => c_.copy(legacyClosedEnum = _root_.scala.Option(f_)))
    def optionalLegacyClosedEnum: _root_.scalapb.lenses.Lens[UpperPB, _root_.scala.Option[_root_.scala.Boolean]] = field(_.legacyClosedEnum)((c_, f_) => c_.copy(legacyClosedEnum = f_))
    def utf8Validation: _root_.scalapb.lenses.Lens[UpperPB, com.google.protobuf.JavaFeatures.Utf8Validation] = field(_.getUtf8Validation)((c_, f_) => c_.copy(utf8Validation = _root_.scala.Option(f_)))
    def optionalUtf8Validation: _root_.scalapb.lenses.Lens[UpperPB, _root_.scala.Option[com.google.protobuf.JavaFeatures.Utf8Validation]] = field(_.utf8Validation)((c_, f_) => c_.copy(utf8Validation = f_))
  }
  final val LEGACY_CLOSED_ENUM_FIELD_NUMBER = 1
  final val UTF8_VALIDATION_FIELD_NUMBER = 2
  def of(
    legacyClosedEnum: _root_.scala.Option[_root_.scala.Boolean],
    utf8Validation: _root_.scala.Option[com.google.protobuf.JavaFeatures.Utf8Validation]
  ): _root_.com.google.protobuf.JavaFeatures = _root_.com.google.protobuf.JavaFeatures(
    legacyClosedEnum,
    utf8Validation
  )
  // @@protoc_insertion_point(GeneratedMessageCompanion[pb.JavaFeatures])
}
pjfanning commented 1 day ago

I am not an expert on this code base but I suspect the issue is here:

https://github.com/scalapb/ScalaPB/blob/c4a3b85df27ecc926b4d102a0f9a593b4676e68d/compiler-plugin/src/main/scala/scalapb/compiler/ProtobufGenerator.scala#L1412

thesamet commented 1 day ago

The PR seem fine, will try it out locally later today. However, it does seem problematic that Pekko is trying to generate code directly into com.google.protobuf, since this is likely to result in conflicts with classes that are provided by protobuf-java. The classes that are being generated should be shipped by scalapb-runtime. Can you figure out why this is happening?

On Tue, Oct 1, 2024 at 10:44 AM PJ Fanning @.***> wrote:

I am not an expert on this code base but I suspect the issue is here:

https://github.com/scalapb/ScalaPB/blob/c4a3b85df27ecc926b4d102a0f9a593b4676e68d/compiler-plugin/src/main/scala/scalapb/compiler/ProtobufGenerator.scala#L1412

— Reply to this email directly, view it on GitHub https://github.com/scalapb/ScalaPB/issues/1746#issuecomment-2386598229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACLBLIYGTG5664QTCQ27HLZZLNOPAVCNFSM6AAAAABPGCKKBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBWGU4TQMRSHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- -Nadav

pjfanning commented 1 day ago

The story with Pekko gRPC is that Lightbend changed their license for Akka and a new set of developers have forked Pekko to keep an OSS alternative. We don't know everything about why the builds and features are all there.

There are a few build issues in Pekko gRPC when I upgrade to protobuf 4 and ScalaPB 1.0.0.alpha1 and this issue is 1 of them.

The issue happens in an integration test as opposed to happening in the main runtime modules. The integration test builds some code based on some test proto files. One of these proto files imports "google/protobuf/timestamp.proto" to use a type defined in this proto.

With protobuf 3, we end up with lots of generated source files because the Google timestamp.proto has its own dependencies. The classes are all generated into com.google.protobuf package. Interestingly, we don't get the problematic class that I posted in the description.

I'm guessing that timestamp.proto or one or more of its Google dependencies has changed in Protobuf 4 so that we end up with this additional source file (the one that exhibits the Option issue).

Also, the classes for these Google protos seem to be shipped in protobuf-java anyway so that we don't really need to be generating these class files - at least in theory.

This is the already compiled class that is causing my issue when Pekko gRPC tries to create its own Scala version of this. https://javadoc.io/doc/com.google.protobuf/protobuf-java/latest/com/google/protobuf/JavaFeaturesProto.JavaFeatures.html

It might be worth adding some feature to Pekko gRPC or ScalaPB to try to skip generating classes for Google protos. This feature would probably need some logic to check if the generated classes are needed by checking if the classpath already has a class with that Fully Qualified Class Name.

thesamet commented 1 day ago

It might be worth adding some feature to Pekko gRPC or ScalaPB to try to skip generating classes for Google protos.

See excludeFilter here: https://scalapb.github.io/docs/faq/#how-do-i-generate-scala-code-for-protos-from-another-jar