Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-5865

getAt(EmptyRange) not called when passing an EmptyRange to getAt(Collection)

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 2.0.6
    • 2.1.0-rc-2, 2.1.0
    • None
    • None

    Description

      Basically, this is an inconsistency of List#getAt(Collection). When its parameters are integer indexes and non-empty ranges, it works as expected. But when an EmptyRange is passed to it, it doesn't behave like List#getAt(EmptyRange):

      def list = [1, 2, 3]
      assert list[3..<3] == []        // This works
      assert list[0, 2..<3] == [1, 3] // This works too
      assert list[0, 3..<3] == [1]    // But this throws IndexOutOfBoundsException: toIndex = 4
      

      Attachments

        1. emptyRangeOnGetAt.groovy
          0.2 kB
          Demian Ferreiro

        Activity

          michal.mally Michał Mally added a comment -

          I've pushed pull request that fixes the issue: https://github.com/groovy/groovy-core/pull/107

          The root cause is the existence of getAt(List, Range) and getAt(List, EmptyRange) methods. The second one has been implemented because empty ranges require some special handling. However there is no guarantee that for EmptyRange the getAt(List, EmptyRange) is always called instead of more general getAt(List, Range).

          And this is exactly what happens in case of getAt(List, Collection) method.

              public static <T> List<T> getAt(List<T> self, Collection indices) {
                  List<T> answer = new ArrayList<T>(indices.size());
                  for (Object value : indices) {
                      if (value instanceof Range) {
                          answer.addAll(getAt(self, (Range) value));
                      } else if (value instanceof List) {
                          answer.addAll(getAt(self, (List) value));
                      } else {
                          int idx = DefaultTypeTransformation.intUnbox(value);
                          answer.add(getAt(self, idx));
                      }
                  }
                  return answer;
              }
          

          Each element of collection is tested for being a Range and if it is then the element is explicitly casted to Range and getAt(List, Range) method is called. Because EmptyRange is also a Range more general method is called which causes EmptyRange special handling (provided by getAt(List, EmptyRange)) not being executed.

          Explicit casting of EmptyRange to Range is nothing improper what brings us to conclusion that the getAt(List, Range) method shall handle all the ranges - also EmptyRanges since they inherits from Range.

          So the fix I pushed adds a check to getAt(List, Range) method if passed range is an instance of EmptyRange. If it is the execution is delegated to specialized method getAt(List, EmptyRange). The test case is also added to ensure proper behavior.

          michal.mally Michał Mally added a comment - I've pushed pull request that fixes the issue: https://github.com/groovy/groovy-core/pull/107 The root cause is the existence of getAt(List, Range) and getAt(List, EmptyRange) methods. The second one has been implemented because empty ranges require some special handling. However there is no guarantee that for EmptyRange the getAt(List, EmptyRange) is always called instead of more general getAt(List, Range). And this is exactly what happens in case of getAt(List, Collection) method. public static <T> List<T> getAt(List<T> self, Collection indices) { List<T> answer = new ArrayList<T>(indices.size()); for ( Object value : indices) { if (value instanceof Range) { answer.addAll(getAt(self, (Range) value)); } else if (value instanceof List) { answer.addAll(getAt(self, (List) value)); } else { int idx = DefaultTypeTransformation.intUnbox(value); answer.add(getAt(self, idx)); } } return answer; } Each element of collection is tested for being a Range and if it is then the element is explicitly casted to Range and getAt(List, Range) method is called. Because EmptyRange is also a Range more general method is called which causes EmptyRange special handling (provided by getAt(List, EmptyRange)) not being executed. Explicit casting of EmptyRange to Range is nothing improper what brings us to conclusion that the getAt(List, Range) method shall handle all the ranges - also EmptyRanges since they inherits from Range. So the fix I pushed adds a check to getAt(List, Range) method if passed range is an instance of EmptyRange. If it is the execution is delegated to specialized method getAt(List, EmptyRange). The test case is also added to ensure proper behavior.
          epidemian Demian Ferreiro added a comment - - edited

          Awesome, Micha

          I remember i traced this bug to that particular function too (getAt(List, Collection)), but i didn't submit any patch because all the other definitions for "getAt" in DefaultGroovyMethods.java scared me. There are different getAt definitions for object arrays, primitive arrays, strings, etc, and i didn't know how/if they interact with each other, and adding a fix for each of those getAt version seemed like a much complicated task.

          Do you know if your pull request works for other "getAt-able" things? String seems to explode on Groovy 2.0.6 ("hello"[0, 5..<5] throws a StringIndexOutOfBoundsException), while primitive arrays seem to work (([1, 2, 3] as int[])[1, 3..<3] == [2]).

          In case it doesn't, i think it's still a good pull request, as it fixes this particular bug, and gives a hint on how to fix the getAt implementations for other objects in case those are broken too

          epidemian Demian Ferreiro added a comment - - edited Awesome, Micha I remember i traced this bug to that particular function too (getAt(List, Collection)), but i didn't submit any patch because all the other definitions for "getAt" in DefaultGroovyMethods.java scared me. There are different getAt definitions for object arrays, primitive arrays, strings, etc, and i didn't know how/if they interact with each other, and adding a fix for each of those getAt version seemed like a much complicated task. Do you know if your pull request works for other "getAt-able" things? String seems to explode on Groovy 2.0.6 ( "hello"[0, 5..<5] throws a StringIndexOutOfBoundsException), while primitive arrays seem to work ( ([1, 2, 3] as int[])[1, 3..<3] == [2] ). In case it doesn't, i think it's still a good pull request, as it fixes this particular bug, and gives a hint on how to fix the getAt implementations for other objects in case those are broken too
          paulk Paul King added a comment - - edited

          Thanks! Applied and tweaked - altered the calling code in getAt(Collection) rather than the called code in getAt(Range)

          paulk Paul King added a comment - - edited Thanks! Applied and tweaked - altered the calling code in getAt(Collection) rather than the called code in getAt(Range)

          People

            paulk Paul King
            epidemian Demian Ferreiro
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: