Skip to content
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

chore: Update scala to 3.3.4 #1474

Closed
wants to merge 2 commits into from
Closed

chore: Update scala to 3.3.4 #1474

wants to merge 2 commits into from

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Sep 11, 2024

test with 3.3.4-RC4

@He-Pin
Copy link
Member Author

He-Pin commented Sep 11, 2024

@raboof
Copy link
Member

raboof commented Sep 11, 2024

These are IncompatibleSignatureProblems, which do not necessarily have to be problematic (https://github.com/lightbend-labs/mima#incompatiblesignatureproblem), but indeed might be worth looking into.

@pjfanning
Copy link
Contributor

See apache/pekko-connectors#749 - has similar mima issues and some comments from the Dotty team.

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Sep 11, 2024

Here's the reproducer:

import scala.annotation.unchecked.uncheckedVariance

object scaladsl{
  trait FlowOpsMat[+Out, +Mat] {
    type Repr[+O] <: ReprMat[O, Mat] {
      type Repr[+OO] = FlowOpsMat.this.Repr[OO]
      type ReprMat[+OO, +MM] = FlowOpsMat.this.ReprMat[OO, MM]
    }
    type ReprMat[+O, +M] <: FlowOpsMat[O, M] {
      type Repr[+OO] = FlowOpsMat.this.ReprMat[OO, M @uncheckedVariance]
      type ReprMat[+OO, +MM] = FlowOpsMat.this.ReprMat[OO, MM]
    }
  }
  trait FlowOps[+Out, +Mat] {
    type Repr[+O] <: FlowOps[O, Mat] {
      type Repr[+OO] = FlowOps.this.Repr[OO]
    }
  }
  trait SubFlow[+Out, +Mat, +F[+_], C] extends FlowOps[Out, Mat] {
    override type Repr[+T] = SubFlow[T, Mat @uncheckedVariance, F @uncheckedVariance, C @uncheckedVariance]
  }
  final class [-In, +Out, +Mat] extends FlowOpsMat[Out, Mat]{
    override type Repr[+O] = Flow[In @uncheckedVariance, O, Mat @uncheckedVariance]
    override type ReprMat[+O, +M] = Flow[In @uncheckedVariance, O, M]
  }
  final class Sink[-In, +Mat]

}
class SubFlow[In, Out, Mat](
    delegate: scaladsl.SubFlow[Out, Mat, scaladsl.Flow[In, Out, Mat]#Repr, scaladsl.Sink[In, Mat]]) {

  def asScala: scaladsl.SubFlow[Out, Mat, scaladsl.Flow[In, Out, Mat]#Repr, scaladsl.Sink[In, Mat]] @uncheckedVariance =
    delegate
}

Compilation with both Scala 2.13 and 3.3.4-RC2 would produce the following signatures:

 private final scaladsl$SubFlow<Out, Mat, ?, scaladsl$Sink<In, Mat>> delegate;
    descriptor: Lscaladsl$SubFlow;
    Signature: #19                          // Lscaladsl$SubFlow<TOut;TMat;*Lscaladsl$Sink<TIn;TMat;>;>;

  public scaladsl$SubFlow<Out, Mat, ?, scaladsl$Sink<In, Mat>> asScala();
    descriptor: ()Lscaladsl$SubFlow;
    Signature: #22                          // ()Lscaladsl$SubFlow<TOut;TMat;*Lscaladsl$Sink<TIn;TMat;>;>;

In Scala 3.3.3 we were generating

 private final scaladsl$SubFlow<Out, Mat, scaladsl$Flow<In, java.lang.Object, Mat>, scaladsl$Sink<In, Mat>> delegate;
    descriptor: Lscaladsl$SubFlow;
    Signature: #19                          // Lscaladsl$SubFlow<TOut;TMat;Lscaladsl$Flow<TIn;Ljava/lang/Object;TMat;>;Lscaladsl$Sink<TIn;TMat;>;>;

  public SubFlow(scaladsl$SubFlow<Out, Mat, scaladsl$Flow<In, java.lang.Object, Mat>, scaladsl$Sink<In, Mat>>);
    descriptor: (Lscaladsl$SubFlow;)V
    Signature: #22                          // (Lscaladsl$SubFlow<TOut;TMat;Lscaladsl$Flow<TIn;Ljava/lang/Object;TMat;>;Lscaladsl$Sink<TIn;TMat;>;>;)V

tl;dr
In 3.3.3 Flow[In, Out, Repr]#Repr was dealiased, but in Scala 2.13 and 3.3.4-RC1/RC2 it is a wildcard param type.

@lrytz / @sjrd I believe it was introduced in scala/scala3#20463 and was targeting a fix to generic signatures of value classes. Do you think it's something that can be considered a regression? From the fact that scaladsl.Flow is a final class it makes sense to constrain the generic parameter type based on the best knowledge about Flow#Repr definition

@lrytz
Copy link
Contributor

lrytz commented Sep 11, 2024

Not immediately obvious to me. Maybe someone can narrow it down to a minimal self-contained reproducer?

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Sep 11, 2024

Not immediately obvious to me. Maybe someone can narrow it down to a minimal self-contained reproducer?

Here's further minimization using both contravariant and covaraint param types

import scala.annotation.unchecked.uncheckedVariance

trait SubFlowDef[+F[+_]]
final class Flow1[-In]{
  type Repr[+O] = Flow1[In @uncheckedVariance]
}
final class Flow2[+Out]{
  type Repr[+O] = Flow2[O]
}
class SubFlow[In, Out](
    delegate1: SubFlowDef[Flow1[In]#Repr],
    delegate2: SubFlowDef[Flow2[Out]#Repr] 
)

Signatures:
Scala 3.3.4-RC1 / 3.5.0

  public SubFlow(SubFlowDef<?>, SubFlowDef<Flow2>);
    descriptor: (LSubFlowDef;LSubFlowDef;)V
    Signature: #9                           // (LSubFlowDef<*>;LSubFlowDef<LFlow2;>;)V

Scala 2.13

  public SubFlow(SubFlowDef<?>, SubFlowDef<?>);
    descriptor: (LSubFlowDef;LSubFlowDef;)V
    Signature: #12                          // (LSubFlowDef<*>;LSubFlowDef<*>;)V

Scala 3.3.3

  public SubFlow(SubFlowDef<Flow1<In>>, SubFlowDef<Flow2<java.lang.Object>>);
    descriptor: (LSubFlowDef;LSubFlowDef;)V
    Signature: #9                           // (LSubFlowDef<LFlow1<TIn;>;>;LSubFlowDef<LFlow2<Ljava/lang/Object;>;>;)V

It seems that in Scala 2.13 all generic param types (both covariant and invariant) are transformed to wildcard types. In 3.3.4-RC2 / 3.5.0 contravariant type is preserved and in 3.3.3- both contravariant and covariant types were preserved.

@lrytz
Copy link
Contributor

lrytz commented Sep 16, 2024

@lrytz / @sjrd I believe it was introduced in scala/scala3#20463

I did a quick test with the current scala3/main branch, the signature change doesn't seem to be caused by this PR. I reverted the PR and the signature stays the same.

@WojciechMazur
Copy link
Contributor

I've bisected the compiler and found out it was due to change in the typer - scala/scala3#21576 (comment). I also suggest to move further discussions to dedicated compiler issue.

@lrytz
Copy link
Contributor

lrytz commented Sep 17, 2024

👍 sorry I missed the scala3 ticket

@WojciechMazur
Copy link
Contributor

The change introducing change to generic signatures of HKT was reverted in 3.3.4-RC4. I've tested locally that it no longer triggers invalid generic signature errors in Pekko. It's because covariant Higher Kinded Types were producing invalid signatures (not following spec).
The change to generic signatures of value classes(issue in pekko-connectors) would still be shipped - it's a bugfix to incorrect signatures.

@WojciechMazur
Copy link
Contributor

There's some weird thing going in the persistence-typed-tests/testOnly org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingContextLoggerSpec

From what I've observed locally test fails if withLoggerName is set to org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$ChattyEventSourcingBehavior$ but passed if it's set to org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$

I would be grateful for any pointers as to why it might happened, so we can prepare a reproducer.

@WojciechMazur
Copy link
Contributor

The outputs of LoggerClass.detectLoggerClassFromStack has changed:
In 3.3.3:

org.apache.pekko.actor.typed.internal.LoggerClass$TrickySecurityManager
org.apache.pekko.actor.typed.internal.LoggerClass$
org.apache.pekko.persistence.typed.scaladsl.EventSourcedBehavior$
org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$ChattyEventSourcingBehavior$ // picked
org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$ChattyEventSourcingBehavior$$$Lambda$472/640362889
org.apache.pekko.actor.typed.internal.BehaviorImpl$DeferredBehavior$$anon$2
...

in 3.3.4

org.apache.pekko.actor.typed.internal.LoggerClass$TrickySecurityManager
org.apache.pekko.actor.typed.internal.LoggerClass$
org.apache.pekko.persistence.typed.scaladsl.EventSourcedBehavior$
org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$ // picked
org.apache.pekko.persistence.typed.EventSourcedBehaviorLoggingSpec$ChattyEventSourcingBehavior$$$Lambda$472/245565542
...

I belive it was introduced in scala/scala3#19803 but need to confirm it yet. Mentioned PR optimized generated code in such a way that it allowed some unused value to be skipped.

@WojciechMazur
Copy link
Contributor

Actually, the change was introduced in scala/scala3#19251 It affects all Scala versions since 3.4.0 and now was backported to 3.3.4.
I agree with the PR comments that it should be backported, but I'd like to your comments on that topic.

@pjfanning
Copy link
Contributor

@WojciechMazur thanks for your research on this latest issue with those log capturing test scenarios.

It would be nice if Scala 3.3.4 was able to handle the existing way these tests run but if not, the Pekko team will have to decide on whether

  • to stick with Scala 3.3.3 for the Pekko 1.1 branch and update Scala when we start dev on Pekko 1.2/2.0
  • or adjust the logger names in the test case so that we work around the issue with the inner class and upgrade Scala to 3.3.4 - and release note the issue so users are aware of it

@WojciechMazur
Copy link
Contributor

I would opt for the second solution. If the user of pekko is compiling its project using 3.4.0 or later, then the new behavior is already applied to them. End users (applications) are encouraged to use the latest Scala version (Scala Next series) so they might already be dealing with it.
The logger class detection happens at the place of behaviour definition, so it's outside pekko project.

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Sep 20, 2024

I think previous behaviour can be restored with a small amount of overhead. What's important here is the fact that we should search for $$Lambda$ entries in the call stack. Based on that we can find the original declaring owner class.
I've made a quick gist with reproducer or the problem and it's fix https://gist.github.com/WojciechMazur/50b68f22a39a71a3c00dc1966ef49f5b
I've tested it to work correctly in all Scala versions (2.12.20, 2.13.14, 3.3.3, 3.3.4-RC4, 3.6.0-Nightly)

What's more by having a little more of additional overhead at the time of behaviour definition, you'd be able to have a consistent behaviour for Scala 3.4.0+ users right now

@pjfanning
Copy link
Contributor

see #1506

@pjfanning pjfanning closed this Oct 3, 2024
@pjfanning pjfanning deleted the He-Pin-patch-1 branch October 3, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants