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 several Abstract* classes public. #3478

Closed
wants to merge 1 commit into from
Closed

SI-6948 Make several Abstract* classes public. #3478

wants to merge 1 commit into from

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[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 public the Abstract* classes which are likely to show
up in type calculations (and remember they must already be assumed to
be in the binary compatibility footprint, since they have been leaking
throughout their existence.) And then, because if they don't have
builders they will continue to create inference failures for all their
subclasses, it means providing them with minimal CanBuildFrom instances.

This is what it takes to make "0 until 5" act the same as "0 to 5", unless
someone has a better idea.

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 public the Abstract* classes which are likely to show
up in type calculations (and remember they must already be assumed to
be in the binary compatibility footprint, since they have been leaking
throughout their existence.) And then, because if they don't have
builders they will continue to create inference failures for all their
subclasses, it means providing them with minimal CanBuildFrom instances.

This is what it takes to make "0 until 5" act the same as "0 to 5", unless
someone has a better idea.
@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

I don't have high hopes for this pull request but my conscience is clean.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

I guess this is even more doomed than I thought. Maybe someone would like to save the comment.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

Hopeless. Has to be dealt with where it's supposed to be dealt with.

@paulp paulp closed this Feb 6, 2014
@retronym
Copy link
Member

retronym commented Feb 6, 2014

The comment is in my vault. Good overview.

I'm still a bit skeptical about access-aware lubbing + inference. I tend to think its better to avoid designs with private parent classes, private types. This might push you one to use composition instead of inheritance. But maybe I'll come around one of these days...

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

I tend to think its better to avoid designs with private parent classes, private types.

But that's a non-sequitur. Either the feature exists or it doesn't. If it exists it should work correctly. If it doesn't work correctly it should not exist. You can argue to remove the feature (limited access on types) and I say sure. You can argue to fix it. But I can't accept an argument that it's broken and it's okay that it's broken because there are better ways to do it.

@retronym
Copy link
Member

retronym commented Feb 6, 2014

I am actually in favour of the part of this PR that makes the base classes public.

Inference is a (big!) convenience, but I don't like to leave API to its whims. Even if all base types are public, LUBs, or directly inferred types, ought to be reviewed, most probably weakened, and then set in stone with an explicit type annotation.

I'd like to invest in tools that helped people do this (e.g. turning inferred types into source code, lint checking that public APIs are annotated, are as-accessible as the enclosing class, don't contain, say, inferred refinement types, enforcing that implicits are annotated, etc.)

I think that's more important than making inference access-savvy.

@retronym
Copy link
Member

retronym commented Feb 6, 2014

But, as I said, I've only started ponderring access-savvy inference a few days ago when you brought it up. And these few days haven't offered a lot of time for me to spend in my pondering-hammock :(

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

Inference is a (big!) convenience, but I don't like to leave API to its whims. Even if all base types are public, LUBs, or directly inferred types, ought to be reviewed, most probably weakened, and then set in stone with an explicit type annotation.

I would go further and say that you can have the type inferred or it can be public. If the type is inferred it's private or it's an error. If you want it to be public then the type must be explicit - though that could mean inferred and injected back into the source code, something I think is way overdue. I can see paulp world winding up like that.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

Among the branches I didn't nuke is the one which injects all inferred types back into source code. It was a lot of work which will date quickly.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

"I think that's more important than making inference access-savvy."

I take no position on the relative importance of things. I take an absolute position on "Make it work or remove it." I think we'd all be a lot better off if this were a commonly held principle.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

You have a pondering hammock? Man I'm blowing it.

@paulp
Copy link
Contributor Author

paulp commented Feb 6, 2014

I forgot I could settle for half a loaf, once the second half of the loaf had blown up in my face. #3480.

@paulp paulp deleted the pr/remove-access-on-collections branch February 7, 2014 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants