Rename ImmutableArray to ArraySeq #6611

lrytz · 2018-05-09T19:02:02Z

lrytz · 2018-05-09T19:24:38Z

(this also needs a rebase on the partest in-source)

NthPortal · 2018-05-09T23:06:12Z

I think we may need another collection - a ConSeq - for this

lrytz · 2018-05-10T15:25:47Z

rebased on the insource-partest pr for now to test, but unfortunately that created 100 jobs on jenkins, as there's no [ci: last-only] here...

lrytz · 2018-05-10T18:28:20Z

Tests look good, but the specialized category failed because I need to build a new instrumented.jar. I did that recently, so no big deal.

lrytz · 2018-05-10T18:31:21Z

Added another commit that renames WrappedArray to ArraySeq. So basically there's mutable.ArraySeq and immutable.ArraySeq. @szeiger suggested this in scala/bug#10836. I'm not 100% sure it's a good idea, because creating a mutable.ArraySeq is not always explicit, due to Predef.wrapXArray. It might be too confusing. Let's discuss!

lrytz · 2018-05-10T20:37:43Z

Also, if we name it mutable.ArraySeq instead of mutable.WrappedArray, we basically change the semantics of mutable.ArraySeq from 2.12 to 2.13. Users of 2.12 ArraySeq would have to use ArraySeq.untagged. We could add a migration warning.

The change of semantics is also there in current 2.13.x, mutable.ArraySeq is a deprecated alias for WrappedArray. So users get a deprecation warning. If we keep this, we should also add a migration warning.

sjrd · 2018-05-11T21:33:23Z

src/library/scala/collection/mutable/package.scala

-  @deprecated("Use WrappedArray instead of ArraySeq; it can represent both, boxed and unboxed arrays", "2.13.0")
-  type ArraySeq[X] = WrappedArray[X]
-  @deprecated("Use WrappedArray instead of ArraySeq; it can represent both, boxed and unboxed arrays", "2.13.0")
-  val ArraySeq = WrappedArray.untagged


I think we need the deprecated aliases in the other direction, if we're doing this change.

Yes, thanks for pointing that out. Though I think we should drop the second commit, keep mutable.WrappedArray. Waiting for opinions on that still :-)

lrytz · 2018-05-13T10:21:47Z

Gah, this also needs a new scalacheck release.. I can do that through a publishLocal'd scala version i guess.

There's also a binary incompatibility in the new macroAnnots tests.

