Correct Java signature for value classes appearing in type arguments

[jtjeferreira]

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

[lrytz]

@jtjeferreira It seems the corresponding code in scala3 is GenericSignatures.scala

https://github.com/scala/scala3/blob/3.4.1/compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala#L275

corresponds to

https://github.com/scala/scala/pull/8127/files#diff-271d576b3a80359cbc5db9e1a8dd2629c2cb1d99bf8a30854c64742f98c7a6b0R354

[jtjeferreira]

@jtjeferreira It seems the corresponding code in scala3 is GenericSignatures.scala

3.4.1/compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala#L275

corresponds to

scala/scala#8127 (files)

Thanks for the pointer

[jtjeferreira]

@lrytz I think I got the port applied correctly, but scalajs tests are failing with :

Test 'tests/run/i10846' failed with output:                                     
org.scalajs.linker.interface.LinkingException: There were linking errors

any hints?

[jtjeferreira]

any hints?

after reading https://www.scala-js.org/doc/project/linking-errors.html#using-unsupported-jdk-libraries-possibly-transitively and earlier output of the tests I think the problem is due to the usage of java.lang.Class related method.

I think we need to disable this test for scalajs. Should I move it to a different folder?

[lrytz]

It seems all tests in run will also be tested on Scala.js.

scala3/sjs-compiler-tests/test/scala/dotty/tools/dotc/ScalaJSCompilationTests.scala

Line 62 in c6fbe6f

compileFilesInDir("tests/run", scalaJSOptions),

Someone else will know better than me where to put the test (eg @bishabosha).

[sjrd]

You should keep it there. You can ignore a test on Scala.js like this:

scala3/tests/run/t8905/Test.java

Line 1 in c6fbe6f

// scalajs: --skip

[jtjeferreira]

This is ready for review. Let me know if I should update the PR description and/or squash commits.

Also as discussed here, should this backported to LTS?

[jtjeferreira]

@lrytz @SethTisue this is ready for review

[lrytz]

LGTM, thank you! Could you squash all the changes into a single commit with a bit of a cleaned up commit message?

[lrytz]

Thank you!

[jtjeferreira]

This will be available in which release? Shall I try to back port it to older releases?

[SethTisue]

It will go in 3.6 and likely in some 3.3.x release unless the backport proves problematic. Backports are done in batches and in order of original merge, so it wouldn't actually be helpful to submit an explicit backport PR.