dart-lang / language

Design of the Dart language
Other
2.67k stars 205 forks source link

Don't implictly use the Never type as default generic type parameter #2336

Open pixelshot91 opened 2 years ago

pixelshot91 commented 2 years ago

For generic type, if the type is not explicitly given, Dart assume the type to be Never. It can cause runtime failure, and I don't know any way of being noticed of this implicit behavior.

Consider the following example, which compile and do not produce any warning or lint. I try to create an Updatable class which can hold some value or none, and update itself with new data coming from a server.

import 'package:freezed_annotation/freezed_annotation.dart';

part 'never_example.freezed.dart';

class MyClass<T> {
  MyClass({this.myUpdatable = const Updatable.notDownloaded()});
  final Updatable<T> myUpdatable;
}

@freezed
class Updatable<T> with _$Updatable<T> {
  const factory Updatable.notDownloaded() = _UpdatableNotDownloaded<T>;
  const factory Updatable.downloaded({
    required DateTime updatedAt,
    required T value,
  }) = UpdatableDownloaded<T>;

  const Updatable._();

  Updatable<T> mergeFromServer(Updatable<T> newUpdatable) => map(
        notDownloaded: (_) => newUpdatable,
        downloaded: (oldDownloaded) =>
            newUpdatable.map(notDownloaded: (_) => oldDownloaded, downloaded: (newDownloaded) => newDownloaded),
      );
}

void main() {
  final c = MyClass<int>();
  print('${c.myUpdatable.runtimeType}');
  // I get '_$_UpdatableNotDownloaded<Never>'
  // I would like to have '_$_UpdatableNotDownloaded<int>'

  c.myUpdatable.mergeFromServer(Updatable.downloaded(updatedAt: DateTime.now(), value: 42));
  /*
  Unhandled exception:
  type '_$UpdatableDownloaded<int>' is not a subtype of type 'Updatable<Never>' of 'newUpdatable'
  #0      Updatable.mergeFromServer (package:freezed_test/never_example.dart)
  #1      main (package:freezed_test/never_example.dart:11:17)
  #2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
  #3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)
   */
}

This is because Dart does not know the type parameter of my default value Updatable.notDownloaded() So it use Updatable<Never>.notDownloaded() which works because Never is a bottom type.

But when I try to update myUpdatable, the function mergeFromServer expect a Updatable<Never> as argument but find a Updatable<int> so it crashes.

I would like to (in order of preference):

I already have strict analysis rules:

analyzer:
  language:
    strict-casts: true
    strict-inference: true
    strict-raw-types: true

Do you have any idea ? Is my design bad ? Is the language to permissive ?

eernstg commented 2 years ago

Could you make the same point using a self-contained example?

eernstg commented 2 years ago

OK, the issue seems to be that you're using a formal parameter default value to obtain an object using const _UpdatableNotDownloaded() in the constructor of MyClass.

A default value must be a constant expression, and this means that it must yield one particular object, and that object must be an appropriate choice in all situations. In particular, its type must be a subtype of Updatable<T> for all T. The only actual type argument that satisfies this requirement is Never. So that's the reason why you get that type argument.

Here is one alternative approach where the type argument is T:

class MyClass<T> {
  MyClass({Updatable<T>? myUpdatable})
      : myUpdatable = myUpdatable ?? Updatable<T>.notDownloaded();
  final Updatable<T> myUpdatable;
}
pixelshot91 commented 2 years ago

Thank you for your quick answer ! Yes I could do what you propose. But my primary complaint is that I don't have any compiler warning about a possible runtime crash. If I do something wrong, I would like to have a compile-time error/warning or a least a lint. Is it feasible in Dart ? Is there any option to enable more strict type check to detect this problem at compile-time ?

eernstg commented 2 years ago

The dynamic error occurs because the type parameter of Updatable is dynamically checked covariant, that is: Updatable<S> is a subtype of Updatable<T> whenever S is a subtype of T, and in the cases where this does not guarantee that any given operation will succeed, a dynamic check is performed.

I don't know how mergeFromServer and various other methods are declared, but there is a standard example using List:

void main() {
  List<num> xs = <int>[]; // Uses covariance.
  xs.add(1.5); // Throws, because `add` requires an `int`.
}

This is exactly the same trade-off that Java and C# have made for many years with arrays, but in Dart it is a general rule. It works surprisingly well, in the sense that there are few complaints about such run time errors.

However, I'd very much like to have a statically checked version as well, cf. #524 which is a proposal to add statically checked declaration-site variance to Dart.

With that feature in place you could declare it as class Updatable<inout T> ... This ensures that the dynamic error you've seen cannot occur at run time, but in return you'll have to write your code in such a way that it does not use the degree of freedom that we've just removed.

ds84182 commented 2 years ago

In this case you could also workaround using an extension on Updatable. An extension will use the static type of T, instead of the runtime type:

class MyClass<T> {
  MyClass({this.myUpdatable = const Updatable.notDownloaded()});
  final Updatable<T> myUpdatable;
}

abstract class Updatable<T> with _$Updatable<T> {
  const factory Updatable.notDownloaded() = _UpdatableNotDownloaded;
  const factory Updatable.downloaded({
    required DateTime updatedAt,
    required T value,
  }) = UpdatableDownloaded<T>;

  const Updatable._();
}

// New code here:
extension<T> on Updatable<T> {
  UpdatableDownloaded<T>? get orNull {
    final v = this;
    return v is UpdatableDownloaded<T> ? v : null;
  }

  Updatable<T> mergeFromServer(Updatable<T> newUpdate) =>
      newUpdate.orNull ?? this;
}

void main() {
  final c = MyClass<int>();
  print('${c.myUpdatable.runtimeType}');
  // I get '_$_UpdatableNotDownloaded<Never>'
  // I would like to have '_$_UpdatableNotDownloaded<int>'

  c.myUpdatable.mergeFromServer(
      Updatable.downloaded(updatedAt: DateTime.now(), value: 42));
}

// Code that would probably be generated by freezed:

mixin _$Updatable<T> {
  V map<V>({
    required V Function(_UpdatableNotDownloaded) notDownloaded,
    required V Function(UpdatableDownloaded<T>) downloaded,
  });
}

class _UpdatableNotDownloaded extends Updatable<Never> {
  const _UpdatableNotDownloaded() : super._();

  @override
  V map<V>({
    required V Function(_UpdatableNotDownloaded) notDownloaded,
    required V Function(UpdatableDownloaded<Never>) downloaded,
  }) =>
      notDownloaded(this);
}

class UpdatableDownloaded<T> extends Updatable<T> {
  final DateTime updatedAt;
  final T value;

  const UpdatableDownloaded({
    required this.updatedAt,
    required this.value,
  }) : super._();

  @override
  V map<V>({
    required V Function(_UpdatableNotDownloaded) notDownloaded,
    required V Function(UpdatableDownloaded<T>) downloaded,
  }) =>
      downloaded(this);
}