Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some class members may not be valid for some actual type arguments: Check that situation statically #2308

Open
eernstg opened this issue Jun 23, 2022 · 2 comments
Labels
request Requests to resolve a particular developer problem

Comments

@eernstg
Copy link
Member

eernstg commented Jun 23, 2022

Some instance methods will only work if certain operations are supported by objects associated with the receiver. For example, a List can be sorted without providing a compare function iff the elements contained in the list are Comparable:

class A {}

void main() {
  List<Object?> xs = <int>[2, 1];
  xs.sort(); // Makes it `[1, 2]`.
  xs = <A>[A(), A()];
  xs.sort(); // Throws.
}

This is currently handled by dynamic type checks. It would be useful to change the situation such that it is at least recognized to be statically type safe in some situations. We can specify this kind of constraint for extension methods:

extension<X extends Comparable> on List<X> {
  void sortWithoutCompareFunction() => sort();
}

void main() {
  List<int> xs = [2, 1];
  xs.sortWithoutCompareFunction();
  List<A> ys = [A(), A()];
  ys.sortWithoutCompareFunction(); // Compile-time error.
}

However, this is not a plug-and-play solution: The extension declaration may declare type parameters that are needed for other purposes, so we may easily get into a situation where we'd want to use intersection types as the bounds, and we don't support intersection types (not for bounds, we do have a special case associated with promotion). An extension method cannot be overridden (so we can't have different implementations of sort for different lists). An extension method cannot be invoked dynamically. Extension methods need special treatment with respect to imports, so they will not become available just because the on-type is available at any given location.

In general, we may wish to declare an instance method m in a given class with type parameters X1 .. Xk, even in the case where the implementation of m will only be type safe when the corresponding type arguments satisfy certain constraints.

Similarly, with null safety, some operations that used to make sense became problematic. For instance, dart-lang/sdk#46646 (comment) describes the use of the setter length= on a List to change the length of the list. This method will use null as the value of newly created entries when this would make the list longer, and discarding entries when the list is made shorter.

It is impossible to determine at compile-time whether any particular operation of the form myList.length = anExpression will make the list shorter or longer (or neither), but when it's made longer it will be a null safety violation to use null as the value at new entries, unless the list has the dynamic type List<T> for some T which is nullable. In other words, the length= setter is safe on a list iff its actual type argument is nullable.

void f<X>(List<X> xs) => xs.length++;

void main() {
  List<int?> xs = <int>[1];
  f(xs); // Statically OK, but throws at run time.
}

We could use void f<X>(List<X?> xs) => xs.length++;, but that doesn't make xs.length++ safe per se, it just "manually" gives rise to the missing static type check on the receiver, for that particular call site.

Another example is #1299, where it is noted that the member signature void complete([FutureOr<T>?]) of Completer<T> isn't null-safety friendly. In particular, it accepts null as the actual argument (by passing null or by omitting the argument entirely). However, if the actual argument is null then (at least with the built-in type _AsyncCompleter) the program will encounter a dynamic error because it invokes a method whose parameter type is FutureOr<T> on a Future<T>, and null does not have type FutureOr<T> unless T is nullable.

Again, the complete operation allows the argument to be optional in all cases, but it is only safe to do so in the case where the actual type argument of the given Future is nullable.

As an aside, the use of dynamically checked covariance causes one more issue: As the example program above shows, it is possible to have a List<int> whose static type is List<int?>. Consequently, we cannot ensure that the actual type argument of the list is a nullable type, just because the static type says so. We could eliminate this problem with use-site invariance, which would allow us to give the list the type List<exactly int?>). In any case, this is an orthogonal issue that we'd deal with separately.

In summary, I think it would be useful to be able to declare methods in need of extra constraints on the type arguments of the enclosing class in a way that yields improved static type safety.

@eernstg eernstg added the request Requests to resolve a particular developer problem label Jun 23, 2022
@Levi-Lesches
Copy link

