Skip to content

preserve immutable collections in to(Target) conversions#495

Merged
lrytz merged 3 commits intoscala:mainfrom
lrytz:toConserve
Nov 8, 2021
Merged

preserve immutable collections in to(Target) conversions#495
lrytz merged 3 commits intoscala:mainfrom
lrytz:toConserve

Conversation

@lrytz
Copy link
Copy Markdown
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
Copy Markdown
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

sharedSourceDir / "scala-2.11_2.12"
}
},
Test / unmanagedSourceDirectories += {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

this might interest @scala/collections

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

lrytz commented Nov 1, 2021

Thank you @NthPortal!

@lrytz lrytz merged commit d55a12f into scala:main Nov 8, 2021
@SethTisue
Copy link
Copy Markdown
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
Copy Markdown
Member Author

lrytz commented Nov 9, 2021

👍 done

@NthPortal
Copy link
Copy Markdown
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