Re: Making @Immutable a meta-annotation

[Paul King <pa...@asert.com.au>]

I ended up removing the makeImmutable attribute and instead just look for
the presence of the @ImmutableBase annotation. It turns out in order for
@MapConstructor or @TupleConstructor to do the right things, they need
access to the information (i.e. knownImmutables and knownImmutableClasses)
that @ImmutableBase conveys. @ImmutableBase isn't retained at runtime and
@KnownImmutable is kept solely as a marker interface. It turns out to be
quite clean that way. A nice feature of this approach is that we can easily
solve GROOVY-7078.

Feedback welcome.

Cheers, Paul.

On Wed, Jan 24, 2018 at 1:39 PM, Paul King <pa...@asert.com.au> wrote:

> 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 <mg...@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 <mg...@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 <mg...@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 <mg...@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 <pa...@asert.com.au> <pa...@asert.com.au>
>>>>> Datum: 13.01.18 13:17 (GMT+01:00)
>>>>> An: MG <mg...@arscreat.com> <mg...@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 <mg...@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 <mg...@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 <pa...@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.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>