[groovy-dev] Improving type information in the GDK methods

[candrews]

I reported 2 issues about improving type information in the Groovy GDK
methods:
http://jira.codehaus.org/browse/GROOVY-7281 IOGroovyMethods.withStream
should use the stream type as the ClosureParam, not simply InputStream
http://jira.codehaus.org/browse/GROOVY-7283 DefaultGroovyMethods methods
should include type information

The first, GROOVY-7281, seems really easy and not problematic. I've
submitted a pull request at https://github.com/groovy/groovy-core/pull/599 -
could someone please review it and perhaps merge it? This seems like it
should be in Groovy 2.4.x.

The second, GROOVY-7283, is the same idea but on a lot more methods (just
adding @ClosureParams and closure return type generics). I've also submitted
a PR https://github.com/groovy/groovy-core/pull/610 on that ticket that
improves type information in many places. I'm hoping that PR can be reviewed
- I'm happy to split it up or make any other changes. Assuming it's
backwards compatible, I think it should be in 2.4.x - but if there's concern
that it's not, perhaps parts of it could be in 2.4.x and parts in a future
version.

I think these kinds of improvements are great - they make the work already
done on @CompileStatic/@TypeChecked much more useful.

Thanks,
~Craig



--
View this message in context: http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710.html
Sent from the groovy - dev mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Dinko Srkoč]

I see from the PR for GROOVY-7823 that you propose to change:

public static Object sum(Iterable self, Closure closure)

to

public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T> closure)

That appears not to be consistent with the current semantics of `sum`, e.g.:

assert (1..4).sum { it.toString() } == '12345'

Namely, the return type of Closure and hence the `sum` method may not
necessarily be the same as the type the Iterable contains.

I agree with you that the more type information there is the better.
It's not just for @CompileStatic/@TypeChecked, the IDEs can then do a
better job too.

Cheers,
Dinko

Post by candrews
I reported 2 issues about improving type information in the Groovy GDK
http://jira.codehaus.org/browse/GROOVY-7281 IOGroovyMethods.withStream
should use the stream type as the ClosureParam, not simply InputStream
http://jira.codehaus.org/browse/GROOVY-7283 DefaultGroovyMethods methods
should include type information
The first, GROOVY-7281, seems really easy and not problematic. I've
submitted a pull request at https://github.com/groovy/groovy-core/pull/599 -
could someone please review it and perhaps merge it? This seems like it
should be in Groovy 2.4.x.
The second, GROOVY-7283, is the same idea but on a lot more methods (just
a PR https://github.com/groovy/groovy-core/pull/610 on that ticket that
improves type information in many places. I'm hoping that PR can be reviewed
- I'm happy to split it up or make any other changes. Assuming it's
backwards compatible, I think it should be in 2.4.x - but if there's concern
that it's not, perhaps parts of it could be in 2.4.x and parts in a future
version.
I think these kinds of improvements are great - they make the work already
Thanks,
~Craig
--
View this message in context: http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710.html
Sent from the groovy - dev mailing list archive at Nabble.com.
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Dinko Srkoč]

Sorry, I meant GROOVY-7283, not GROOVY-7823.

- Dinko

Post by Dinko Srkoč
public static Object sum(Iterable self, Closure closure)
to
public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T> closure)
assert (1..4).sum { it.toString() } == '12345'
Namely, the return type of Closure and hence the `sum` method may not
necessarily be the same as the type the Iterable contains.
I agree with you that the more type information there is the better.
better job too.
Cheers,
Dinko

Post by candrews
I reported 2 issues about improving type information in the Groovy GDK
http://jira.codehaus.org/browse/GROOVY-7281 IOGroovyMethods.withStream
should use the stream type as the ClosureParam, not simply InputStream
http://jira.codehaus.org/browse/GROOVY-7283 DefaultGroovyMethods methods
should include type information
The first, GROOVY-7281, seems really easy and not problematic. I've
submitted a pull request at https://github.com/groovy/groovy-core/pull/599 -
could someone please review it and perhaps merge it? This seems like it
should be in Groovy 2.4.x.
The second, GROOVY-7283, is the same idea but on a lot more methods (just
a PR https://github.com/groovy/groovy-core/pull/610 on that ticket that
improves type information in many places. I'm hoping that PR can be reviewed
- I'm happy to split it up or make any other changes. Assuming it's
backwards compatible, I think it should be in 2.4.x - but if there's concern
that it's not, perhaps parts of it could be in 2.4.x and parts in a future
version.
I think these kinds of improvements are great - they make the work already
Thanks,
~Craig
--
View this message in context: http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710.html
Sent from the groovy - dev mailing list archive at Nabble.com.
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[candrews]

So the return value of sum should be the return value of the closure - that
makes sense! My apologies for missing that and thanks for noticing.

For what was originally:
public static Object sum(Iterable self, Closure closure)
Instead of your suggestion, which is:
public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T> closure)
How about:
public static <T,U> U sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<U> closure)
?

