Skip to content

Analyzer doesn't catch type errors and it's throwing errors in DDC runtime in the browser. #33447

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

Closed
zolotyh opened this issue Jun 14, 2018 · 11 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-as-intended Closed as the reported issue is expected behavior

Comments

@zolotyh
Copy link

zolotyh commented Jun 14, 2018

I just created a repo for the demo. Here is the code:

void main() {
  final cat = new Cat();
  final list = [cat];

  final newList = filter(list)
      ..add(new Animal())
      ..add(new BlackCat());

  print(newList);
}

List<Animal> filter(List<Animal> input){ return input; }

class Animal {}

class Cat implements Animal {}

class BlackCat implements Cat {}

PS: I know how to fix it. But it's very inconvenient to find such errors in runtime. It would be awesome if you tell me how to see such errors in IDE (maybe analysis options or etc).

PS1: Code is working on DartVM

Errors in browser

Dart version: Dart VM version: 2.0.0-dev.62.0 (Wed Jun 13 16:50:22 2018 +0200) on "macos_x64"
dartdevc version 2.0.0-dev.62.0

Thank you in advance

@eernstg eernstg added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-as-intended Closed as the reported issue is expected behavior labels Jun 14, 2018
@eernstg
Copy link
Member

eernstg commented Jun 14, 2018

It is well-known that the covariance rule for generic classes that Dart uses is not sound. The situation is the same as it has always been with arrays in Java and C#. It has been well-documented at least since this paper from 1989.

So covariant arrays weren't introduced into Java or C# by accident, it was a well known trade-off (albeit a controversial one). Actually, Dart extended the unsound covariance to all generic classes, not just arrays, because it is sound for reading (which accounts for a large proportion of the usages of objects after a certain point in their lifetime), and it is dynamically checked for writing (such that you may get a run-time type error, but you won't get an inconsistent heap).

So that's a very fundamental design choice for Dart: It has a type system that trades a certain amount of soundness for a "natural" subtype relationship which is sound for a substantial subset of the possible usages, with heap invariants enforced at run-time for the remaining ones.

