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: Making @Immutable a meta-annotation
Date Wed, 24 Jan 2018 03:39:56 GMT
Okay, I made those changes. There is now a makeImmutable annotation
attribute. Still a bunch of tidying up work to do including documentation
refinements but any feedback welcome.

cheers, Paul.

On Sat, Jan 20, 2018 at 10:03 AM, MG <mgbiz@arscreat.com> wrote:

> @MapConstructor(makeImmutable=true)  (or maybe:
> @Constructor(defensiveCopying=true, cloneNonImmutableObjects = false,
> /*etc*/) ?) for me would be the way to go.
>
> (No implementation detail smell imho, just fine granularity for
> developers*, and not reusing the same annotation for (slightly) different
> things.)
>
> Cheers,
> mg
>
> *Which is always good in my book, plus most people will use @Immutable
> meta-annotation anyway, plus everyone can create their own meta-annotations
> from these fine granular annotations (to avoid code clutter)
>
>
> On 17.01.2018 01:48, Paul King wrote:
>
>
> Response inline.
>
> On Wed, Jan 17, 2018 at 9:54 AM, MG <mgbiz@arscreat.com> wrote:
>
>> Hmmm.... If the argument for naming the marker annotation @KnownImmutable
>> was that the existing parameters have similar names (and cannot be changed)
>> then it seems to me the "KnownImmutable" name choice was pretty immutable
>> to begin with...
>>
>> Apart from that, there is still the inconsistency what @KnownImmutable
>> really expresses:
>>
>>    - Class that carries @KnownImmutable is "fully immutable": When a
>>    developer puts the annotation on a class
>>    - Class that carries @KnownImmutable is "base immutable" (i.e. no
>>    defense-copying ctors etc): When being put by the Groovy compiler on a
>>    class after having applied @ImmutableBase transformations to it.
>>
>> This second bullet is the wrong way to look at what is going on. Just
> using @ImmutableBase (or whatever name) on a class doesn't add the
> @KnownImmutable annotation. The @Immutable annotation collector adds
> @KnownImmutable knowing that @ImmutableBase and the various constructor
> annotations are going to be processed. So in fact it's just the first case
> but with the compiler indicating that it is "fully immutable". So I don't
> see a conflict as far as the @Immutable annotation goes. What you could
> argue is that users of the constructor transformations might want the
> defensive copying etc. but might not want to make the class fully immutable
> in which case having an annotation attribute like
> @MapConstructor(makeImmutable=true) would make more sense. This would
> provide maximum flexibility and remove the slight dual usage of the
> annotation. But the other way to look at this is that having a
> "makeImmutable" annotation attribute smells of implementation detail and
> just using @KnownImmutable is the more declarative way to express what we
> want to achieve with less noise.
>
>>
>>
>> The way it looks to me you are trying to express two different things
>> through the same annotation - but to have a clean design you would need two
>> seperate annotations. Maybe that is also why you do not like any of my
>> alternatives, because you are looking for one name that expresses both use
>> cases - and that does not exist, because the use cases differ (?)
>>
>> I am still convinced that while knownUmmutable semi-works as a parameter
>> name inside of @Immutable (I would have picked guaranteed here also), that
>> does not mean it is a good choice for the annotation name. But as I said,
>> if you are convinced that one requires the other, this discussion is mute
>> anyway...
>>
>> On 16.01.2018 01:56, Paul King wrote:
>>
>> Explanations below.
>>
>> On Tue, Jan 16, 2018 at 12:56 AM, MG <mgbiz@arscreat.com> wrote:
>>
>>> Hi Paul,
>>>
>>> 1) for me, if you have to explain a name better, then it is already a
>>> bad name. Intuitively suggesting the correct interpretation to another
>>> developer, without requiring him to thoroughly read through the
>>> documentation, is the art of picking good names (which imho Groovy overall
>>> does a good job at).
>>> With regards to @KnownImmutable, "someone (the compiler or the
>>> developer) knows" is even more confusing. If it is in fact irrelevant who
>>> knows it, why is there a "Known" in the name in the first place ? And why
>>> is therefore e.g. @IsImmutable not a better name (it could also carry a
>>> parameter which can be true or false, with false allowing a developer to
>>> express that a class is definitely not immutable (even if it might look
>>> that way on first glance; e.g. effectively blocking or issuing a warning in
>>> certain parallel execution scenarios)).
>>>
>>
>> We have since the introduction of @Immutable used the knownImmutable and
>> knownImmutableClasses annotation attributes and people seem to grok what
>> they mean. This is a very similar use case. I think it would be hard to
>> justify renaming @KnownImmutable without renaming the annotation attributes
>> as well.
>>
>>
>>> 2) There seems to be a contradiction in your statements: You say that
>>> "Once @ImmutableBase (or whatever name) processing has finished its checks,
>>> it can then vouch for the class and puts the marker interface
>>> [@KnownImmutable] "rubber stamp" on it", but further down you say that
>>> "These changes [that @ImmutableBase applies] alone don't guarantee
>>> immutability.". Is it a "known immutable" after @ImmutableBase has done its
>>> thing, or not ?
>>>
>>
>> Only after all transformations have completed it is guaranteed (see
>> below).
>>
>>
>>> 3) If I did not miss something the new @Immutable meta annotation is
>>> made up of the following annotations:
>>> @ImmutableBase
>>> @KnownImmutable
>>> @ToString
>>> @EqualsAndHashCode
>>> @MapConstructor
>>> @TupleConstructor
>>>
>>> How is any of the last four necessary for a class to be immutable ?
>>> Immutability to me means, that the state of the class cannot be changed
>>> after it has been created. How are @ToString, @EqualsAndHashCode,
>>> @MapConstructor, and @TupleConstructor helping with this ?
>>> At least one ctor to initialize the class fields is basically necessary
>>> to make this a practically usable immutable class, yes, but @IsImmutable it
>>> must be after @ImmutableBase does its job, or it will not be immutable in
>>> the end. All the other annotations are just icing on the cake (see
>>> "@Immutable should be named @ImmutableCanonical").
>>>
>>
>> @MapConstructor and @TupleConstructor do different things if they find
>> the @KnownImmutable marker interface on the class they are processing
>> (defensive copy in/clone/wrap etc.) which is needed for immutable classes.
>> We could have used an additional annotation attribute (makeImmutable =
>> true) or something but the marker interface is useful in its own right and
>> it seems sensible to not duplicate the information it conveys. Besides we'd
>> have to choose a name for "makeImmutable" and again since it's only part of
>> the immutable story good naming would be hard.
>>
>>
>>> If you keep @ImmutableBase, at least consider replacing @KnownImmutable
>>> with @GuaranteedImmutableTag or @GuaranteedImmutableMarker ? The "Tag" or
>>> "Marker" postfix at least expresses that this annotation just tags the
>>> class as having certain properties, and that this is a general fact, and
>>> not only known to developers or compilers in the know...
>>>
>>
>> Marker interfaces are commonplace within the Java world and we don't name
>> them as such. It's not CloneableTag or SerializableMarker. I think adding
>> such a suffix would be confusing.
>>
>>
>>> I hope I do not completely miss your point, but this is how it looks to
>>> me from what I read :-),
>>> Cheers,
>>> mg
>>>
>>>
>>>
>>> On 15.01.2018 14:08, Paul King wrote:
>>>
>>>
>>> Response below.
>>>
>>> On Sun, Jan 14, 2018 at 6:11 AM, MG <mgbiz@arscreat.com> wrote:
>>>
>>>> Hi Paul,
>>>>
>>>> now I get where you are coming from with @KnownImmutable. I agree with
>>>> splitting the two concepts: Flexible & elegant :-)
>>>>
>>>> Transferring the parameter name knownImmutables (which exists inside
>>>> the @Immutable context) to the annotation name KnownImmutable (which has
no
>>>> such context) still does not work for me, though.
>>>> In addition having @Immutable = @KnownImmutable + @ImmutableBase
>>>> violates the definition you give for @KnownImmutable, because either the
>>>> class is "known to be immutable" = "immutable by implementation by the
>>>> developer", or it becomes immutable through @ImmutableBase & Groovy...
>>>>
>>>
>>> Well that is perhaps an indication that it needs to be explained better
>>> rather than necessarily a bad name. I'll try again. It just means that
>>> someone (the compiler or the developer) knows that it is immutable. If that
>>> marker interface is on the class there is no need to look further inside
>>> the class, you can assume it is vouched for as immutable. Once
>>> @ImmutableBase (or whatever name) processing has finished its checks, it
>>> can then vouch for the class and puts the marker interface "rubber stamp"
>>> on it.
>>>
>>>
>>>> What do you think about:
>>>> @IsImmutable
>>>> @ImmutableContract
>>>> @GuaranteedImmutable
>>>> instead
>>>> ?
>>>>
>>>> Thinking about this some more, still don't like @ImmutableBase. Sounds
>>>> too much like a base class to me - and what would be the "base"
>>>> functionality of being immutable ? Something either is immutable, or not
>>>> (@ImmutableCore also fails in this regard ;-) ).
>>>> So still would prefer @ImmutableOnly o.s. ..
>>>>
>>>
>>> @ImmutableOnly indicates that it is somehow immutable at that point - it
>>> isn't really a finished immutable class until all the other related
>>> transforms have done their thing. Perhaps it is useful to reiterate what it
>>> does. It does a whole pile of validation (you can't have public fields, you
>>> can't have certain annotation attributes on some of the other annotations
>>> that wouldn't make sense for an immutable object, you can't have your own
>>> constructors, it can't be applied on interfaces, it checks spelling of
>>> property names referenced in annotation attributes) plus some preliminary
>>> changes (makes class final, ensures properties have a final private backing
>>> field and a getter but no setter, makes a copyWith constructor if needed).
>>> These changes alone don't guarantee immutability. Would you prefer
>>> @ImmutablePrelim?
>>>
>>>
>>>> Cheers,
>>>> mg
>>>>
>>>>
>>>> -------- Urspr√ľngliche Nachricht --------
>>>> Von: Paul King <paulk@asert.com.au> <paulk@asert.com.au>
>>>> Datum: 13.01.18 13:17 (GMT+01:00)
>>>> An: MG <mgbiz@arscreat.com> <mgbiz@arscreat.com>
>>>> Betreff: Re: Making @Immutable a meta-annotation
>>>>
>>>> I should have explained the @KnownImmutable idea a bit more. I guess I
>>>> was thinking about several possibilities for that in parallel. What I
>>>> really think is the way to go though is to split out the two different
>>>> aspects that I was trying to capture. One is triggering the AST
>>>> transformation, the other is a runtime marker of immutability. With that
in
>>>> mind I'd suggest the following:
>>>>
>>>> @KnownImmutable will be a marker interface and nothing more. Any class
>>>> having that annotation will be deemed immutable.
>>>> E.g. if I write my own Address class and I know it's immutable I can
>>>> mark it as such:
>>>>
>>>> @KnownImmutable
>>>> class Address {
>>>>   Address(String value) { this.value = value }
>>>>   final String value
>>>> }
>>>>
>>>> Now if I have:
>>>>
>>>> @Immutable
>>>> class Person {
>>>>   String name
>>>>   Address address
>>>> }
>>>>
>>>> Then the processing associated with @Immutable won't complain about a
>>>> potentially mutable "Address" field.
>>>>
>>>> Then we can just leave @ImmutableBase (or similar) as the AST transform
>>>> to kick off the initial processing needed for immutable classes.
>>>> The @Immutable annotation collector would be replaced by the
>>>> constructor annotations, ToString, EqualsAndHashcode and both ImmutableBase
>>>> and KnownImmutable.
>>>> The name KnownImmutable matches existing functionality. Two
>>>> alternatives to annotating Address with KnownImmutable that already exist
>>>> would be using the following annotation attributes on @Immutable:
>>>> @Immutable(knownImmutableClasses=[Address]) or
>>>> @Immutable(knownImmutables=[address]).
>>>>
>>>> Cheers, Paul.
>>>>
>>>>
>>>>
>>>> On Sat, Jan 13, 2018 at 1:43 PM, MG <mgbiz@arscreat.com> wrote:
>>>>
>>>>> Hi Paul,
>>>>>
>>>>> I think the core of the problem is, that @Immutable as a
>>>>> meta-annotation woud be better off being called something along the line
of
>>>>> @ImmutableCanonical (see: If you do no need the immutability, use
>>>>> @Canonical), since it does not solely supply immutability support - then
it
>>>>> would be natural to call the actual core immutability annotation just
>>>>> "Immutable".
>>>>>
>>>>> That is probably off the table, since it would be a breaking change -
>>>>> so we are stuck with the problem of naming the immutability annotation
part
>>>>> something else.
>>>>>
>>>>> @ImmutableClass would imply to me that the "Class" part carries some
>>>>> meaning, which I feel it does not, since
>>>>> a) "Class" could be postfixed to any annotation name that applies to
>>>>> classes
>>>>> b) The meta-annotation should accordingly also be called
>>>>> "ImmutableClass"
>>>>> Because of that I find postfixing "Immutable" with "Class" just
>>>>> confusing. It also is not intuitive to me, which annotation does only
>>>>> supply the core, and which supplies the extended (canonical)
>>>>> functionality...
>>>>>
>>>>> I do not understand where you are going with @KnownImmutable (known to
>>>>> whom ?-) To me this seems less intuitive/more confusing than
>>>>> @ImmutableClass...).
>>>>>
>>>>> @ImmutableCore is similar to @ImmutableBase (because I intentionally
>>>>> based it on it :-) ), but different in the sense that it imho expresses
the
>>>>> semantics of the annotation: Making the object purely immutable-only,
>>>>> without any constructors, toString functionality, etc.
>>>>>
>>>>> How about:
>>>>> @ImmutableOnly
>>>>> @PureImmutable
>>>>> @ModificationProtected
>>>>>
>>>>> @Locked
>>>>> @Frozen
>>>>>
>>>>> @Unchangeable
>>>>> @Changeless
>>>>>
>>>>> @InitOnly
>>>>> @InitializeOnly
>>>>>
>>>>> @Constant
>>>>> @Const
>>>>>
>>>>> @NonModifieable
>>>>> @NonChangeable
>>>>>
>>>>> ?
>>>>> mg
>>>>>
>>>>>
>>>>>
>>>>> On 12.01.2018 08:01, Paul King wrote:
>>>>>
>>>>> @ImmutableCore is similar to @ImmutableBase - probably okay but I
>>>>> don't think ideal. Another alternative would be @ImmutableInfo or have
an
>>>>> explicit marker interface with a different package, e.g.
>>>>> groovy.transform.marker.Immutable but that might cause IDE completion
>>>>> headaches. Perhaps @KnownImmutable as a straight marker interface might
be
>>>>> the way to go - then it could be used explicitly on manually created
>>>>> immutable classes and avoid the need to use the
>>>>> knownImmutableClasses/knownImmutables annotation attributes for that
>>>>> case.
>>>>>
>>>>> Cheers, Paul.
>>>>>
>>>>> On Thu, Jan 11, 2018 at 9:34 PM, mg <mgbiz@arscreat.com> wrote:
>>>>>
>>>>>> Hi Paul,
>>>>>>
>>>>>> great to make @Immutable more fine granular / flexible :-)
>>>>>>
>>>>>> what about
>>>>>> @ImmutabilityChecked
>>>>>> or
>>>>>> @ImmutableCore
>>>>>> instead of @ImmutableClass ?
>>>>>>
>>>>>> Cheers
>>>>>> mg
>>>>>>
>>>>>> -------- Urspr√ľngliche Nachricht --------
>>>>>> Von: Paul King <paulk@asert.com.au>
>>>>>> Datum: 11.01.18 08:07 (GMT+01:00)
>>>>>> An: dev@groovy.apache.org
>>>>>> Betreff: Making @Immutable a meta-annotation
>>>>>>
>>>>>>
>>>>>> There has been discussion on and off about making @Immutable a
>>>>>> meta-annotation (annotation collector) in much the same way as @Canonical
>>>>>> was re-vamped. (This is for 2.5+).
>>>>>>
>>>>>> I have a preliminary PR which does this:
>>>>>> https://github.com/apache/groovy/pull/653
>>>>>>
>>>>>> Preliminary because it still needs a bit of refactoring to reduce
>>>>>> some duplication of code that exists between the normal and immutable
map
>>>>>> and tuple constructors. I still need to do this but that can happen
>>>>>> transparently behind the scenes as an implementation detail if we
don't
>>>>>> finish it straight away. As well as reducing duplication, the pending
>>>>>> refactoring will enable things like the pre and post options for
>>>>>> MapConstructor and TupleConstructor which aren't currently working.
>>>>>>
>>>>>> I am keen on any feedback at this point. In particular, while most
of
>>>>>> the functionality is pushed off into the collected annotations/transforms,
>>>>>> I ended up with some left over checks which I kept in an annotation
>>>>>> currently called @ImmutableClass. I tried various names for this
class,
>>>>>> e.g. @ImmutableBase and @ImmutableCheck but finally settled on
>>>>>> @ImmutableClass since the annotation causes the preliminary checks
to be
>>>>>> performed but also acts as a marker interface for the MapConstructor
and
>>>>>> TupleConstructor transforms to do the alternate code needed for
>>>>>> immutability and to indicate that a class is immutable when it might
itself
>>>>>> be a property of another immutable class. Let me know if you can
think of a
>>>>>> better name or have any other feedback.
>>>>>>
>>>>>> Cheers, Paul.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>
>

Mime
View raw message