I've updated the PR with this change, too:
https://github.com/groovy/groovy-core/pull/610



--
View this message in context: http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722721.html
Sent from the groovy - dev mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Cédric Champeau]

Hi Craig and thanks for the PR.

Unfortunately I don't think it is possible to express the return type of
sum just using generics. Take this (contrived) example:

class A {
String name
}
class B {
String id
A plus(B other) { new A(name:"${id}${other.id}") }
}

[new A(name:'a'), new A(name:'b')].sum { new B(id:it.name) }

With your new signature, "sum" would return an instance of B, while in my
example it returns an instance of A. I am not saying this example makes a
lot of sense, there are probably better examples, but it explains that if
you change the signature to what you suggest, it's a breaking change, even
if in terms of binary compatibility everything is ok...

Post by candrews
So the return value of sum should be the return value of the closure - that
makes sense! My apologies for missing that and thanks for noticing.
public static Object sum(Iterable self, Closure closure)
public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T> closure)
public static <T,U> U sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<U> closure)
?
https://github.com/groovy/groovy-core/pull/610
--
http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722721.html
Sent from the groovy - dev mailing list archive at Nabble.com.
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

[Dinko Srkoč]

I was about to say that it looks OK to me, but Cédric's insidious ;-)
example shows that putting the generics there as proposed might not be
a good idea after all.

Cheers,
Dinko

Post by Cédric Champeau
Hi Craig and thanks for the PR.
Unfortunately I don't think it is possible to express the return type of sum
class A {
String name
}
class B {
String id
A plus(B other) { new A(name:"${id}${other.id}") }
}
[new A(name:'a'), new A(name:'b')].sum { new B(id:it.name) }
With your new signature, "sum" would return an instance of B, while in my
example it returns an instance of A. I am not saying this example makes a
lot of sense, there are probably better examples, but it explains that if
you change the signature to what you suggest, it's a breaking change, even
if in terms of binary compatibility everything is ok...

Post by candrews
So the return value of sum should be the return value of the closure - that
makes sense! My apologies for missing that and thanks for noticing.
public static Object sum(Iterable self, Closure closure)
public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T> closure)
public static <T,U> U sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<U> closure)
?
https://github.com/groovy/groovy-core/pull/610
--
http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722721.html
Sent from the groovy - dev mailing list archive at Nabble.com.
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[candrews]

Wow, that's very interesting (and quite insidious)!

This information also means that the closure parameter can't be
specified by a generic either - all of the parameters and return types
of the "sum" methods must be untyped.

With that said, I just pushed to the PR removing all changes to the
"sum" methods. I'm kind of sad about that - the lack of type information
for "sum" was what I originally encountered causing me to start work in
this area. But I learned something, so I have that going for me.

Thanks again,
~Craig

I was about to say that it looks OK to me, but Cédric's insidious ;-)
example shows that putting the generics there as proposed might not be
a good idea after all.
Cheers,
Dinko

Post by Cédric Champeau
Hi Craig and thanks for the PR.
Unfortunately I don't think it is possible to express the return

type of sum

Post by Cédric Champeau
class A {
String name
}
class B {
String id
A plus(B other) { new A(name:"${id}${other.id}") }
}
[new A(name:'a'), new A(name:'b')].sum { new B(id:it.name) }
With your new signature, "sum" would return an instance of B, while

in my

Post by Cédric Champeau
example it returns an instance of A. I am not saying this example

makes a

Post by Cédric Champeau
lot of sense, there are probably better examples, but it explains

that if

Post by Cédric Champeau
you change the signature to what you suggest, it's a breaking

change, even

Post by Cédric Champeau
if in terms of binary compatibility everything is ok...

Post by candrews
So the return value of sum should be the return value of the

closure -

Post by Cédric Champeau

Post by candrews
that
makes sense! My apologies for missing that and thanks for

noticing.

Post by Cédric Champeau

Post by candrews
public static Object sum(Iterable self, Closure closure)
public static <T> T sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<T>

closure)

Post by Cédric Champeau

Post by candrews
public static <T,U> U sum(Iterable<T> self,
@ClosureParams(FirstParam.FirstGenericType.class) Closure<U>

closure)

Post by Cédric Champeau

Post by candrews
?
https://github.com/groovy/groovy-core/pull/610
--

http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722721.html

Post by Cédric Champeau

Post by candrews
Sent from the groovy - dev mailing list archive at Nabble.com.

---------------------------------------------------------------------

Post by Cédric Champeau

Post by candrews
http://xircles.codehaus.org/manage_email

---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
______________________________________________________________________
If you reply to this email, your message will be added to the
http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722726.html
To unsubscribe from Improving type information in the GDK methods,
click here.
NAML

--
View this message in context: http://groovy.329449.n5.nabble.com/Improving-type-information-in-the-GDK-methods-tp5722710p5722729.html
Sent from the groovy - dev mailing list archive at Nabble.com.