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