Undefined behavior of PekkoPduProtobufCodec$

[nicolasstucki]

The Scala spec states that names in the source should not contain $. These are reserved for code generation. Using a name with a $ will have undefined behavior.

https://github.com/apache/incubator-pekko/blob/fbac70e9dd57050449b6d00b7b9c6a2669d600d3/remote/src/main/scala/org/apache/pekko/remote/transport/PekkoPduCodec.scala#L131

A change in this undefined behavior was found in scala/scala3#18227

Solution: change the name of this object.

[pjfanning]

@mdedetrich any idea why the $ was added? It was added during one of your commits when we were renaming the akka packages. The Akka 2.6 code does not have a $ in the equivalent name.

[mdedetrich]

This was likely a mistake, @nicolasstucki Thanks foe the report, does that mean we should make a new release?

[pjfanning]

• we can start by release noting that this affects users of Scala 3.3.2 (which is only in preview)
• still works with Scala 3.3.1-RC4 as per @nicolasstucki
• we should release Pekko 1.0.1 but we may not need to start the RC process yet
• we may still be better off starting Pekko HTTP 1.0.0 release process first

[mdedetrich]

• we can start by release noting that this affects users of Scala 3.3.2 (which is only in preview)
• still works with Scala 3.3.1-RC4 as per @nicolasstucki
• we should release Pekko 1.0.1 but we may not need to start the RC process yet
• we may still be better off starting Pekko HTTP 1.0.0 release process first

So just to clarify (and @nicolasstucki can either confirm or deny this), but afaik this issue of using $ in class names is not Scala 3 specific, its just that because incubator-pekko got added to Scala 3 integration testing (i.e. open community build) where it got revealed but its a general Scala issue.

The core problem is that Scala (regardless if its Scala 2 or Scala 3) you should never have a $ in class names that you define because Scala compiler itself will generate classes internally which can contain the $ (both Scala 2 and Scala 3 do this) and if you happen to define your own class with $ then conflicts/collisions can arise (this is the core undefined behaviour).

For this reason I would recommend making a new release irrespective of Scala 3, we still need to go through the release process. If possible Scala 2/3 should also add checks/warnings to prevent this in the future.

[He-Pin]

i think this is not a bug of pekko but a scala3 regression.

[pjfanning]

i think this is not a bug of pekko but a scala3 regression.

Scala 3 is not likely to be changed to allow us not to fix the class name.

I'm happy to change this package private class name since the Scala spec says we shouldn't have $ suffix in class names.

[mdedetrich]

i think this is not a bug of pekko but a scala3 regression.

The Scala spec does state that you should not create classes with $ signs so its not a regression, we broke the spec. See https://www.scala-lang.org/files/archive/spec/2.11/01-lexical-syntax.html for reference, specifically

The ‘$’ character is reserved for compiler-synthesized identifiers. User programs should not define identifiers which contain ‘$’ characters.

classes are such an identifier. And note that is for Scala 2.11, not Scala 3

I'm happy to change this package private class name since the Scala spec says we shouldn't have $ suffix in class names.

Its actually in any identifier, not just classes and changing things to package private (or not) doesn't make a difference here. The best way to generally solve this problem is Scala compiler refusing to compile code that breaks the spec (maybe with some flag to override in true exception cases, ours is not such a case).

[He-Pin]

@mdedetrich So why the compiler did not prevent this but produced an artifact?

[mdedetrich]

@mdedetrich So why the compiler did not prevent this but produced an artifact?

It should have prevented this, and at least for Scala 3 there is a discussion to detect this and make the compiler produce a warning (see scala/scala3#18227 (comment)).

This not being there was an oversight

[nicolasstucki]

A linting option will be added to detect these issues: scala/scala3#18234