Downcasting generic-types crashes with an Unhandled type exception

Question

Downcasting generic-types crashes with an Unhandled type exception

SaadArdati opened this issue a year ago · 14 comments

Given the following example:

abstract class Foo {}

class Bar extends Foo {}

typedef BuilderDef<T extends Foo> = String Function(T someFoo);

class FooMatcher<T extends Foo> {
  final BuilderDef<T> builder;

  const FooMatcher({required this.builder});
}

final FooMatcher<Bar> implementation = FooMatcher<Bar>(
  builder: (Bar bar) => '<3',
);

void main() {
  final FooMatcher matcher = implementation;

  print(matcher.builder.runtimeType);
}

There are two ways to crash it. The first way is downcasting FooMatcher<Bar> to FooMatcher like you see in the first line of the main function. This crashes with the following error message when trying to access the builder field. No need to run the builder, just accessing it crashes. runtimeType is not necessary.

Unhandled exception:
type '(Bar) => String' is not a subtype of type '(Foo) => String'
#0      main (package:dart_testing_grounds/dart_testing_grounds.dart:20:17)
#1      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:296:19)
#2      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

Another way to crash it is to remove the generic from the implementation's return type. So instead of

final FooMatcher<Bar> implementation = FooMatcher<Bar>(

doing this instead will crash when trying to access implementation.builder. It's another way of downcasting the generic.

final FooMatcher implementation = FooMatcher<Bar>(

Full example:

abstract class Foo {}

class Bar extends Foo {}

typedef BuilderDef<T extends Foo> = String Function(T someFoo);

class FooMatcher<T extends Foo> {
  final BuilderDef<T> builder;

  const FooMatcher({required this.builder});
}

final FooMatcher implementation = FooMatcher<Bar>(
  builder: (Bar bar) => '<3',
);

void main() {
  print(implementation.builder.runtimeType);
}

It produces the same error:

Unhandled exception:
type '(Bar) => String' is not a subtype of type '(Foo) => String'
#0      main (package:dart_testing_grounds/dart_testing_grounds.dart:18:24)
#1      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:296:19)
#2      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

I'm using a Macbook Pro with Apple M1 Max.

Dart version:
Dart SDK version: 3.0.6 (stable) (Tue Jul 11 18:49:07 2023 +0000) on "macos_arm64"

Answer 1 · 2023-07-15T18:16:21.000Z

This is all working at intended.

The occurrence of T in the type alias, and therefore in the field type, is contravariant.
Dart class generics are unsafely covariant. The "unsafely" comes from allowing you to write a class like this, rather than disallowing the contravariant occurrence of the class type parameter entirely.

It's still sound, which means that any time an expression can end up with a runtime value with a type that is not a subtype of the static type, the compiler has instead a check and throws to prevent unsoundness.

That's what happens here.

The static type of matcher/implementation is Builder<Foo>, because you typed it as the raw type Builder, which gets instantiated to bound. I'll just use implementation from here, it's the same thing.
The runtime type is Builder<Bar>, which is a subtype because of covariant generics.

The static type of implementation.builder is String Function(Foo).
The runtime type is String Function(Bar), which is not a subtype of the static type.
The compiler knew that could happen, so it checked and throws to avoid unsoundness.

Answer 2 · 2023-07-15T18:25:15.000Z

@lrhn What would be the correct way to do what I'm trying to do in the code sample? I have a more complex case in which I have multiple implementations and their type gets stripped to the equivalent of Foo when stored in lists and such, causing the same crash. Is there a workaround?

I'm also curious if the Dart analyzer should catch this error earlier. This feature of the Dart SDK does not seem intuitive, or if it is, it's not made clear.

Answer 3 · 2023-07-15T18:38:39.000Z

For the record for others to see, a strongly typed class instead of a field implementation will not fix this issue. Here's a code sample that produces the same problem but with a class:

abstract class Foo {}

class Bar extends Foo {}

typedef BuilderDef<T extends Foo> = String Function(T someFoo);

class FooMatcher<T extends Foo> {
  final BuilderDef<T> builder;

  const FooMatcher({required this.builder});
}

// final FooMatcher<Bar> implementation = FooMatcher<Bar>(
//   builder: (Bar bar) => '<3',
// );

class Implementation extends FooMatcher<Bar> {
  Implementation() : super(builder: (Bar bar) => '<3');
}

void main() {
  final FooMatcher matcher = Implementation();

  print(matcher.builder.runtimeType);
}

Answer 4 · 2023-07-15T18:44:01.000Z

The static type of implementation.builder is String Function(Foo). The runtime type is String Function(Bar), which is not a subtype of the static type.

Why is String Function(Bar) not a subtype? Bar is a subtype of Foo, therefore, it is legal and safe to downcast to Foo since Bar extends Foo. Is it because Dart cannot resolve the inheritence of field function parameters?

Answer 5 · 2023-07-15T18:44:20.000Z

You have a Builder<T> which accepts a T and creates a String.
That means that a Builder<Bar>, which knows how to create a string from a Bar, can be cast to Builder<Foo>, a supertype, which claims to know how to handle a Foo, something the Builder<Bar> never promised. That can't just be made to work.

What you can do is to drop the class entirely, and use the function type typedef Builder<T> = String Function(T); everywhere you'd use an object.
That doesn't allow you to add extra methods to the Builder type. You may be able to do that using extension methods, but it can easily become a mess to add extra methods to any function which returns a String.

If you want a list of builders, it would be a List<String Function(Never)>, which you won't be able to call elements of without first checking whether they'd accept the argument.

Alternatively, hide the builder field, and require access to go through methods which check the input.

class Builder<T> {
  final String Function(T) _builder;
  Builder(this._builder);
  bool isValidArgument(Object? o) => o is T;
  String build(Object? input) {
    if (input is! T) throw ArgumentError.value(input, "input", "Not a $T");
    return _builder(input);
  }
  String? tryBuild(Object? input) {
    if (input is! T) return null;
    return _builder(input);
  }
}

You can't just call with any object (except tryBuild) and expect it to work.

There has to be tests (until Dart gets variance modifiers like dart-lang/language#524).

Answer 6 · 2023-07-15T18:48:53.000Z

The reason String Function(Bar) is not a subtype of String Function(Foo) is that functions parameters are contravariant.
(They vary contra, aka opposite, to the type itself.)

The point of subtyping in object orientation (and in general) is that an instance of a subtype can be used anywhere the supertype is expected.

A String Function(Bar) can be called with objects that are subtypes of Bar.
A String Function(Foo) can be called with objects that are subtypes of Foo.
Since all Bar objects are also Foo objects, the latter function can be used anywhere the former is expected, because it accepts all the same arguments. The opposite is not true, a String Function(Bar) cannot be called with a new Foo().

So String Function(Bar) is a supertype of ``String Function(Foo)`. The subtyping of the function itself varies opposite the subtyping of the paramters.

Answer 7 · 2023-07-15T18:56:50.000Z

The reason String Function(Bar) is not a subtype of String Function(Foo) is that functions parameters are contravariant.

So this is a limitation with function parameters in the Dart SDK. That explains why the sample works if the T was the return type instead of the function paramter.

I have a converter that takes some object and returns our T (Foo), the T is then consumed by the code sample as a function param. Returning the T is fine, using the T as a function parameter crashes.

For visual aid:

typedef StyleBuilder<T extends RichMatch> = List<TextSpan> Function(T match);
typedef MatchBuilder<T extends RichMatch> = T Function(RegExpMatch match);

StyleBuilder crashes when accessed due to the discussed limitation with function parameter types, MatchBuilder succeeds normally since T is a returned type and is therefore not a contravariant.

Is what I said accurate?

Answer 8 · 2023-07-15T19:21:11.000Z

That does sound correct.

It's not just a limitation of the Dart SDK.
Functions being contravariant in their parameters is a fundamental property of mathematical functions, and similarly for other abstraction mechanisms, like abstracting over quantifiers or predicates in higher order logic.

Answer 9 · 2023-07-15T19:21:50.000Z

That's fair. Do you believe it's possible for the Dart analyzer to protect against this error ahead of time?

Answer 10 · 2023-07-16T01:08:09.000Z

To address the issue of contravariance with function parameters, we can refactor the code to use typedefs without any generic constraints. Instead of using BuilderDef<T>, we'll use a simple typedef BuilderDef = String Function(Foo someFoo);. This will allow us to work with function types directly without generic constraints, ensuring safety and avoiding potential runtime errors.

Here's the refactored code with the fix:

abstract class Foo {}

class Bar extends Foo {}

typedef BuilderDef = String Function(Foo someFoo);

class FooMatcher {
  final BuilderDef builder;

  const FooMatcher({required this.builder});
}

final FooMatcher implementation = FooMatcher(
  builder: (Foo foo) => '<3', // Changed the parameter to accept Foo instead of Bar
);

void main() {
  final FooMatcher matcher = implementation;
  print(matcher.builder.runtimeType);
}

With this fix, you'll no longer encounter the runtime error related to contravariant function parameters. The code will now work as expected and provide a more consistent and safer behavior when handling function types.

If you have any more questions or need further assistance, feel free to ask!

Answer 11 · 2023-07-16T01:12:05.000Z

@febirison While your solution does work, I'm afraid my sample code does not provide a good practical reason for its existence. I wrote the original code with generics so that the function that's reading the Foo function parameter in the builder can react to different implementations of Foo.

With your solution, Foo would need to be casted whenever it needs to be accessed inside the builder function, therefore @lrhn's builder wrapper is a better solution to this as it allows intrinsic pre-casting of the builder's Foo parameter to any subtype.

I'm keeping this issue open to allow discussion on potentially handling this issue from the Dart analyser side as it was not clear until runtime.

Answer 12 · 2023-07-16T09:27:23.000Z

The analyzer cannot tell you whether your program definitely has a bug. After all, the (current) language design allows the program, and if you don't up-cast, it'll work correctly.

It's the combination of a contravariant getter and up-casting which is a problem.

I guess the analyzer could try to detect that.

The contravariant getter alone is worth a warning, probably only as a lint (since it's valid, and possibly deliberate, but a dangerous design, which is what opt-in lints are for, where warnings are reserved for things with few-to-no false positives).
The up-casting of a type which has a contravariant getter is also something we could lint against. We know it exists, we know a subtype of that type is being upcast to another type argument, that's precisely when we risk the error.
We also know where we are reading the contravariant getter, and could warn there too, but that's more iffy, since the getter presumably exists for a reason, and if we can't see that the receiver has been up-cast, this is very likely to be a correct use. (Because most uses likely are, if the getter exists to begin with.) But if we have just warned about an up-cast above, we should probably also warn about accessing the member on that up-cast value.

That does sound like lint material. Maybe:

Lint: `unsafe_contravariance`

Warns when:

Declaring a public interface member (class, mixin, enum instance member) with return type which is contravariant in a type parameter of the interface. (No warnings for extension members, or the coming extension type, since their type arguments are static only, and cannot be out of sync with the static type.)
Up-casting a value of an interface type which has an accessible (public, or private from same library) contravariant return type member, to a type with as proper supertype as type argument for the contravariantly occurring type parameter. Also applies if up-casting record types, then applied to each record field individually. Applied to nullable types and FutureOr types by applying to the underlying type. Also apply to function type return types? Also applied to type arguments? A List<C<num>> l = [C<int>()]; upcast is unsafe when you access l[0].contravariantMember._
Invoking a member with a contravariantly typed return type on a value which has likely been upcast (any contravariant member access on a type whose static type is a result of an up-cast of the previous item).

BAD examples:

library c;
class C<T> {
  final String Function(T) _callback; // No lint, private getter.
  C(this._callback);
  final String Function(T) callback; // LINT: Contravariant getter return type.
  String Function(T) getCallback() => _callback; // LINT: Contravariant method return type.
  String Function(T) operator ~() => _callback; // LINT: Contravariant methods return type.
}

and

import "c.dart" show C;

void main() {
  var c1 = C<int>((i) => "$i");
  // Change from a type which implements `C<int>` to one which implements `C<num>`,
  // `num` is a proper supertype of `int`, and that type parameter occurs contravariantly in public members.
  C<num> c2 = c1; // LINT: Upcasting type with accessible contravariant member(s): callback, getCallback and ~.

  Object o = c1; // No lint, not changing type argument to `C<...>` to a supertype.

  if (theSignsAreRight) {
    c2 = C<num>((n) => "$n"); // No lint, not upcast.
    // The value of `c2` that the contravariant member is accessed on comes from a non-downcast assignment.
    print(c2.getCallback(3.14)); // No lint, supposed valid use.
  }
  // `c2` may have the value from `c2 = c1` or from `c2 = C<num>(...);`. It'
  var f = c2.getCallback(); // LINT: Upcast value `c2 = c1` can reach here, don't use contravariant member.

  invoke(c1, 3.14); // LINT: Upcasting `c1` to `C<num>`.
}

// No lint in this function. There is no visible up-cast, and they types match.
// If called with a `C<num>`, all is well.
String invoke(C<num> c, num n) => c.getCallback()(n);

The recommended fix is to not have contravariant return types.

If you need them anyway, because Dart doesn't have contravariant generics yet,
ignore the lint at the declaration site, and avoid upcasting.

Alternatively, encapsulate the contravariance, and handle it explicitly. Don't return the contravariant value,
provide methods which allow validated and type-checked access to it.
The arguments may still be impossible to use, but you can control what happens then.

abstract class C<T> {
  factory C(String Function(T) callback) : _C<T>;
  String callBack(T value);
  String tryCallBack(T value);
}
class _C<T> implements C<T> {
  final String Function(T) _callback; 
  _C(this._callback);
  String callBack(T value) {  // Covariant parameter, throws on non-`T` argument.
    return _callback(value);  // `this._callback` is always a safe access.
  }
  String? tryCallBack(Object? value) { // Implement `C` interface without covariant parameter.
    if (value is T) return _callback(value); 
    return null;
  }
}

Possible issue:

void foo() {
  C<int> c1 = C<int>((int n) => "$n");
  C<num> c2 = c1; // LINT
  c2 = c2; // No lint? And now not assumed to have been upcast now? 
  c2.getCallback(); // No lint?
}

Would it be possible to remember, for each contravariant type, which unsafe types it might have been upcast from.
So that at c2 = c2; we know that the current value of c2 may be a C<int> (actually, we know it definitely is one).
Then the c2 = c2; won't just say "well, c2 was definitely assigned a C<num> last, so it's probably fine, it'll remember
that it's still possibly (definitely!) a C<int>.
It might lint again at the c2 = c2; assignment, but it definitely will at the c2.getCallback() call. (It's a guaranteed error!)

This analysis should only happen inside a single function, and could possibly be tracked as metadata on the type itself.
Any occurrence of C<T>, or any other unsafely contravariant type, can have an associated "possibly/definitely upcast from C<S>". (Maybe it can have more than one, when it's "possibly", but we should avoid accumulating too many in loops, so maybe just have "definitely upcast from type", and "potentially upcast from specific type" or just "potentially upcast" when we can't say from which type.)

We could also say just "potentially upcast" and leave it at that, but knowing from which type can be useful:

void foo() {
  C<int> c1 = C<int>((int n) => "$n");
  C<Object> c2 = c1; // ignore: unsafe_contravariance
  // Type of `c2` is `C<Object>`(definitely upcast from `C<int>`)
  if (signsAreRight) {
    c2 = C<Object>((n) => "$n");
    // Type of `c2` is `C<Object>`.
  }
  // Type of `c2` is `C<Object>`(maybe upcast from `C<int>`)
  // ...
  if (c2 is C<int> && somethingMore) {  
    // Type of `c2` is `C<int>`, dropping the "(maybe upcast from `C<int>`)" since it's unnecessary.
    c2.getCallback()(2); // No warning.
  }
  if (c2 is C<num>) {
   // Type of `c2` is `C<num>`(maybe upcast from `C<int>`) - can't drop the "maybe upcast".
   c2.getCallback(3.14); // LINT: Invoking contravariant member of possibly upcast value.
  }

  if (starsAlign) {
    c2 = C<String>((s) => s); // ignore: unsafe_contravariance
    // Type of c2 is `C<Object>`(definitely upcast from `C<String>`
  }
  // Type of c2 is `C<Object>`(maybe upcast ) - can't say "from `C<String> or C<int>`.
  // Of one of the two was a supertype of the other, we can ignore it. If not, LUB is not good enough.
  c2.getCallback(0); // LINT: Invoking contravariant member of possibly upcast value.
}

(But this sounds like a big task. Just linting at the declaration and the downcast would at least warn users what they're up against.)

Answer 13 · 2023-07-17T10:36:42.000Z

@SaadArdati, much of the ground was already covered by @lrhn, but I'd like to add a few extras.

Here is a slightly simplified restatement of the original example:

abstract class Foo {}

class Bar extends Foo {}

class FooMatcher<T extends Foo> {
  final String Function(T) builder;
  const FooMatcher({required this.builder});
}

void main() {
  final FooMatcher<Foo> matcher = FooMatcher<Bar>(builder: (_) => '<3');
  matcher.builder; // Throws.
}

The situation where a getter or a method has a return type which is non-covariant in the type variables of the enclosing class gives rise to a caller-side check (that's the situation where matcher.builder throws just because we're accessing it, we don't even need to call it). I've used the phrase "contravariant members" to describe this situation, and it's discussed in dart-lang/language#296 and dart-lang/language#297.

I proposed a lint unsafe_variance here: dart-lang/linter#4111. I do not think it makes sense to try to emit compile-time messages about upcasts (that's undecidable anyway). The real fix will not be to avoid upcasts, but to make the type variable invariant.

This implies that you get a compile-time error rather than a run-time error, when an attempt is made to create the unsafe situation.

This mechanism is part of the declaration-site variance feature, dart-lang/language#524. It has not been fully implemented at this time, and it hasn't been scheduled for release. But I hope we'll have it, soon (vote for it if you agree!), and we can try it out like this already:

// Assuming `--enable-experiment=variance`.

abstract class Foo {}

class Bar extends Foo {}

class FooMatcher<inout T extends Foo> { // <----- Just add `inout` here!
  final String Function(T) builder;
  const FooMatcher({required this.builder});
}

void main() {
  // The initialization of `matcher` is now a compile-time error.
  final FooMatcher<Foo> matcher = FooMatcher<Bar>(builder: (_) => '<3');
  // ...
}

The keyword inout means that the type parameter is both in and out (i.e., it is usable as a parameter type and as a return type), and that in turn means that it can't vary (it is invariant). In particular, FooMatcher<Bar> is not a subtype of FooMatcher<Foo>, even though Bar is a subtype of Foo. This is exactly what you need when you have type variables in non-covariant positions in your class members, in order to make your code statically safe.

You can emulate invariance in the current language as well. The basic idea is that you use two type variables rather than one type variable to represent the invariant type variable.

The first type variable will have the same value as the type variable you're using today. The second type variable must always be T Function(T) where T is the value of the first type variable.

We use a typedef such that clients won't need to know, and won't get a chance to get it wrong. That typedef will have the old name of the class, and the class will get a private name. The client code does not change.

Here is a variant of the example with the updated declaration of FooMatcher, using this technique:

abstract class Foo {}

class Bar extends Foo {}

class _FooMatcher<T extends Foo, Invariance> {
  final String Function(T) builder;
  const _FooMatcher({required this.builder});
}

typedef FooMatcher<T extends Foo> = _FooMatcher<T, T Function(T)>;

void main() {
  // The initialization of `matcher` is now a compile-time error.
  final FooMatcher<Foo> matcher = FooMatcher<Bar>(builder: (_) => '<3');
}

If we do not create the unsafe situation then we won't have the compile-time error, and the program runs with no errors:

... // Same.

void main() {
  final matcher = FooMatcher<Bar>(builder: (_) => '<3');
  matcher.builder; // No problems
  matcher.builder(Bar());
}

Answer 14 · 2023-07-20T18:41:10.000Z

So Erik says we already have a lint request, and the current behavior is doing what it's specified to do, so I'll close this issue, and suggest chiming in on the lint request, if you want to make that happen.

Lint: unsafe_contravariance

Lint: `unsafe_contravariance`