Skip to content

Emit mixin forwarders as ordinary, non-bridge methods again#21890

Merged
hamzaremmal merged 6 commits intoscala:mainfrom
dotty-staging:emit-mixin-forwards-as-non-bridges-again
Jul 27, 2025
Merged

Emit mixin forwarders as ordinary, non-bridge methods again#21890
hamzaremmal merged 6 commits intoscala:mainfrom
dotty-staging:emit-mixin-forwards-as-non-bridges-again

Conversation

@sjrd
Copy link
Copy Markdown
Member

@sjrd sjrd commented Nov 5, 2024

Forward port of scala/scala#8037

Reverts #6352 and #6141.

Fixes #19270.

@hamzaremmal hamzaremmal self-assigned this Feb 17, 2025
@sjrd sjrd force-pushed the emit-mixin-forwards-as-non-bridges-again branch 2 times, most recently from 0af3b78 to 9a896d5 Compare February 25, 2025 14:53
@sjrd sjrd force-pushed the emit-mixin-forwards-as-non-bridges-again branch from 9a896d5 to 66f90c6 Compare April 30, 2025 12:46
@jchyb jchyb self-assigned this Jul 10, 2025
@jchyb
Copy link
Copy Markdown
Contributor

jchyb commented Jul 10, 2025

The only remaining community-build test failure seems to be in scala-parallel-collections in SerializationStabilityTest.scala. After checking the failure, it seems to be a more-or-less expected consequence of this PR. Different generated classifies generated here cause a different SerialVersionUID to be used (which happened even when I modified the PR to only change flags, keeping the types of the methods same as before). The only solution is to stabilize the UID and change the test - thankfully there is a precedent for it:
scala/scala-parallel-collections#220 (here a direct consequence of a related flag removal affecting things)
scala/scala-parallel-collections#223
scala/scala-parallel-collections#240

I’ve submitted a PR that fixes this, so we can then bring it into the dotty-staging fork: scala/scala-parallel-collections#303

About the Mima errors, there are two types here:

  • DirectMissingMethodProblem - those appear in all of the mixins of Ordered, comparing across the old and new versions, they exist in both with different flags (which is the expected change here) so I feel that this is overall expected and could be filtered out.
  • FinalMethodProblem - we get errors like synthetic method sizeHint$default$2()Int in class scala.collection.mutable.ArrayBuilder is declared final in other version which is baffling as comparing the classfiles reveals that it is final in neither version. What used to not be final and after this PR is, is the generated sizeHint method, which is not reported (and which looks like it should be final anyway). I feel like this might be a bug in Mima.

sjrd added 5 commits July 14, 2025 13:10
@sjrd @jchyb
Revert "Don't emit mixin forwarders as final"
5841870 
This reverts commit 700eea7.

Forward port of
scala/scala@96f844e
@sjrd @jchyb
Revert "Emit mixin forwarders as bridges"
d5449ac 
This reverts commit 6d0f9ca.

Forward port of most commits in
scala/scala#8037

Compensate some of the consequences by adding the `MixedIn` flag.
In addition to the use seen in the diff in `BCodeHelpers`, the JS
backend has an existing test for `isOneOf(Bridge | MixedIn)` which
needs this compensation.
@sjrd @jchyb
Add test case for scala#19270.
31017b1 
Closes scala#19270.
@sjrd @jchyb
Emit Java generic signatures for mixin forwarders.
ef84d41 
This is a forward port of
scala/scala@6fc2202
In Scala 2 that commit was reverting a commit that had stopped
emitting Java generic signatures. In dotc we never had that logic
before, so this is new code.
@sjrd @jchyb
Mixins were not the only bridges. Bridges must actually not be final.
0585508
@jchyb jchyb force-pushed the emit-mixin-forwards-as-non-bridges-again branch 2 times, most recently from 30b9b8e to eb11192 Compare July 15, 2025 14:02
@jchyb jchyb force-pushed the emit-mixin-forwards-as-non-bridges-again branch from eb11192 to d242a4b Compare July 16, 2025 09:58
@tgodzik
Copy link
Copy Markdown
Contributor

tgodzik commented Jul 21, 2025

Thanks @jchyb for helping out! @sjrd do you think we are ready with this PR?

@jchyb
Copy link
Copy Markdown
Contributor

jchyb commented Jul 21, 2025

@tgodzik I think @hamzaremmal also wanted to take a look and review my changes after he comes back from the break.
I did change some things in our scala-parallel-collecitions staging fork - I wanted to have it up to date with the actual scala-parallel-collections (needed for the PR linked above) and for that I had to update and remove Scala Native and Mima checks (on the scala-parallel-collections side)

@sjrd
Copy link
Copy Markdown
Member Author

sjrd commented Jul 21, 2025

If it's green it's fine by me. :p

@hamzaremmal hamzaremmal merged commit 9f20aa5 into scala:main Jul 27, 2025
29 checks passed
@hamzaremmal hamzaremmal deleted the emit-mixin-forwards-as-non-bridges-again branch July 27, 2025 16:22
@hamzaremmal
Copy link
Copy Markdown
Member

hamzaremmal commented Jul 28, 2025

I'm reverting this as the new build catched an issue and I don't have the resources at the moment to dig deeper and fix it.

@hamzaremmal hamzaremmal mentioned this pull request Jul 28, 2025
Merged
hamzaremmal added a commit that referenced this pull request Jul 28, 2025
@hamzaremmal
Revert "Emit mixin forwarders as ordinary, non-bridge methods again" (#…
4b543e8 
…23613)

Reverts #21890

The new build catches an issue when building
`scala3-sbt-bridge-bootstrapped`.
At the moment, I don't have the resources to handle the failure.
@WojciechMazur WojciechMazur mentioned this pull request Jul 28, 2025
Closed
@WojciechMazur WojciechMazur added this to the 3.7.3 milestone Jul 29, 2025
@hamzaremmal hamzaremmal mentioned this pull request Sep 17, 2025
Merged
hamzaremmal added a commit that referenced this pull request Sep 21, 2025
@hamzaremmal
Emit mixin forwarders as ordinary, non-bridge methods again (#23942)
9bd7774 
Lets try to merge again #21890
@hamzaremmal hamzaremmal mentioned this pull request Nov 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hamzaremmal hamzaremmal hamzaremmal approved these changes

Assignees

@jchyb jchyb

@hamzaremmal hamzaremmal

Labels

None yet

Projects

None yet

Milestone

3.7.3

Development

Successfully merging this pull request may close these issues.

Scala 3.3.1: Concrete method inside trait marked (incorrectly?) as ACC_BRIDGE and ACC_SYNTHETIC

6 participants

@sjrd @jchyb @tgodzik @hamzaremmal @Kordyjan @WojciechMazur