"Field declaration xxx.yyy can not be overridden in zzz" in strong mode

[zoechi]

Seems to be https://github.com/dart-lang/sdk/blob/master/pkg/analyzer/lib/src/task/strong/info.dart#L460

abstract class CopyItem {
  final io.FileSystemEntity source;
  io.FileSystemEntity destination;
  CopyItem._(this.source, this.destination);
}

/// A file to copy to the deployable directory.
class CopyFile extends CopyItem {
  CopyFile._(io.File file, io.File destination) : super._(file, destination);

  @override
  io.File get destination => super.destination; // <= error
}

/// A directory to copy to the deployable directory.
class CopyDirectory extends CopyItem {
  CopyDirectory._(io.Directory directory, io.Directory destination)
      : super._(directory, destination);

  @override
  io.Directory get destination => super.destination; // <= error
}

Is this intentional?

Dart VM version: 1.13.0-edge.73b6080a6b26ad24380477c4a584edd6cb2e2ab5 (Fri Nov 13 14:15:20 2015) on "linux_x64"

[vsmenon]

Yes, this is intentional. The motivation is touched on here, but I need to expand that:

https://github.com/dart-lang/dev_compiler/blob/master/STRONG_MODE.md#open-items

One possible fix is to place the fields with the correct type in the subclass instead:

abstract class CopyItem {
  final io.FileSystemEntity source;
  io.FileSystemEntity get destination;
  CopyItem._(this.source);
}

/// A file to copy to the deployable directory.
class CopyFile extends CopyItem {
  final io.File destination;

  CopyFile._(io.File file, this.destination) : super._(file);
}

/// A directory to copy to the deployable directory.
class CopyDirectory extends CopyItem {
  final io.Directory destination;

  CopyDirectory._(io.Directory directory, this.destination)
      : super._(directory);
}

Note, this is more efficient in checked mode and dev_compiler as it should avoid any type check on an access to destination.

[zoechi]

Ok, thanks for the explanation.
So this is only when the base class is abstract, right?

[leafpetersen]

This applies whenever you extend a class, abstract or not. If you extend something which has a field x (or which transitively extends something which has a field x), then you can't override x: neither by overriding the field, not by overriding the implicit getter/setters.

You can go the other way (override a getter and/or setter with a field).

You can also implement an interface that declares a field using a getter/setter pair.

Basically, once you have a field, you're stuck with it forever with respect to subclassing, but from the perspective of implementing an interface, a field is just a shorthand for a getter/setter pair.

[zoechi]

Thanks for the explanation!

[zoechi]

What if I want some validation or other additional logic related to the field in the constructor of the base class? It can't be the intention to add this to every subclass.
I also can't change the field in the base class to a getter/setter because I pass it with a named argument and this doesn't allow to assign to final private fields.

[zoechi]

Actually the analyzer doesn't complain about the MyClass({this._somefield}) {} constructor argument. Have to investigate more.

[vsmenon]

@zoechi - can you clarify the example you're working through? Seems different from the original comment now.

In general, if the field is final, it's more efficient to do that validation on the write (in the constructor) than on every read / getter. The original code, in checked mode, would check the type of destination on every read. The modified code (in my comment above) would not.

[zoechi]

Yup, it's a slightly different example where I run into this again.

I have to investigate more. I'm currently in the middle of a big refactoring.

Why is performance in checked mode important anyway?
Checked mode is development only, or are you considering changing that?

[zoechi]

class EventData {
  final sender;
  final dom.Event causedBy;
  EventData({this.sender, this.causedBy}) {
    if(_causedBy, ...) {
    }
  }
}

// I have dozens of such classes extending EventData
class Click extends EventData {
  final Cell cell;
  dom.MouseEvent get causedBy => super.causedBy;

  Click(sender, this.cell, {dom.MouseEvent causedBy})
      : super(sender: sender, _causedBy: causedBy);
}

class CellRangeSelected extends EventData {
  final Range range;

  CellRangeSelected(sender, this.range) : super(sender: sender);
}

I can't use this workaround because {this._causedBy} isn't allowed.

class EventData {
  final sender;
  final dom.Event _causedBy;
  dom.Event get causedBy => _causedBy;
  EventData({this.sender, this._causedBy}) {
    if(_causedBy, ...) {
    }
  }
}

class Click extends EventData {
  final Cell cell;
  dom.MouseEvent get causedBy => super.causedBy;

  Click(sender, this.cell, {dom.MouseEvent causedBy})
      : super(sender: sender, _causedBy: causedBy);
}

[vsmenon]

This is a bit annoying, but you could do:

EventData({this.sender, dom.Event causedBy}) : _causedBy = causedBy { ... }

right?

[vsmenon]

Re perf, this pattern (getter overriding field) does result in suboptimal dart2js code today (even in production mode). dart2js generates a function call instead of a simple field access.

In general, making fields virtual (i.e., overridable) often forces the compiler to generate code defensively.

An option we've discussed is allow code to opt into this behavior. E.g.,

class EventData {
  @virtual final dom.Event causedBy;
}

This effectively makes the compiler generate the boilerplate of a private field / getter for you (and allows it to avoid it otherwise).

[zoechi]

@vsmenon thanks for the workaround, didn't think of this. It's fine then, I just don't want to spread the same code over all sub-classes.
Performance is important, so a bit working around is fine.

I'm still a bit worried because it works against the intention to unify field-, property- and functions- (without parameters) access, but not too much ;-)

Thanks for your support!

[zoechi]

Just to let you know:
The linter produces

Information:(17, 7) Avoid wrapping fields in getters and setters just to be "safe".

because this is against the Dart style guide:

  List<T> _items;
  List<T> get items => _items;
  set items(List<T> items) {
    _items = items;
  }

[leafpetersen]

@pq @munificent Field overrides cause problems for DDC code gen (see some commentary from @vsmenon above). Should the linter/style guide be tolerant of this pattern, at least in strong mode, since it seems to be the best pattern we've found for implementing this, and since field overriding tends to be something of a memory size/performance anti-pattern?

[zoechi]

Just to be sure. I didn't want to argue against this approach. Just stumbled upon this hint and thought it might not hurt if I point it out.

[bwilkerson]

@leafpetersen @vsmenon I'll actually turn the question around slightly: Should this error be a DDC error rather than a strong-mode error? It seems like people that want the extra type safety, but are not targeting DDC, might not want this error.

[munificent]

Is this about overriding a field with another field or overriding it with a getter? The former is, in my opinion, a pure anti-pattern anyway and I think it's fine to have the tooling yell at you.

The latter is useful (since the getter may call super) and it would make me sad if that wasn't supported.

[vsmenon]

Both. The latter is a both a perf and a readability (of output) issue. It forces us to treat every field as virtual in terms of dispatch - i.e, for every field, we have to defensively generate a getter/setter wrapper just in case some other code happens to override it. We don't have the whole program, so we don't know a priori.

It also happens to be a pattern (last I checked) that generates worse code in dart2js production mode.

An option here is to allow it via opt-in:

class A {
  @virtual BaseType field;
}
...
class B extends A {
  SubType get field => super.field;
}

[leafpetersen]

@bwilkerson The issue of DDC specific errors vs general Strong Mode errors does keep coming up, I think we'll need to address it at some point. It's related to the dual issue which is that strong mode as it exists only gives you soundness if you're running with DDC runtime checks (though checked mode probably gives you a superset of those checks). I think our top priority right now is providing a great dev experience with DDC, and I don't want to lose momentum on this. But yes, I think we need to do some work to address this use case at some point.

[munificent]

I like the idea of something like @virtual because it both helps generated code and communicates something meaningful to the reader and can make it easier for them to maintain their application.

[jmesserly]

Update: we haven't lost track of this one. We're considering options like @virtual or its opposite (@sealed?), downgrading to a warning, and things of that nature.

[zoechi]

@jmesserly is this still relevant?

[jmesserly]

no, this was fixed. thanks for catching!