[test] [error] - macroAnnot/test
[test] [error]   - macroAnnot/compile:compileIncremental failed: java.lang.NoSuchMethodError: scala.runtime.ScalaRunTime$.wrapRefArray([Ljava/lang/Object;)Lscala/collection/immutable/ImmutableArray;

@adriaanm are there any pre-compiled binaries involved here? Otherwise it must be that code is being compiled with the starr compiler, but then executed with locker or quick. Just running macroAnnot/test locally worked for me, so I'm not sure.

lrytz · 2018-05-13T14:48:52Z

It turns out just rebuilding scalacheck with a local Scala version is not enough, because the scalacheck artifact depends on the Scala library it was built with. So that Scala version needs to be on maven... We can either

Release another 2.13.0-M4-pre-abcd123 to maven (with the rename) and the build scalacheck using that
In-source scalacheck
other ideas?

I vote for 2.

SethTisue · 2018-05-13T15:19:49Z

👍 to in-sourcing

lrytz · 2018-05-13T22:34:09Z

Added a commit that in-sources scalacheck. TODO: update IntelliJ templates.

szeiger · 2018-05-14T12:26:29Z

src/library/scala/Array.scala

    // in pattern matcher.  @PP: I noted in #4364 I think the behavior is correct.
-    // Is ImmutableArray safe here? In 2.12 we used to call .toIndexedSeq which copied the array
-    // instead of wrapping it in a WrappedArray but it appears unnecessary.
+    // Is ArraySeq safe here? In 2.12 we used to call .toIndexedSeq which copied the array


It turns out it is not. We need to make a defensive copy. Varargs use immutable.Seq so we can't just return a mutable.ArraySeq but the Seq can leak out of a pattern match.

sjrd · 2018-05-14T13:13:24Z

FYI, it turns out Scala.js also depends on this PR to be merged, if it does and what portions of it do :-s I had not realized that before.

I've already updated after #6620.

Sources from lrytz/scalacheck@1fd5eef

We get some linkage errors now. sbt seems to compile the compiler interface against the current version, which is 2.13.0-pre-SNAPSHOT, so something arbitrary. The scalaInstance in the macroAnnot project is the quick instance. That's probably the cause. We should convert macroAnnot tests to partest, where we do this bootstrap.

lrytz · 2018-05-14T13:18:50Z

I'll merge this as soon as it goes green.

lrytz added the WIP label May 9, 2018

scala-jenkins added this to the 2.13.0-M5 milestone May 9, 2018

lrytz modified the milestones: 2.13.0-M5, 2.13.0-M4 May 9, 2018

lrytz force-pushed the renameImmutableArray branch from a5ef951 to 52d755a Compare May 10, 2018 15:23

lrytz closed this May 10, 2018

lrytz changed the title ~~Rename ImmutableArray to ArraySeq~~ Rename ImmutableArray to ArraySeq [ci: last-only] May 10, 2018

lrytz reopened this May 10, 2018

lrytz changed the title ~~Rename ImmutableArray to ArraySeq [ci: last-only]~~ Rename ImmutableArray to ArraySeq May 11, 2018

lrytz force-pushed the renameImmutableArray branch from d7260f8 to 708551f Compare May 11, 2018 15:29

This comment has been minimized.

Sign in to view

sjrd reviewed May 11, 2018

View reviewed changes

julienrf closed this May 12, 2018

julienrf reopened this May 12, 2018

lrytz force-pushed the renameImmutableArray branch 2 times, most recently from 6da383c to 39bb80c Compare May 13, 2018 09:14

lrytz force-pushed the renameImmutableArray branch from 39bb80c to e1dcb16 Compare May 13, 2018 22:33

lrytz force-pushed the renameImmutableArray branch 2 times, most recently from 7ed3f52 to b83a39a Compare May 13, 2018 22:37

lrytz force-pushed the renameImmutableArray branch 3 times, most recently from 0251064 to e60f3e0 Compare May 14, 2018 08:45

lrytz removed the WIP label May 14, 2018

lrytz mentioned this pull request May 14, 2018

Make StringOps a pure value class #6565

Merged

lrytz force-pushed the renameImmutableArray branch from e60f3e0 to e5a25cc Compare May 14, 2018 12:22

szeiger reviewed May 14, 2018

View reviewed changes

szeiger approved these changes May 14, 2018

View reviewed changes

sjrd mentioned this pull request May 14, 2018

Compatibility with Scala 2.13.0-M4. scala-js/scala-js#3361

Merged

lrytz added 4 commits May 14, 2018 15:18

In-source scalacheck

f03fe6f

Sources from lrytz/scalacheck@1fd5eef

Rename ImmutableArray to ArraySeq

f58c9a8

Rename WrappedArray to ArraySeq

ec070b7

lrytz force-pushed the renameImmutableArray branch from e5a25cc to ec070b7 Compare May 14, 2018 13:18

lrytz merged commit 68bad81 into scala:2.13.x May 14, 2018

Rename ImmutableArray to ArraySeq #6611

Rename ImmutableArray to ArraySeq #6611

Uh oh!

Conversation

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

Uh oh!

Uh oh!

lrytz commented May 9, 2018

Uh oh!

NthPortal commented May 9, 2018

Uh oh!

lrytz commented May 10, 2018

Uh oh!

lrytz commented May 10, 2018

Uh oh!

lrytz commented May 10, 2018

Uh oh!

lrytz commented May 10, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

sjrd May 11, 2018

Choose a reason for hiding this comment

Uh oh!

lrytz May 12, 2018

Choose a reason for hiding this comment

Uh oh!

lrytz commented May 13, 2018

Uh oh!

lrytz commented May 13, 2018

Uh oh!

SethTisue commented May 13, 2018

Uh oh!

lrytz commented May 13, 2018

Uh oh!

szeiger May 14, 2018

Choose a reason for hiding this comment

Uh oh!

sjrd commented May 14, 2018

Uh oh!

lrytz commented May 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lrytz commented May 9, 2018 •

edited

Loading