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

Implement experimental rule for unordering operation on Ordered collection #122

Closed
wants to merge 8 commits into from

Conversation

MasseGuillaume
Copy link
Contributor

@MasseGuillaume MasseGuillaume commented Aug 1, 2018

Depends on: #121
Related to: scala/bug#11037

Summary:

map/flatMap behave differently on Ordered collection in 2.13 vs 2.12:

In Scala 2.12, if there is no Ordering when doing map or flatMap, it will convert the collection to the unordered one:

val bitset = collection.BitSet(1)

// with ordering
bitset.map(x => x) // collection.BitSet(1)

// without ordering
case class Unordered(v: Int)
bitset.map(x => Unordered(x)) // Set(Unordered(1))

In Scala 2.13, it's a compilation error:

bitset.map(x => Unordered(x)) // Set(Unordered(1))
          ^
error: No implicit Ordering defined for Unordered.

It's in the Experimental rules because we need the type of the collection (bitset in our example).

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

I think we should not change 2.13’s syntax: we should produce code that uses unsorted.

@MasseGuillaume
Copy link
Contributor Author

@julienrf we should fix: scala/bug#11037. Let's merge this PR and create a followup PR to change it back to unsorted.

@julienrf
Copy link
Contributor

julienrf commented Aug 1, 2018

We should first fix the issue in scala/scala, and then open a PR here. I see no point in merging the current PR if we plan to undo it in a few days.

@MasseGuillaume
Copy link
Contributor Author

Ok, I will fix scala/bug#11037 now.

@julienrf
Copy link
Contributor

julienrf commented Aug 3, 2018

The issue has been fixed in Scala.

@julienrf julienrf closed this Aug 3, 2018
@MasseGuillaume
Copy link
Contributor Author

@julienrf I'm not sure this was fixed. I only modified the .unsorted method to return the most precise collection. We still have the case where SortedMap.map(unOrdered) does not compile in 2.13.

Also, I forgot in this PR to handle for comprehensions.

@julienrf
Copy link
Contributor

julienrf commented Aug 7, 2018

We still have the case where SortedMap.map(unOrdered) does not compile in 2.13

The cross-compatible way to achieve that is:

import scala.collection.compat._
SortedMap(1 -> 2).unsorted.map(toUnordered)

@MasseGuillaume
Copy link
Contributor Author

@julienrf unsorted is not available in 2.12 I just have to rename unsortedSpecific to unsorted and remove unsortedSpecific from 2.13 in this PR. The rewrite rule in this PR is also valuable, you closed this too early.

@julienrf
Copy link
Contributor

julienrf commented Aug 7, 2018

unsorted is not available in 2.12

It should be provided via import scala.collection.compat._.

Sorry for having closed this too early, I missed that we also needed something on the scala-collection-compat side.

@julienrf julienrf reopened this Aug 7, 2018
…ctions

in 2.12 a Ordered collection like SortedSet looses it's ordering on certain
operation like map, when the result does not have an ordering.
lazy val scala213 = "2.13.0-M4"
// lazy val scala213 = "2.13.0-pre-3ae6282" // use the sbt command `latest-213` to fetch the latest version
lazy val scala213 = "2.13.0-M4"
lazy val scala213Pre = LatestScala.getLatestScala213()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a both a blessing and a curse. It's good since it can allow us to catch bugs early, It's bad since it's makes things more fragile.

build.sbt Outdated
lazy val output212Plus =
Def.setting((baseDirectory in ThisBuild).value / "scalafix/output212+/src/main")
lazy val addOutput212Plus = unmanagedSourceDirectories in Compile += output212Plus.value / "scala"
val sources = Map( // scala211 scala212 scala213 scala213Pre
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since .unsorted is only available in 2.13.0-pre-... I need a way to make the output compile with a complex build matrix. I tought it would make more sense to just remove the directory structure (output212, output212Plus, output213) and make this more fine-grained, per-file.

@MasseGuillaume
Copy link
Contributor Author

@julienrf This PR is ready.

martijnhoekstra pushed a commit to martijnhoekstra/scala-collection-compat that referenced this pull request Nov 9, 2022
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.

2 participants