Skip to content

Fix #8001: Erase polymorphic value classes like Scala 2 #10765

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

Merged
merged 3 commits into from
Dec 14, 2020

Conversation

smarter
Copy link
Member

@smarter smarter commented Dec 12, 2020

Previously given:

class Poly[A](val value: A) extends AnyVal

We always erased Poly[X] to Object, no matter the value X, because
the erasure was the erased underlying type as seen from its definition,
and not as seen from the current prefix. But it turns out that in Scala
2, Foo[Int] will be erased to Integer instead (it would have made
more sense to use int but I suspect this is more accidental than
designed).

To be binary-compatible with Scala 2 and to support the same kind of
overloads we need to replicate its behavior, more precisely the rules I
was able to reverse-engineer are:

  • Given class Foo[A](x: A) extends AnyVal, Foo[X] should erase like
    X, except if its a primitive in which case it erases to the boxed
    version of this primitive.
  • Given class Bar[A](x: Array[A]) extends AnyVal, Bar[X] will be
    erased like Array[A] as seen from its definition site, no matter
    the X (same if A is bounded).

I was able to adapt our implementation of value class erasure to these
new rules without too much refactoring through one compromise: a value
class can no longer wrap another value class. This was never supported
by Scala 2 so we can afford to not support it either.

Previously given:

    class Poly[A](val value: A) extends AnyVal

We always erased `Poly[X]` to `Object`, no matter the value `X`, because
the erasure was the erased underlying type as seen from its definition,
and not as seen from the current prefix. But it turns out that in Scala
2, `Foo[Int]` will be erased to `Integer` instead (it would have made
more sense to use `int` but I suspect this is more accidental than
designed).

To be binary-compatible with Scala 2 and to support the same kind of
overloads we need to replicate its behavior, more precisely the rules I
was able to reverse-engineer are:

- Given `class Foo[A](x: A) extends AnyVal`, `Foo[X]` should erase like
  `X`, except if its a primitive in which case it erases to the boxed
  version of this primitive.
- Given `class Bar[A](x: Array[A]) extends AnyVal`, `Bar[X]` will be
  erased like `Array[A]` as seen from its definition site, no matter
  the `X` (same if `A` is bounded).

I was able to adapt our implementation of value class erasure to these
new rules without too much refactoring through one compromise: a value
class can no longer wrap another value class. This was never supported
by Scala 2 so we can afford to not support it either.
@bishabosha
Copy link
Member

bishabosha commented Dec 12, 2020

it probably doesn't add much new as a test case but i noticed this erases differently on Scala 2 vs Dotty

object Values {
  def reify[I <: Int with Singleton](implicit I: ValueOf[I]): I = valueOf[I]
}

@smarter
Copy link
Member Author

smarter commented Dec 12, 2020

@bishabosha erasure of intersection types is something I'll address in another PR.

@bishabosha
Copy link
Member

bishabosha commented Dec 12, 2020

@bishabosha erasure of intersection types is something I'll address in another PR.

I've removed the singleton bound and its the same result,

object Values {
  //public <I extends java.lang.Object> I reify(I);
  //    descriptor: (Ljava/lang/Object;)I
  def reify[I <: Int](implicit I: ValueOf[I]): I = valueOf[I]
}

@smarter
Copy link
Member Author

smarter commented Dec 12, 2020

OK, that should be covered by class Poly in the tests.

After the previous commit, this randomly fails the CI with "class
defined twice module class scala" and I don't want to add more special
cases to support this, instead we should probably just disallow `scala`
as a top-level classname.
This test printed an error message in the log output which was extremely
misleading since it looked like a weird test failure. Removing it
instead of fixing it because it's not actually testing anything useful:
it's supposed to check that we don't get a cascade of error messages if
scala3-library is missing from the classpath, but after
24eb6cf this only worked by chance
because the code we're compiling is extremely simple (Foo.scala only
contains "class Foo"), so it's doubly misleading.
@smarter smarter requested a review from odersky December 13, 2020 01:56
@smarter smarter added this to the 3.0.0-M3 milestone Dec 13, 2020
@odersky odersky merged commit 1f5e98c into scala:master Dec 14, 2020
@odersky odersky deleted the poly-vc-new-3 branch December 14, 2020 12:36
@Kordyjan Kordyjan modified the milestones: 3.0.0-M3, 3.0.0 Aug 2, 2023
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.

4 participants