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

preserve immutable collections in to(Target) conversions #495

Merged
merged 3 commits into from
Nov 8, 2021

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Oct 29, 2021

In Scala 2.13, collection.to(Target) conversions return the collection object unchanged if it is immutable and is already a subtype of the Target type.

This PR brings the same behavior to Scala 2.12 when using scala-collection-compat.

The 2.12 codebase is unchanged, so a to[Target] conversion may still create unnecessary copies.

Remaining differences between 2.12 and 2.13:

  • static types: 2.12's to is defined as def to[Col[_]](implicit cbf: CanBuildFrom[Nothing, A, Col[A]]): Col[A], so to(Map) has static type Iterable[(K, V)], not Map[K, V]

  • lazy maps: mapValues returns a lazy map that's a subtype of immutable.Map, so

    scala> val mv = Map(1 -> 1).mapValues(_ + 1)
    scala> mv.to(Map) eq mv
    res7: Boolean = true // false in 2.13, a strict copy is created
    

@lrytz lrytz force-pushed the toConserve branch 2 times, most recently from 0af7d33 to 0b2fc40 Compare October 29, 2021 12:40
Copy link
Member Author

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

review by @retronym

@@ -72,19 +72,29 @@ lazy val compat = new MultiScalaCrossProject(
sharedSourceDir / "scala-2.11_2.12"
}
},
Test / unmanagedSourceDirectories += {
Copy link
Member Author

Choose a reason for hiding this comment

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

copy paste from above, not sure how to factor things out in sbt

@lrytz
Copy link
Member Author

lrytz commented Oct 29, 2021

cc @martijnhoekstra, since you originally wrote IdentityPreservingBuilder for #238. This PR applies it more widely.

@SethTisue
Copy link
Member

this might interest @scala/collections

Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

initial review is that it looks correct. if I get more energy, hopefully I can do a more thorough review.

I did not review of the sbt changes, as I'm less confident in my ability to refactor sbt configs than in yours

@lrytz
Copy link
Member Author

lrytz commented Nov 1, 2021

Thank you @NthPortal!

@lrytz lrytz merged commit d55a12f into scala:main Nov 8, 2021
@SethTisue
Copy link
Member

@lrytz can you fill in the PR description?

@SethTisue SethTisue changed the title preserve immutable collections in to(Target) conversions preserve immutable collections in to(Target) conversions Nov 9, 2021
@lrytz
Copy link
Member Author

lrytz commented Nov 9, 2021

👍 done

@NthPortal
Copy link
Contributor

oh, this is already released, awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants