Linter: Add a lint to require types on generic methods

[Hixie]

Right now, there's no lint that catches (e.g.) the lack of a type argument on the Iterable.map method.
It would be nice to be able to know where we're missing them.
Also, ideally such a lint would not trigger if the method in question is labeled with @optionalTypeArgs.

cc @a14n

[matanlurey]

Doesn't this sort of defeat the purpose of using Dart 2 at that point?

[Hixie]

The purpose of using Dart2 in Flutter is to enable more AOT optimisations. I don't see how this affects that?

[MichaelRFairhurst]

I've also has some discussion here https://github.com/dart-lang/sdk/issues/34058

I had proposed something like:

• find generic methods where inference will always be satisfied by, for instance, dynamic
• either lint them when used without type parameters, or when written, or when the inference doesn't affect the return type
• (not discussed as a lint in that thread) or lint when type inference "invents" a type (ie, Null when your code has no explicit Null references). In theoretical terms, we would unify constraints with greatest() and least() rather than LUB and GLB

an example is

expect<T>(T a, T b);

which will always find some T for any two args. Either we could lint and require:

@requiredTypeParameters
expect<T> ...

And we could lint

expect(123, "123"); // error: type parameters required

either based on the @requiredTypeParameters, or by purely what inference has to work with here.

And we could even get smart enough to make an exception for:

List<T> pair<T>(T a, T b) => [a, b];

final foo = pair(123, "123"); // no lint, because "foo" has List<Object>, so assume if that's not the right type, errors will happen later

[a14n]

@Hixie I have a patch for always_specify_types. Running it on flutter triggers 998 lints with a lot of math.max(... , ...). Is it what you want?

[Hixie]

Can we put @optionalTypeArgs on max and min?

[a14n]

IIRC dart libraries don't use package:meta. So I don't think it's possible to put @optionalTypeArgs on them.

We could have a special case for those 2 methods but are those 2 methods really different from other generic methods?

[Hixie]

could we move @optionalTypeArgs to where @override is defined, maybe?

I guess min and max aren't that special; you could accidentally use it like this:

   double a;
   int b, c;
   c = math.min<num>(a, b);

...which is bogus, and we'd want to catch it. But really I think it'd be better if we just had two overloaded versions, min<int> and min<double>, and disallowed min<num>. Having to say min<double> everywhere when both arguments are double is not a great experience.

There's other cases that are more special where it's ok to remove the type, because there's no way to use the method incorrectly that would be caught by requiring types but not caught some other way as well. But by and large most of these methods giving the type explicitly will prevent unintentional mistakes.

[bwilkerson]

could we move @optionalTypeArgs to where @override is defined, maybe?

That would be up to the language team to decide (and they might not be reading this thread).

Are there patterns for methods that don't need to be checked that the lint could look for? Is it reasonable to omit the type argument(s) when they are only used for parameter types? Or only when two or more parameters have the same type parameter (T and List<T>)? I'm not sure why it's reasonable to omit the type arguments for min and max.

If nothing else, instead of special-casing min and max we should notice that the type parameter has an bound of num, which cannot be subclassed outside the SDK, and handle that case in a special way.

[Hixie]

I think a good argument can be made for saying that min and max should have the type arguments, it's just too ugly in that specific case. :-)

[Hixie]

I find it hard to know where we are currently omitting type arguments so it's hard for me to say with any certainty whether there's any patterns to where I think it makes sense to require them and where I think we should allow them to be omitted.

(All this only applies to framework code. Whether end developers would want to give types or not for their code is entirely up to them; we wouldn't enable this by default for them.)

[bwilkerson]

@a14n Would you mind special casing min and max and see how many violations are left in the Flutter framework?

[a14n]

Here's which methods are used without specifying type parameters in flutter:

    258 dart:math:max
    234 dart:core:Iterable:map
    129 dart:async:Stream:transform
    101 dart:math:min
     41 dart:async:Future:then
     22 packages/flutter_test/lib/src/test_async_utils.dart:TestAsyncUtils:guard
     19 dart:async:Future:wait
     17 dev/devicelab/lib/framework/utils.dart:inDirectory
     14 dart:core:Iterable:expand
     12 packages/flutter/lib/src/material/animated_icons.dart:_interpolate
      7 dart:core:Iterable:fold
      6 dart:async:runZoned
      5 dev/manual_tests/lib/text.dart:pickFromList
      4 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:push
      4 dart:async:Stream:map
      3 packages/flutter/test/semantics/custom_semantics_action_test.dart:_nonconst
      3 dart:async:Future:any
      3 dart:_http:HttpOverrides:runZoned
      2 packages/flutter/lib/src/widgets/routes.dart:ModalRoute:of
      2 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:pop
      2 packages/flutter_tools/test/integration/test_driver.dart:FlutterTestDriver:_timeoutWithMessages
      2 packages/flutter_tools/lib/src/vmservice.dart:VM:invokeRpc
      2 dev/bots/test.dart:_deepSearch
      2 dart:isolate:Isolate:spawn
      1 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:pushNamed
      1 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:maybePop
      1 packages/flutter/lib/src/widgets/navigator.dart:Navigator:pop
      1 packages/flutter/lib/src/rendering/layer.dart:ContainerLayer:find
      1 packages/flutter_test/lib/src/goldens.dart:LocalFileComparator:_areListsEqual
      1 packages/flutter_test/lib/src/controller.dart:WidgetController:state
      1 packages/flutter_test/lib/src/binding.dart:TestWidgetsFlutterBinding:runAsync
      1 dev/bots/test.dart:_matches
      1 dart:io:IOOverrides:runZoned
      1 dart:core:Map:map
      1 dart:async:Zone:runBinary
      1 dart:async:Zone:run
      1 dart:async:Future:forEach

[Hixie]

I'm not sure what the pattern is, but here are the ones I think should always have types:

    234 dart:core:Iterable:map
    129 dart:async:Stream:transform
     41 dart:async:Future:then
     22 packages/flutter_test/lib/src/test_async_utils.dart:TestAsyncUtils:guard
     19 dart:async:Future:wait
     17 dev/devicelab/lib/framework/utils.dart:inDirectory
     14 dart:core:Iterable:expand
      7 dart:core:Iterable:fold
      6 dart:async:runZoned
      4 dart:async:Stream:map
      3 dart:async:Future:any
      3 dart:_http:HttpOverrides:runZoned
      2 packages/flutter_tools/test/integration/test_driver.dart:FlutterTestDriver:_timeoutWithMessages
      2 packages/flutter_tools/lib/src/vmservice.dart:VM:invokeRpc
      2 dart:isolate:Isolate:spawn
      1 packages/flutter/lib/src/rendering/layer.dart:ContainerLayer:find
      1 packages/flutter_test/lib/src/goldens.dart:LocalFileComparator:_areListsEqual
      1 packages/flutter_test/lib/src/controller.dart:WidgetController:state
      1 packages/flutter_test/lib/src/binding.dart:TestWidgetsFlutterBinding:runAsync
      1 dart:io:IOOverrides:runZoned
      1 dart:core:Map:map
      1 dart:async:Zone:runBinary
      1 dart:async:Zone:run
      1 dart:async:Future:forEach

(For some of these I would prefer to say had a default value which would get used if no type was specified. Default type arguments aren't currently a language feature, however.)

The following are cases where you often don't know the type and it's ok to pass dynamic, which is the default, and so omitting it is fine (and we should use @optionalTypeArgs to do this):

      4 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:push
      2 packages/flutter/lib/src/widgets/routes.dart:ModalRoute:of
      2 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:pop
      1 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:pushNamed
      1 packages/flutter/lib/src/widgets/navigator.dart:NavigatorState:maybePop
      1 packages/flutter/lib/src/widgets/navigator.dart:Navigator:pop

The following are cases where the type is used for both the arguments and the return value and it's fine whatever the values are, except that in the case of min and max I'd rather we banned num as an option:

    258 dart:math:max
    101 dart:math:min
      3 packages/flutter/test/semantics/custom_semantics_action_test.dart:_nonconst

The following cases I am not familiar with off-hand:

     12 packages/flutter/lib/src/material/animated_icons.dart:_interpolate
      5 dev/manual_tests/lib/text.dart:pickFromList
      2 dev/bots/test.dart:_deepSearch
      1 dev/bots/test.dart:_matches

[MichaelRFairhurst]

I don't know the flutter code, but I would say fold is the only one of the core places where I would want to include a type parameter.

names.map((name) => name.length);

You really want to have to specify <int> here?

And here?

fetchAll.then(countActiveItems);

Can you maybe give some more info on what bugs you're trying to avoid? I fear that, if written this way, this lint will be far too strict to get much usage. OTOH, I know that there are sometimes issues from inference choosing the wrong thing and I'd like to have lints targeted at those cases.

For instance, this throws me off:

expect("foo", 123);

as does this:

// fold names into a counted map
names.fold({}, (acc, next) => acc.insertOrReplace(next, 0, (v) => v + 1)));

Bad example because this code is too complicated (an => inside an =>? no thanks!), but the important part here is that you need to either type acc or parameterize fold unless you want dynamic:

names.fold<Map<String, int>>({}, ...);
// or
names.fold({}, (Map<String, int> acc, next) => ...);

Luckily, if you have implicit downcasts turned off, the resulting List will usually get flagged.

But notably, if you don't have implicit downcasts off, its often better to let the types flow:

Object lastState(Foo foo) => ...;

// without type parameter, you get a static error "unknown member gcd"
getFoo().then(lastState).gcd(...);
// with type parameter, you get a runtime cast error "Object is not int"
getFoo().then<int>(lastState).gcd(...);

So I'd rather try to lint inputs & outputs to functions that appear wrong.

We could lint places where

• inference isn't an exact match (Math.max(num, int) infers num but should it given that not all are num?), equivalent to using == for unification
• inference contains no match (Math.max(int, double) infers num but no num provided), equivalent to using greatest() and least() for unification
• inference failed and used dynamic: (.fold(...))
• inference "zigzags", (int x = math.max(1, 2.0))
• inference's results weren't used as expected (.fold() inferred dynamic, but got Map<String, String>), done after inference rather than during inference, to cast a wider net.

[Hixie]

I definitely think it helps to specify the type on map. The point of adding types is to add redundancy so that the analyzer can help you if you messed up. Without the type on map, how can the analyzer know that you meant to return an integer vs anything else?

Same with then: without an explicit type, how can the analyzer know what you intended to return?

This is especially important because lambdas don't have a way to specify their return type for some reason.

We definitely want to turn off implicit casts ASAP for Flutter framework code. (All of this discussion is only about the lints we have on in Flutter framework code. Flutter developers use a much less stringent set of lints by default.)

I'm all for the other lints you suggest, but they are solving a different problem. The lint suggested for this issue is really just dealing with the way "always_specify_types" doesn't actually work "always". (We enable "always_declare_return_types" and "always_specify_types" but they don't cover generic type arguments for some reason.)

[jmesserly]

Just catching up to this one. Have y'all chatted with @leafpetersen ? Leaf and I are working on some strict analysis flags (https://github.com/dart-lang/sdk/issues/33749). For example --strict-inference so we do not infer top types (Object, dynamic) when we had more specific types available. Another one is --strict-inferred-casts to prevent implicit casts from inferred types.

(The hard part is coming up with rules that have a low false-positive rate. We found it difficult to get adoption of --no-implicit-dynamic and --no-implicit-casts, because they reported too many things at once. The new rules will be more targeted, aimed at catching the most common issues people hit.)

[MichaelRFairhurst]

Without the type on map, how can the analyzer know that you meant to return an integer vs anything else?

I'd like to point out that the analyzer can never know what you intended.

What you're really doing here is writing the type twice, hoping that if there's an inconsistency it will be reported. The analyzer already reports inconsistencies, though, where one thing says something is an int and another thing says it isn't:

foo.then((x) => "$x").then((x) => x + 1);

So one way to attempt to catch more inconsistencies is to double, triple, quadruple specify types. It stands that any value that gets both _created and used_ is already double specified, though, and I question why having a third redundancy would catch bugs and not just increase headache.

"always_specify_types" [doesn't] cover generic type arguments for some reason.

The true answer here is almost certainly more a historical thing, such as specifying types being more common before type inference, and generic methods coming after type inference, and maybe just nobody revisited the issue after generic methods came out.

But if I may digress and offer my opinion on this very controversial topic.

Firstly I'll point out that the discussion here already isn't suggesting to "always specify generic types," or we wouldn't have the issue with "min"/"max." So always is not really always.

I'd also argue this is for the same more or less the same reason that they don't require this:

  ((foo as Foo).bar as Bar).baz as Baz;

The fact that we don't require that, and that we can type check that. is quite literally the exact process of downwards type inference. Another example of how "always" is not really "always."

I don't want to make a slippery slope argument. I draw the line somewhere, and Flutter's style guide draws it somewhere else, and that is :+1:. I don't want to fraction Dart though, and a line _has_ been drawn by the Dart language team, so I don't know how much I want us to invest in tooling that supports a different line, without a higher burden of usability evidence.

I'd rather focus on example code snippets that have problems and see what we can do about those particularly/generally.

[Hixie]

Is there some way we can write our own lints, if this can't be an officially supported lint? I remember there was some talk of an analyzer plugin mechanism a while back; did that ever get implemented by any chance?

[bwilkerson]

We can certainly implement a lint after we have a clear understanding of what the lint should do, but I, at least, do not yet have a clear understanding of the semantics that you're asking for. (And @a14n already has an initial implementation that is primarily awaiting a resolution of this discussion in order to be landed.)

I believe that you want to enforce the rule that invocations of generic methods should have explicit type arguments, but it sounds like there are exceptions to that rule (min and max being two explicitly mentioned examples). What are the exceptions to the rule that should be implemented in order to keep the rule from having false positives? Or, if it's easier to phrase an answer, under which conditions should the rule produce a lint?

[Hixie]

I would be happy with a lint that triggered for all cases except those marked @optionalTypeArgs; see my comment above for which I would mark like that.

If we can't annotate min and max that way, and we can't change min and max to not take num, then I guess I would hard-code min and max as being excluded from this lint.

For min and max maybe the best behaviour would be to have the lint (or another lint) complain when num is inferred, but allow int and double to be inferred. Ideally that would be implemented as an annotation as well, so that we could mark other generic methods as being OK with inference except in cases where the inference doesn't find anything more specific than the default.

[jmesserly]

For min and max maybe the best behaviour would be to have the lint (or another lint) complain when num is inferred, but allow int and double to be inferred

This sounds a lot like something I was talking with Mike & Leaf about. There are two different strictness levels we could choose for --strict-inference. One version would flag cases where we fall back to top/bottom types (e.g. Object, dynamic). A stricter version would also catch cases where we rely on least upper bound/greatest lower bound, for example min(1, 2.0). That's something we've wanted to be stricter about for a long time. I'm not sure if it will be one flag or two, but either way, we'll be able to catch cases where inference "succeeds" by choosing a wide(/narrow) type. Usually those cases indicate a bug in the program, and we should've reported it instead of silently "making it work" at compile time, then failing at run time.

[Hixie]

I think it would have to be an optional lint. There's plenty of people who would have no objection to this code:

  int width = foo.width(); // returns the pixel width
  double textSize = text.measure(); // returns the width of the text, in fractional pixels
  if (max(width, textSize) > maxWidth)
    // ...

[jmesserly]

ah yes, I should've been more clear. This will be an opt-in flag.

[a14n]

I'm not sure to understand why min(1, 1.2) is a problem as long as you use the result as a num. The problem is rather implicit downcasts when you assign it to double because you get a type error.

I also don't understand why min/max should be annotated with @optionalTypeArgs.

FWIW I'm testing a lint with a special case for methods having T type for all parameters and return type as suggested in https://github.com/dart-lang/linter/issues/1138#issuecomment-417814244

The following are cases where the type is used for both the arguments and the return value and it's fine whatever the values are, except that in the case of min and max I'd rather we banned num as an option

With that rule min/max are not linted.

You can see the changes on flutter consecutive to this lint in flutter/flutter#22096 ( I'm close to throw a TooMuchTypesError :) )

[Hixie]

maybe a better lint to address my concern is one to ban num entirely (whether given explicitly or by inference).

[a14n]

maybe a better lint to address my concern is one to ban num entirely (whether given explicitly or by inference).

Created #1177 to track

[a14n]

maybe a better lint to address my concern is one to ban num entirely

@Hixie Does it mean that with a avoid_num lint you wouldn't require types on generics methods?

[Hixie]

With avoid_num preventing/catching cases where min and max infer num for T, the only cases left for min and max are cases that are fine (int or double inferred for T) and therefore I'd be fine with them not having explicit types (). They would then fit the pattern of "parameters and return value are all T", which is a case where I think being explicit doesn't give much value (since you have to be explicit for the arguments and for the return value in other places already).

[Hixie]

(The case that made me file this bug that I think we definitely want to be explicit for are map, then, and so on, where the type is only used for a return value, or is used for some but not all of the arguments, or where the arguments and the return type aren't the same, or where there's multiple generic type arguments, etc.)

[bwilkerson]

@Hixie Are you trying to express the conditions under which type inference can validly infer the return type (T)?

If so, then it seems like it ought to be sufficient to have at least one parameter of that type rather than requiring that all parameters are of that type, such as in T m<T>(T p1, int p2). More generally, I think we only need at least one parameter whose type uses that type, such as in T m<T>(List<T> p1).

[Hixie]

The idea is that ideally in Flutter framework code we would never rely on inference. In some cases (pretty much only min, max, and _nonconst, from what I can tell), the methods are conceptually more like operators than methods and giving an explicit type doesn't help track down bugs because there's enough explicit types around the code already to make a mistake there impossible (well, that would be the case except for the whole num issue, but there's a separate discussion for that).

[bwilkerson]

So, are you suggesting that any method where all of the parameters have the type parameter as their type are likely to be more like operators than methods? Like join from package:path?

[MichaelRFairhurst]

It also sounds like maybe you don't _really_ want min/max to be examples of good inference...its mostly just that you want annotations pretty much everywhere, and min/max turn out to be really common code that doesn't benefit much.

You could always make an int minInt(int x, int y) and maxInt, minDouble, maxDouble functions, of course, or hide max and write your own implementation of it, etc. ("largest", "least", there's a good number of options here).

Then there'd only be the three usages of packages/flutter/test/semantics/custom_semantics_action_test.dart:_nonconst, which I imagine is a sufficiently low false positive rate for you...not to mention, you control the implementation and could add @optionalTypeArgs on them.

From a philosophical perspective, though, I don't think @optionalTypeArgs is the right design. It looks like the perspectives in this thread on where type args should or should not be included is in the eye of the beholder. Flutter may, for instance, not want @optionalTypeArgs on Future.then, but I myself would. Therefore I'm not sure what advice I would give to a library author on when to use it or not, and so its not really a useful thing to offer/expect from them.

[jmesserly]

@Hixie - Is there any way we could adjust/fix inference to make it work for you? Are there any particular problematic examples you're trying to solve by disabling it? Happy to chat offline too, if that'd be easier.

We tried to tighten up the type checking in Dart 2, but we didn't get as far as we'd like. Since the language is pretty forgiving still (implicit casts, instantiate-to-bounds, dynamic, least upper bounds, etc.) type inference can find solutions that are valid in Dart 2, but they're too broad & result in implicit casts. This isn't necessarily a fault of type inference per se (you could write the exact same type explicitly), but rather the "loose" constraints/checking it's operating under.

That's the motivation behind the strict flags: create a more constrained (& hence, predictable) environment for inference to work in, and improve the type checks that depend on inferred types. With strict mode, you'll likely get stronger checking from inference compared to explicit generic method type arguments (because inference won't allow implicit casting).

[MichaelRFairhurst]

This isn't necessarily a fault of type inference per se (you could write the exact same type explicitly)

Yes, this.

Any time inference chose a "wrong" type, causing you a headache, that same error would have happened if you'd written it yourself:

num x = ...;
int y = ...;
y = max(x, y); // shoot, it inferred <num>, then upcast to <int>!
y = max<num>(x, y); // same exact error because I expressed the same intention!
y = max<int>(x, y); // still, an equivalent error even though I expressed a different intention!

moreover, specify the type can _create_ problems:

num x;
num y;
y = max<int>(x, y); // I wrote int because I am not as smart as the compiler, and caused myself problems!

Not to say inference is perfect. Notably, inference chooses "Null"/"Object" a _lot_ more than a person would write that themselves. I think strict mode is planning on rejecting such cases. And implicit downcasts create problems but we are definitely planning on making it common to disallow them.

So its like "bowl is hot, warn me if I use bowl!" instead of "bowl is hot, use oven mitts!"

[bwilkerson]

Would it be better to have a way to detect places where there is an implicit downcast than to have a lint like this?

[Hixie]

We definitely want to turn on the lints that ban implicit downcasts, but that's a separate issue.

This issue is about being explicit with types everywhere. We already require that all arguments, fields, variables, etc, be typed, and that all generic classes be explicitly specialised. It's just that generic methods aren't caught by that lint today.

The problem isn't that inference does the wrong thing, it's that I would like us (in Flutter framework code) to use the language (analyzer, compiler, inference, etc) to verify that the code does what we think it does, instead of having it write part of the code for us implicitly.

Re minInt/maxInt, yeah, what I said earlier in this thread is that my idea situation would be that we would use overloading to define two methods, one for int and one for double, and not have one for num at all. (Having one for num means that the "int-literals-as-doubles" thing won't work if you say min(0, foo) where foo is double.)

[MichaelRFairhurst]

to verify that the code does what we think it does, instead of having it write part of the code for us implicitly

I don't think this is a good metaphor.

You aren't simply writing the code it would have written implictly, you are _overriding what it knows_. This can _suppress_ type errors:

// example a:
var items = foo.items;
var first = items.first;
var name = first.name;
print(name.toUpperCase());

// example b:
List<Object> items = foo.items;
Bar first = items.first;
String name = first.name;
print(name.toUpperCase());

Here, if foo.items is a List<Baz> and not a List<Bar>, you _only_ get errors in example A.

Now you may say its absurd to use List<Object> here. But what if foo.items is typed List<Object> and only full of Bars at runtime? Now you have the same problem in different form: writing List<Bar> will pass statically, just like Bar first is allowed and introduces an implicit cast.

Similarly, Bar.name may be of type Null:

class Base {
  String get name;
}

class Bar extends Base {
  @override
  Null get name => null; // a Bar never has a name
}

In example A, if this is the case, you get a static warning. Example B fails at runtime.

As long as you have implicit downcasts, writing types does _more_ than just make implicit code explicit. Writing types _asserts_ that values are of a certain type when there may be zero reason to believe they actually are. It _introduces_ implicit code (downcasts) if you do it wrong.

(nevermind the case of where adding types asserts something is _not_ something it _is_, ie Object x = 4;).

I'd highly recommend turning off implicit downcasts _before_ considering this as a useful restriction....and maybe even giving it some time to settle, how inference feels in a safer environment, before going full in on explicit types everywhere.

[Hixie]

We don't want implicit downcasts either, and will be dropping them ASAP. That's blocked by various things currently.

I'm confident that I want this lint. We already have a lint that would require every var in the example you have above to be replaced by an explicit type. The only types that we're not currently requiring is those on generic methods, and that's just because that lint predates generic methods.

[Hixie]

Actually I just got a lint about missing a type on map, so maybe this is solved now?

[srawlins]

Agree.