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.

Sign up for GitHub

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

Jump to bottom

Rename ImmutableArray to ArraySeq #6611

Merged
merged 4 commits into from
May 14, 2018
Merged

Rename ImmutableArray to ArraySeq #6611

merged 4 commits into from
May 14, 2018

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented May 9, 2018 •
edited
Loading

@lrytz lrytz added the WIP label May 9, 2018
@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone May 9, 2018
@lrytz lrytz modified the milestones: 2.13.0-M5, 2.13.0-M4 May 9, 2018
@lrytz
Copy link
Member Author

lrytz commented May 9, 2018

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

@NthPortal
Copy link
Contributor

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

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

lrytz commented May 10, 2018

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 lrytz closed this May 10, 2018
@lrytz lrytz changed the title Rename ImmutableArray to ArraySeq Rename ImmutableArray to ArraySeq [ci: last-only] May 10, 2018
@lrytz lrytz reopened this May 10, 2018
@lrytz
Copy link
Member Author

lrytz commented May 10, 2018

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
Copy link
Member Author

lrytz commented May 10, 2018

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
Copy link
Member Author

lrytz commented May 10, 2018

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.

@lrytz lrytz changed the title Rename ImmutableArray to ArraySeq [ci: last-only] Rename ImmutableArray to ArraySeq May 11, 2018
@lrytz lrytz force-pushed the renameImmutableArray branch from d7260f8 to 708551f Compare May 11, 2018 15:29
@adriaanm

This comment has been minimized.

Sign in to view
@adriaanm

This comment has been minimized.

Sign in to view
sjrd
sjrd reviewed May 11, 2018
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@julienrf julienrf closed this May 12, 2018
@julienrf julienrf reopened this May 12, 2018
@lrytz lrytz force-pushed the renameImmutableArray branch 2 times, most recently from 6da383c to 39bb80c Compare May 13, 2018 09:14
@lrytz
Copy link
Member Author

lrytz commented May 13, 2018

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
Copy link
Member Author

lrytz commented May 13, 2018

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

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

I vote for 2.

@SethTisue
Copy link
Member

👍 to in-sourcing

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

lrytz commented May 13, 2018

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

@lrytz lrytz force-pushed the renameImmutableArray branch 2 times, most recently from 7ed3f52 to b83a39a Compare May 13, 2018 22:37
@lrytz lrytz force-pushed the renameImmutableArray branch 3 times, most recently from 0251064 to e60f3e0 Compare May 14, 2018 08:45
@lrytz lrytz removed the WIP label May 14, 2018
@lrytz lrytz mentioned this pull request May 14, 2018
Merged
@lrytz lrytz force-pushed the renameImmutableArray branch from e60f3e0 to e5a25cc Compare May 14, 2018 12:22
szeiger
szeiger reviewed May 14, 2018
View reviewed changes
src/library/scala/Array.scala
// !!! the null check should to be necessary, but without it 2241 fails. Seems to be a bug
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sjrd mentioned this pull request May 14, 2018
Merged
@sjrd
Copy link
Member

sjrd commented May 14, 2018

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.

lrytz added 4 commits May 14, 2018 15:18
@lrytz
In-source scalacheck
f03fe6f 
Sources from lrytz/scalacheck@1fd5eef
@lrytz
Don't run macroAnnot tests for now
1190208 
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
Rename ImmutableArray to ArraySeq
f58c9a8
@lrytz
Rename WrappedArray to ArraySeq
ec070b7
@lrytz lrytz force-pushed the renameImmutableArray branch from e5a25cc to ec070b7 Compare May 14, 2018 13:18
@lrytz
Copy link
Member Author

lrytz commented May 14, 2018

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

@lrytz lrytz merged commit 68bad81 into scala:2.13.x May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd left review comments

@szeiger szeiger szeiger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.13.0-M4
Development

Successfully merging this pull request may close these issues.
8 participants
@lrytz @NthPortal @adriaanm @SethTisue @sjrd @szeiger @julienrf @scala-jenkins