New lint: Avoid unstable final fields

[eernstg]

This PR implements the lint avoid_unstable_final_fields, cf. issue #3440.

The basic idea is that a final field declaration is taken to indicate that the given property is immutable, and hence every override which is not guaranteed to return the same value each time it is invoked will be linted.

class A {
  final int i; // Taken to be immutable, because it's a final variable.
  A(this.i);
}

class B1 implements A {
  static _i = 0;
  int get i => ++_i; // LINT: Does not return the same object every time.
}

class B2 implements A {
  final A a = A(42);
  int get i => a.i; // OK.
}

The PR is intended to serve as a point of reference, facilitating discussion and feedback on the lint and the underlying ruleset, as well as on the implementation.

[Edit: Removed remarks about landing this PR.]

[bwilkerson]

This looks good. Just a couple of nits and forward pointers to changes that will land soon.

[eernstg]

Got it, thanks!

[coveralls]

coverage: 95.911% (-0.6%) from 96.551% when pulling f9966a0 on avoid_unstable_final_fields_jul22 into 5ee6a74 on main.

[srawlins]

Now that Dart 3 has landed, I'd love to hear your thoughts on continuing down this road, @eernstg . The description above says this is not intended to be landed at this point.

I think I'd be very interested in hearing whether stable getters (dart-lang/language#1518) is still on the table for any upcoming release. It is probably my favorite possibly upcoming feature :)

That being said, this rule would still provide great value even if we never land that feature, yes?

Thanks!

[eernstg]

Thanks for the kind words! The 'final getters'/'stable getters' feature does come up in discussions in the language team repeatedly, and I'm not alone in pushing it. But most of the language team is worried about the potential breakage (if we change the meaning of the existing modifier final on non-local variable declarations) or the potentially small impact (if we require a new keyword like stable in order to make any getter stable/final). So it's not a given that any version of this feature will be added to any release of Dart.

Nevertheless, I think it would be great if we could land the lint: It flags situations where a final instance variable is overridden by a getter which is not known to return the same object on repeated invocations, and that seems to be a somewhat delicate situation anyway. It would hardly hurt anyone if the lint exists, as long as it isn't part of a set like 'recommended' or 'core'.

One thing needs to be settled, however: There must be a way to indicate that a given final instance variable induces a getter which is not final/stable. In the current version of the lint I'm using @Object() to indicate that, just because this was something that I could do without changing anything in package meta or dart:core (and also because this annotation is basically never used today). We could use a random value like const unstableGetter = '2d1ae4a0241b54c8ab4f4b55fc546079a1fe4191'; which can be declared anywhere it is needed, and then @unstableGetter can be used to play this role; however, that's probably too brittle to be acceptable.

[srawlins]

Nevertheless, I think it would be great if we could land the lint

Yeah I think this is a good path forward (to enable the language feature, or as its own end).

One thing needs to be settled, however: There must be a way to indicate that a given final instance variable induces a getter which is not final/stable.

I think we'd be very open to adding an annotation to package:meta. @unstableGetter perhaps. I like the idea of the magic, equal but distant constants that any developer can declare and then use, but I think the UX there is pretty weird. Fine to stay conventional with package:meta. I can start that conversation.

[bwilkerson]

It shouldn't be a problem to add an annotation to mark an unstable getter. Just to double check though, are we sure we want a lint with an annotation to opt out rather than no lint and an annotation to opt in (that is, assume every getter is unstable unless it's marked as being @stable)? As I understand it, without the @stable annotation users can't mark getters as being stable, only final fields get that designation.

Assuming we want the lint, we'll probably need a fix for cases where the lint breaks the code. Is there any remediation other than marking all overriding getters as being unstable?

If we have the notion of a stable getter, is there anything we can/should do at the invocation sites? I could, for example, imagine suggesting that invocations within a loop could be hoisted (as long as the receiver doesn't change), or suggesting that a single invocation would be more efficient when the same getter is invoked multiple times. These are, of course, things that it would be nice for the compiler to optimize automatically, but if this isn't a language feature the compiler probably can't depend on the semantics.

[eernstg]

without the @stable annotation users can't mark getters as being stable, only final fields get that designation.

That is true in the general case. The stable getters / final getters proposal allows declarations like final T get name => e;, which would make it possible to mark any getter as stable, but we don't have that now.

However, it is already possible to do it with an abstract getter: Replace T get name; by abstract final T name;. Those two declarations are equivalent today, but if we consider every final instance variable to have a final/stable getter then the latter would imply that the getter is stable.

If we have the notion of a stable getter, is there anything we can/should do at the invocation sites? I could, for example, imagine suggesting that invocations within a loop could be hoisted (as long as the receiver doesn't change), or suggesting that a single invocation would be more efficient when the same getter is invoked multiple times. These are, of course, things that it would be nice for the compiler to optimize automatically, but if this isn't a language feature the compiler probably can't depend on the semantics.

These things are part of the motivation for having stable/final getters in the first place. Another benefit would be that every stable expression (like aFinalLocalVariable.aStableGetter.aStableGetter) can be promoted.

But those benefits are specific to the language mechanism. As long as we only have a lint we can only used it to give developers a heads-up if there is an apparently-immutable property which isn't actually immutable (that is: a final instance variable which is overridden by a getter which isn't known to be stable).

[bwilkerson]

Since the last comment on this PR we've moved the linter sources into the sdk repo and stopped accepting PRs against this repo. Would you mind converting this into a Gerrit CL?

[eernstg]

Sure! I'll do that tomorrow.

[eernstg]

@bwilkerson, I've created https://dart-review.googlesource.com/c/sdk/+/326342.

[eernstg]

Closing: This effort is now handled via https://dart-review.googlesource.com/c/sdk/+/326342.