New lint: prefer_void_to_null

[MichaelRFairhurst]

To help encourage people to use void, and discourage use of the academic
& suprising Null type now that void is here to better replace it and
serve its functions.

[bwilkerson]

I'm concerned about the number of false positives for a rule like this. It's clear from the comment that there are cases where Null is appropriate. Can we identify them so that we don't have false positives? If not, have you run this rule over any large code bases to estimate the false positive rate?

[MichaelRFairhurst]

The Null type is extremely academic. A function should return Null when it never returns (loops forever or throws an error). A function should have a Null parameter when the function is never called.

The same is true of functions returning Future<Null> (should return a future that never completes or always contains an error), Stream<Null> (should return a stream that never fires any events), etc etc.

And even in these cases, you can and debatably should return void.

There is a high false positive rate, and we can't really detect those cases. I think I've seen about 100 cases of where Null was correctly used internally.

The value in this lint is something to the effect of, "if you don't need to use Null to make your code work, you don't want to use Null to make your code work," and a tool to change people's mindsets from "Null is my default behavior" to "void is my default behavior" and skip straight past "I need to have a deep understand of both types to know when to use which" because most people don't ever want to use Null unless they (most likely) already understand it.

[bwilkerson]

I'm not questioning the validity of using Null, nor the value of a "just use void" mindset. From both a theoretic and practical perspective, I agree with you.

But research shows that lints that have large numbers of false positives are generally not used, which means that they don't bring the intended benefit to the user. As a result, we have attempted to maintain a very high bar for the lints in the linter package.

Unless there's evidence of a large amount of support for a rule with a high rate of false positives, we tend to not allow them. Perhaps @davidmorgan could weigh in on internal use, or @leafpetersen on the desirability of effectively preventing the use of Null everywhere.

That said, there might be other ways to address the issue. We could, for example, always remove Null from code completion suggestions so that we don't put the idea in user's minds. We could provide a quick assist to convert Null to void (though that's admittedly a little obscure because users won't necessarily look for it).

[leafpetersen]

Not sure of all of the context, but I'd be hesitant to have a blanket warning about using Null. There are valid and good uses for it that aren't covered by void. I'm also worried that it's hard to avoid using Null in places because we weren't able to change inference to infer void as the return type of lambdas with no return. Have we tested this out?

[MichaelRFairhurst]

The two thumbs-ups are proof that there is some desire for this. Theres also a g/dart-lints thread where everyone seems open to this.

In my experience, even the best google engineers are completely confused by the difference between the two types, and trying to use Null where its not required. Null is best as an obscure type that people can use if they are interested in type theory, like F-bounded quantification, etc. Those people won't enable this lint.

Its hard to fairly count usages of this, but my best estimate is that Null has a valid use for one in 28000 LOC.

Given that void is used once per 125 LOC, I firmly believe that you want void instead of Null about 200 times to 1. That's accurate enough for me.

[natebosch]

Null still has a valid use case for creating Function types which check arity right? someFunc is void Function(Null).

[MichaelRFairhurst]

Yep, and I see 3 cases of that having been used internally: lang:dart content:is\sFunction\([^)]+Null

I also have used it to do poor man's pattern-matching: https://github.com/dart-lang/angular_analyzer_plugin/blob/master/angular_analyzer_plugin/lib/src/model.dart#L626

Null has uses.

The description of the lint says: "Do not use Null where void will do." In these cases, "void" will not work, so the lint is not advocating a full-on ban, and even gives practical instructions on when to use Null and ignore the lint.

I have simply never made a customer happy about Dart by describing Null vs void. They get frustrated and confused. There is no need for that confusion, except in rare cases. The only reason we have this problem is people are used to using Null and have misconceptions about it. The best way to fix that is to offer them a simple new option -- such as, "enable prefer_void_to_null, and see if you still have any questions."

[bwilkerson]

Yep, and I see 3 cases of that having been used internally ...

That sounds like an easily identifiable false positive that the rule could easily check for and eliminate. Are there others?

