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

Support Scala 2.13.0-M5. #3434

Merged
merged 5 commits into from Aug 30, 2018

Conversation

Projects
None yet
2 participants
@sjrd
Copy link
Member

sjrd commented Aug 23, 2018

There is one non-trivial change: js.ArrayOps had to be completely reimplemented for 2.13.0-M5, because the collection framework does not accept collections classes whose repr is not themselves anymore. This means that js.ArrayOps must be separated from the collection hierarchy per se, and instead be an independent AnyVal that happens to implement the entire same API. This is a lot of code, but is similar to what scala.collection.ArrayOps does for scala.Array (only that we have the API of Buffer to support, in addition of just Seq).

Ideally, all those methods should receive dedicated tests now, since their implementation is not inherited from the collections. However, this commit does not go to such lengths.

The PR also includes

  • one commit to implement the addSuppressed mechanism of Throwable from JDK 7, which is now reached by the new Future implementation of 2.13.0-M5
  • a few preparatory commits for overrides and similar

@sjrd sjrd requested a review from gzm0 Aug 23, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 23, 2018

The first commit should be amended to include unit tests. I'm already opening the PR so that you can start reviewing it, should you have time to do so before I get to writing them.

I have locally tested with ++2.13.0-pre-53c4be1 and testSuite/test. That version is supposed to become M5 in the very short future (we're talking days).

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from f7b54f0 to ee67991 Aug 24, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 24, 2018

Updated with an amended commit for the suppression mechanism of Throwable. The rest is unchanged for now.

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 24, 2018

Erm ... your comments (and my answers) disappeared. I think you commented on the commit itself, rather than via the PR view of the commit, so GitHub did not transfer the comments to the amended commits.

I tried pushing the recovered commits from the reflog to https://github.com/sjrd/scala-js/tree/recover-the-comments, but it doesn't seem like the comments are there :-(

i += 1
}
-1
// scalastyle:on return

This comment has been minimized.

@sjrd

sjrd Aug 24, 2018

Member

From my mails: @gzm0 says:

Why not a tailrec method instead? Should compile to roughly the same code.

This comment has been minimized.

@sjrd

sjrd Aug 24, 2018

Member

Because it's copied from scala.collection.ArrayOps.scala.

In general, virtually all the methods are copied from their implementation in upstream scala/scala.

val r = new js.ArrayOps(xs).slice(pos, pos + groupSize)
pos += groupSize
r
}

This comment has been minimized.

@sjrd

sjrd Aug 24, 2018

Member

From my mails: @gzm0 says:

Two things in general here:

  • I do not understand when something is marked inline and when it is not (e.g. iterator is not, but should?).
  • How do we ensure that new methods on collections also appear here?

This comment has been minimized.

@sjrd

sjrd Aug 24, 2018

Member

I do not understand when something is marked inline and when it is not (e.g. iterator is not, but should?).

A method is @inline if at least one of the following applies:

  • It was @inline in its implementation upstream
  • It delegates to a JS operation
  • It is of the form { someMethodCall(...); xs }

Arguably, the last two bullets should be integrated as heuristics in the optimizer, but that's for another day.

How do we ensure that new methods on collections also appear here?

We do as they do for scala.collection.ArrayOps: we monitor the Seq, Buffer, Growable and Shrinkable APIs, and we replicate the changes. It's sad, but the new collection design does not seem to allow anything else.

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Another thing in general: There are a lot of while loops in this file to replace foreach on js.Array[_]. From what I have understood, our optimizer should be able to inline foreach completely. Shouldn't we use the higher level construct then?

@gzm0
Copy link
Contributor

gzm0 left a comment

I have left a bunch of comments where I do not understand (even based on your explanation) why there are no inlines. I also don't really know how to review the overrides, as a lot of it is just copying.

Also since all implementations are from scala/scala it seems to make little sense to fully review them as well.

}

/** An array containing the first `n` elements of this array. */
def take(n: Int): js.Array[A] =

This comment has been minimized.

@gzm0

gzm0 Aug 24, 2018

Contributor

Inline

xs.jsSlice(0, max(n, 0))

/** The rest of the array without its `n` first elements. */
def drop(n: Int): js.Array[A] =

This comment has been minimized.

@gzm0

gzm0 Aug 24, 2018

Contributor

Inline

xs.jsSlice(lo, xs.length)
}

def iterator: scala.collection.Iterator[A] =

This comment has been minimized.

@gzm0

gzm0 Aug 24, 2018

Contributor

Inline?

* less than size `size` if the elements don't divide evenly.
*/
def grouped(size: Int): scala.collection.Iterator[js.Array[A]] =
new js.ArrayOps.GroupedIterator[A](xs, size)

This comment has been minimized.

@gzm0

