@Immutable backwards compatibility

classic Classic list List threaded Threaded
12 messages Options
12
Reply | Threaded
Open this post in threaded view
|

@Immutable backwards compatibility

Cédric Champeau
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.
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

paulk_asert
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 <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

paulk_asert
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 <[hidden email]> 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 <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

Jochen Theodorou
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
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

paulk_asert
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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

Jochen Theodorou


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
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

paulk_asert
I'll move this to the dev list.

On Thu, Sep 27, 2018 at 10:26 PM Jochen Theodorou <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

Daniel.Sun
In reply to this post by Cédric Champeau
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
Daniel Sun
Apache Groovy committer

Blog: http://blog.sunlan.me
Twitter: @daniel_sun
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

Cédric Champeau
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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: @Immutable backwards compatibility

paulk_asert
In reply to this post by Daniel.Sun


On Sun, Oct 14, 2018 at 1:41 AM Daniel.Sun <[hidden email]> 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
12