search

dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
635 stars 170 forks source link

Report unrestricted use of runtimeType #1475

Open srawlins opened 5 years ago

srawlins commented 5 years ago

Usage of the runtimeType property of Object is expensive in dart2js. The compiler has written some optimizations however, so that restricted usage of runtimeType are less expensive. The lint rule would report on any usage of runtimeType except the following cases:

  1. Type == x.runtimeType,
  2. a.runtimeType == b.runtimeType (including this.runtimeType), where a and b are not both dynamic,
  3. x.runtimeType.toString().
srawlins commented 5 years ago

FYI @sigmundch

matanlurey commented 5 years ago

re: 2, it should be where either a or b is this.runtimeType, I think.

For example:

void anExample(Function a, Object b) {
  print(a.runtimeType == b.runtimeType);
}

... is going to be a huge de-optimizer despite not violating the rules in 2. You want either a or b to be this, so that the JavaScript code for doing a type check can be as local as possible, I think.

sigmundch commented 5 years ago

Agree that it is a good idea to restrict (2) further.

I'm not as familiar with the linter, but if it cannot do checks based on type information, then I would also suggest using this.runtimeType (it is easy to do completely syntactically)

If the linter has a way to understand the types of expressions themselves, then we could allow more, but the rules get trickier as @matanlurey showed. We could check whether a or b is a type that has only non-generic subclasses or that has a relatively small type hierarchy underneath. The main thing is that we want to avoid preserving type-parameters of unrelated types. Defining what is a "relatively small" hierarchy can get confusing as well.

My guess is that the this.runtimeType is the simplest and most intuitive way to move forward.

srawlins commented 5 years ago

I can first write it to only allow a.runtimeType == b.runtimeType if one of a or b is the identifier this. If that is too restrictive, or we can be smarter, we can loosen later. Tightening later gets dodgy though, for projects which break on lint reports.

srawlins commented 5 years ago

@davidmorgan suggested a dart2js flag instead. I've filed that as https://github.com/dart-lang/sdk/issues/36154.

Note that we don't have to choose one or the other, but if both the lint and dart2js flag are ideas we like, it would be prudent to decide which to run with first, in order to help customers to just clean up existing unrestricted runtimeType usage.