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

Incorrect Java signature for value classes #10846

Closed
dwijnand opened this issue Dec 17, 2020 · 6 comments · Fixed by #20463
Closed

Incorrect Java signature for value classes #10846

dwijnand opened this issue Dec 17, 2020 · 6 comments · Fixed by #20463
Assignees
Milestone

Comments

@dwijnand
Copy link
Member

Minimized code

final class Foo(val value: String) extends AnyVal

final class Bar {
  def boxedStringy = Option(new Foo("hi"))
}

Output

$ javap Bar
Compiled from "A.scala"
public final class Bar {
  public Bar();
  public scala.Option<java.lang.String> boxedStringy();
}

Expectation

Scala 2 (2.13.4) is right:

$ javap Bar
Compiled from "A.scala"
public final class Bar {
  public scala.Option<Foo> boxedStringy();
  public Bar();
}

AKA forward-port scala/scala#8127 for Scala 3.

@kubukoz
Copy link
Contributor

kubukoz commented May 15, 2024

My team has just hit this. Kindly requesting some love for this ticket 😅

Our understanding is that any signatures that mention value classes should currently be avoided in calls from Java.

@jtjeferreira
Copy link
Contributor

I think I was also hit by this today in FasterXML/jackson-module-scala#675

@lrytz
Copy link
Member

lrytz commented May 30, 2024

On that jackson-module-scala ticket, @jtjeferreira suggests that the bug was fixed in Scala 2 in scala/scala#8127. Maybe it can be forward ported, I don't know if java signature building is similar in Scala 3.

@SethTisue
Copy link
Member

@lrytz there's a WIP forward-port at #20463 (where the contributor is stuck and could use help)

@jtjeferreira
Copy link
Contributor

@lrytz there's a WIP forward-port at #20463 (where the contributor is stuck and could use help)

Yes, I tried to fix it but this was my first time looking into the compiler code so I got stuck. Would be helpful if someone pointed me in the right direction and point in which phase (or phases) I would have to do the fix.

OTOH I was also wondering if the same fix is applied like in scala2, this will be a binary incompatible change. So it can only be done in scala 3.6?

@SethTisue
Copy link
Member

SethTisue commented May 30, 2024

OTOH I was also wondering if the same fix is applied like in scala2, this will be a binary incompatible change. So it can only be done in scala 3.6?

fwiw, the Scala 2 fix was made in 2.12.9, it wasn't held for 2.13.0, so we already decided once that this is fair game to change

as @raboof wrote at lightbend-labs/mima#299 (comment),

when a method's erased signature has remained the same, but its full signature has changed [...] Such a change is not necessarily binary incompatible, but typically worthwhile to manually investigate and explicitly confirm

offhand, it seems to me like it would be fair game for an LTS backport as long as it was carefully release-noted, but I'm not fully confident in my own opinion here

jtjeferreira added a commit to jtjeferreira/scala3 that referenced this issue Jun 19, 2024
As suggested in scala#10846 the fix to this issue should be to port scala/scala#8127 to scala3
lrytz added a commit that referenced this issue Jun 20, 2024
…20463)

As suggested in #10846 the fix to this issue should be to port
scala/scala#8127 to scala3

I started by adding the same tests as in the scala2 PR and then tried to
find the place where to do the fix by adding some log traces.
Unfortunately I am still pretty lost because this is my first time
looking at the compiler code.

Any tips where this needs to be fixed are very welcome. Meanwhile a few
questions:
* The scala2 fix was done in
`src/compiler/scala/tools/nsc/transform/Erasure.scala`, should I do the
fix for scala3 in
`compiler/src/dotty/tools/dotc/transform/Erasure.scala` as well?
* I think I need to do the fix around `ErasedValueType`, either when it
is created (in `TypeErasure#eraseDerivedValueClass`) or when it is used
(in `Erasure#unbox`).

Please also send me any pointers besides
https://dotty.epfl.ch/docs/contributing/index.html regarding compiler
contributions
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
WojciechMazur pushed a commit that referenced this issue Jul 10, 2024
As suggested in #10846 the fix to this issue should be to port scala/scala#8127 to scala3

[Cherry-picked 2217634]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants