Lint to check for a union type

[eernstg]

We have a number of requests for adding union types to Dart (esp. https://github.com/dart-lang/sdk/issues/4938), and some of the reasons given are concerned with corelib methods (which means that they have been discussed extensively, and no good solution has been found). I'll focus on one particular example because it is illustrative and frequently encountered, but I believe that other use cases calling for a union type will have many properties in common with this example:

import 'dart:async';

void main(){
  final completer = new Completer<int>();

  final future = completer.future;

  future.catchError((Error e){
    print(e);
  });

  new Timer(const Duration(milliseconds: 10), (){
    print('runtime error right here');
    completer.completeError(new CastError());
  });
}

This example was first given here.

The issue that this example demonstrates is basically that catchError is much too permissive in that it accepts an argument of type Function, but in the implementation it actually enforces a much stronger requirement—we must get a function with one of two given types, but we just can't express that statically:

The declared type of the first parameter of catchError is Function, but the actual constraints are only specified in the DartDoc comment: The argument to catchError should be either a FutureOr<T> Function(dynamic) or a FutureOr<T> Function(dynamic, StackTrace). Obviously, we could have specified that parameter type as a union of those two types, if union types were available.

As a simpler alternative to the addition of full-fledged union types, we could actually handle many of these difficulties using a lint which relies on an annotation:

// Add `AdmittedType` somewhere, e.g., 'package:meta/meta.dart'.
class AdmittedType<X> {
  const AdmittedType();
}

// Example usage with `Future.catchError`.
abstract class Future<T> {
  ...
  Future<T> catchError(
      @AdmittedType<Function(dynamic)>()
      @AdmittedType<Function(dynamic, StackTrace)>()
      Function onError,
      {bool test(Object error)});
  ...
}

(OK, we don't yet support type arguments on metadata, but we can use two const declarations to express the same thing in a slightly more verbose manner).

The idea is that the lint would perform additional type checks on each invocation of Future.catchError, and it would consider the invocation to be admissible as soon as just one of the "admitted types" of the associated declaration makes the invocation type correct. If none of them suffice then the invocation will be flagged.

We don't have a precise definition of 'assignability' for union types (because union types haven't been fully fledged out yet), but this kind of checking is a very good proxy for an actual type check of a union type. We could also have a built-in "--no-implicit-casts" on these lints, i.e., we could simply check whether the static type of the given actual argument is a subtype of each admitted type, and flag those actual arguments that aren't a subtype of any of them.

It is likely that only contravariant positions can meaningfully apply this kind of checking (because we want to get feedback in the cases where the type of an actual argument or variable initializer comes from a list of acceptable types, and anything not in that list should be flagged). If we restrict the list in this manner then we could flag all other (covariant and invariant) occurrences of AdmittedType as a lint violation in itself, or at least emit an informational message that such metadata is ignored.

Of course, the annotation based approach is not quite as general as a real language mechanism. For instance, we cannot use T in the admitted types, because a type variable cannot be used in a constant expression. However, it does allow us to statically specify a larger part of the actual requirement even in tricky cases like catchError, and it covers a lot of simpler union types directly and fully.

(It would actually be a non-breaking change for the implementation of Future.catchError to accept functions returning dynamic/Object (that is: returning anything you want), and then fail dynamically if the returned value isn't a T or a Future<T>, in which case we would be able to specify the actual constraint precisely, and we would still get the same level of checking on the actual returned value, and since no caller has ever had tool support to actually provide a function returning anything in particular, it would consistently be a more precise typing than the one we have today).

I believe that this lint could give us support for many of the benefits that full-fledged union types would otherwise give us, and it would be much simpler to add to the ecosystem.

[pq]

/cc @bwilkerson

[bwilkerson]

I think that this could provide value to users, and I would be happy to see analyzer support it. But I do have two minor comments.

1. The annotation needs to be defined in core if it's going to be used in the dart: libraries.

2. If we add support for this we should add it as a hint rather than a lint, because that's how we implement other annotations from core. That means it would be opt-out rather than opt-in, but I think that's a good thing.

Hence, I think this issue needs to be moved to the sdk repo and assigned to the language team to decide whether this is something that will be supported.

[eernstg]

Seems like we could make it more heavy (in order to let the core libs use this mechanism), or we could keep it lightweight (putting the annotation class into package meta).

If the former won't fly (you don't easily get to add names to core ..) then the latter could still allow us to support this mechanism outside the core.

The linter could even special-case Future.catchError (and maybe a couple of other similar methods) and treat them as if they had those annotations, in which case nobody would need to know that core can't really use this feature, except that we're cheating. ;-)

[bwilkerson]

The linter could even special-case Future.catchError ...

True, and we've done that a few times when necessary, but I don't want to do so unless there's no alternative because the analyzer checks tend to get out of sync with the library definition way too easily. I'm open to adding it to meta if we have to, but it would be much better if it were in core.

[insidewhy]

This is horribly verbose, I want union types bad but not like this.

[kasperpeulen]

Have you thought about assert statements for this feature?

I mean in the example, you could write at the top of the function (or constructor) the following code:

assert(onError is Function(dynamic) || onError is Function(dynamic, Stracktrace));

I dont know the technical aspect, but I believe it would be a better candidate to give union type support for the analyzer. It merges in nicely with an already existing feature, it is very readable and clear.

It is already a good idea to write assert statements. In the the example you gave, it would have been better code in my opinion, if this assert statement was written. It gives an early runtime error in development if the assert failes. That is what you want.

If the analyzer would analyse assert statement like this, to see if it can deduce union types, that would also give support in development, while writing code. This would make an existing feature better and more powerful.

[eernstg]

@kasperpeulen, it is an interesting idea to use asserts. But the dynamic check is already part of the implementation (it could of course be moved up to the beginning of the body of catchError, but even though it doesn't happen exactly at that point, the dynamic checks will be performed already). So the interesting part here is whether we could use such an assert during type checking.

That's not particularly obvious. One thing is that asserts contain expressions, so we need to discover that some of those expressions are so simple that a type checker can predict their outcome (as opposed to starting to solve the halting problem). Next, we'd need to look into the body of a given method implementation in order to find that assert, and this means that we would need to repeat that assert in implementations in all subtypes (that we may have as the statically known type of the receiver), or we would have to check all method bodies in all supertypes for that method name, to see whether one of them has such a "type checkable" assert. I think it might not be easy to make that idea work smoothly.

[eernstg]

@bwilkerson, it seems likely to me that the corelibs would be able to use an internal class as an alternative to an AdmittedType in meta.dart (maybe it could even be private), let's call it _CoreAdmittedType, and the linter would then only have to recognize that annotations of type AdmittedType from meta.dart and of _CoreAdmittedType from the core get the same treatment: Their type argument is a summand of a union type for the carrier of that annotation. With that, the linter wouldn't have to have any special knowledge about Future.catchError or any other particular method, it would just have to check for two types of metadata rather than one. Wouldn't that be relatively maintainable?

[matanlurey]

I think this issue should be discussed and moved to the SDK proper. I don't think it's likely you'd want something like this to be opt-in versus opt-out and it would be better to have more of the core team involved in the discussion.

[bwilkerson]

... corelibs would be able to use an internal class ... Wouldn't that be relatively maintainable?

Yes, that would be perfectly fine.

What I would object to is adding an assumption to the analyzer that catchError works this way because I know from experience how easy it is for the definition of such a method to change without the analyzer being updated to correspond to the new definition.

[pq]

@eernstg, @bwilkerson unless you disagree, I'm inclined to move this over to the SDK... WDYT?

[bwilkerson]

I don't care where the issue is located.

[pq]

@eernstg: I think we agreed to move this issue to the SDK but before I do, is the context still useful?

[github-actions]

Without additional information we're not able to resolve this issue, so it will be closed at this time. You're still free to add more info and respond to any questions above, though. We'll reopen the case if you do. Thanks for your contribution!

[eernstg]

I don't know how I missed this heads-up (probably in the fog of COVID-19 ;-), but I think it's OK to close this issue: The proposed kind of union type support is quite thin (e.g., there's no support for the structural subtyping rules that you'd expect with union types), and the mechanism is somewhat heavy syntactically.

[eernstg]

(Aha, it's auto-reopened. I'll re-close it.)