Proposal: avoid_unstable_final_field

[eernstg]

avoid_unstable_final_field

Description

Lint the declaration of a getter whose return value might differ from invocation to invocation when it overrides or implements a final instance variable.

Details

avoid_unstable_final_field lints the declaration of a getter in the case where it overrides or implements a declaration of a final instance variable, provided that said getter returns a value which isn't known to be the same for subsequent invocations on the same receiver.

The motivation for linting in this manner is that it is reasonable and common to assume that a final instance variable declaration introduces an immutable property of the object, and it is a source of bugs if it actually returns different results in different invocations on the same receiver.

To define a property of an object that does not introduce this discipline (that is, it is intended to be allowed to change over time), it can be declared as a getter or as a non-final variable.

Kind

This lint guards against logical errors.

Good Examples

class A {
  final int i;
  A(this.i);
}

class B1 implements A {
  late final int i = someExpression; // OK.
}

class B2 extends A {
  int get i => super.i + 1; // OK.
}

Bad Examples

class A {
  final int i;
  A(this.i);
}

var j = 0;

class B implements A {
  int get i => ++j; // LINT.
}

Discussion

avoid_unstable_final_field lints the declaration of a getter in the case where it overrides or implements a declaration of a final instance variable, provided that said getter returns a value which isn't known to be the same for subsequent invocations on the same receiver.

The detailed behavior of the lint is the same as the static analysis that would yield a compile-time error if Dart adopted this proposal about 'final getters' (that's the second half of the first message in that issue), except that no declaration can have final var and getters can't have final, so those subfeatures are not supported with this lint.

In short, we specify that the getter which is induced by a final instance variable declaration is a final getter, and any overriding concrete getter declaration (via implements or extends or with, directly or indirectly) is linted unless (1) it is also final instance variable declaration, or (2) it is a getter declaration whose body is a stable expression.

A stable expression is, mainly, a constant expression, an expression which is similar to a constant expression except that subexpressions are stable rather than constant, or an invocation of a final getter on a receiver which is a stable expression. For example:

class C {
  // A 'late final' yields a final getter, too, and `someExpression` does not have to be stable.
  late final int i = someExpression;
  final C? next;
  C(this.next);
  // Some stable expressions: 1; const []; this; i; next?.next?.i;
}

The motivation for linting in this manner is algorithmic correctness based on an assumption of immutability: It is very common to write code where several instance variables are evaluated, and the correctness of the underlying algorithm relies on the obtained values to be valid even after several other actions have been taken; that is, we expect to get the same answer if we read those instance variables again. It is a very subtle bug if this assumption is invalid in a few cases now and then, and the logic of the algorithm is wrong for that case. The bug fix would generally involve a renewed read on the given instance variable, but it might also include deeper changes to the algorithm, because decisions taken several steps earlier may now have to be reconsidered.

In short, when a "property" of an object is assumed to be immutable, but it actually turns out to mutate, the algorithm needs to handle a significantly more complex situation than was anticipated. For example:

var elevator = ...;
const maxWeight = ...;

void main() {
  Person person = ...;
  if (person.weight <= maxWeight) {
    ... // Stuff.
    if (rareCondition) person.eatSnack(); // OK, maybe not so rare.
    ... // Stuff.
    person.stepInto(elevator); // Just above `maxWeight`, elevator drops into the abyss!
  }
}

This lint would reduce this problem by making it a safe assumption that a final instance variable declaration will imply immutability. We would then be able to concentrate on the remaining cases where mutation is possible, and put more effort into the management of the complexity associated with those cases.

This motivation grows even stronger when Dart adds support for pattern matching, because pattern matching on "properties whose value may be different the next time you ask" is a very confusing concept.

The lint would have false positives in the case where a final instance variable is declared, but the intention is actually that it should be considered unstable, and any getter declaration should be allowed to override or implement it. That case could be handled by renaming the instance variable final T x; to something like final T _x;, and introducing a getter T get x => _x;. Now we have no constraints on overriding declarations (and, of course, no guarantees about immutability).

Prior art

I do not believe that there is any prior art, because the approach where immutability is ensured via a notion of stable expressions is new.

Relationships with other lints

The lint overridden_fields is in the recommended set of lints, and it is even more strict in the case of extends relations (avoid_unstable_final_field is only concerned with final instance variables, overridden_fields applies to all instance variables). overridden_fields remains relevant in cases where an object contains unused storage because it contains multiple instance variables with the same name, and the storage for one or more of those instance variables is unused.

However, avoid_unstable_final_field covers different situations: It includes cases where the superinterface relationship is implements rather than extends (which will include more locations than overridden_fields), but it allows overriding or implementing declarations, as long as they return a stable expression. Of course, this doesn't cover all cases where the immutability assumption is valid (that's undecidable), but it is presumably a reasonably useful set of cases.

So these two lints do not subsume each other in any direction, and they have different purposes, and hence it is not wrong to have both.

The lint unnecessary_overrides will lint a getter declaration if it invokes and returns the result from the overridden getter, and does nothing else. This means that there is no overlap: That particular case is allowed by avoid_unstable_final_field. Moreover, no getter which is linted by avoid_unstable_final_field will also be linted by unnecessary_overrides, because unnecessary_overrides does not target any other expression than the superinvocation of the same getter.

Potential Effective Dart advice

Effective Dart: Design could have a rule saying 'AVOID overriding a final instance variable by a getter that may return different values at each call'.

Discussion checklist

• [x] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• [x] List any relevant issues (reported here, the [SDK Tracker], or elsewhere).
• [ ] If there's any prior art (e.g., in other linters), please add references here.
• [x] If this proposal corresponds to [Effective Dart] or [Flutter Style Guide] advice, please call it out. (If there isn’t any corresponding advice, should there be?)
• [ ] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[bwilkerson]

It might be better to define an annotation (such as stable or immutable) that can be associated with a field that enforces the semantics above. The advantages that I can see are

• it's a more direct way of expressing the developer's intent,
• it would avoid false positives and the need to work around them,
• it could also be associated with instance getters, allowing abstract classes to impose the restraint without needing to declare a field, and
• clients of the package containing the field declaration wouldn't need to enable the lint in order to be told when they were violating the constraints on the object, making the mechanism more secure.

[eernstg]

The explicit mark on a member declaration would make the proposal a lot more like the first half of the initial message in dart-lang/language#1518, that is, the proposal named 'stable getters'. In this proposal, a getter is stable if and only if it has the mark or it overrides (directly or indirectly) a getter that has the mark. The constraints (such as returning a stable expression) are the same.

The trade-off is indeed all the things you mention on one side, but on the other side is the consideration that an implicit mechanism would be applied broadly (without the massive number of source code changes), and it might very well enforce a discipline which is already nearly in place. It was argued (in the language team) that the mechanism would not matter enough to be worthwhile unless it were enabled by default on final instance variables.

It could also be very interesting to see how it would work if all final instance variables were taken to have a final/stable getter by default, and an annotation like @stable could be used to mark abstract getters etc. as stable as well.