groovy
  1. groovy
  2. GROOVY-4742

Documentation for Collection.sort(Comparator comparator) is wrong

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.1
    • Component/s: groovy-jdk
    • Labels:
      None
    • Number of attachments :
      3

      Description

      The documentation for Collection.sort(Comparator comparator) at http://groovy.codehaus.org/groovy-jdk/java/util/Collection.html#sort%28java.util.Comparator%29 states

      Sorts the Collection using the given comparator. The elements are sorted into a new list, and the existing collection is unchanged.

      However, it does seem to mutate the original list

        Activity

        Hide
        Donal Murtagh added a comment - - edited

        Here's a test case

        class ISO3LangComparator implements Comparator<Locale> {
        
            int compare(Locale locale1, Locale locale2) {
                locale1.ISO3Language <=> locale2.ISO3Language
            }
        }
        
        List<Locale> locales = [Locale.FRENCH, Locale.ENGLISH]
        def sortedLocales = locales.sort(new ISO3LangComparator())
        
        // This assertion fails
        assert locales[0] == frenchLocale
        
        Show
        Donal Murtagh added a comment - - edited Here's a test case class ISO3LangComparator implements Comparator<Locale> { int compare(Locale locale1, Locale locale2) { locale1.ISO3Language <=> locale2.ISO3Language } } List<Locale> locales = [Locale.FRENCH, Locale.ENGLISH] def sortedLocales = locales.sort( new ISO3LangComparator()) // This assertion fails assert locales[0] == frenchLocale
        Hide
        Paul King added a comment -

        Potential patch attached. Fixes Javadoc but also proposes a few new overloaded methods which allow a boolean to be provided to turn off any mutating.

        Show
        Paul King added a comment - Potential patch attached. Fixes Javadoc but also proposes a few new overloaded methods which allow a boolean to be provided to turn off any mutating.
        Hide
        Paul King added a comment -

        Same additions could be made for the Array versions of the methods. There is also the possibility of using a Map instead of the boolean to allow sort(mutate: true) etc.

        Show
        Paul King added a comment - Same additions could be made for the Array versions of the methods. There is also the possibility of using a Map instead of the boolean to allow sort(mutate: true) etc.
        Hide
        Tim Yates added a comment -

        Trying to come up with a patch for unique as well

        Show
        Tim Yates added a comment - Trying to come up with a patch for unique as well
        Hide
        Tim Yates added a comment -

        Attached a patch which hopefully should add the mutate flag to Collection.unique as well

        Included test cases which should (I think) cover the new code.

        Iterator.unique does not require the flag, as it wraps the Iterator wth asList before the unique call is made, so the original Collection should remain untouched.

        Hope it's ok!

        Show
        Tim Yates added a comment - Attached a patch which hopefully should add the mutate flag to Collection.unique as well Included test cases which should (I think) cover the new code. Iterator.unique does not require the flag, as it wraps the Iterator wth asList before the unique call is made, so the original Collection should remain untouched. Hope it's ok!
        Hide
        Tim Yates added a comment -

        Sorry, I had unique(Collection, Closure, boolean) rather than unique(Collection, boolean, Closure)

        Doing it boolean first means you can have:

        def list = [ 1, 2, 3, 2, 1 ]
        list.unique( false ) { it % 2 }
        

        The other way round is clunky and non-ideomatic

        Show
        Tim Yates added a comment - Sorry, I had unique(Collection, Closure, boolean) rather than unique(Collection, boolean, Closure) Doing it boolean first means you can have: def list = [ 1, 2, 3, 2, 1 ] list.unique( false ) { it % 2 } The other way round is clunky and non-ideomatic
        Hide
        Paul King added a comment - - edited

        Extended sort patch to cover arrays plus merged in unique patch from Tim. Only considered the boolean variant as I believe the map variant could be autogenerated at a later point.

        Now we can consider whether we should extend this approach for:

        • addAll, removeAll, retainAll (which are modify semantics by default)
          • for these I think we can refer to their copy semantics counterparts in the javadoc and vice versa (i.e. plus and findAll/grep)
          • in addition we will need a plus variant that takes an index
        • reverse (which is copy semantics by default in DGM but modify in Collections)
          • another boolean would allow both semantics in this case
        Show
        Paul King added a comment - - edited Extended sort patch to cover arrays plus merged in unique patch from Tim. Only considered the boolean variant as I believe the map variant could be autogenerated at a later point. Now we can consider whether we should extend this approach for: addAll, removeAll, retainAll (which are modify semantics by default) for these I think we can refer to their copy semantics counterparts in the javadoc and vice versa (i.e. plus and findAll/grep) in addition we will need a plus variant that takes an index reverse (which is copy semantics by default in DGM but modify in Collections) another boolean would allow both semantics in this case
        Hide
        Paul King added a comment - - edited

        Also worth pointing out that for legacy reasons, we have the following slightly annoying discrepancy (I believe created for ease of implementation rather than a specific requirement):

        • sort(T[] self, Closure closure) has copy semantics
        • all other sort variants (without the boolean mutate flag) have modify semantics

        The boolean variants allow this discrepancy to be worked around but I am wondering whether this should be considered a bug and all variations should be aligned.

        Show
        Paul King added a comment - - edited Also worth pointing out that for legacy reasons, we have the following slightly annoying discrepancy (I believe created for ease of implementation rather than a specific requirement): sort(T[] self, Closure closure) has copy semantics all other sort variants (without the boolean mutate flag) have modify semantics The boolean variants allow this discrepancy to be worked around but I am wondering whether this should be considered a bug and all variations should be aligned.
        Hide
        Paul King added a comment - - edited

        There is also another consideration which arises that is related to earlier points. For methods which imply ordering and have modify semantics (such as sort), we currently perform them in place for lists (which by their nature are also ordered) but convert to a new list (indirectly giving copy semantics) for non-list collections, e.g. Set. However there are non-list collection types which are ordered, e.g. LinkedHashSet. Perhaps we should retain the modify semantics for such types. I.e. instead of using asList we should make a new asOrderedCollection method which catered for things like LinkedHashSet and possibly Queue.

        Show
        Paul King added a comment - - edited There is also another consideration which arises that is related to earlier points. For methods which imply ordering and have modify semantics (such as sort ), we currently perform them in place for lists (which by their nature are also ordered) but convert to a new list (indirectly giving copy semantics) for non-list collections, e.g. Set . However there are non-list collection types which are ordered, e.g. LinkedHashSet . Perhaps we should retain the modify semantics for such types. I.e. instead of using asList we should make a new asOrderedCollection method which catered for things like LinkedHashSet and possibly Queue .
        Hide
        Paul King added a comment -

        A version of above patch has been applied in trunk for review. It doesn't attempt to fix the sort discrepancy or cater for LinkedHashSet etc.

        Show
        Paul King added a comment - A version of above patch has been applied in trunk for review. It doesn't attempt to fix the sort discrepancy or cater for LinkedHashSet etc.
        Hide
        Paul King added a comment -

        The javadoc parts from trunk (and two non-contentious methods) have been merged into the 1.8 branches.

        The variants with the mutate boolean (covering sort and unique) are still pending finalization and potential merging for 1.8.1.

        Show
        Paul King added a comment - The javadoc parts from trunk (and two non-contentious methods) have been merged into the 1.8 branches. The variants with the mutate boolean (covering sort and unique) are still pending finalization and potential merging for 1.8.1.
        Hide
        Pascal Schumacher added a comment - - edited

        Any updates on this?

        Show
        Pascal Schumacher added a comment - - edited Any updates on this?
        Hide
        Pascal Schumacher added a comment -

        I took a look at the doc and it seems like the unique and sort methods with boolean mutate were added with 1.8.1. Therefore I'm closing this issue.

        Show
        Pascal Schumacher added a comment - I took a look at the doc and it seems like the unique and sort methods with boolean mutate were added with 1.8.1. Therefore I'm closing this issue.

          People

          • Assignee:
            Paul King
            Reporter:
            Tim Yates
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: