"Do not override fields" for field overridden from implements SuperClass

[zoechi]

abstract class BaseLoggingHandler {
  LogRecordTransformer transformer;
  void call(LogRecord logRecord);
}


class LogPrintHandler implements BaseLoggingHandler {
  @override
  LogRecordTransformer transformer;
}

complaints about "Do not override fields"

[alexeieleusis]

This rule was introduced here dart-archive/linter#217 and requested here #25567, I think this is working as intended, below you can see the docs from the rule. Regardless, do you think the rule is wrong? How would you like it to be changed? It should be noted that the rule will only trigger if it is overriden with a field, if instead you have a getter and setter it won't complain.

http://dart-lang.github.io/linter/lints/overriden_field.html

DO Do not override fields.

BAD:

class Base {
  Object field = 'lorem';

  Object something = 'change';
}

class Bad1 extends Base {
  @override
  final field = 'ipsum'; // LINT
}

class Bad2 extends Base {
  @override
  Object something = 'done'; // LINT
}

GOOD:

class Base {
  Object field = 'lorem';

  Object something = 'change';
}

class Ok extends Base {
  Object newField; // OK

  final Object newFinal = 'ignore'; // OK
}

[alexeieleusis]

Thinking a bit more about it, I think it is a false positive (which I confirmed) if the implementing class (e.g. LogPrintHandler) has a field from a derived class, which would restrict the values that can be set on instances of such class. Is that what you had in mind?

[zoechi]

AFAIR the intention of the rule was that it is inefficient when a subclass overrides a field with a field, because then there are two variables for one value (in superclass and subclass) while only one is actually used.
That is correct if I extend a class.
When I implement a class I only enforce the interface but don't get any implementation from the interface class, therefore there is still only one field (at least my understanding how implements works).

[alexeieleusis]

I don't know about that case, but as I said above, when you want to restrict the type in the child class, it makes sense to override, e.g.:

abstract class BaseLoggingHandler {
  Base transformer;
}

class LogPrintHandler implements BaseLoggingHandler {
  @override
  Derived transformer; // OK
}

I am working on a PR for that case. While in review we can confirm what you mention above.

By the way, thanks for the report!

[zoechi]

Here I got some great explanations to this topic #24928 (comment)

Therefore it should only check for overridden fields of classes it extends but not the ones it implements.

[alexeieleusis]

This should be fixed now!

[pq]

Thanks for the fix @alexeieleusis !

[alexeieleusis]

my pleasure!

[olof-dev]

With null safety, shouldn't overriding a nullable type with its non-nullable version be OK?

abstract class BaseLoggingHandler {
  Base? transformer;
}

class LogPrintHandler implements BaseLoggingHandler {
  @override
  Base transformer; // Shouldn't this be OK?
}

The linter complains "Don't override fields".

[srawlins]

No, it is still bad practice. https://dart-lang.github.io/linter/lints/overridden_fields.html. See discussion here: #58311

[olof-dev]

Thanks. So there's no built-in mechanism to indicate that a field in a subclass is non-nullable, when the field is nullable in the base class? It seems like that would be useful. (I do understand the rationale of not wanting two storage locations for a field -- I was hoping there'd be some syntax for the nullable case that avoids that.)

As for work arounds, I guess I can litter non-null-asserts ! on every access of the field in the derived class. I guess one could also do something like the following:

class Animal {
  int? age;
  Animal({required this.age});
}

class Cat extends Animal {
  @override
  int get age => super.age!;

  @override
  set age(int? value) {
    if (value == null) throw Exception('Cats can only have non-null ages');
    super.age = value;
  }

  Cat({required int age}) : super(age: age);
}

It'd be nice if one didn't have to resort to throwing the runtime exception in the setter, but I guess dealing with setters is less of an issue than getters.

[eernstg]

So there's no built-in mechanism to indicate that a field in a subclass is non-nullable,
when the field is nullable in the base class?

If you wish to model the situation where the type of an inherited instance variable changes then you need to change the meaning of that declaration. You can do that by introducing some kind of parameterization, in this case a type parameter. For instance:

class Animal<AgeType extends int?> {
  AgeType age;
  Animal({required this.age});
}

class Cat extends Animal<int> {
  Cat({required super.age});
}

void main() {
  Cat cat = Cat(age: 3);
  Animal animal = cat;
  
  cat.age = null; // Compile-time error.
  animal.age = null; // OK at compile time, throws at run time.
}

This means that the treatment of nullability will be expressed in the type Cat, so the compiler will know that you can't assign null to its age.

However, if you wish to make the age nullable in Animal then we don't have that level of type safety for expressions of type Animal, and you'll get the error at run time.

Yet another variant with this setup is the case where you have an Animal<int>: In this case you can have null safety also for the Animal<int>.

Of course, you wouldn't actually use a type parameter in this manner, because it's silly to have a type parameter whose value can only meaningfully be int and int?, but it illustrates the underlying trade off.

Another thing you can do is to declare the dynamic checking explicitly, by adding the modifier covariant:

class Animal {
  int? age;
  Animal({required this.age});
}

class Cat implements Animal {
  @override
  covariant int age;
  Cat({required this.age});
}

void main() {
  Cat cat = Cat(age: 3);
  Animal animal = cat;
  
  cat.age = null; // Compile-time error.
  animal.age = null; // OK at compile time, throws at run time.
}

This means that you no longer have the option to work with a safe type at the Animal level, you just have to accept that the run-time error may occur when the receiver type is Animal.

In any case, as mentioned already above, you'd either use implements in order to avoid the space leak of having two storage locations named age in the same object (one of which is unused), or you'd use an abstract declaration in Animal (of course, an abstract instance variable does not have an allocated storage location, that's basically just a way to declare an abstract getter and an abstract setter):

abstract class Animal {
  abstract int? age;
}

class Cat extends Animal {
  @override
  covariant int age;
  Cat({required this.age});
}

void main() {
  Cat cat = Cat(age: 3);
  Animal animal = cat;
  
  cat.age = null; // Compile-time error.
  animal.age = null; // OK at compile time, throws at run time.
}

[olof-dev]

Fantastic -- thanks a lot for that illustration of the options and trade-offs.

[olof-dev]

Btw, I hadn't come across the ability to declare an instance variable in an abstract class as abstract, and always found it very perplexing. Perhaps something about this could be added to the relevant part of the language tour? I can open a new issue if that'd be helpful.

[eernstg]

Yes, that would be good! I think it would fit in section 'Abstract methods', adding something like

An instance variable can be declared abstract by adding the keyword abstract. This has the same effect as declaring an abstract getter and an abstract setter with the same return type respectively parameter type.

[bwilkerson]

Yes, please create a new issue for the documentation request here: https://github.com/dart-lang/site-www/issues/new.