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

SI-6948 Make the Abstract* classes public. #3480

Merged
merged 1 commit into from Feb 10, 2014
Merged

SI-6948 Make the Abstract* classes public. #3480

merged 1 commit into from Feb 10, 2014

Conversation

paulp
Copy link
Contributor

@paulp paulp commented Feb 6, 2014

Several weaknesses in the implementation converge and force multiply.

  1. Type constructor inference is not persistent. During implicit search
    it will give up after the first seen parent even if some deeper base type
    (even another direct parent) would satisfy the search.

  2. Type inference is not aware of access restrictions. Inferred types are
    calculated with disregard for whether the inferred type is visible at
    the point of inference. That means that package-private types - which may be
    private for any number of good reasons, such as not wanting them to appear in
    bytecode thus creating binary compatibility obligations - are not private.
    There is no such thing as a qualified private type.

  package p {
    trait PublicInterface[T] { def foo(): Int }
    private[p] trait ImplementationOnly[T] extends PublicInterface[T] { def foo(): Int = 1 }
    class PublicClass extends ImplementationOnly[PublicClass]
  }
  package q {
    object Test {
      def f[A, CC[X]](xs: CC[A]): CC[A] = xs
      def g = f(new p.PublicClass) // inferred type: p.ImplementationOnly[p.PublicClass]
      def h = g.foo()
      // Bytecode contains:
      // public p.ImplementationOnly<p.PublicClass> g();
      // public int h();
      //    0: aload_0
      //    1: invokevirtual #30                 // Method g:()Lp/ImplementationOnly;
      //    4: invokeinterface #33,  1           // InterfaceMethod p/ImplementationOnly.foo:()I
      //    9: ireturn
    }
  }
  1. The trait encoding leads to a proliferation of forwarder methods, so much so that
    1.5 Mb of bytecode was taken off of the standard library size by creating abstract classes
    which act as central mixin points so that leaf classes can inherit some methods the
    old fashioned way rather than each receiving their own copy of every trait defined method.
    This was done for 2.10 through the creation of the Abstract* classes, all of which were
    given reduced visibility to keep them out of the API.
  private[scala] class AbstractSeq extends ...

This achieved its intended goal very nicely, but also some unintended ones.

In combination with 1) above:

  scala> val rand = new scala.util.Random()
  rand: scala.util.Random = scala.util.Random@7f85a53b

  // this works
  scala> rand.shuffle(0 to 5)
  res1: scala.collection.immutable.IndexedSeq[Int] = Vector(4, 0, 1, 2, 5, 3)

  // and this doesn't! good luck reasoning that one out
  scala> rand.shuffle(0 until 5)
  <console>:9: error: Cannot construct a collection of type scala.collection.AbstractSeq[Int]
    with elements of type Int based on a collection of type scala.collection.AbstractSeq[Int].
                rand.shuffle(0 until 5)
                            ^

  // Somewhat comically, in scala 2.9 it was flipped: to failed (differently), until worked.
  scala> scala.util.Random.shuffle(0 to 5)
  <console>:8: error: type mismatch;
   found   : scala.collection.immutable.Range.Inclusive
   required: ?CC[?T]

  scala> scala.util.Random.shuffle(0 until 5)
  res2: scala.collection.immutable.IndexedSeq[Int] = Vector(4, 3, 1, 2, 0)

In combination with 2) above:

  scala> def f[A, CC[X]](xs: CC[A]): CC[A] = xs
  f: [A, CC[X]](xs: CC[A])CC[A]

  scala> var x = f(1 until 10)
  x: scala.collection.AbstractSeq[Int] = Range(1, 2, 3, 4, 5, 6, 7, 8, 9)

  // It has inferred a type for our value which it will not allow us to use or even to reference.
  scala> var y: scala.collection.AbstractSeq[Int] = x
  <console>:10: error: class AbstractSeq in package collection cannot be accessed in package collection
         var y: scala.collection.AbstractSeq[Int] = x
                                 ^
  // This one is a straight regression - in scala 2.9,
  scala> var x = f(1 until 10)
  x: scala.collection.immutable.IndexedSeq[Int] = Range(1, 2, 3, 4, 5, 6, 7, 8, 9)

Since 1) and 2) are essentially unfixable - at least by me - I propose
to ameliorate these regressions by attacking the symptoms at the leaves.
That means making all the Abstract* classes public - keeping in mind that
they must already be assumed to be in the binary compatibility footprint,
since they have been leaking throughout their existence. This only impacts
the inference of inaccessible collections types - it doesn't help with the
more serious issue with type inference.

Several weaknesses in the implementation converge and force multiply.

1) Type constructor inference is not persistent. During implicit search
it will give up after the first seen parent even if some deeper base type
(even another direct parent) would satisfy the search.