gzm0 Aug 24, 2018

Contributor

Inline?

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from ee67991 to 9555f47 Aug 24, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 24, 2018

I have left a bunch of comments where I do not understand (even based on your explanation) why there are no inlines.

Yes I missed those. I've added all the relevant @inlines.

I also don't really know how to review the overrides, as a lot of it is just copying.
Also since all implementations are from scala/scala it seems to make little sense to fully review them as well.

Sure, makes sense.

@gzm0
Copy link
Contributor

gzm0 left a comment

Reviewed ArrayOps up to distinct (exclusive).

Also: If we introduce ArrayOps like this, we also should introduce DictionaryOps (which we didn't have because the repr restriction always existed for Map IIRC).

* @return an iterator over all the tails of this array
*/
def tails: scala.collection.Iterator[js.Array[A]] =
iterateUntilEmpty(xs => xs.tail)

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

_.tail?

* the ordering `ord`.
*/
def sorted[B >: A](implicit ord: Ordering[B]): js.Array[A] =
new js.WrappedArray(xs).sorted(ord.asInstanceOf[Ordering[A]]).array

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

This makes me wonder: Should we do this everywhere to share implementation? We could even introduce a private, @inline version of wrappedarray to shave the overhead away.

This comment has been minimized.

@sjrd

sjrd Aug 25, 2018

Member

Actually WrappedArray is already @inline, so we would just need to delegate to WrappedArray.

However, even with our kick-ass optimizer, the implementations inherited from the collections into WrappedArray are not as good as those we can directly write on js.Array, which means that js.ArrayOps would not be on par with scala.collection.ArrayOps if we do that. I did it for sorted because the original delegates to java.util.Arrays.sort anyway, which in Scala.js goes back to the collections (and also, it's really non-trivial code!).

* `==`) to `elem`, `false` otherwise.
*/
def contains(elem: A): Boolean =
exists(elem == _)

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

indexOf(elem) > 0 otherwise indexOf should be implemented in terms of imdexWhere.

i += 1
}
b
}

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

There is a problem in this method: r and a forteriori replaced is unused.

bs(i).push(x)
i += 1
}
}

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

The usage of foreach here is inconsistent with other usages of while.

}
}
for (b <- bs)
bb.push(b)

This comment has been minimized.

@gzm0

gzm0 Aug 25, 2018

Contributor

Why copy again?

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from 9555f47 to 5e61cad Aug 25, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 25, 2018

Addressed the other comments. And also I had the courage to write tests until distinct (excluded).

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 25, 2018