[eernstg]

Sounds like a good idea to flag all those questionable uses of Null. Trying to come up with an overview, we certainly have the following kinds of situations:

• It's a bad habit to use things like Future<Null> foo() async {...} in order to indicate that synchronization is relevant, but the value used for completion should be ignored: It is true that null does not carry any information (so it's "ignorable" by nature), but the subtype relationship from Null to all other types (except non-null ones if/when we get them) allows for lots of error-prone usages: T t = await foo(); is allowed for any type T, both statically and dynamically. In general, Null as a type argument should not be used to indicate that any values having the corresponding type variable as its type should be discarded (directly or indirectly, could be x.y[0] where x.y is a List<Null>). This is a problem because developers were told for a while that they should use Future<Null>, Visitor<Null>, etc., for exactly that.

• It is appropriate to use Null for parameter types of first class functions, thus allowing all types, in order to check for a specific argument list shape, including arity: f is void Function({Null x}); or in order to admit all such functions: void foo(void Function({Null x}) f, dynamic d) { (f as dynamic)(x: d); }. This case is clear: Allow it.

• It is appropriate to use Null as the return type (of a function that actually returns null, or throws, or doesn't return at all) in order to obtain a first class function which can serve as a T Function(...) for all T (where the parameter types match up), assuming that locations in code where that function is used are designed for handling the situation where null is actually returned, or the function throws, etc. With non-null types that would be a reasonable assumption anyway, because we are letting a function that does return null masquerade as a function that might return null, and the caller should be able to handle that; similarly, it might be perfectly appropriate in a given context for a first class function to throw when invoked. Also, Null can be used meaningfully as an instance method return type with nullability (see below). This case may be more specialized, and it might make sense to flag it in a lint and require some // ignore: annotation in order to explicitly say "I know what I'm doing!"', except that an inferred Null return type for a function literal should always be allowed (cf. Leaf's remark).

// Assume non-null types are available.
// `T?` is nullable, plain `T` is non-null except that `Null` == `Null?`.

abstract class Thing {
  // Things may or may not have a name.
  String? name();
}

abstract class BelovedThing implements Thing {
  // A beloved thing definitely has a name.
  String name();
}

abstract class BoringThing implements Thing {
  // A boring thing never has a name. If we know statically that it's
  // a boring thing, we might as well know that there is no name.
  Null name();
}

• We could prohibit Null x = null; and similar "silly" stuff, but I don't think that's useful: It's easy to spot anyway, and it might be required in order to satisfy things like implements Foo<Null>, and if we haven't flagged that Foo<Null> for other reasons then the whole class might be perfectly meaningful. So we should allow Null as a type annotation for a field.

So the status might be as follows: Every occurrence of Null as an actual type argument should be flagged, every non-inferred occurrence of Null as a return type should be flagged, and no other occurrences of Null should be flagged.

WDYT? If that's too harsh, do we then need to add support for some @typeNullAllowed annotation (or a magic // ignore_at_use_sites: comment) in order to make it possible for a class designer to say that it's perfectly OK to use Null as a type argument for this particular type parameter? Presumably we'd already have // ignore: .. which could be used to allow a function to have an explicitly declared Null return type, say, in the case where a local function is intended to be passed in those situations where we'd otherwise pass a function literal whose return type would be inferred as Null.

[eernstg]

As a followup on this, I certainly overlooked at least a couple of cases where Null may be a perfectly appropriate type argument:

// Default values may need a `Null` type argument, possibly inferred.
class A<X> { final X x; const A([this.x]); }
A<X> foo<X>([A<X> a = const A<Null>(), List<X> l = const []]) => a;

// An empty map may use a `Null` value type for wider applicability.
const hitmap = <String, Null>{};

main() {
  void bar(Map<String, Map<int, int>> map) {}
  bar(hitmap); // OK.
}

So we do probably want to have an escape hatch such that some actual type arguments can be Null. See this comment for more context about hitmap.

[bwilkerson]

I think we can safely ignore inferred types and allow this lint to flag places where the user has explicitly used Null incorrectly. If there are places where users should be adding types because type inference is inferring undesirable type information (which I would hope isn't the case), then I think that should be reported by a separate hint or lint.

[MichaelRFairhurst]

So we do probably want to have an escape hatch such that some actual type arguments can be Null. See this comment for more context about hitmap.

Yeah, I think this change will definitely need an escape hatch.

Even Future<Null> has a good use -- a Future that never completes! And if we mark that with some kind of @allowNullTypeParameter then we defeat the main benefit of this lint.

I assumed // ignore would be a good enough escape hatch externally, but for g3 its likely that that won't be considered good enough, based on conversations about unawaited_futures.

I'll change it so that Null within a generic function type declaration is OK, that seems like a really strong indication the user is doing something useful & intentional.

[MichaelRFairhurst]

Also made allowances for <Null>[], since that's easy enough, though it is by no means conclusive (nothing to handle <List<Null>, Map<Null, Null>>{} type stuff).

[bwilkerson]

What kind of false positive rate are we seeing with these changes in place?

[bwilkerson]

What kind of "complex literals" are you thinking of?

[MichaelRFairhurst]

Something like this: <List<Null>, Map<Null, Null>>{}

[bwilkerson]

How often does that arise in practice?

[MichaelRFairhurst]

I think this is very rare. I'm not sure how to search for it.

[pq]

Assuming @bwilkerson has no lingering concerns, LGTM!

[pq]

Kicking appveyor in the hopes that its break is transient. 🤞

[bwilkerson]

Only the question I asked before:

What kind of false positive rate are we seeing with these changes in place?

[pq]

appveyor is still unhappy but surely unrelated to this.

https://ci.appveyor.com/project/pq/linter#L129

No need to hold for it.

[MichaelRFairhurst]

Its extremely hard to come up with a false positive rate, but best I can tell, I would expect there to be nom much more than 100 false positives internally, excluding third party code.

I found ~6600 violations that are likely >99% real, ~200 violations that seem >80% real. That'd be about ~106. But its impossible to triage so large a number.

I also don't know what the denominator on that is. Arguably void seems ~450x more common than valid usages of Null. That number would rise as violations get fixed, but it also counts void where people would never write Null in the first place.

[pq]

FWIW issue tracking appveyor failure opened: dart-lang/sdk#57759.

[MichaelRFairhurst]

For external code I've analyzed, there are some hardcore users like mockito and package:expect and the SDK itself. Otherwise, about 70 or so false positives, and valid voids outnumber valid Nulls by about 100x. Same caveats apply

[pq]

@MichaelRFairhurst: if you do want to flag this as experimental, that's described by a lint's Maturity:

https://github.com/dart-lang/sdk/blob/329e029bd6572481af3aeccd2377cb7f52e7dab2/pkg/analyzer/lib/src/lint/linter.dart#L303-L325

[MichaelRFairhurst]

So in terms of false positives, the false positive rate will be extremely, extremely low based on the prevalence of Future and such.

Over time, if people fix all their Futures, and get out of the habit of it, the false positive rate will approach 100%. Assuming you only use Null when you thoroughly think it through first, you can probably get good enough that you never misuse it. (and that includes "Future" which itself has valid usages).

If everyone is OK with that being true, but we also accept that we need some kind of tooling in the meantime to encourage a switch from Null to void, then this lint can probably go out as such and we can see if people request it to be relaxed in any kinds of ways.

Maybe we can introduce a new Maturity later, "Maturity.sunsetting" that we can add to this when we have good evidence that Null over void is no longer a mistake made by habit/misunderstanding. That maturity almost describes it already, given that the existence of the lint will drive itself into a need to no longer exist.

[bwilkerson]

Thanks for working hard to minimize the possibility of false positives! I think this is a much more valuable lint because of it.

[pq]

Hear, hear. Thanks a million @MichaelRFairhurst !

PS: the appveyor failures appear to be unrelated (see: dart-lang/sdk#57759).