2) Type inference is not aware of access restrictions. Inferred types are
calculated with disregard for whether the inferred type is visible at
the point of inference. That means that package-private types - which may be
private for any number of good reasons, such as not wanting them to appear in
bytecode thus creating binary compatibility obligations - are not private.
There is no such thing as a qualified private type.

  package p {
    trait PublicInterface[T] { def foo(): Int }
    private[p] trait ImplementationOnly[T] extends PublicInterface[T] { def foo(): Int = 1 }
    class PublicClass extends ImplementationOnly[PublicClass]
  }
  package q {
    object Test {
      def f[A, CC[X]](xs: CC[A]): CC[A] = xs
      def g = f(new p.PublicClass) // inferred type: p.ImplementationOnly[p.PublicClass]
      def h = g.foo()
      // Bytecode contains:
      // public p.ImplementationOnly<p.PublicClass> g();
      // public int h();
      //    0: aload_0
      //    1: invokevirtual #30                 // Method g:()Lp/ImplementationOnly;
      //    4: invokeinterface #33,  1           // InterfaceMethod p/ImplementationOnly.foo:()I
      //    9: ireturn
    }
  }

3) The trait encoding leads to a proliferation of forwarder methods, so much so that
1.5 Mb of bytecode was taken off of the standard library size by creating abstract classes
which act as central mixin points so that leaf classes can inherit some methods the
old fashioned way rather than each receiving their own copy of every trait defined method.
This was done for 2.10 through the creation of the Abstract* classes, all of which were
given reduced visibility to keep them out of the API.

  private[collection] class AbstractSeq extends ...

This achieved its intended goal very nicely, but also some unintended ones.

In combination with 1) above:

  scala> val rand = new scala.util.Random()
  rand: scala.util.Random = scala.util.Random@7f85a53b

  // this works
  scala> rand.shuffle(0 to 5)
  res1: scala.collection.immutable.IndexedSeq[Int] = Vector(4, 0, 1, 2, 5, 3)

  // and this doesn't! good luck reasoning that one out
  scala> rand.shuffle(0 until 5)
  <console>:9: error: Cannot construct a collection of type scala.collection.AbstractSeq[Int]
    with elements of type Int based on a collection of type scala.collection.AbstractSeq[Int].
                rand.shuffle(0 until 5)
                            ^

  // Somewhat comically, in scala 2.9 it was flipped: to failed (differently), until worked.
  scala> scala.util.Random.shuffle(0 to 5)
  <console>:8: error: type mismatch;
   found   : scala.collection.immutable.Range.Inclusive
   required: ?CC[?T]

  scala> scala.util.Random.shuffle(0 until 5)
  res2: scala.collection.immutable.IndexedSeq[Int] = Vector(4, 3, 1, 2, 0)

In combination with 2) above:

  scala> def f[A, CC[X]](xs: CC[A]): CC[A] = xs
  f: [A, CC[X]](xs: CC[A])CC[A]

  scala> var x = f(1 until 10)
  x: scala.collection.AbstractSeq[Int] = Range(1, 2, 3, 4, 5, 6, 7, 8, 9)

  // It has inferred a type for our value which it will not allow us to use or even to reference.
  scala> var y: scala.collection.AbstractSeq[Int] = x
  <console>:10: error: class AbstractSeq in package collection cannot be accessed in package collection
         var y: scala.collection.AbstractSeq[Int] = x
                                 ^
  // This one is a straight regression - in scala 2.9,
  scala> var x = f(1 until 10)
  x: scala.collection.immutable.IndexedSeq[Int] = Range(1, 2, 3, 4, 5, 6, 7, 8, 9)

Since 1) and 2) are essentially unfixable - at least by me - I propose
to ameliorate these regressions by attacking the symptoms at the leaves.
That means making all the Abstract* classes public - keeping in mind that
they must already be assumed to be in the binary compatibility footprint,
since they have been leaking throughout their existence. This only impacts
the inference of inaccessible collections types - it doesn't help with the
more serious issue with type inference.
@paulp
Copy link
Contributor Author

paulp commented Feb 7, 2014

Review @retronym

@adriaanm
Copy link
Contributor

Note that this is a workaround, so let's not close the issue on merging.

@adriaanm
Copy link
Contributor

@Ichoran, could you work steal the review here?

@Ichoran
Copy link
Contributor

Ichoran commented Feb 10, 2014

@adriaanm @retronym - Okay, I've got the review on this one.

@Ichoran
Copy link
Contributor

Ichoran commented Feb 10, 2014

LGTM.

gkossakowski added a commit that referenced this pull request Feb 10, 2014
@gkossakowski gkossakowski merged commit 90aa12e into scala:master Feb 10, 2014
@paulp paulp deleted the pr/publicize-abstract-star branch February 14, 2014 01:37
@tvierling
Copy link
Contributor

Since I contributed the original, I do need to provide an apology that I missed these corner cases. I'm glad to know that the usefulness is still here, but perhaps future exploitation of the Java 8 feature of interface default methods can finally kill off this grand hack (as it really was only a hack to begin with).

@paulp
Copy link
Contributor Author

paulp commented Apr 18, 2014

@tvierling the level of familiarity with scala implementation quirks one would have needed to anticipate these issues is higher than any contributor could be expected to have. Based on the comments one might reasonably wonder whether any single individual could have done so. Not me anyway.

@retronym
Copy link
Member

Yep, binary compatibility is hard, source compatibility is probably intractable.

@tvierling
Copy link
Contributor

Yeah, I know, just wanted to acknowledge that I noticed this pull request and its symptomatic needs, anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants