[Analyzer] null-safe no warning or compilation error for Completer.complete()

[bitsydarel]

This tracker is for issues related to:

• Analyzer

• Dart SDK Version (dart --version)
  Dart SDK version: 2.12.0 (stable) (Thu Feb 25 19:50:53 2021 +0100) on "macos_x64"

Dart analyzer don't provide warning or compilation error when Map<String, Completer> is passed to a function with argument Map<String, Completer<Object?>>. But throws a error at runtime which basically remove all the benefits of a null/type safe code.

As you can see Completer is not equal to Completer<Object?>

code to replicate:

import 'dart:async';

void main() {
  final Map<String, Completer<Object>> tracker = <String, Completer<Object>>{
    '1': Completer(),
    '2': Completer(),
    '3': Completer(),
  };

  allCompleted(tracker);
  
  // trows Uncaught Error: TypeError: null: type 'Null' is not a subtype of type 'Object'
  tracker['2']?.complete();
  
  allCompleted(tracker);
}

void allCompleted(final Map<String, Completer<Object?>> tracker) {
  tracker.forEach(
    (key, completer) => print("${key} is completed ${completer.isCompleted}"),
  );
}

[srawlins]

According to the Completer.complete() documentation:

If the value is omitted or null, and T is not nullable, the call to complete throws.

So it is statically valid to complete a Completer<T>, for a non-nullable T, with an omitted value, even though the code is guaranteed to throw at runtime.

This is also true for Future.value() for a non-nullable T.

@lrhn @bwilkerson I think it makes sense for the analyzer to Hint here, given the guarantee to throw at runtime. WDYT? Something like:

• Given a Completer<T>, c, for a non-nullable T, report a Hint if:
  1. c.complete() is called with no argument.
  2. c.complete() is called with a Null-typed argument.
  3. c.complete() is called with a Never-typed argument.
  4. c.complete() is called with a Future<Never>-typed or FutureOr<Never>-typed argument. (I think we can simplify these examples to something about assignability...?)
• Mutatis mutandis for Future<T>.value() for a non-nullable T.

Examples:

void m1()                   => Completer<int>().complete(); // proposed Hint
void m2()                   => Completer<int>().complete(null); // proposed Hint
void m3(Null a)             => Completer<int>().complete(a); // proposed Hint
void m4(FutureOr<Null> a)   => Completer<int>().complete(a); // existing Error
void m5(Future<Null>? a)    => Completer<int>().complete(a); // existing Error
void m6(void a)             => Completer<int>().complete(a); // existing Error
void m7(Future<void> a)     => Completer<int>().complete(a); // existing Error
void m8(FutureOr<void> a)   => Completer<int>().complete(a); // existing Error
void m9(Never a)            => Completer<int>().complete(a); // proposed Hint
void m10(Future<Never> a)   => Completer<int>().complete(a); // proposed Hint
void m11(FutureOr<Never> a) => Completer<int>().complete(a); // proposed Hint

[bwilkerson]

I think it makes sense for the analyzer to Hint here, given the guarantee to throw at runtime.

I agree. Any time there is guaranteed to be a runtime exception and we can accurately determine that case statically then it makes sense for analyzer to produce a hint.

iii. c.complete() is called with a Never-typed argument.

I'm not sure about Never. I might have the wrong understanding of Never, but doesn't the Never guarantee that we won't actually invoke complete? Hence there is no runtime exception thrown in that case. It seems like we should instead be letting the user know that the invocation of complete is unreachable.

[lrhn]

If the argument has type Never, then we know the call is dead code (in sound mode).
If we don't warn about other Never-guaranteed dead code, I can't see why this should be special.

For Future<Never> or FutureOr<Never>, it's not necessarily dead code. That value can be a future containing an error. (In fact, it's the perfect type for creating an error future that can be reused at any type).

The cases that really needs help is "no argument, non-nullable type" and possibly "nullable argument, non-nullable type", where the latter is enabled because the value is optional and therefore forced to be nullable. The latter call might actually be correct if you know, for some reason not reflected in the type system, that the value is not null. Wouldn't hurt you to write the ! then.

[srawlins]

Excellent, thanks for the feedback!

[simolus3]

I've opened https://dart-review.googlesource.com/c/sdk/+/202622. It handles "no argument, non-nullable type" and "null argument, non-nullable type" since those can't have false positives. I can change it to handle "nullable argument, non-nullable type" too but I'm not sure if it should be a default diagnostic if it can have false positives? In particular, it would warn about Future.value(x as dynamic) then.

[srawlins]

@simolus3 I think your decision is sound. Later on, we can trigger on nullable arguments either in the same Hint, or with a new Hint (with a different message).