You could also say that Dart generics uses wildcarded types everywhere (because you'll actually get List<? extends T> whenever you write List<T>), and then the operations that are prohibited in Java (like adding an element to a list) are supported in Dart, but checked implicitly at run time. Note that you can't make this trade-off in Java, because you cannot express the witness type of a wildcard (in Java, with List<? extends T> myTs;, you can't perform a cast like myList.add((S)myExpression) where the S in the cast somehow stands for the unknown type represented by the ?: there is no such S that you can write).

In particular, when you pass list through filter the static type will be List<Animal>, so you are allowed to add an Animal to it—even though this is not sound and you may get a type error at run-time. The list is a List<Cat> in the first place, because that's the inferred type of the initializing expression in the declaration of list, and that's the reason why it is accepted as an argument to filter (if it had been a List<Object>, say, then it would have been rejected at that point).

We have discussed introducing support for invariance (which could look like List<exactly Animal> myAnymals, which would not be able to refer to a List<Cat>), and that would make it an error to pass list to filter, and in return you would have a static guarantee that ..add(new Animal()) will succeed.

However, we have no concrete plans for when to introduce support for invariance (or contravariance for that matter), nor do we have a commitment to do it at all. #29777 may be used to push such things, but it is not universally considered to be an improvement, and when Dart was created it was actually an explicit goal to make it less complex for developers to handle than wildcards in Java.

So I'd suggest that you start thinking about Dart generics as a mechanism that shares the degree of unsoundness that you (presumably) already know from arrays in other object-oriented languages, and then you can use various idioms to contain the unsoundness, and otherwise enjoy the added flexibility. ;-)

To conclude, it is correct that the static analysis of your program is not sound (except that the actual type requirements are enforced at run time, so you won't get an inconsistent heap), but it is not something that happened by accident. So I'll close this issue with 'working as intended'.

PS: Thanks for creating a repo showing exactly what is going on, that is always a great help when a topic is being explored!

@eernstg eernstg closed this as completed Jun 14, 2018
@nickclmb
Copy link

Hi @eernstg
It is the ambiguous behavior in dartVM and dartdevc, isn't it?
Developers should expect that same code will work exactly the same.

You mentioned c#, and for same example it shows compile time error: https://dotnetfiddle.net/b5jjoo , so can we expect the same from dart analyzer? Because it really distracting to find such errors in runtime, because both analyzer and dartdevc compiler says there is no error.

Thanks in advance

@eernstg
Copy link
Member

eernstg commented Jun 14, 2018

It is the ambiguous behavior in dartVM and dartdevc, isn't it?

The execution of the example program must raise a dynamic error when an attempt is made to add a new Animal() to a List<Cat> in Dart 2. However, it is possible that the VM execution which is mentioned was performed using Dart 1 semantics (use dart --preview-dart-2 to ensure that you get Dart 2 semantics). Apart from that there is no ambiguity that I can see.

Developers won't get exactly the same behavior in every respect, e.g., because different hardware/configurations may have a different amounts of memory, etc. For int on the web there is an even bigger difference: We are using JavaScript numbers because anything else would be prohibitively expensive. But apart from these special corners where there is a robust pragmatic pressure for specific incompatibilities, developers should certainly be able to get the same semantics everywhere.

With respect to https://dotnetfiddle.net/b5jjoo, the error says that a List<Cat> is not acceptable as an actual argument for a formal parameter whose type is List<Animal>. This is the standard (and sound) invariant generics rule.

I mentioned that C# arrays are covariant, but C# generics in general do not support covariance. You can declare a particular formal type parameter as being covariant using out, e.g. IEnumerable<out T>, which means that C# supports declaration-site variance (as opposed to the use-site variance that Java supports), but when a C# class declares a given type parameter as out then it can only be used in certain ways (it cannot occur in contravariant positions, e.g., as parameter types for methods in the class).

So the List example in C# is not comparable to the List example in Dart, because the former is invariant and the latter is covariant.

I think the overall message here is that Dart uses covariant generics with dynamic checks to make this trade-off where programming is more flexible but less safe, just like those other languages always did with arrays. It's not an accident, and it's not sound, and lots of people jump to the conclusion that it's just wrong, but you may still get to like it in practice. ;-)

@zolotyh
Copy link
Author

zolotyh commented Jun 14, 2018

@eernstg

The execution of the example program must raise a dynamic error when an attempt is made to add a new Animal() to a List in Dart 2

Do we have a theoretical possibility to track such errors statically? (analyzer plugin or something else). We trying to migrate a huge application from dart 1.24.3 to dart 2.0 + DDC. And it's really hard to find errors in runtime on working application.

@eernstg
Copy link
Member

eernstg commented Jun 14, 2018

Do we have a theoretical possibility to track such errors statically?

Sure, it is not difficult to come up with a sound set of rules (e.g., Java or C# generics are both sound). One way to do that is to say that C<A> and C<B> are simply unrelated types even when A is a subtype of B (this is invariance), another one is to say that you can only use covariant type variables in certain locations (like Scala, C#), or the compiler will prevent usage of members whose signature contains a type variable which is not in a covariant position when the receiver has a type where that type variable is covariant (as in Java).

However, it is an undecidable problem to track anything like the dynamic flow of objects, so as soon as we do allow (and use) covariance as in Dart (that is, as soon as we have an upcast of the form List<B> <: List<A> where B <: A anywhere in the program, or anything like that based on some other generic class than List), we cannot hope to statically determine where any particular expression whose type is a generic class will or will not yield an instance with a type which uses covariance.

But migration from Dart 1 to Dart 2 should not be affected by this kind of error because both Dart 1 and Dart 2 use covariant generics.

However, there is one other issue to consider: In Dart 1 the type dynamic is capable of being both a top type (so T <: dynamic for all T) and a bottom type (so dynamic <: T for all T as well), which means that subtyping is not transitive (which means that you have to be really careful with various definitions). So if you are relying on passing around objects whose dynamic type is a generic class where some type arguments are dynamic, the transition from Dart 1 to Dart 2 can be a little more difficult: A List<dynamic> is no longer able to masquerade as a List<int>, so you need to create "the correct type of list" in the first place, or you need to recreate the list as needed (e.g., List.cast<T>).

It can be tricky to find and fix all those situations where an object has a run-time type that contains dynamic, but a large subset of those situations are eliminated in Dart 2 because it uses type inference to provide more specific type arguments (e.g., List<int> xs = []; means List<int> xs = <int>[];).

If you have been using --strong mode and/or running programs using DDC during the past year or so, you should already have most of it in place, because you will then have experienced the stricter rules of Dart 2 (or at least something very similar) for a while.

@eernstg
Copy link
Member

eernstg commented Jun 14, 2018

One more comment about covariant generics—I think there is a general software engineering structure that tends to help managing covariance such that you don't get run-time errors in practice:

Many complex objects are created by an "owner" and populated during an initialization phase, and during the rest of their lifetime they are used in client code in an exclusively (or at least predominantly) read-only manner. This means that it is very easy for the class that implements the owner to maintain precise information about the actual type arguments (so we have invariance in practice, though without compiler enforcement), e.g., by using declarations like final List<T> xs = <T>[];. The field xs won't ever refer to an instance of List<S> for any S which is different from T, so the owner of that field can safely fill it in with values of type T. For clients it may be accessed as a List<dynamic> or List<T> etc, but they will only read entries out of the list, and that's a safe case for covariance.

So if you can get used to a functional style (in the sense that objects are created during an initialization phase and henceforth used in a read-only manner) then covariance is just safe.

We may of course still give developers the extra safety of being able to declare invariance (as in List<exactly T>) at some point in the future, but the very fact that we haven't been forced to do this so far might be taken as a hint that covariant generics is a meaningful trade-off between convenience and safety.

@zolotyh
Copy link
Author

zolotyh commented Jun 20, 2018

@eernstg I have made yet one example. The code throws an exception after the print statement in runtime. This is really inconvenient and unintuitive.
https://github.com/zolotyh/dart_example/blob/master/web/case2.dart

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());
  });
}

