dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.59k forks source link

Lint when an explicit type would be inferred #57989

Open bwilkerson opened 5 years ago

bwilkerson commented 5 years ago

It would be nice to have a lint to flag code where an explicit type annotation is provided that is unnecessary because the same type would be inferred if the annotation were omitted.

Examples

A non-exhaustive list of examples.

A type annotation on a variable is unnecessary when the variable type could be inferred from the type of the initializer, such as

String s = '';

Similarly, type annotations on collection literals can be omitted if the collection type could be inferred, such as:

List<String> sList = <String>[];

Parameter types in function literals can also sometimes be inferred, such as:

List<int> list;
list.forEach((int i) { print(i); });
srawlins commented 5 years ago

CC @munificent who I recall wishes for this dearly.

RedBrogdon commented 5 years ago

The particular reason I'd love to see this lint is that we often see things like this in user code:

Row(
  children: <Widget>[
   SomeWidget(),
   SomeOtherWidget(),
   YetAnotherWidget(),
 ],
)

Folks are very used to that <Widget> because it appears in some old examples and is still present in some autocompletes in the IDE plugins (start typing "chil" and hit tab inside a Row widget, and you'll see what I mean). In most cases, though, it's completely unnecessary, and bums me out.

munificent commented 5 years ago

YES PLZ.

eernstg commented 5 years ago

When we have SomeType x = complexExpression; and complexExpression refers to code that is maintained by a different set of developers in some other package P then SomeType serves as an interface contract between the code right here and the code in P.

Without these contracts it may be considerably more complex to determine why a particular piece of code breaks when we upgrade to a new version of some packages: if foo(y) has an error in line 2250, is it because foo has a different argument type, or because y has a different inferred type? And the declaration of y initializes it with an expression that uses x, that also has an inferred type, etc.

So it seems reasonable to give developers good support for eliminating some "trivial" type annotations, and still allow them to make the judgment that certain types need to be documented explicitly, even for some local variables whose type can be inferred—without forcing them to have lots of // ignore:type_can_be_inferred comments in their code.

Maybe analysis_options could give a list of "well-known packages" for this lint, and then the lint would not complain about explicit types declared in packages outside that list?

munificent commented 5 years ago

Without these contracts it may be considerably more complex to determine why a particular piece of code breaks when we upgrade to a new version of some packages

This is true. There's a trade-off between brevity of inference versus the "safety" of explicitly pinning down known types. We've got a good amount of experience in Dart now with navigating the boundary between those constraints, and "Effective Dart" reflects what we consider to be the best pattern for most users. It's (basically):

These are @bwilkerson's three examples. Then:

I think linting the first three bullet points would cover almost all cases, would be the right choice for most users, and would make Dart code much more consistent.

eernstg commented 5 years ago

I think we generally agree that it's a good idea to document the intended typing properties of public APIs, that wasn't really on the table here. So the point I was making was exactly about local variables, with function literal parameters as a less important case (being in the middle of an expression they don't serve so well as documentation of and commitment to an interface, but it may be useful if the function literal has a large body). Type arguments even more marginal.

I'm just worried that if we consistently nudge developers in the direction of not documenting any typing dependencies in implementation code (esp. dependencies on "remote code", say, when calling a method from another package, written by another organization) then we will consistently promote Dart code which is less maintainable and less readable than it could be with a few judiciously selected type annotations.

One very simple improvement could be to use // commit_to_type, which is directly informing the reader that the following local variable type annotation is considered important and volatile enough to document, rather than using // ignore:type_annotation_can_be_inferred which suggests that there is a problem and we'll fix it at some point. ;-)

RedBrogdon commented 5 years ago

We don't give any guidance on override inference for parameters. My impression is few users know the feature even exists and few rely on it.

Could you point me to a doc that explains what override inference for parameters is? I mean, I'm familiar with that feature, obviously, but a doc would help me explain it to other people better. 😄

munificent commented 5 years ago

Umm... I don't know of any docs on it. :-/