Scala Programming Language
  1. Scala Programming Language
  2. SI-3326

Unexpected behavioural difference between collection.SortedMap and immutable.SortedMap

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Scala 2.10.0-M4
    • Component/s: Collections
    • Labels:
      None
    • Environment:

      collections implicits

      Description

      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)
      
        }
      }
      

        Activity

        Hide
        Paul Phillips added a comment -

        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.

        Show
        Paul Phillips added a comment - 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.
        Hide
        Chris Marshall added a comment -

        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!

        Show
        Chris Marshall added a comment - 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!
        Hide
        Paul Phillips added a comment -

        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.

        Show
        Paul Phillips added a comment - 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.
        Hide
        Gilles Dubochet added a comment -

        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.

        Show
        Gilles Dubochet added a comment - 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.
        Hide
        Aleksandar Prokopec added a comment -

        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.

        Show
        Aleksandar Prokopec added a comment - 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.

          People

          • Assignee:
            Aleksandar Prokopec
            Reporter:
            Chris Marshall
            TracCC:
            Aleksandar Prokopec, Alex Cruise, Ismael Juma, Johannes Rudolph, Paul Phillips, Seth Tisue
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development