groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul King <pa...@asert.com.au>
Subject Re: @Immutable backwards compatibility
Date Wed, 03 Oct 2018 07:09:19 GMT
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 <paulk@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 <blackdrag@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
>>
>

Mime
View raw message