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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> <[email protected]> >>>>> Datum: 13.01.18 13:17 (GMT+01:00) >>>>> An: MG <[email protected]> <[email protected]> >>>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>> Datum: 11.01.18 08:07 (GMT+01:00) >>>>>>> An: [email protected] >>>>>>> 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. >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >>> >> >> >
