prefer_void_to_null false positive

[gabrc]

The lint prefer_void_to_null reports issue with the following code:

abstract class A {
  dynamic get foo;
}

class B extends A {
  @override
  Null get foo => null;
}

[gabrc]

Another example is as follows:

import 'dart:async';

// The lint prefer_void_to_null reports an issue with this code.
// Future.catchError expects a function with type `(dynamic) -> FutureOr<int>`,
// and so the following signature is appropriate.
Null handleError(error) => null;

void main() async {
  var result = await Future<int>.error('42').catchError(handleError);
}

[pq]

Thanks for the feedback @gabrc!

Mike put a bunch of thought into this one so I'll defer to @MichaelRFairhurst for thoughts...

[pq]

Any thoughts on this one @MichaelRFairhurst?

[MichaelRFairhurst]

Yeah, so these both actually stem from a kind of dubious usage of Null in my mind but I think others are free to disagree with me and therefore it probably should be allowed.

But let me explain the issue with this pattern which most people don't consider.

Firstly, it can be a totally invalid override to use Null:

class Person {
  String get name;
}

class NamelessPerson implements Person{
  Null name => null;
}

There are two potential issues here; one is that this may be a case of making the "billion dollar problem" worse. Is NamelessPerson really a good pattern here? Should I really be able to define an interface like "a person must have a name string" and then have someone implement it by saying "this person's name is the lack of a name" ? It depends. This may just be the billion dollar mistake under a different color. In this case, you're probably not hitting this issue. But still, it's worth stopping to question yourself in this pattern.

Of course, the intention may have been (in a future where we have nullable/non-nullable types):

class Person {
  String? get name;
}

even in this case there's a potential issue with defining Null get name => null;.

The type String? says essentially "may be a string, but may be a name. Only let this value be used if it in ways that would be valid behavior for both a string and null." So things like "person.name.toString()" etc. By overriding name to return Null, you've removed the limitation that it can only be used as a string, even though a missing name still bears some kind of relationship to the type "String." You've used the most specific type, which is the most useful type.

So where your intention may be to mark it as Null so that it can't be used as a String, you are actually giving the missing name new behaviors:

int? age = person.name; // error: String? and int? are unrelated.
age = namelessPerson.name; // succeeds! Null and int? actually are related.

This isn't a large risk, especially in a land of non-nullable types. But it poses some risk, especially today:

String name = namelessPerson.name; // succeeds, even though Null.
int age = namelessPerson.name; // succeeds, only because of type Null

Imagine for instance that a user misunderstood handleError and thought it returned something useful:

String errorMessage = handleError(err); // succeeds

In this case, I think the best type is

FutureOr<int> handleError(error) => null;

and while currently

@override
Null get foo => null;

is probably best, I'd say that in a world of NNBD it is likely slightly safer to use

Foo? get foo => null;

I'm not sure at what point we call these false positives or false negatives. Ultimately, the rule is doing its job by alerting you to places where you may be opening up your program to more fallout from the billion dollar mistake. However, it's certainly not valid to use void in these cases.

I'm guessing that while the concerns I raise here probably make sense, you probably still view this as a useful pattern and we should probably suppress these errors. I also think that as long as there's some kind of dubiousness to the pattern then maybe its best to rely on // ignore for these. cases