Re: @Immutable backwards compatibility

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

Re: @Immutable backwards compatibility

paulk_asert

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 <[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

paulk_asert
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:


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 <[hidden email]> 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 <[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