Command is:

dart --preview-dart-2 web/case2.dart

Here is the trace:

Unhandled exception:
type '(Error) => Null' is not a subtype of type '(Object) => dynamic'
#0      _FutureListener.handleError (dart:async/future_impl.dart:147:46)
dart-lang/sdk#57147      Future._propagateToListeners.handleError (dart:async/future_impl.dart:650:47)
dart-lang/sdk#57148      Future._propagateToListeners (dart:async/future_impl.dart:671:24)
dart-lang/sdk#57149      Future._completeError (dart:async/future_impl.dart:490:5)
dart-lang/sdk#57150      Future._asyncCompleteError.<anonymous closure> (dart:async/future_impl.dart:538:7)
dart-lang/sdk#57151      _microtaskLoop (dart:async/schedule_microtask.dart:41:21)
dart-lang/sdk#57152      _startMicrotaskLoop (dart:async/schedule_microtask.dart:50:5)
dart-lang/sdk#57153      _Timer._runTimers (dart:isolate/runtime/libtimer_impl.dart:391:30)
dart-lang/sdk#57154      _Timer._handleMessage (dart:isolate/runtime/libtimer_impl.dart:416:5)
dart-lang/sdk#57155      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

How I can get from trace, that error is in line before print statement?

@eernstg
Copy link
Member

eernstg commented Jun 22, 2018

This is essentially a Dart-1-to-Dart-2 transition problem in the declared type. In the documentation for Future.catchError we have this description of the type of the first parameter:

// The `Function` below stands for one of two types:
// - (dynamic) -> FutureOr<T>
// - (dynamic, StackTrace) -> FutureOr<T>

So catchError will admit any function (statically and dynamically) including the one that we have in the example (of type Null Function(Error)), but the implementation of that method will enforce the stronger type constraint which is documented in the comment. Hence, there is no hope that static analysis can complain about the function literal which is passed, it just doesn't know more than Function.

We don't have union types (cf. #59446 ;-), so we cannot just specify the desired constraint directly (FutureOr<T> Function(dynamic) | FutureOr<T> Function(dynamic, StackTrace)) in the signature of catchError.

Various alternatives have been proposed for the typing of catchError, but nothing has come up that works really well. For instance, a typing of FutureOr<T> Function(dynamic, [StackTrace]) would require the actual argument to be a function that can take 1 or 2 arguments, but we want to pass a function that only accepts one argument (and then have that function treated differently from a function that accepts two arguments), so that's certainly not a solution—it would simply break all call-sites like the one in the example, statically or dynamically: .catchError((MaybeAType arg) => something).

So the real problem here has to do with the transition from a very dynamic language (and recommended style) as in Dart 1, to an approach where static checks are supported as much as possible, as in Dart 2, and it is very specific to the individual method signature of Future.catchError, and there is no particular connection to covariance.

Edit: Forgot to respond to this:

How I can get from trace, that error is in line before print statement?

Currently the error will actually occur deep in the implementation of Future (or in some private class that is used by that Future implementation), so there is no obvious solution in the language (or in any "small" improvement of the language and its type system).

But it would certainly be possible for a lint to special-case Future.catchError and actually check that the given argument has one of those two types, in which case you'd get the message that the argument should have been of type dynamic (and any top type would do, e.g., Object) rather than Error.

@eernstg
Copy link
Member

eernstg commented Jun 22, 2018

I've created a request for a lint which could be helpful here: #57725.

@zolotyh
Copy link
Author

zolotyh commented Jun 22, 2018

Oh, thank you! It's a great news. Waiting for a merge

@eernstg
Copy link
Member

eernstg commented Jun 22, 2018

(PS: I don't know whether I have sufficient support for introducing such a lint to actually make it happen, I just think that it might be useful ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-as-intended Closed as the reported issue is expected behavior
Projects
None yet
Development

No branches or pull requests

3 participants