[Extension types] Allow @immutable on extension types

[pq]

The following should be legal IMO:

@immutable
extension type  E(int i) {}

but is currently disallowed by our TargetKind validation (edit: INVALID_IMMUTABLE_ANNOTATION may just need broadening).

Related: #52274

[pq]

The motivation for this is a desire to enforce prefer_const_constructors_in_immutables, but maybe we should just treat extension types as immutable (and effectively annotated as such). Unless there are times when someone would enable this lint but NOT want it to flag a non const constructor in an extension type? 🤔

[bwilkerson]

(A response to the original question.)

I'm not sure I see the value in adding the annotation. Extension types are immutable by definition (they can only have one field, and that field is final), and the only subtypes of an extension type are also extension types, so the subclass case is also already covered. I think that allowing the annotation wouldn't allow us to catch any problems that aren't already covered by the spec. Am I missing something?

[bwilkerson]

(A response to the second comment.)

Maybe. My understanding is that the lint was motivated by widgets to allow more of the widget structure to be const. I don't know how often an extension type is likely to show up in a widget tree. But if there's another motivation, or if we think extension types will be used in widget trees, then that might increase the utility of doing so. (I'm not necessarily opposed to doing so.)

As for wanting a non-const constructor, I can't really think of any reason why anyone would want to disallow const instances of a candidate class. In my mind it's more a question of value. If I have to add const to every extension type and don't plan on using them in a constant context, then I might find it annoying. But I also might not, especially if extension types are not used very often outside of JS interop support.

[pq]

Yeah. These are good points. I think the only value is if for some reason we don't want prefer_const_constructors_in_immutables firing on all non const extension type constructors. Not at all sure about that.

@goderbauer @a14n?

[lrhn]

The extension type itself is "immutable", but I think that's a red herring. It's a view on another object, which holds all the state. If the view mutates that state, then it's not really immutable. At runtime, it's like calling extension methods on the representation object, which mutates it.

I think it makes sense to allow the annotation, depending on what it actually means, maybe then check whether the representation type is also immutable.

[bwilkerson]

If the view mutates that state, then it's not really immutable.

I agree. The same is true for a class with a final field: if the state of the object stored in the field can change then the class declaring the field isn't really immutable either. The annotation isn't currently enforcing that the fields contain immutable objects, only that the fields are final.

It isn't clear to me why we'd want it to do more checking for extension types than for classes.

And if the only purpose served by the annotation (irrespective of the original intent) is to ensure that all of the fields are final, then given that the single field of an extension type is always final it isn't clear to me what value allowing the annotation adds.

If we want to extend the behavior of the annotation for classes, then it would make sense to me to allow it on extension types. I just don't know whether that's what the community is asking for.

[eernstg]

@pq wrote:

I think the only value is if for some reason we don't want prefer_const_constructors_in_immutables firing on all non const extension type constructors

I certainly think extension type constructors can be non-const for good reasons. For instance, they can have a body, or they can have an initializer list containing expressions that aren't constant.

Presumably, this means that allowing @immutable on extension types could be useful: When we have that, every non-constant constructor in that extension type will be flagged.

As far as I can see here @immutable isn't intended to specify that the annotated class must be transitively immutable, but that could be a separate lint (and that lint would check that the representation type of an @immutable extension type is also an @immutable type, as well as checking that every instance variable of an @immutable class has a declared type which is also @immutable). Hence, transitivity probably shouldn't be handled separately, and any outcome ("@immutable is transitive" as well as "@immutable is not transitive") would just be applied on extension types as well.