Also: If we introduce ArrayOps like this, we also should introduce DictionaryOps (which we didn't have because the repr restriction always existed for Map IIRC).

We should do it eventually, but it is not necessary right now. In this PR I'm only aiming for parity in the support of 2.13.0-M5 wrt. the support of 2.12.x and 2.13.0-M4.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from 5e61cad to 5abc430 Aug 25, 2018

@gzm0
Copy link
Contributor

gzm0 left a comment

Reviewed all non-tests.

@inline final def toSeq: immutable.Seq[A] =
toIndexedSeq

def toIndexedSeq: immutable.IndexedSeq[A] =

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Inline?

* the element to remove.
* @return
* the arrayitself
*/

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Typo

}
ys match {
case ys: scala.collection.LinearSeq[A] =>
loop(ys)

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

I do not understand why this special case improves anything.

* @return
* the array itself
*/
def prepend(elem: A): js.Array[A] = {

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Inline?

* @throws IndexOutOfBoundsException
* if the index `idx` is not in the valid range `0 <= idx <= length`.
*/
@throws[IndexOutOfBoundsException]

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

updated doesnt have this annotation (but throws). Make consistent?

val len = xs.length
while (i < len) {
if (p(xs(i))) {
if (i != j) {

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Is this necessary for efficiency? IMHO it hurts readability.

@inline
def empty[A]: js.Array[A] = js.Array()
object ArrayOps {
@SerialVersionUID(3L)

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Why do we need this?

@inline
def newBuilder[A]: mutable.Builder[A, js.Array[A]] =
new ArrayOps[A]
override protected[this] def className = "ArrayView"

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Shouldn't this return a JS specific prefix? Otherwise toString will not be distinguishable from a scala.Array view (I suppose).

pos += 1
r
} catch {
case _: ArrayIndexOutOfBoundsException =>

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

This does not work IIUC (and below)

val r = new js.ArrayOps(xs).slice(pos, pos + groupSize)
pos += groupSize
r
}

This comment has been minimized.

@gzm0

gzm0 Aug 26, 2018

Contributor

Another thing in general: There are a lot of while loops in this file to replace foreach on js.Array[_]. From what I have understood, our optimizer should be able to inline foreach completely. Shouldn't we use the higher level construct then?

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from 5abc430 to 42d8b95 Aug 26, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 26, 2018

Updated. I have addressed your comments except the fact that we should use foreach everywhere. I'll do that tomorrow if I get the chance. I have also added many more tests. Only a few methods are still missing their tests.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch 2 times, most recently from ac6ff40 to 9853238 Aug 27, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 27, 2018

OK done. I have used foreach wherever possible. There are still a number of while loops because we often need the indices.

I have also finished the tests.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch 2 times, most recently from bd39628 to 2f777c4 Aug 27, 2018

@@ -123,7 +123,6 @@ abstract class GenJSCode extends plugins.PluginComponent

override def name: String = phaseName
override def description: String = GenJSCode.this.description
override def erasedTypes: Boolean = true

This comment has been minimized.

@sjrd

sjrd Aug 27, 2018

Member

I had to add this change to support the new candidate for 2.13.0-M5, i.e., 2.13.0-pre-8a52aa1. It turns out that the inherited value of erasedTypes is already always true (across all versions of Scala we support) so this line has always been redundant.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from 2f777c4 to d519de7 Aug 28, 2018

sjrd added some commits Aug 22, 2018

Bring the overrides for 2.13 up to date with 2.13.0-M5.
Due to the massive changes that have happened in the 2.13.0-M5
branch in these files, I simply bulk checked them out from the
originals in 2.13.0-M5, and reapplied our changes on them.
Introduce a "scala-m4-collections" src folder.
It contains a copy of `scala-new-collections`, and is used
specifically for 2.13.0-M4. This will allow further changes to
support 2.13.0-M5+ to be applied in `scala-new-collections`.
Implement the suppression mechanism of `Throwable`.
In the process, also handle the `writableStackTrace` parameter of
the new constructor.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from d519de7 to d64e416 Aug 28, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 28, 2018

Now that 2.13.0-M5 is on Maven Central, I added CI runs for that version in Jenkinsfile.

Support Scala 2.13.0-M5.
There is one non-trivial change: `js.ArrayOps` had to be completely
reimplemented for 2.13.0-M5, because the collection framework does
not accept collections classes whose `repr` is not themselves
anymore. This means that `js.ArrayOps` must be separated from the
collection *hierarchy* per se, and instead be an independent
`AnyVal` that happens to implement the entire same API. This is a
lot of code, but is similar to what `scala.collection.ArrayOps`
does for `scala.Array` (only that we have the API of `Buffer` to
support, in addition of just `Seq`).

These methods receive corresponding tests, since they are not
inherited from the upstream implementation anymore.

@sjrd sjrd force-pushed the sjrd:scala-2.13.0-m5 branch from d64e416 to 1ab9fce Aug 29, 2018

implicit def iterableOps[A](iterable: Iterable[A]): IterableOps[A] =
new IterableOps(iterable)
implicit def arrayAsIterable[A](array: js.Array[_ <: A]): scala.collection.Iterable[A] =
new js.WrappedArray(array)

This comment has been minimized.

@sjrd

sjrd Aug 29, 2018

Member

And I had to add this new implicit to take precedence over iterableOps, in case the expected type is already known to be scala.collection.Iterable[A] but the type of the array is a js.Array[B] with B <: A. This may be due to subtle changes in type inference in Scala 2.13.0-M5.

@gzm0

This comment has been minimized.

Copy link
Contributor

gzm0 commented Aug 30, 2018

I have given this another quick look and from what I see it looks fine. Unfortunately this review is not as proper as typically, but given the time pressure, I guess we have to accept that.

Do you mind me asking what the current process, especially in terms of timeline is for Scala milestones. It seems we are having a quite serious problem in there somewhere, since we always end up rushing to keep up with their milestones. This is not good, as it degrades review quality (like this PR). We should make sure we address this somehow.

@gzm0

gzm0 approved these changes Aug 30, 2018

@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Aug 30, 2018

Do you mind me asking what the current process, especially in terms of timeline is for Scala milestones. It seems we are having a quite serious problem in there somewhere, since we always end up rushing to keep up with their milestones. This is not good, as it degrades review quality (like this PR). We should make sure we address this somehow.

The problem is the extremely fast pace at which interfaces have been shifting in 2.13.x. This many changes in this little time have never happened in the past 5 years. We used to be able to back-publish several versions of Scala.js for new milestones of Scala. The rework of the collections in 2.13 has completely destroyed our approach to this issue.

I sincerely hope that things settle down now that M5 is the feature freeze. We shouldn't have to deal with such pressure from now on.

@sjrd sjrd merged commit be70907 into scala-js:0.6.x Aug 30, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@sjrd sjrd deleted the sjrd:scala-2.13.0-m5 branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment