Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8-rc-2, 1.9-beta-1
    • Component/s: None
    • Labels:
      None

      Activity

      Hide
      Guillaume Delcroix added a comment -

      Good counter example from Cédric Champeau on the mailing list:

      In Groovy (and generally in a dynamically typed language), as far as I know, there's no reason why the return value should be the same type of the injected value. Here's a simple example :

      (1..10).inject(1)

      { x,y -> x/y }

      1 is an int, but the result of the injection will be a BigDecimal.

      Show
      Guillaume Delcroix added a comment - Good counter example from Cédric Champeau on the mailing list: In Groovy (and generally in a dynamically typed language), as far as I know, there's no reason why the return value should be the same type of the injected value. Here's a simple example : (1..10).inject(1) { x,y -> x/y } 1 is an int, but the result of the injection will be a BigDecimal.
      Hide
      Peter Niederwieser added a comment -

      The signature that I gave covers this.

      Show
      Peter Niederwieser added a comment - The signature that I gave covers this.
      Hide
      Paul King added a comment - - edited

      Yes, T would be Number in this case but more generally, could be Object.

      Show
      Paul King added a comment - - edited Yes, T would be Number in this case but more generally, could be Object .
      Hide
      Guillaume Delcroix added a comment -

      In this example, yes, it would work, but we could also imagine an inject() that takes a String as initial value, and returns a Number. Or even a GString and returning a String.
      So this is a restriction to some currently possible use cases with inject().

      Show
      Guillaume Delcroix added a comment - In this example, yes, it would work, but we could also imagine an inject() that takes a String as initial value, and returns a Number. Or even a GString and returning a String. So this is a restriction to some currently possible use cases with inject().
      Hide
      Paul King added a comment -

      committed to 1_8_X so far

      Show
      Paul King added a comment - committed to 1_8_X so far
      Hide
      Paul King added a comment -

      Not applied to the 1_7_X branch as originally planned as it doesn't have the prerequisite Closure generics changes.

      Show
      Paul King added a comment - Not applied to the 1_7_X branch as originally planned as it doesn't have the prerequisite Closure generics changes.
      Hide
      Peter Niederwieser added a comment -

      Reopening since Guillaume reverted the commit.

      Show
      Peter Niederwieser added a comment - Reopening since Guillaume reverted the commit.
      Hide
      Peter Niederwieser added a comment -

      Here is a patch for trunk, very similar but not identical to Paul's (now reverted) commit. Solves all problems that I'm aware of.

      Show
      Peter Niederwieser added a comment - Here is a patch for trunk, very similar but not identical to Paul's (now reverted) commit. Solves all problems that I'm aware of.
      Hide
      Peter Niederwieser added a comment -

      With my patch, adding the evil "foo" method posted by Guillaume on the mailing list...

         public static Integer foo(Integer self) {
              Collection c = new ArrayList();
              Number x = inject(c, (Integer) 3, new Closure<BigInteger>(c) {
                  void doCall() {
                  }
              });
              return self;
          }
      

      ...still produces a compile error, but this error can now be avoided by casting to Number instead of Integer. I don't know why the compiler needs this assistance, but I think the patch is nevertheless safe and should go in.

      Show
      Peter Niederwieser added a comment - With my patch, adding the evil "foo" method posted by Guillaume on the mailing list... public static Integer foo( Integer self) { Collection c = new ArrayList(); Number x = inject(c, ( Integer ) 3, new Closure<BigInteger>(c) { void doCall() { } }); return self; } ...still produces a compile error, but this error can now be avoided by casting to Number instead of Integer. I don't know why the compiler needs this assistance, but I think the patch is nevertheless safe and should go in.
      Hide
      Paul King added a comment -

      Peter, as per your email I originally tried something very close to your patch. The only difference was that I removed the two casts to (T) as Intellij was saying they were redundant but then it didn't compile. Hence my earlier variation. But with the two casts, it compiles fine and seems to be the correct way to go. I'll apply the patch.

      Show
      Paul King added a comment - Peter, as per your email I originally tried something very close to your patch. The only difference was that I removed the two casts to (T) as Intellij was saying they were redundant but then it didn't compile. Hence my earlier variation. But with the two casts, it compiles fine and seems to be the correct way to go. I'll apply the patch.
      Hide
      Peter Niederwieser added a comment -

      The other difference is in the signature: Closure<V> vs. Closure<? extends T>. For some reason I don't understand, only with the former signature can the evil foo method be made to compile by casting to Number instead of Integer.

      I've seen quite a few cases where IDEA is wrong about generics. Eclipse does a much better job in this regard.

      Show
      Peter Niederwieser added a comment - The other difference is in the signature: Closure<V> vs. Closure<? extends T>. For some reason I don't understand, only with the former signature can the evil foo method be made to compile by casting to Number instead of Integer. I've seen quite a few cases where IDEA is wrong about generics. Eclipse does a much better job in this regard.
      Hide
      Paul King added a comment -

      I think the lesson here is that Java generics leaves much to be desired (but we all knew that). The added Java integration test looks like this:

      Collection<Integer> c = Arrays.asList(2, 4, 5, 20);
      Number initial = BigDecimal.ZERO;
      Closure<? extends Number> closure = new Closure<BigDecimal>(c) {
          BigDecimal doCall(BigDecimal total, Integer next) {
              return total.add(BigDecimal.ONE.divide(new BigDecimal(next)));
          }
      };
      assertTrue(DefaultTypeTransformation.compareEqual(BigDecimal.ONE, inject(c, initial, closure)));
      

      Several bits of this you would think are possible to simplify but currently you can't or the Java compiler gets upset. Nonetheless, this example illustrates the recommended way of using the current API.

      Show
      Paul King added a comment - I think the lesson here is that Java generics leaves much to be desired (but we all knew that). The added Java integration test looks like this: Collection< Integer > c = Arrays.asList(2, 4, 5, 20); Number initial = BigDecimal.ZERO; Closure<? extends Number > closure = new Closure<BigDecimal>(c) { BigDecimal doCall(BigDecimal total, Integer next) { return total.add(BigDecimal.ONE.divide( new BigDecimal(next))); } }; assertTrue(DefaultTypeTransformation.compareEqual(BigDecimal.ONE, inject(c, initial, closure))); Several bits of this you would think are possible to simplify but currently you can't or the Java compiler gets upset. Nonetheless, this example illustrates the recommended way of using the current API.

        People

        • Assignee:
          Paul King
          Reporter:
          Paul King
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development