new lint: "Avoid unnecessary type annotations"

[pq]

Avoid type explicit type annotations when they can be inferred.

From the style guide:

If an invocation’s type argument list is correctly inferred with the types you want, then omit the types and let Dart do the work for you.

GOOD:

var strings = ['foo', 'bar', 'baz'];

var fakeSimStore = FakeStore(Sim());

var stars = Row(
  mainAxisSize: MainAxisSize.min,
  children: [
    Icon(Icons.star, color: Colors.green[500]),
    Icon(Icons.star, color: Colors.black),
  ],
);

BAD:

var strings = <String>['foo', 'bar', 'baz'];

var fakeSimStore = FakeStore<Sim>(Sim());

var stars = Row(
  mainAxisSize: MainAxisSize.min,
  children: <Widget>[
    Icon(Icons.star, color: Colors.green[500]),
    Icon(Icons.star, color: Colors.black),
  ],
);

/cc @jefflim-google

[pq]

/cc @goderbauer @Hixie @a14n (flutter)

/cc @munificent (style)

/cc @davidmorgan (pedantic)

/fyi @scheglov (analyzer API implications)

[a14n]

Shouldn't fakeSimStore examples be inversed?

[Hixie]

I don't have a problem with this lint existing, but I would strongly object to turning it on by default.

I think it makes intent harder to discern. For example, maybe you want to return a List<Foo> and it happens that bar() and baz are of type Foo, but that's not obvious:

   return [bar(), baz]; // what's the type of the list?

Contrast:

   return <Foo>[bar(), baz];

A similar problem exists with other types. For example:

   var foo = Bar(baz()); // what's the type argument to the Bar constructor?

Contrast:

   var foo = Bar<Quux>(baz());

I think it makes certain patterns ugly. For example, suppose you have three lists of nums, if any of them accidentally infer the wrong type then that one has to be explicitly stated:

  var a = [3, 1.5];
  var b = [1.5, 0];
  var c = <num>[0]; // have to say <num>[] otherwise it would be a List<int>

Contrast:

  var a = <num>[3, 1.5];
  var b = <num>[1.5, 0];
  var c = <num>[0];

[pq]

Shouldn't fakeSimStore examples be inversed?

😬

Ummmm. Yeah. Thanks! 🤦

[goderbauer]

I am with Hixie: In most of these cases I like the added clarity that those explicit types bring to the table. I am not against the existence of the lint, I hope it wouldn't be forced upon me, though :)

[davidmorgan]

Thanks everyone!

I suspect there is room for a enforceable (in google3/pedantic) lint here.

Note that in google3 we have already enforced omit_local_variable_types, which is very much along these lines. What we found is that it removes a lot of noise from the code. There were a very small number of general objections to the lint, but I haven't had any reports of specific cases where it caused problems, and no requests to relax the lint after it was enforced.

I suspect we will also enforce avoid_types_on_closure_parameters although IIRC that needs a few tweaks first.

The argument for both of those is that the important places where you explicitly specify types are in api not implementation. E.g. the type of the thing you return is the method's return type, you don't need to also specify it in the implementation.

As usual for google3 we'll evaluate based on pre-existing violations and how the code would change to see if this looks right in practice.

Thanks :)

[eernstg]

Maybe you could consider a lint that flags cases where the declared type is non-trivial? In particular, it seems directly counterproductive if developers are punished for documenting the intended element type of a list literal with many elements of different types, just because the desired type argument T is exactly the least upper bound of those many types. Months later one of those elements gets deleted, the least upper bound is now a subtype S of what it was previously, and we get a type error when we try to add an object that is typable as a T but not as an S (and that's a run-time error if the list has been assigned to a variable or parameter with type List<T> on the way to that situation).

The notion of being trivial can be defined in many ways, of course, but for collection literals it could be: A collection literal has a non-trivial element type if it contains elements (for maps: key/value pairs) that do not have the same static type.

This means that it's allowed (but not required) to specify the element type in the cases where relying on inference is obviously error prone.

[davidmorgan]

Thanks Erik. In general I agree there's room for weakening the lint for collection literals along the lines you suggest--need to figure out if that looks more or less confusing in the end :)

[munificent]

Does this lint rule apply to fields and top level variables as well? I'm 1000% excited about this for local variables and type arguments, but for fields and top level variables, we do support users annotating those explicitly even when the annotation is redundant.

[pq]

Does this lint rule apply to fields and top level variables as well?

I think it's all very much open to discussion and really, we should just define it to be as aligned w/ "canonical" style. Maybe we could add a clarifying note in the style guide?

[munificent]

I think Effective Dart is already pretty clear. (Especially now that I've rewritten the type guidelines a couple of months ago.) It has separate rules for type arguments, locals, and other declarations:

• DON’T write type arguments on generic invocations that are inferred.

• DO type annotate fields and top-level variables if the type isn’t obvious.

• DON’T redundantly type annotate initialized local variables.

So, according to the guidelines, locals and type arguments should never be annotated redundantly, but fields and top levels can if you feel that the type isn't "obvious", which it's deliberately pretty vague about.

Maybe split it into two lints?

[pq]

I'm on the same page.

So, according to the guidelines, locals and type arguments should never be annotated redundantly, but fields and top levels can if you feel that the type isn't "obvious", which it's deliberately pretty vague about.

💯

This is exactly what I'd propose unnecessary_type_annotations enforce: locals and type args, skipping fields and top level variables entirely.

/fyi @jefflim-google

[munificent]

This is exactly what I'd propose unnecessary_type_annotations enforce: locals and type args, skipping fields and top level variables entirely.

💯💯💯💯💯

I would maybe consider putting "local" in the lint name too (because I could see some users potentially wanting a separate lint that does the same for fields and top level variables).

[eernstg]

There seems to be a lot of overlap between avoid_unnecessary_type_annotations and omit_obvious_local_variable_types when it comes to local variable declarations, and an upcoming omit_obvious_property_types for other variables (except that avoid_unnecessary_type_annotations might only target locals).

The main difference would be that avoid_unnecessary_type_annotations would flag actual type parameters passed to generic function/method invocations or typed literals, and it would check expressions in general, not just the initializing expressions of local variable declarations (IIUC).

So maybe it could be renamed into avoid_unnecessary_type_arguments?