dart-lang / language

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

An unexpected type error caused by legal code that uses covariance, type inference. #1619

Open timnew opened 3 years ago

timnew commented 3 years ago

The issue is best explained with code, here is a minimal reproduction of the issue.

What is the the output of the following code?

abstract class Stated<T> {
 const Stated._();

  factory Stated.idle() => const _Idle();
  const factory Stated.value(T value) = _Value;

  Type get valueType => T;
}

class _Idle<T> extends Stated<T>{
  const _Idle(): super._();
}

class _Value<T> extends Stated<T>{
  final T value;
  const _Value(this.value): super._();
}

void main(){
  final Stated<String> stated = Stated.idle();

  print(stated.valueType);
}

Without going through every the details, I would assume majority would say String, but the answer is wrong, the actual output of the code is Never. You can try it out here: https://dartpad.dev/c52700c7b4543d216ad1f1324a96ef3e?null_safety=true

The unexpected outcome is a result produced by the join-force from generic covariance, constant constructor and type inference:

  1. Stated.idle as a factory method of Stated<T>, so it should returns an instance assignable to Stated<T>.
  2. const _Idle() requires the created instance is a constant, same instance would be returned on every call.
  3. Explicit type parameter is not allowed for const _Idle(), so T of _Idle<T> is inferred by compiler.

With all 3 constraints, the actual instance created by Stated.idle() is _Idle<Never>. And by definition, Never is assignable to any type. So _Idle<Never> is a legal Stated<T>.

Everything is fine, until T of the instance is actually being used.

leafpetersen commented 3 years ago

@timnew Your description of the reasons that Never is fairly accurate though this isn't quite right:

Explicit type parameter is not allowed for const _Idle(), so T of _Idle is inferred by compiler.

An explicit type argument is allowed. However, T is not a valid const type, so _Idle<T> is not legal. The type argument itself must be const. In Dart 1, dynamic was used as the type argument, and dynamic was both top and bottom, so it worked. In Dart 2, we chose to infer Never (the bottom type) in covariant positions instead. So inference infers _Idle<Never>. You can choose to write this explicitly if you prefer.

So that's what's going on . I'm not sure what the specific request you have here is?

timnew commented 3 years ago

@leafpetersen Thanks for the quick response and and detailed explanation.

In your reply, would you mind to go into a bit detail on this topic

You can choose to write this explicitly if you prefer.

I actually tried to explicitly specify _Idle<dynamic> when I identified the issue in my code, and dynamic would potentially solves my issue. But the compiler actually complains on dynamic too, and I can reproduce this issue on dart pad too:

image

So that's what's going on . I'm not sure what the specific request you have here is?

Sorry for not explicitly expressing the expectation, as I'm not quite sure what to expect. The outcome itself is completely correct, but it also could be very unexpected to developer. And the result could be damaging and tricky to diagnose. And I felt this issue is kind of defeated the purpose of having a strong type system.

Let me use a more close-to-reality example to explain it:

abstract class Stated<T> {
 const Stated._();

  factory Stated.idle() => const _Idle<Never>();
  const factory Stated.value(T value) = _Value;

  // Spread T to another type, which could pass on for a couple of layers
  TypeChecker<T> typeChecker() => TypeChecker<T>(); 

  // Constraint input type by using T, which could cause runtime type error
  void someMethod(Future<T> future) async {
    print(await future);
  }
}

class _Idle<T> extends Stated<T>{
  const _Idle(): super._();
}

class _Value<T> extends Stated<T>{
  final T value;
  const _Value(this.value): super._();
}

class TypeChecker<T> { 
  TypeChecker();

  bool checkType(Object another) => another.runtimeType == T;
}

// Well constrained type, code should work 
void doSomething<T>(Stated<T> stated, Future<T> future){
  stated.someMethod(future);
}

void main(){
  late Stated<String> stated;

  void runTest(){
    print(stated.typeChecker().checkType("Not always true"));

    doSomething(stated, Future.value("not always works"));
  }

  stated = Stated.value("value");
  runTest();  
  stated = Stated.idle();  
  runTest();
}

