Linter: overridden_fields is overly restrictive

[gabrc]

// The following should be allowed.

class Base {
final Object field = 'lorem';
}

class Foo extends Base {
@override
final Object field = 'ipsum';
}

[pq]

At first blush, I wouldn't think we'd want to allow this but maybe @gabrc can you provide some rationale? Thanks in advance!

/cc @bwilkerson

[gabrc]

The subclass is simply trying to override the getter for field. Why wouldn't that be allowed?

Let me modify the code a bit:

class Foo extends Base {
  @override
  final Object field = foo();
}

Now the overridden getter does not return a constant, and the lint rule does not like this overridden field. I would have to rewrite the getter in this way to avoid the lint warning, and it's unnecessarily complex:

class Foo extends Base {
  final Object _field = foo();
  @override
  Object get field => _field;
}

We have a lot of code like this in google3.

[alexeieleusis]

I agree with Gabriel, as long as the type is preserved and the field remains final, it should be OK. From a consumer perspective it is a getter so it could change from one invocation to the next, in this case they are always receiving the same instance, we can also think of it as if the base class provides with a default value.

[matanlurey]

Overridden fields is actually dangerous, hence this lint. It should _probably_ be prevented (or made a warning) at the _language_ level, not even the linter itself (/cc @leafpetersen - wasn't this in an earlier version of strong mode?).

Consider (https://dartpad.dartlang.org/fdc859ac038f8f0ae8e0646ebe84a780):

void main() {
  new Base();
  new Super();
}

class SideEffect {
  SideEffect() {
    print('>>> SIDE EFFECT <<<');
  }
}

class NoopSideEffect implements SideEffect {}

class Base {
  final sideEffect = new SideEffect();
}

class Super extends Base {
  @override
  final sideEffect = new NoopSideEffect();
}

This prints >>> SIDE EFFECT <<<, not once, but twice. Imagine scenarios where streams/futures are created in the constructor (they do happens, despite not being good style). You could, accidentally, be creating lots of garbage work that is never used.

It should be OK to _implement_ something abstract, but not something concrete, though?

[gabrc]

In your example, I can get around the lint and still produce the side effect:

class Super extends Base {
  @override
  get sideEffect => new NoopSideEffect();
}

If the purpose of the lint is to prevent side effects in the superclass, then the lint is not implemented correctly. The documentation does not explain why overriding fields is bad:
http://dart-lang.github.io/linter/lints/overridden_fields.html

[matanlurey]

I mean, the fact that the lint doesn't have 100% of cases covered is not a good rationale in my head to ignore or remove the lint?

[gabrc]

+1 to remove the lint

What is the original rationale for this lint? The hiding of the superclass field? The presence of side effects?

[pq]

What is the original rationale for this lint?

The original rationale is to catch _accidental_ field overrides.

[matanlurey]

I mean just don't use the lint if you don't agree? I don't understand with the intent of this issue, you want to delete a lint that actually catches errors because you don't like it?

[leafpetersen]

Strong mode originally disallowed overriding fields. Most examples that we found were undesirable. In the original example from this thread there will almost certainly be a "dead" field left on the object using up space unless the compiler can eliminate it (which is unlikely). When we did the original strong mode cleanup, we also found a number of examples of code which did a field override with a final field (or with just a getter) leaving a dead setter left lying around:

class Super {
  int x = 0;
}

class Sub extends Super {
  final int x = 0;
}

void main() {
 Super a = new Sub();
 a.x = 3; // allowed
 print(a.x); // prints 0
}

I personally think that overriding a field from a non-abstract superclass is an anti-pattern, but YMMV.

[matanlurey]

Good example! I guess we'd need to (re-add?) non-virtual fields to get this behavior back?

[gabrc]

"Leaving a dead field" is a good enough reason for the lint. We'll either remove the lint or add an //ignore comment.