Remove unnecessary type parameter on `IterableOnce#stepper` #8083

szeiger · 2019-05-24T17:39:14Z

This also requires a contravariant StepperShape. Without this change it is impossible to delegate easily (without casting) from such a stepper implementation to one without the extra type parameter (see StepperTest). It also makes the signatures less ugly.

szeiger · 2019-05-24T19:31:31Z

Huh, that's unexpected. The method was final before and it's final after. I expected these changes to be binary compatible.

[error]  * method stepper(scala.collection.StepperShape)scala.collection.Stepper in class scala.jdk.Accumulator is declared final in current version
[error]    filter with: ProblemFilters.exclude[FinalMethodProblem]("scala.jdk.Accumulator.stepper")

retronym · 2019-05-27T08:44:16Z

The MiMa change is due to specialization:

$ jardiff -c $(scala-classpath $(scala-ref-version 90491f5~1)) $(scala-classpath $(scala-ref-version 90491f5))
...
diff --git a/scala/jdk/Accumulator.class.asm b/scala/jdk/Accumulator.class.asm
index ca5cb79..935cf3a 100644
--- a/scala/jdk/Accumulator.class.asm
+++ b/scala/jdk/Accumulator.class.asm
@@ -401,27 +401,9 @@
     // parameter final  p

   // access flags 0x401
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
+  // signature <S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TA;TS;>;)TS;
+  // declaration: S <S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<A, S>)
   public abstract efficientStepper(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public efficientStepper$mcD$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public efficientStepper$mcI$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public efficientStepper$mcJ$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
     // parameter final  shape

   // access flags 0x1
@@ -1204,28 +1186,10 @@
   // declaration: int <B>()
   public startsWith$default$2()I

-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public stepper(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public stepper$mcD$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public stepper$mcI$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
-    // parameter final  shape
-
-  // access flags 0x1
-  // signature <B:Ljava/lang/Object;S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TB;TS;>;)TS;
-  // declaration: S <B, S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<B, S>)
-  public stepper$mcJ$sp(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
+  // access flags 0x11
+  // signature <S::Lscala/collection/Stepper<*>;>(Lscala/collection/StepperShape<TA;TS;>;)TS;
+  // declaration: S <S extends scala.collection.Stepper<?>>(scala.collection.StepperShape<A, S>)
+  public final stepper(Lscala/collection/StepperShape;)Lscala/collection/Stepper;
     // parameter final  shape

   // access flags 0x1

martijnhoekstra · 2019-05-27T13:47:17Z

src/library/scala/collection/convert/StreamExtensions.scala

@@ -84,8 +84,8 @@ trait StreamExtensions {

  implicit class MapHasParKeyValueStream[K, V, CC[X, Y] <: collection.MapOps[X, Y, CC, _]](cc: CC[K, V]) {
    private type MapOpsWithEfficientKeyStepper[K, V] = collection.MapOps[K, V, CC, _] { def keyStepper[S <: Stepper[_]](implicit shape : StepperShape[K, S]) : S with EfficientSplit }


Is the shadowing of K and V here intended?

Maybe not intended, but it's not a bug :-)

https://github.com/scala/scala/pull/8105/files

adriaanm · 2019-05-28T12:48:28Z

@szeiger could you appease mima, perhaps also rebase to avoid potential conflict?

adriaanm · 2019-05-28T14:16:46Z

did the rebase

adriaanm · 2019-05-28T18:05:17Z

Looks like this also has a junit test failure?

This also requires a contravariant `StepperShape`. Without this change it is impossible to delegate easily (without casting) from such a `stepper` implementation to one without the extra type parameter (see `StepperTest`). It also makes the signatures less ugly.

sjrd · 2019-05-28T21:26:20Z

Oh no! No, no, no, no, no. This PR broke Scala.js 0.6.28, which I had specifically released to support 2.13. It triggers a compilation error in the file Range.scala, for which we have an override (so we override the source of the upstream Range.scala with some changes):

[error] /localhome/doeraene/projects/scalajs-0.6.x/scalalib/overrides-2.13/scala/collection/immutable/Range.scala:73: error: method stepper overrides nothing.
[error] Note: the super classes of class Range contain the following, non final members named stepper:
[error] override def stepper[S <: scala.collection.Stepper[_]](implicit shape: scala.collection.StepperShape[Int,S]): S with collection.Stepper.EfficientSplit
[error]   override final def stepper[B >: Int, S <: Stepper[_]](implicit shape: StepperShape[B, S]): S with EfficientSplit = {
[error]                      ^
[error] /localhome/doeraene/projects/scalajs-0.6.x/scalalib/overrides-2.13/scala/collection/immutable/NumericRange.scala:57: error: method stepper overrides nothing.
[error] Note: the super classes of class NumericRange contain the following, non final members named stepper:
[error] override def stepper[S <: scala.collection.Stepper[_]](implicit shape: scala.collection.StepperShape[T,S]): S with collection.Stepper.EfficientSplit
[error]   override def stepper[B >: T, S <: Stepper[_]](implicit shape: StepperShape[B, S]): S with EfficientSplit = {
[error]                ^
[info] two errors found

Is there anything that can be done on Scala's side to fix this? Because otherwise I'll despair at having to re-do yet another release in emergency mode (and I really don't have time for this now, with Scala Days approaching).

adriaanm · 2019-05-28T21:29:42Z

I can't think of anything except for reverting this PR or delaying the RC. (EDIT: more inclined towards the latter, as this PR was the whole reason for doing RC3)

sjrd · 2019-05-28T21:52:23Z

Well, I think what I'll do is cheat. I will live-patch Range.scala (and NumericRange.scala) on top of 0.6.28 to be able to publish it for 2.13.0-RC3 (and final I guess). That means that the sources for that release will not correspond to any preserved commit sha, but maybe that's OK in this case because it's just adapting an override file for an upstream change...

adriaanm · 2019-05-28T21:55:14Z

Ok, let me know if there's something we can do to make this annoyance somewhat less annoying. We're all really eager to get that final release out the door!

sjrd · 2019-05-28T21:59:48Z

Nah, I don't think there's anything that can be done. Just ship it.

Ported from scala/scala#8083 upstream.

szeiger added this to the 2.13.0-RC3 milestone May 24, 2019

szeiger requested a review from lrytz May 24, 2019 17:39

diesalbla added the library:collections PRs involving changes to the standard collection library label May 26, 2019

martijnhoekstra reviewed May 27, 2019

View reviewed changes

lrytz approved these changes May 27, 2019

View reviewed changes

adriaanm mentioned this pull request May 28, 2019

Release 2.13.0-RC3 scala/scala-dev#628

Closed

54 tasks

lrytz mentioned this pull request May 28, 2019

Generalize MapOps signature for keyStream / valueStream extension methods #8105

Merged

adriaanm force-pushed the wip/contravariant-steppershape branch from 90491f5 to 4b00073 Compare May 28, 2019 14:16

adriaanm force-pushed the wip/contravariant-steppershape branch from 4b00073 to f3b3f9a Compare May 28, 2019 19:04

Merge branch '2.13.x' into wip/contravariant-steppershape

85515d8

adriaanm merged commit a574e3a into scala:2.13.x May 28, 2019

adriaanm mentioned this pull request May 28, 2019

immutable.SeqOps#sorted is lazy by default #8100

Closed

SethTisue added the release-notes worth highlighting in next release notes label May 30, 2019

sjrd added a commit to sjrd/scala-js that referenced this pull request Jun 4, 2019

Adapt overrides for Scala 2.13.0-RC3 and 2.13.0 final.

71435b3

Ported from scala/scala#8083 upstream.

sjrd added a commit to sjrd/scala-js that referenced this pull request Jun 4, 2019

Adapt overrides for Scala 2.13.0-RC3 and 2.13.0 final.

a04df36

Ported from scala/scala#8083 upstream.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unnecessary type parameter on `IterableOnce#stepper` #8083

Remove unnecessary type parameter on `IterableOnce#stepper` #8083

Uh oh!

szeiger commented May 24, 2019

Uh oh!

szeiger commented May 24, 2019

Uh oh!

retronym commented May 27, 2019 •

edited

Loading

Uh oh!

martijnhoekstra May 27, 2019

Uh oh!

lrytz May 27, 2019

Uh oh!

lrytz May 28, 2019 •

edited

Loading

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

sjrd commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019 •

edited

Loading

Uh oh!

sjrd commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

sjrd commented May 28, 2019

Uh oh!

Uh oh!

		@@ -84,8 +84,8 @@ trait StreamExtensions {

		implicit class MapHasParKeyValueStream[K, V, CC[X, Y] <: collection.MapOps[X, Y, CC, _]](cc: CC[K, V]) {
		private type MapOpsWithEfficientKeyStepper[K, V] = collection.MapOps[K, V, CC, _] { def keyStepper[S <: Stepper[_]](implicit shape : StepperShape[K, S]) : S with EfficientSplit }

Remove unnecessary type parameter on IterableOnce#stepper #8083

Remove unnecessary type parameter on IterableOnce#stepper #8083

Uh oh!

Conversation

szeiger commented May 24, 2019

Uh oh!

szeiger commented May 24, 2019

Uh oh!

retronym commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnhoekstra May 27, 2019

Choose a reason for hiding this comment

Uh oh!

lrytz May 27, 2019

Choose a reason for hiding this comment

Uh oh!

lrytz May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

sjrd commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjrd commented May 28, 2019

Uh oh!

adriaanm commented May 28, 2019

Uh oh!

sjrd commented May 28, 2019

Uh oh!

Uh oh!

Remove unnecessary type parameter on `IterableOnce#stepper` #8083

Remove unnecessary type parameter on `IterableOnce#stepper` #8083

retronym commented May 27, 2019 •

edited

Loading

lrytz May 28, 2019 •

edited

Loading

adriaanm commented May 28, 2019 •

edited

Loading