interactive code here: https://dartpad.dev/3863274c4ffcebe5118cd53eefebac0f

In the previous example, you can see 3 things:

The idea to have a strong typed system, or even null-safety is to avoid the runtime type error, or when it happens, capture it onsite. But this issue actually opened a loophole on the type system, so even with completely legal code, we still could have runtime type issue without involving commonly known risky code such as type cast or null. And what's worse is it could spread across the code and and data dependant.

I know this is a tricky issue, and I'm not a compiler or language expert, so I'm not sure how to solve this issue from compiler/language design point of view. But I assume at least we could discourage the implicit type inference in some cases, so it would remind developer that Never or other magic types such as dynamic void is actually used.

  factory Stated.idle() => const _Idle(); // Generate compiler warning or linter error/warning.
  factory Stated.idle() => const _Idle<Never>(); // Recommended.

Hope this explain the issue better

lrhn commented 3 years ago

I'm all for a lint giving warnings whenever dynamic is implicitly introduced (I think "no-implicit-dynamic" is an analyzer flag instead of a lint now), and it might be worth expanding that to implicit Never in this case. It's surprising, yet unavoidable (it's the only valid type argument in that position), so you might want to get warned when it happens.

eernstg commented 3 years ago

Hope this explain the issue better

Your classes are actually soundly covariant, and this means that a Stated<S> is perfectly usable in the situation where the static type is Stated<T> for some T such that S <: T. In other words, an _Idle<Never> will work where a Stated<T> is expected, for any T.

There is a subtle exception in that valueType uses the type argument T as an expression, and this means that it is actually possible to make a distinction between two different values for T, so in that sense it does matter that the type argument turns out to be Never. However, even if it is a surprise that print(stated.valueType) prints 'Never', it is not a soundness issue.

So the following variant of your first example is actually more informative:

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

abstract class Stated<inout T> {
 const Stated._();

  factory Stated.idle() => _Idle(); // `const _Idle()` is an error.
  const factory Stated.value(T value) = _Value;

  Type get valueType => T;
  void useTContravariantly(T t) {}
}

class _Idle<inout T> extends Stated<T> {
  const _Idle(): super._();
}

class _Value<inout T> extends Stated<T> {
  final T value;
  const _Value(this.value): super._();
}

void main() {
  final Stated<String> stated = Stated.idle();

  print(stated.valueType);
  stated.useTContravariantly('A string!');
}

This variant of the program uses T in a contravariant position in a member signature (in addition to the covariant usage in the declaration of value that we had already). It also uses the experimental feature sound declaration site variance. With sound variance, we can make different choices, but the obvious one is to make all three classes invariant in their type argument. If you do that then it becomes a compile-time error to return const _Idle() from Stated.idle, and the error message tells you that const _Idle() has type _Idle<Never>.

So the invocation of useTContravariantly succeeds. But if we remove the use of sound variance (delete inout) then const _Idle() is accepted, and the program fails at run time.

In summary, I believe it's fair to say that the original example did not really cause any problems, but the variant where T is actually used in a contravariant position illustrates that const and inference can conspire to cause a dynamic type error.

So the actual point is that you want sound variance. I hope it won't take too long before we get that.

Levi-Lesches commented 3 years ago

I believe the issue can be described with much shorter code:

class Foo<T> {
  const Foo();

  Type get typeName => T;
  void printValue(T value) => print(value);
}

/// In order to return a `Foo<T>`, we should not call `const`
Foo<T> getFoo<T>() => const Foo();

void main() {
  final Foo<String> foo = getFoo();  // getFoo<String> is inferred
  print(foo.typeName);  // Never
  foo.printValue("Hello, World!");  // TypeError: type 'String' is not a subtype of type 'Never'
}

How about making this an error? Specifically, using const guarantees that Foo will never be what the caller (getFoo) specifies in its return type, so getFoo<String> does not return a Foo<String>, it always returns Foo<Never>. I can't imagine a scenario where it would be wrong to make this an error. For instance, it breaks even the simple Foo.printValue

By definition, Never is assignable to any type.

Why exactly is this true? If it weren't it would make getFoo an error, catching the problem at the source.

eernstg commented 3 years ago

How about making this an error?

That is exactly what sound variance would do, if it is an error (in terms of soundness).

timnew commented 3 years ago

@eernstg Thanks for the detailed explanation, it makes a lot sense now. After read through the issue you mentioned ( a few more linked by that issue) is really helpful. And I strongly agree with you on the sound variance would be mandatory to type safe. As I found it is quite strange and could be annoying that still having unexpected runtime type error in a null-safe strong type language.

It is quite excited to know the feature has been available as experimental one, and hope it could be get released soon.

Levi-Lesches commented 3 years ago

@eernstg got it, seems like a good feature to have.

Out of curiosity, does the compiler currently see that getFoo returns Foo<Never>, or is that only known in runtime? If it's compile-time, why does the compiler currently allow it, given that Foo<Never> is not a Foo<String>?

timnew commented 3 years ago

given that Foo is not a Foo?

Per my understanding, Foo<Never> is actually a legal Foo<String> under the covariance rule. As Never means the instance would never exists, as you can't instantiate a Never. So in practise, T foo = returns<Never>() would never happen. So Never can be assigned to any type without causing runtime type error, as it would not actually happening, bu it makes the code more fluent.

For example:

T ensureNotNull<T>(T? nullable) => nullable ?? throwNullException();

Never throwNullException() => throw ArugmentError.notNull();
Levi-Lesches commented 3 years ago

But if Never always causes a runtime error, shouldn't it cause a static error? It feels wrong for something to be allowed statically even though the compiler can guarantee a runtime error.

In other words, is there ever a valid reason for Never to be assigned?

eernstg commented 3 years ago

@Levi-Lesches wrote:

does the compiler currently see that getFoo returns Foo<Never>

No. The Dart static analysis will consider any function with declared return type T to return a T, even in the case where it would in fact be very easy for a static analysis to prove that the returned object actually has some other type S (S is then a subtype of T, or dynamic, because otherwise we'd have a compile-time error). That is the only reasonable thing to do, anyway. For instance:

class A { num get g => 1; } // Really returning an `int`.
class B implements A { num get g => 1.5; }

We wouldn't want the class B to be a compile-time error, just because the compiler/analyzer has taken a look at the implementation of A.g and concluded that it returns an int, hurrah!, so the compiler changes the return type of A.g to int, and then it's a compile-time error to return a double from B.g.

So if the developer declares that a function f returns a T then the compiler/analyzer had better trust the developer and also treat f as a function that returns a T, even in the case where it "knows better" based on the implementation of f.

On the other hand, the compiler/analyzer must of course verify that a function f that returns a T will actually return an object whose run-time type is a subtype of T, and that's based on a compile-time guarantee for every type except dynamic, and on a dynamic check if we're returning an expression of type dynamic.

why does the compiler currently allow it, given that Foo<Never> is not a Foo<String>?

We can return a Foo<Never> in a function whose return type is Foo<String> because Foo<Never> <: Foo<String>, that is, a Foo<Never> is a Foo<String>, because the type parameter of Foo is covariant.

This is exactly the same property that ensures that if an object o is a List<int> then o is also a List<num>, and a List<Object>, etc.

However, in the case where we're using covariance for the type parameter of a class like List (where some members use the type parameter in a contravariant location), we can only maintain soundness if the compiler generates some dynamic type checks at specific locations (for instance, the type of the argument to List.add is checked dynamically).

Sound variance allows us to have a different trade-off: We make it a compile-time error to use a covariant type parameter in a contravariant location. One way out is then to make the type parameter invariant, which is what I did here.

is there ever a valid reason for Never to be assigned?

It is certainly possible to use objects whose type includes Never. For instance, <Never>[] is a perfectly meaningful empty list, and it's allowed to occur in any context where a List<T> is required, for any T whatsoever. Also, you can use it without ever encountering any exceptions. The fact that we can create a const _Idle<Never>() is just another example of the same thing.

The connection between Never and run-time failures only arises for expressions whose type is a bare Never (so Never is the type, it is not just a type argument of some other type). For instance, the following program won't terminate, but it is type correct:

Never f() { 
  while (true);
}

void main() => f();
Levi-Lesches commented 3 years ago

Thanks for the long explanation

We can return a Foo<Never> in a function whose return type is Foo<String> because Foo<Never> <: Foo<String>

I guess what I'm asking is, since I couldn't find any documentation on Never (only found out about it in this issue), why is it true that Never <: T for most if not all T?

EDIT: I found some docs for the Never type and when to use it, but no talk about why it's considered a subtype of every type.

lrhn commented 3 years ago

The Never type is defined to be a subtype of all types.

The type system generally needed a bottom type. It's good to have for a number of reasons, similar to why it's good to have a top type which allows you to range over all objects. The bottom type allows us to write the "type of all unary functions", Object? Function(Never). All unary functions are assignable to that type. Because parameters are contravariant, you need a bottom type to generalize over all function parameters. It also allows us to create, as in this example, a const Foo<Never>() which is assignable to all Foo<X> types, even Foo<T> where T is a type parameter. Without the bottom type, the only subtype of T is T itself, and it's not constant.

So, a bottom type is useful, and we wanted to have one.

We used to have Null as the bottom type, but with null safety, Null was no longer a subtype of any non-nullable type, so we had to introduce a new one. That's Never. The Never type should satisfy all interfaces, which is clearly impossible in practice, so therefore the Never type is empty. A function returning Never is guaranteed to not return any value, because any value would have a type which is a proper supertype of Never, and returning it as Never would not be sound. The function can still throw or fail to return.

Levi-Lesches commented 3 years ago

A function returning Never is guaranteed to not return any value

Oh, so the real error here isn't that Foo<Never> is returned as a Foo<String>, it's that Foo<Never> is returned at all. But it's not caught because that type of analysis is not done when Never is used a type argument, and sound variance will fix that. Sounds good, thanks.

eernstg commented 3 years ago

Oh, so the real error here isn't that Foo<Never> is returned as a Foo<String>, it's that Foo<Never> is returned at all.

Nope, there is no reason to assume that there is a problem with an object of type Foo<Never> just because it has a type argument whose value is Never. Even though there is no object whose run-time type is Never, we can certainly have a generic class with a type argument which is never used, and that's already enough to ensure that there is no necessary connection between having an object with a type argument whose value is Never (no problem) and having an object of type Never (which is impossible).

In particular, it is not a problem to return an object of type _Idle<Never> in the given example (without useTContravariantly), just like it's not a problem to use the list <Never>[]. That list must be empty (because any elements in the list must have type Never), but as long as it is actually empty there is no problem.

The dynamic type errors associated with covariance may arise with no reference to Never (or any other specific type which is somehow dangerous in itself), you just need non-trivial subtyping among types that are otherwise perfectly unremarkable:

void main() {
  List<num> xs = <int>[1, 2, 3]; // `xs` can be a `List<int>` by covariance.
  xs.add(3.1); // Throws at run time.
}

If we were to use sound declaration-site variance to make List invariant in its type argument then it would be a compile-time error for xs to be initialized to <int>[...], and then the dynamic error wouldn't occur. So that's the kind of dynamic failure which could be eliminated by means of sound variance.

(OK, we won't make List invariant, because that's far too breaking, but it illustrates how it would work. ;-)

Levi-Lesches commented 3 years ago

Nope, there is no reason to assume that there is a problem with an object of type Foo<Never> just because it has a type argument whose value is Never

RIght, I just meant that in this case, where (in my example) printValue threw an error when it wasn't expected. But I get how technically it was valid.