Completer.complete() signature is not null-safe-friendly

[tvolkert]

Consider the following definition of Completer.complete():

abstract class Completer<T> {
  void complete([FutureOr<T>? value]);
}

Now look at the following code:

void main() {
  Completer<int> completer = Completer<int>();
  completer.future.then((int value) {
    print(value);
  });
  completer.complete(null);
}

When running in sound null-safety, that code will produce no static analysis errors, compile cleanly, and throw a runtime error when it hits the call to completer.complete(null). This is highly counterintuitive and confusing to developers (at least it was to me when I ran into it).

Upon inspection, it's clear that this is clean to analysis because the completion value argument is optional. While this signature made sense in a pre-null-safety world, it's just going to be a source of errors when running with null safety.

I think we should consider a language change to fix this.

[leafpetersen]

cc @natebosch @lrhn @munificent this is pretty painful. Unfortunately, I imagine this would be massively breaking to fix by making the argument required.

What if we made the argument type FutureOr<T> (no ?) and used a default sentinel value to detect when nothing was passed?

class _FutureNever implements Future<Never> {
  const _FutureNever();
  noSuchMethod(_) => throw "Badness";
}

const Future<Never> sentinel = _FutureNever();

[natebosch]

I imagine this would be massively breaking to fix by making the argument required.

Yes it would be.

What if we made the argument type FutureOr<T> (no ?) and used a default sentinel value to detect when nothing was passed?

This would at least warn about completer.complete(null), but it wouldn't warn about completer.complete() which I think is more common. It would at least make it easier to catch things like completer.complete(someNullableVar) which could be helpful.

[tvolkert]

It would at least make it easier to catch things like completer.complete(someNullableVar) which could be helpful.

fwiw this was the actual case I ran into. The example above was a distillation of that.

[lrhn]

We have a number of functions which allow you to omit the argument and default to null, and which cause a run-time error if omitted when the type variable is non-nullable. They all suck as APIs, but would be breaking to change.

I'll look into whether using a default sentinel value makes sense in some cases. The completer example here seems to work because the sentinel can implement Future, but generally we can't have a default value of a generic type.

[mraleph]

What if this:

abstract class Completer<T> {
  void complete([FutureOr<T> value]);
}

actually behaved differently depending on whether T is nullable or not? e.g. if T is non-nullable then value became required rather than optional if no default value is provided?

[lrhn]

@mraleph That would be a nice language feature. We have the problem that this function is perfectly valid already because it's abstract. It doesn't say how it plans to implement that (likely a default value, but you never know). We can't see from the abstract function signature whether the invocation is allowed or not because we can't see whether it has a default value.

I suggested (somewhere) to allow a (generic or in a generic interface) function type to declare a default value type, and allow potentially unsafe default values and default values types, so you'd write:

 void complete([FutureOr<T> value = Null]); // abstract functions have default value types
/// and, having that type,
void Complete([FutureOr<T> value = null]) { ... } // concrete functions have concrete default values

and then an invocation of a generic function with an unsafe default value type for an omitted argument would only be valid if the type argument makes the default value valid.
(Non-generic functions can't really use this for anything, they're either always valid or always invalid).

That also allows you to have unsound defaults other than null. I think it's too specialized to only hang this on nullability.
You can also do something like: void sort([int compare(E a, E b) = Comparable.compare]) { ... } which can be called without an argument only if E implements Comparable.

[lrhn]

This particular case is actually simpler than it looks. We can just change the signature of complete to void complete([FutureOr<T> value]); and not bother finding a default value. The actual implementation classes can then implement it as void complete([FutureOr<T>? value]). Since those implementations are private, callers must still pass a non-nullable value, or not pass the argument at all, in which case the implementation uses null as the sentinel for non-nullable types and as the value for nullable types.

Even if you tear off the function, you can't tell the difference because it's already covariant by generic, so the run-time type is void Function(Object?) anyway.

Looking at this actually suggests to me that there is a use-case for having an optional argument with a tighter bound than the what the default value requires. So, a foo([int x = null]) where the internal type of x is int?, but the function doesn't accept null as argument. I can do that with overriding

abstract class C {
  factory C() = D;
  void foo([int x]);
}
class _D implements C {
  void foo([int? x = null]) { ... }
}

Maybe we should be able to do that in a single declaration as well. Say, if you can set the internal type of a parameter using as type:

class C {
  void foo([int x as int? = null]) { ... }
}

(Then we could allow the as to be both looser and stricter than the public type, to allow the use-case futureOfIterable.then((x as List) { ... I know it's a list ... }); which has been requested a few times already.

[tvolkert]

void complete([required? FutureOr value]);

Yes, it would be handy to be able to express "this argument is optional, but if it is specified, it needs be type X (e.g. FutureOr)"

[lrhn]

@tvolkert You can have optional and non-nullable parameters now, but only if you can provide a default value within that type. It's not possible to have a concrete method with an optional parameter with a restricted type, and a default value outside of that type. (And if that type happens to be a type variable, there is no way to create a sentinel value within the type).
That's why nullable optional parameters are so easy to work with, they have a default default value which is always valid. Non-nullable optional parameters need a specific default value to be provided (and a constant one, just to make it even harder).

In some situations, you can cheat.
An instance method can have an abstract superclass type with void foo([int x]); and a private implementation subclass with void foo([int? x]) { ... }. A factory constructor can be Foo([int x]) = _Foo; with _Foo([int? x]) => ...;. Those are the only two cases where you can write a stricter type in the interface than in the implementation, without having to provide an actual default value in the interface.
It would be nice if we could get the same effect for concrete methods, without going through hoops. For example, I can't make the Iterable interface stricter because it's also a skeleton implementation class.

[tvolkert]

Yep. I was saying that a way in the language to differentiate between an argument being specified vs unspecified could be handy, but the cognitive overhead it would cause is probably not worth it.

[lrhn]

About detecting whether an optional parameter was passed as an argument or not ... we tried that, and it made forwarding arguments more complicated. It needs to be combined with a way to specify "no argument" conditionally, so you can do

foo({int x}) => bar(x: x::wasPassed ? x : __nothing__);

(or some much better syntax) in order to be able to wrap functions and forward arguments without having an exponential number of cases in the number of optional parameters).

My idea of allowing bar({int x as int? = null}) to add a sentinel value outside of the outside parameter type has the same issue.
You can't forward that to another function without doing => x == null ? foo() : foo(x: x); and then we're back to being exponential in the independent (named) optional parameters if you want to optionally forward all arguments.

We could take a hint from list literals and allow: bar({int x as int? = null}) => foo(x: if (x != null) x); which will pass nothing if the condition is false. (Only works for named arguments, unless we start allowing omitting non-trailing positional arguments too, which we can: qux(2, 4, , , 5) would omit the third and fourth positional argument and still pass the fifth. It's definitely doable.

[rrousselGit]

As a comeback to this, would it be possible to instead deprecate complete in favor of a null-safe variant?

Especially now that we have dart fix, a fix could be implemented for this to automatically migrate.

[lrhn]

It's such a good name!

I'd rather have the option to "deprecate optionality", maybe just @deprecateOptional, meaning that the parameter will become required in the future. You can start passing arguments now nd get rid of the early warnings, but it won't break before the parameter actually becomes required.

Maybe we should have other deprecation-for-change markers, instead of just the current "will be removed" variant.

[rrousselGit]

maybe just @deprecateOptional,

That sounds like a good addition. And it could be applied to many things, like Future.value() or Iterable.firstElse(orElse:)