You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@groovy.apache.org by Cédric Champeau <ce...@gmail.com> on 2018/09/25 06:39:02 UTC
@Immutable backwards compatibility
Hi folks,
Gradle 5 is migrating to Groovy 2.5 (yay!). However, we discovered several
regressions (in @CompileStatic, in covariant return type checking, ...)
that may make the migration painful. One of them is unexpected for our
users: the @Immutable AST transformation changed the runtime checks, so a
class compiled with 2.4 running on 2.5 would suddenly fail. An example of
such a problem has been reported at
https://github.com/ajoberstar/grgit/issues/237
Our partners at Netflix already mentioned they had to fork several plugins
to accommodate the problem. While the new checks are legit, the fact that
it's an AST xform (happening at compile time) and that the additional check
happens at runtime can be surprising.
I'm not sure if we need to change this, but having an incompatibility may
be annoying.
Re: @Immutable backwards compatibility
Posted by "Daniel.Sun" <su...@apache.org>.
Hi Cédric,
Paul told me what had happened. We should try our best to keep binary
compatibility between minor versions.
Cheers,
Daniel.Sun
-----
Daniel Sun
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun
--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Users-f329450.html
Re: @Immutable backwards compatibility
Posted by Cédric Champeau <ce...@gmail.com>.
The "migration" may not be painful but breaking user builds or plugins when
they upgrade Gradle is not cool. Several issues have been filed already as
I understand.
Le sam. 13 oct. 2018 à 17:41, Daniel.Sun <su...@apache.org> a écrit :
> Hi Cédric,
>
> > However, we discovered several regressions (in @CompileStatic, in
> > covariant return type checking, ...) that may make the migration
> painful.
> According to the changed files shown in the PR (
> https://github.com/gradle/gradle/pull/6903/files ), it seems that the
> migration is not that painful ;-)
>
> BTW, all changes for 2.5.x pass all existing tests in Groovy project.
> If you find some breaking change, feel free to submit JIRA ticket to track.
>
> Cheers,
> Daniel.Sun
>
>
>
>
> -----
> Daniel Sun
> Apache Groovy committer
> Blog: http://blog.sunlan.me
> Twitter: @daniel_sun
>
> --
> Sent from: http://groovy.329449.n5.nabble.com/Groovy-Users-f329450.html
>
Re: @Immutable backwards compatibility
Posted by "Daniel.Sun" <su...@apache.org>.
Gotcha :-)
-----
Daniel Sun
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun
--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Users-f329450.html
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
On Sun, Oct 14, 2018 at 1:41 AM Daniel.Sun <su...@apache.org> wrote:
> Hi Cédric,
>
> > However, we discovered several regressions (in @CompileStatic, in
> > covariant return type checking, ...) that may make the migration
> painful.
> According to the changed files shown in the PR (
> https://github.com/gradle/gradle/pull/6903/files ), it seems that the
> migration is not that painful ;-)
>
> BTW, all changes for 2.5.x pass all existing tests in Groovy project.
> If you find some breaking change, feel free to submit JIRA ticket to track.
>
The problem is that none of our tests in 2.5.x include tests against 2.4.x
compiled code.
This is what impacts Gradle, Grails and Micronaut plugins. They might be
compiled
with 2.4.x and then used with 2.5.x. The checkBinaryCompatibility task
helps somewhat
but we could no doubt also add some cross version integration tests as well.
>
> Cheers,
> Daniel.Sun
>
>
>
>
> -----
> Daniel Sun
> Apache Groovy committer
> Blog: http://blog.sunlan.me
> Twitter: @daniel_sun
>
> --
> Sent from: http://groovy.329449.n5.nabble.com/Groovy-Users-f329450.html
>
Re: @Immutable backwards compatibility
Posted by "Daniel.Sun" <su...@apache.org>.
Hi Cédric,
> However, we discovered several regressions (in @CompileStatic, in
> covariant return type checking, ...) that may make the migration painful.
According to the changed files shown in the PR (
https://github.com/gradle/gradle/pull/6903/files ), it seems that the
migration is not that painful ;-)
BTW, all changes for 2.5.x pass all existing tests in Groovy project.
If you find some breaking change, feel free to submit JIRA ticket to track.
Cheers,
Daniel.Sun
-----
Daniel Sun
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun
--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Users-f329450.html
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
I'll move this to the dev list.
On Thu, Sep 27, 2018 at 10:26 PM Jochen Theodorou <bl...@gmx.org> wrote:
>
>
> Am 27.09.2018 um 01:24 schrieb Paul King:
> > The String check for "groovy.transform.Immutable" would work fine if the
> > 2.4 compiled class was actually loaded with it;s annotations but
> > AnnotationCollector changes any meta-annotation to no longer be an
> > annotation but a class to store the serialized information for the
> > pre-compiled case.
>
> Was that already in 2.4 the case for @Immutable? I thought it was not
> expressed by the annotation collector back then.
>
> And why do we do all those checks for if the type is annotated with
> @Immutable then? We also check for if the annotation starts with
> groovy.transform.Immutable, which would cover ImmutableBase as well,
> which imho stays on the class... wait...it is source only.. oh..
>
> > I think we need to perhaps adjust what we do there
> > slightly to leave the original annotation intact. I'll try to work on
> > that on the plane trip home - perhaps we should delay 2.5.3 a few days
> > to see if we can address this but any alternative suggestions welcome.
>
> leaving the original annotation in tact is what I suggested to you
> before even writing here, because somehow I knew this was the problem,
> without even having looked at it properly ;)
>
> Still I'd like to really understand it, and I am not there yet.
>
> bye Jochen
>
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
I have a potential fix for this issue. I'd appreciate if anyone who has had
issues could apply the PR and see if it fixes their problems:
https://github.com/apache/groovy/pull/803
For anyone who has dived into the current implementation, they will know
that currently a collector annotation definition is converted into a helper
class of sorts containing the serialized annotation data so that
pre-compiled meta-annotations can be processed.
Basically the fix allows the original annotation to be left in the codebase
and instead creates an inner helper class with the serialized data and a
reference to the class in an annotation attribute called serializeClass on
AnnotationCollector.
I don't face the issue described in GROOVY-8806 when using Groovy 2.5.2
normally with asmResolving in place, I only see it when using classloader
resolution. The proposed fix works for either and is backwards compatible
with 2.4.x and 2.5.2 and earlier.
My cross version testcase which isn't covered by existing tests is to
create an immutable class in 2.5.3-SNAPSHOT that references a property with
a 2.4.15 compiled @Immutable type and another with a 2.5.2 compiled
@Immutable type and compile it up with asmResolving set to false.
Normally, serializeClass is set automatically for you as part of
AnnotationCollector processing but there is also a fallback to the existing
behavior by explicitly setting the serializeClass to the collector
annotation. So in the following example, Foo will use the new approach and
Bar the current one as per 2.5.2.
@AnnotationCollector
@ToString
@interface Foo { }
@AnnotationCollector(serializeClass=Bar)
@EqualsAndHashCode
@interface Bar { }
Currently, the proposal makes the new approach the default. Any feedback
welcome.
Cheers, Paul.
On Fri, Sep 28, 2018 at 3:14 AM Paul King <pa...@asert.com.au> wrote:
>
> Properties within @Immutable classes need to be immutable. We have a bunch
> of well-known immutable types we check against but we also allow
> other @Immutable classes. In 2.4, we double dip on the @Immutable
> annotation. It is used to trigger the ImmutableASTTransformation but also
> left on the class at runtime as a marker interface. In 2.5, @Immutable
> became a meta-annotation and we stopped double dipping on the @Immutable
> annotation, we now have the dedicated @KnownImmutable annotation that can
> even be used with Java classes etc. All good so far.
>
> The issue is that the annotation collector moves @Immutable sideways to no
> longer be an annotation. All okay for 2.5 but for 2.4 compiled classes,
> that stops the @Immutable annotation from being read in and of course the
> JDK just loads such classes but ditches annotations it doesn't find classes
> for (or in our case classes which aren't valid annotation definitions). So
> we don't need to leave @Immutable there but we should not alter it from
> being an annotation defn. So perhaps a slightly differently named class to
> store the serialization info.
>
> Cheers, Paul.
>
>
>
> On Thu, Sep 27, 2018 at 10:26 PM Jochen Theodorou <bl...@gmx.org>
> wrote:
>
>>
>>
>> Am 27.09.2018 um 01:24 schrieb Paul King:
>> > The String check for "groovy.transform.Immutable" would work fine if
>> the
>> > 2.4 compiled class was actually loaded with it;s annotations but
>> > AnnotationCollector changes any meta-annotation to no longer be an
>> > annotation but a class to store the serialized information for the
>> > pre-compiled case.
>>
>> Was that already in 2.4 the case for @Immutable? I thought it was not
>> expressed by the annotation collector back then.
>>
>> And why do we do all those checks for if the type is annotated with
>> @Immutable then? We also check for if the annotation starts with
>> groovy.transform.Immutable, which would cover ImmutableBase as well,
>> which imho stays on the class... wait...it is source only.. oh..
>>
>> > I think we need to perhaps adjust what we do there
>> > slightly to leave the original annotation intact. I'll try to work on
>> > that on the plane trip home - perhaps we should delay 2.5.3 a few days
>> > to see if we can address this but any alternative suggestions welcome.
>>
>> leaving the original annotation in tact is what I suggested to you
>> before even writing here, because somehow I knew this was the problem,
>> without even having looked at it properly ;)
>>
>> Still I'd like to really understand it, and I am not there yet.
>>
>> bye Jochen
>>
>
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
Properties within @Immutable classes need to be immutable. We have a bunch
of well-known immutable types we check against but we also allow
other @Immutable classes. In 2.4, we double dip on the @Immutable
annotation. It is used to trigger the ImmutableASTTransformation but also
left on the class at runtime as a marker interface. In 2.5, @Immutable
became a meta-annotation and we stopped double dipping on the @Immutable
annotation, we now have the dedicated @KnownImmutable annotation that can
even be used with Java classes etc. All good so far.
The issue is that the annotation collector moves @Immutable sideways to no
longer be an annotation. All okay for 2.5 but for 2.4 compiled classes,
that stops the @Immutable annotation from being read in and of course the
JDK just loads such classes but ditches annotations it doesn't find classes
for (or in our case classes which aren't valid annotation definitions). So
we don't need to leave @Immutable there but we should not alter it from
being an annotation defn. So perhaps a slightly differently named class to
store the serialization info.
Cheers, Paul.
On Thu, Sep 27, 2018 at 10:26 PM Jochen Theodorou <bl...@gmx.org> wrote:
>
>
> Am 27.09.2018 um 01:24 schrieb Paul King:
> > The String check for "groovy.transform.Immutable" would work fine if the
> > 2.4 compiled class was actually loaded with it;s annotations but
> > AnnotationCollector changes any meta-annotation to no longer be an
> > annotation but a class to store the serialized information for the
> > pre-compiled case.
>
> Was that already in 2.4 the case for @Immutable? I thought it was not
> expressed by the annotation collector back then.
>
> And why do we do all those checks for if the type is annotated with
> @Immutable then? We also check for if the annotation starts with
> groovy.transform.Immutable, which would cover ImmutableBase as well,
> which imho stays on the class... wait...it is source only.. oh..
>
> > I think we need to perhaps adjust what we do there
> > slightly to leave the original annotation intact. I'll try to work on
> > that on the plane trip home - perhaps we should delay 2.5.3 a few days
> > to see if we can address this but any alternative suggestions welcome.
>
> leaving the original annotation in tact is what I suggested to you
> before even writing here, because somehow I knew this was the problem,
> without even having looked at it properly ;)
>
> Still I'd like to really understand it, and I am not there yet.
>
> bye Jochen
>
Re: @Immutable backwards compatibility
Posted by Jochen Theodorou <bl...@gmx.org>.
Am 27.09.2018 um 01:24 schrieb Paul King:
> The String check for "groovy.transform.Immutable" would work fine if the
> 2.4 compiled class was actually loaded with it;s annotations but
> AnnotationCollector changes any meta-annotation to no longer be an
> annotation but a class to store the serialized information for the
> pre-compiled case.
Was that already in 2.4 the case for @Immutable? I thought it was not
expressed by the annotation collector back then.
And why do we do all those checks for if the type is annotated with
@Immutable then? We also check for if the annotation starts with
groovy.transform.Immutable, which would cover ImmutableBase as well,
which imho stays on the class... wait...it is source only.. oh..
> I think we need to perhaps adjust what we do there
> slightly to leave the original annotation intact. I'll try to work on
> that on the plane trip home - perhaps we should delay 2.5.3 a few days
> to see if we can address this but any alternative suggestions welcome.
leaving the original annotation in tact is what I suggested to you
before even writing here, because somehow I knew this was the problem,
without even having looked at it properly ;)
Still I'd like to really understand it, and I am not there yet.
bye Jochen
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
The String check for "groovy.transform.Immutable" would work fine if the
2.4 compiled class was actually loaded with it;s annotations but
AnnotationCollector changes any meta-annotation to no longer be an
annotation but a class to store the serialized information for the
pre-compiled case. I think we need to perhaps adjust what we do there
slightly to leave the original annotation intact. I'll try to work on that
on the plane trip home - perhaps we should delay 2.5.3 a few days to see if
we can address this but any alternative suggestions welcome.
On Thu, Sep 27, 2018 at 8:14 AM Jochen Theodorou <bl...@gmx.org> wrote:
> On 26.09.2018 12:58, Paul King wrote:
> > I shouldn't try to respond to emails while rushing between conference
> > sessions. Refreshed my memory and yes, the current provisions for 2.4
> > compatibility don't really help. I'll see if Jochen has some ideas on
> > how we could improve that.
>
> I guess we have to compare
>
>
> https://github.com/apache/groovy/blob/e16728b91601573ee18475ec5dee5355817f670c/src/main/org/codehaus/groovy/transform/ImmutableASTTransformation.java#L738
>
> and
>
>
> https://github.com/apache/groovy/blob/9914ae4a0b4273a5a4cb8822699a9b2dbb3c3215/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java#L352
>
> which tells me the knownImmutableClasses part is gone from @Immutable
> and the 2.5.x version has no way of getting this information anymore,
> since this is supposed to be given directly to the method.
>
> Bad, but let's continue
>
> https://github.com/ajoberstar/grgit/issues/237 talks about for example
> the Commit.groovy, that can be found here:
>
> https://github.com/ajoberstar/grgit/blob/master/grgit-core/src/main/groovy/org/ajoberstar/grgit/Commit.groovy
>
> it does have a knownImmutableClasses=[ZonedDateTime], but looking at
>
> https://github.com/apache/groovy/blob/GROOVY_2_5_X/src/main/java/org/apache/groovy/ast/tools/ImmutablePropertyUtils.java#L101
> it is in the list of builtinImmutables.
>
> And the error message is of course not about that, it is about Person:
>
> https://github.com/ajoberstar/grgit/blob/master/grgit-core/src/main/groovy/org/ajoberstar/grgit/Person.groovy
>
> And that is where I am actually stuck in understanding the issue...
>
> > if (field == null || field instanceof Enum ||
> builtinOrMarkedImmutableClass(field.getClass())) {
> > return field;
> > }
>
> this code for the 2.4 check should have returned, because
> builtinOrMarkedImmutableClass checks if the class of the value for the
> field has an annotation named groovy.transform.Immutable, which is the
> case for Person (see
>
> https://github.com/apache/groovy/blob/9914ae4a0b4273a5a4cb8822699a9b2dbb3c3215/src/main/java/org/apache/groovy/ast/tools/ImmutablePropertyUtils.java#L194
> )
>
> It is similar for Branch.groovy...
>
> So what exactly is not working anymore? I am confused (and it is well
> too late here for looking into such an issue). Cedric, Paul, can you
> explain what exactly goes wrong?
>
> bye Jochen
>
Re: @Immutable backwards compatibility
Posted by Jochen Theodorou <bl...@gmx.org>.
On 26.09.2018 12:58, Paul King wrote:
> I shouldn't try to respond to emails while rushing between conference
> sessions. Refreshed my memory and yes, the current provisions for 2.4
> compatibility don't really help. I'll see if Jochen has some ideas on
> how we could improve that.
I guess we have to compare
https://github.com/apache/groovy/blob/e16728b91601573ee18475ec5dee5355817f670c/src/main/org/codehaus/groovy/transform/ImmutableASTTransformation.java#L738
and
https://github.com/apache/groovy/blob/9914ae4a0b4273a5a4cb8822699a9b2dbb3c3215/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java#L352
which tells me the knownImmutableClasses part is gone from @Immutable
and the 2.5.x version has no way of getting this information anymore,
since this is supposed to be given directly to the method.
Bad, but let's continue
https://github.com/ajoberstar/grgit/issues/237 talks about for example
the Commit.groovy, that can be found here:
https://github.com/ajoberstar/grgit/blob/master/grgit-core/src/main/groovy/org/ajoberstar/grgit/Commit.groovy
it does have a knownImmutableClasses=[ZonedDateTime], but looking at
https://github.com/apache/groovy/blob/GROOVY_2_5_X/src/main/java/org/apache/groovy/ast/tools/ImmutablePropertyUtils.java#L101
it is in the list of builtinImmutables.
And the error message is of course not about that, it is about Person:
https://github.com/ajoberstar/grgit/blob/master/grgit-core/src/main/groovy/org/ajoberstar/grgit/Person.groovy
And that is where I am actually stuck in understanding the issue...
> if (field == null || field instanceof Enum || builtinOrMarkedImmutableClass(field.getClass())) {
> return field;
> }
this code for the 2.4 check should have returned, because
builtinOrMarkedImmutableClass checks if the class of the value for the
field has an annotation named groovy.transform.Immutable, which is the
case for Person (see
https://github.com/apache/groovy/blob/9914ae4a0b4273a5a4cb8822699a9b2dbb3c3215/src/main/java/org/apache/groovy/ast/tools/ImmutablePropertyUtils.java#L194)
It is similar for Branch.groovy...
So what exactly is not working anymore? I am confused (and it is well
too late here for looking into such an issue). Cedric, Paul, can you
explain what exactly goes wrong?
bye Jochen
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
I shouldn't try to respond to emails while rushing between conference
sessions. Refreshed my memory and yes, the current provisions for 2.4
compatibility don't really help. I'll see if Jochen has some ideas on how
we could improve that.
On Wed, Sep 26, 2018 at 12:01 PM Paul King <pa...@asert.com.au> wrote:
> I'll have to look a little more closely. There is some provision for
> handling backwards compatibility. The string value of the class name of
> Annotations is compared with "groovy.transform.Immutable", which will
> handle some cases but we've removed the annotation attributes like
> knownClasses so that is probably going to impact the ability of those
> annotations to be even read. We don't need those annotation attributes any
> more but I am unsure whether we can add them back in a way that won't
> impact our annotation collector usage. I'll have to research unless someone
> else knows off the top of their head.
>
> Cheers, Paul.
>
>
> On Tue, Sep 25, 2018 at 4:39 PM Cédric Champeau <ce...@gmail.com>
> wrote:
>
>> Hi folks,
>>
>> Gradle 5 is migrating to Groovy 2.5 (yay!). However, we discovered
>> several regressions (in @CompileStatic, in covariant return type checking,
>> ...) that may make the migration painful. One of them is unexpected for our
>> users: the @Immutable AST transformation changed the runtime checks, so a
>> class compiled with 2.4 running on 2.5 would suddenly fail. An example of
>> such a problem has been reported at
>> https://github.com/ajoberstar/grgit/issues/237
>>
>> Our partners at Netflix already mentioned they had to fork several
>> plugins to accommodate the problem. While the new checks are legit, the
>> fact that it's an AST xform (happening at compile time) and that the
>> additional check happens at runtime can be surprising.
>>
>> I'm not sure if we need to change this, but having an incompatibility may
>> be annoying.
>>
>
Re: @Immutable backwards compatibility
Posted by Paul King <pa...@asert.com.au>.
I'll have to look a little more closely. There is some provision for
handling backwards compatibility. The string value of the class name of
Annotations is compared with "groovy.transform.Immutable", which will
handle some cases but we've removed the annotation attributes like
knownClasses so that is probably going to impact the ability of those
annotations to be even read. We don't need those annotation attributes any
more but I am unsure whether we can add them back in a way that won't
impact our annotation collector usage. I'll have to research unless someone
else knows off the top of their head.
Cheers, Paul.
On Tue, Sep 25, 2018 at 4:39 PM Cédric Champeau <ce...@gmail.com>
wrote:
> Hi folks,
>
> Gradle 5 is migrating to Groovy 2.5 (yay!). However, we discovered several
> regressions (in @CompileStatic, in covariant return type checking, ...)
> that may make the migration painful. One of them is unexpected for our
> users: the @Immutable AST transformation changed the runtime checks, so a
> class compiled with 2.4 running on 2.5 would suddenly fail. An example of
> such a problem has been reported at
> https://github.com/ajoberstar/grgit/issues/237
>
> Our partners at Netflix already mentioned they had to fork several plugins
> to accommodate the problem. While the new checks are legit, the fact that
> it's an AST xform (happening at compile time) and that the additional check
> happens at runtime can be surprising.
>
> I'm not sure if we need to change this, but having an incompatibility may
> be annoying.
>