Levi-Lesches commented Jun 23, 2022

Looking at the code for both _AsyncCompleter.complete and _SyncCompleter.complete, seems like the culprit is the dynamic cast.

void complete([FutureOr<T>? value]) {
  if (!future._mayComplete) throw new StateError("Future already completed");
  future._asyncComplete(value == null ? value as dynamic : value);
}

and _Future._asyncComplete/_Future._syncComplete has the following signature:

void _asyncComplete(FutureOr<T> value) {

Is there any reason for the dynamic cast besides being able to make the parameter optional? Neither _syncComplete nor _asyncComplete actually allows a nullable value (if T isn't already nullable) yet complete disregards this by explicitly checking for null and casting it to dynamic instead. Essentially, Completer.complete is allowing an optional argument when it's actually not optional at all (in any non-nullable or unknown case).

Removing the cast would only be a breaking change if someone omits the argument in a nullable case, ie, Completer<int?>.complete(). Any non-nullable case would already be a runtime error.

To test, I simplified the Completer > _AsyncCompleter > Future > _Future chain to the following:

class A<T> {  // equivalent to [Completer]
  void b(FutureOr<T> value) { }  // equivalent to [_Future._asyncComplete]
  void c(FutureOr<T>? value) => b(value ?? value as dynamic);  // equivalent to [Completer.complete]
}

void main() {
  A<int>().c();   // case 1: throws when [A.c] calls [A.b]
  A<int?>().c();  // case 2: OK
}

As expected, Changing the call to A<int>.b(null) instead fixes the issue for case 1, but breaks case 2. Still a pretty decent deal, considering that the parameter was never semantically optional to begin with.


Going a bit deeper, the same issue arises when calling A<int>().c(Future.value(null)), and changing the call to A.b doesn't fix that. This case is already documented by the null_argument_to_non_null_type lint. It's because of Future.value's signature:

factory Future.value([FutureOr<T>? value]) {
  return new _Future<T>.immediate(value == null ? value as T : value);
}

Again, the explicit cast to T in the null case hints that value isn't really optional here. In a nullable case, sure. But generally, value is needed to tell whether T is nullable or not. Indeed, changing the above to A<int>().b(Future<Null>.value()) correctly identifies the error. While changing this parameter to non-optional may be a breaking change in some nullable cases, it is the semantically correct choice since it is essentially non-optional in the non-nullable or unknown case.

@eernstg
Copy link
Member Author

eernstg commented Jun 24, 2022

@Levi-Lesches wrote:

the culprit is the dynamic cast.

True. But in pre-null-safety Dart, null is always a value of type FutureOr<T> for any T, so it's an appropriate and type safe actual argument to _asyncComplete.

Is there any reason for the dynamic cast besides being able to make the parameter optional?

The task was not to make the parameter optional, it was to avoid widespread breakage when the introduction of null safety caused the existing declaration to be a compile-time error. That parameter could be made mandatory (which would require insertion of an actual argument at a potentially very large number of call sites across the entire Dart community), or it could be made nullable (which was the choice that we actually made, and which gave rise to the dynamic check).

Going a bit deeper, the same issue arises when calling .. Future.value ..

Yes, we had similar discussions about that one.

To go back one step and get an overview:

  • The cases where the constraints on class type arguments enable invocation of specific members (like compareTo which is used by sort when the list elements are Comparable) are the most profound ones. In this case we really need to use the more specific type.
  • The cases where the constraints on class type arguments are there to ensure nullability are typically motivated by the need to avoid widespread breakage. So you could say that we should ignore cases like Completer.complete where the method could have been declared to accept a mandatory parameter rather than an optional one (void complete(FutureOr<T> x)), but the management of breaking changes is so important in practice that I think we should take them into account in relation to discussions like this one as well.

@eernstg eernstg changed the title Some class members may not be valid for some values of the type parameters Some class members may not be valid for some actual type arguments: Check that situation statically Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

2 participants