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

Unexpected behavioural difference between collection.SortedMap and immutable.SortedMap #3326

Closed
scabug opened this issue Apr 20, 2010 · 6 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Apr 20, 2010

The ++ method behaves differently on a variable types as a collection.SortedMap from an immutable.SortedMap. In the former case, the "current" ordering is ignored and the default ordering used instead.

In the code appended below, if you run it as is: you will get the following output (which I would expect):

Map(5 -> Foo, 4 -> Bar, 2 -> Hello, 1 -> World)

If, however, you change the import line as I have indicated, the following (unexpected) output occurs (i.e. the resulting map m3 has the default ordering and not that inherited from m1).

Map(1 -> World, 2 -> Hello, 4 -> Bar, 5 -> Foo)

This is because the immutable.SortedMap trait overrides the ++ method whereas the collection.SortedMap does not (and hence the default CanBuildFrom comes into scope which, in turn, pulls in the default Ordering[Int])

Here is the code example:

object SMtest {
  def main(args: Array[String]) {

    import scala.collection.immutable.SortedMap //(change to scala.collection.SortedMap)
    import scala.math.Ordering

    val order = implicitly[Ordering[Int]].reverse
    var m1: SortedMap[Int, String] = SortedMap.empty(order)
    var m2: SortedMap[Int, String] = SortedMap.empty(order)

    m1 += (1 -> "World")
    m1 += (2 -> "Hello")

    m2 += (4 -> "Bar")
    m2 += (5 -> "Foo")

    val m3: SortedMap[Int, String] = m1 ++ m2
    println(m3)

  }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 20, 2010

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 20, 2010

@paulp said:
In the future, please delimit your code with triple braces so I don't have to fix the formatting.

Also: although I did say on the list that it is hard to say this is desirable or expected beahvior, that may not translate to it being a bug, because it is at least predictable if you know how to predict. Whether or not we do much about this one, the more general issue of controlling the impact of expected return type bears more examination.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 20, 2010

@oxbowlakes said:
Apologies - I wasn't aware of the formatting options. I should have read the guidelines.

As for whether this is a bug, I guess that because of the scala implicits, it is difficult to know that a method on a sub-trait does not actually override a method on a super-trait, but instead overload it. I would say that such overloading in the standard library should be kept to an absolute minimum, or removed altogether.

I lost half a day yesterday debugging this problem and I suspect that it lies in wait for many others!

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 20, 2010

@paulp said:
Replying to [comment:2 oxbow_lakes]:

it is difficult to know that a method on a sub-trait does not actually override a method on a super-trait, but instead overload it. I would say that such overloading in the standard library should be kept to an absolute minimum, or removed altogether.

You could not be more right about that. The example which springs to mind is MapLikeBase, which until I removed it relatively recently, overloaded '+' across subtypes of Map, causing a massive behavioral change if the static type of the argument changed.

See: https://lampsvn.epfl.ch/trac/scala/browser/scala/trunk/src/library/scala/collection/mutable/MapLikeBase.scala?rev=21218

Unfortunately in addition to it not being easy to know as a user of the libraries whether a method overloads across subtyping, it's not easy to know as an AUTHOR of the libraries whether it does so. I mean there's nothing which says "hey man, that method has the same name as a method in the supertype and an only slightly different signature." So the great advantage of having "override" language-enforced slips away. Sounds like a warning-to-be for my ever-expanding list of things which ought to be in a linty tool.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 28, 2010

@dubochet said:
Paul, can you please follow up on this issue, and close the ticket if you think the discussion is exhausted, or forward it to scala_meeting if you need additional input from Martin or other people.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 27, 2012

@axel22 said:
The only thing I can think of here is to override the map-related ++ overload in collection.SortedMap once again.

Other than that, the return type of + and ++ in MapLike should be This, to avoid the same problem for all the other map-likes.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.