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

Iterable as List and Iterable.asList() have different semantics

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.5
    • Fix Version/s: 2.4.12
    • Component/s: groovy-jdk
    • Labels:
      None

      Description

      For an Iterable `foo` which is not also a collection, `foo.asList()` and `foo as List` are not equivalent. The latter goes through the asType(Object, Class) path and ultimately returns a proxy. This is unexpected, and can result in some inconsistencies. A simple example:

      class IterableWrapper implements Iterable {
          Iterable delegate
      
          Iterator iterator() {
              delegate.iterator()
          }
      }
      
      def itw = new IterableWrapper(delegate: [1,2,3])
      def itwAsList = itw.asList()
      def itwAsTypeList = itw as List
      assert itwAsList == itwAsTypeList
      assert itwAsList[0] == itwAsTypeList[0]
      

      The first assertion passes, but the second fails with:

      groovy.lang.MissingMethodException: No signature of method: IterableWrapper.get() is applicable for argument types: (java.lang.Integer) values: [0]
      

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user armsargis opened a pull request:

          https://github.com/apache/groovy/pull/546

          GROOVY-7654: Iterable as List and Iterable.asList() have different semantics

          Introduce DefaultGroovyMethods.asType(Iterable, Class) to handle correctly 'asType' to iterables

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/armsargis/groovy GROOVY-7654

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/groovy/pull/546.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #546


          commit faabd7bd65ef9437add8ce451f84a09066e0b448
          Author: Sargis Harutyunyan <sargis.harutyunyan@webbfontaine.com>
          Date: 2017-05-21T19:35:16Z

          GROOVY-7654: Introduce DefaultGroovyMethods.asType(Iterable, Class) to
          handle correctly 'asType' to iterables


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user armsargis opened a pull request: https://github.com/apache/groovy/pull/546 GROOVY-7654 : Iterable as List and Iterable.asList() have different semantics Introduce DefaultGroovyMethods.asType(Iterable, Class) to handle correctly 'asType' to iterables You can merge this pull request into a Git repository by running: $ git pull https://github.com/armsargis/groovy GROOVY-7654 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/groovy/pull/546.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #546 commit faabd7bd65ef9437add8ce451f84a09066e0b448 Author: Sargis Harutyunyan <sargis.harutyunyan@webbfontaine.com> Date: 2017-05-21T19:35:16Z GROOVY-7654 : Introduce DefaultGroovyMethods.asType(Iterable, Class) to handle correctly 'asType' to iterables
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/546#discussion_r117640763

          — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy —
          @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase

          { assertEquals(3, list.get(2)); }

          + // GROOVY-7654
          + public void testIterableAsList()

          { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + }

          +
          private static class MyList extends ArrayList {
          public MyList() {}
          }
          +
          + private static class IterableWrapper implements Iterable {
          + Iterable delegate
          +
          + Iterator iterator()

          { + delegate.iterator() + }

          + }
          }
          — End diff –

          I am missing a test

          def iterableAsIterable = iterable as Iterable
          assert iterable.AsIterable.is(iterable)
          def iterableAsIterableWrapper = iterable as IterableWrapper
          assert iterable.AsIterable.is(iterable)

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/546#discussion_r117640763 — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy — @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase { assertEquals(3, list.get(2)); } + // GROOVY-7654 + public void testIterableAsList() { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + } + private static class MyList extends ArrayList { public MyList() {} } + + private static class IterableWrapper implements Iterable { + Iterable delegate + + Iterator iterator() { + delegate.iterator() + } + } } — End diff – I am missing a test def iterableAsIterable = iterable as Iterable assert iterable.AsIterable.is(iterable) def iterableAsIterableWrapper = iterable as IterableWrapper assert iterable.AsIterable.is(iterable)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user armsargis commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/546#discussion_r117642093

          — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy —
          @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase

          { assertEquals(3, list.get(2)); }

          + // GROOVY-7654
          + public void testIterableAsList()

          { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + }

          +
          private static class MyList extends ArrayList {
          public MyList() {}
          }
          +
          + private static class IterableWrapper implements Iterable {
          + Iterable delegate
          +
          + Iterator iterator()

          { + delegate.iterator() + }

          + }
          }
          — End diff –

          Actually, test I got from jira issue and meaning I think is to create concrete implementation of Iterable which is not extending from collection. And for concrete implementation we need return some iterator.

          Also I thinking to change code like:

          ```
          public static <T> T asType(Iterable iterable, Class<T> clazz) {
          if (Collections.class.isAssignableFrom(clazz))

          { return asType((Collection) toList(iterable), clazz); }

          return asType((Object) toList(iterable), clazz);
          }
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user armsargis commented on a diff in the pull request: https://github.com/apache/groovy/pull/546#discussion_r117642093 — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy — @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase { assertEquals(3, list.get(2)); } + // GROOVY-7654 + public void testIterableAsList() { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + } + private static class MyList extends ArrayList { public MyList() {} } + + private static class IterableWrapper implements Iterable { + Iterable delegate + + Iterator iterator() { + delegate.iterator() + } + } } — End diff – Actually, test I got from jira issue and meaning I think is to create concrete implementation of Iterable which is not extending from collection. And for concrete implementation we need return some iterator. Also I thinking to change code like: ``` public static <T> T asType(Iterable iterable, Class<T> clazz) { if (Collections.class.isAssignableFrom(clazz)) { return asType((Collection) toList(iterable), clazz); } return asType((Object) toList(iterable), clazz); } ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user armsargis commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/546#discussion_r117642241

          — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy —
          @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase

          { assertEquals(3, list.get(2)); }

          + // GROOVY-7654
          + public void testIterableAsList()

          { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + }

          +
          private static class MyList extends ArrayList {
          public MyList() {}
          }
          +
          + private static class IterableWrapper implements Iterable {
          + Iterable delegate
          +
          + Iterator iterator()

          { + delegate.iterator() + }

          + }
          }
          — End diff –

          I will add missing test cases

          Show
          githubbot ASF GitHub Bot added a comment - Github user armsargis commented on a diff in the pull request: https://github.com/apache/groovy/pull/546#discussion_r117642241 — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy — @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase { assertEquals(3, list.get(2)); } + // GROOVY-7654 + public void testIterableAsList() { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + } + private static class MyList extends ArrayList { public MyList() {} } + + private static class IterableWrapper implements Iterable { + Iterable delegate + + Iterator iterator() { + delegate.iterator() + } + } } — End diff – I will add missing test cases
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/546#discussion_r117645390

          — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy —
          @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase

          { assertEquals(3, list.get(2)); }

          + // GROOVY-7654
          + public void testIterableAsList()

          { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + }

          +
          private static class MyList extends ArrayList {
          public MyList() {}
          }
          +
          + private static class IterableWrapper implements Iterable {
          + Iterable delegate
          +
          + Iterator iterator()

          { + delegate.iterator() + }

          + }
          }
          — End diff –

          If the iterable is an instance of Collection, then why isn´t the Collection taking method called directly?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/546#discussion_r117645390 — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy — @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase { assertEquals(3, list.get(2)); } + // GROOVY-7654 + public void testIterableAsList() { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + } + private static class MyList extends ArrayList { public MyList() {} } + + private static class IterableWrapper implements Iterable { + Iterable delegate + + Iterator iterator() { + delegate.iterator() + } + } } — End diff – If the iterable is an instance of Collection, then why isn´t the Collection taking method called directly?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/546#discussion_r117645462

          — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy —
          @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase

          { assertEquals(3, list.get(2)); }

          + // GROOVY-7654
          + public void testIterableAsList()

          { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + }

          +
          private static class MyList extends ArrayList {
          public MyList() {}
          }
          +
          + private static class IterableWrapper implements Iterable {
          + Iterable delegate
          +
          + Iterator iterator()

          { + delegate.iterator() + }

          + }
          }
          — End diff –

          interesting choice to use Collection.class.isAssignableFrom(clazz). But sure, why not... +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/546#discussion_r117645462 — Diff: src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy — @@ -236,7 +236,28 @@ public class DefaultGroovyMethodsTest extends GroovyTestCase { assertEquals(3, list.get(2)); } + // GROOVY-7654 + public void testIterableAsList() { + def list = [1, 2, 3] + def iterable = new IterableWrapper(delegate: list) + + def iterableAsList = iterable.asList() + def iterableAsType = iterable as List + + assertEquals(iterableAsList, iterableAsType) + assertEquals(1, iterableAsList[0]) + assertEquals(1, iterableAsType[0]) + } + private static class MyList extends ArrayList { public MyList() {} } + + private static class IterableWrapper implements Iterable { + Iterable delegate + + Iterator iterator() { + delegate.iterator() + } + } } — End diff – interesting choice to use Collection.class.isAssignableFrom(clazz). But sure, why not... +1
          Hide
          paulk Paul King added a comment - - edited

          Given what was produced was at least partially unusable, I think it is safe to assume this is a straight out bug and not some semi-useful behavior that folks might be relying upon - so I've merged back to 2_4_X.

          Show
          paulk Paul King added a comment - - edited Given what was produced was at least partially unusable, I think it is safe to assume this is a straight out bug and not some semi-useful behavior that folks might be relying upon - so I've merged back to 2_4_X.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/groovy/pull/546

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/groovy/pull/546

            People

            • Assignee:
              paulk Paul King
              Reporter:
              shils Shil Sinha
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development