Unified errors with implicit/explicit down-casts

[mkustermann]

This example:

% cat test.dart
main() {
  List<dynamic> values = <int>[1, 2];

  try {
    getValue<double>(values, 0);
  } catch (e, s) {
    print(e.runtimeType);
    print(e);
    print(s);
    print('');
  }


  try {
    getValueExplicit<double>(values, 0);
  } catch (e, s) {
    print(e.runtimeType);
    print(e);
    print(s);
    print('');
  }
}

T getValue<T>(List<dynamic> values, int i) {
  return values[i];
}

T getValueExplicit<T>(List<dynamic> values, int i) {
  return values[i] as T;
}

prints

% dart test.dart
_TypeError
type 'int' is not a subtype of type 'double'
#0      getValue (test.dart:25:16)
#1      main (test.dart:5:5)
#2      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#3      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)


_CastError
type 'int' is not a subtype of type 'double' in type cast
#0      Object._as (dart:core/runtime/libobject_patch.dart:67:25)
#1      getValueExplicit (test.dart:29:20)
#2      main (test.dart:15:5)
#3      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#4      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

Notice that

• We get _TypeError with implicit down casts and _CastError on explicit down casts
• We get two different stack traces, once with and once without Object._as
• We can catch _TypeErrors with try { ... } on AssertionError (e, s) { ...} (I guess we should not make _TypeError implement AssertionError)

The VM currently has two (very) different implementations of explicit and implicit down casts. It would be really nice if we could always throw _TypeError for implicit as well as explicit downcasts.

[mkustermann]

@eernstg @lrhn @leafpetersen

[eernstg]

@leafpetersen and @lrhn, do you have strong preferences for maintaining this distinction, is it important for developers to be able to catch a CastError in order to intercept a failure in as, and catch a TypeError in order to intercept a failure in a generated type check? Similarly, I cannot find any justification for TypeError <: AssertionError in the spec; should we add that, or should we remove that relation in the vm and elsewhere, or should we just keep things the way they are? (There's also nothing in the spec that prevents that subtype relationship ;-).

[eernstg]

Adding some background info: The spec currently requires every failing as expression evaluation to throw a CastError, every event described by dynamic type error to throw a TypeError, and there is no general rule for a dynamic error; but there are several specific situations where a specific error must be thrown (e.g., NoSuchMethodError, CyclicInitializationError, AssertionError).

[eernstg]

@mkustermann suggested that the type used when a CastError must be thrown and when a TypeError must be thrown could simply be the same type, implementing both of them. Again, the spec certainly does not explicitly prevent this, but it might be argued that it is against an implicit intention in the spec.

On the positive side, code catching either one of them would would continue to work when such an error this thrown. On the other hand, code written to catch one of them and ignore/rethrow the other one would be broken. I don't know if that occurs so frequently that it will break anything in practice.

[matanlurey]

See also #33950, where I asked about the difference (and hoped that we can remove one or the other in Dart 3).

[leafpetersen]

cc @vsmenon

I believe that in DDC we are considering having a mode where failure of implicit checks becomes a hard failure rather than an exception, or perhaps just an exception that you need to explicitly opt into catching. This would be to help users avoid silently swallowing type errors with overly broad exception handlers (particularly since some internal users turn off type checks in dart2js). So DDC at least may want to be able to distinguish between them. This is probably not definitive either way on whether the production platforms choose to unify them: DDC could still have an opt in mode to behave differently on type errors as a debugging aid.

I do have the sense that in so far as users are catching things which are in the Error category, catching cast errors is more reasonable than catching type errors. My reasoning is that users can see where explicit casts are, and hence reason about what state the program is in if one occurs: I doubt that most users can correctly reason about the state of programs when implicit errors are thrown. I'd probably prefer that people not catch either though, at least not in a catch and continue sense.

[mkustermann]

I believe that in DDC we are considering having ...

Does this mean that different backends can decide themselves how to handle explicit/implicit downcast errors?

So DDC at least may want to be able to distinguish between them.

Do you mean distinguish between cast errors and other exceptions or between implicit cast errors and explicit cast errors?

Personally I think it is just a matter of style whether to rely on implicit downcasts or writing them explicitly in code. Therefore IMHO we should behave the same way.

For example in the protobuf package we have this code

  T _$get<T>(int index, T defaultValue) {
    var value = _values[index];
    if (value != null) return value as T;
    if (defaultValue != null) return defaultValue;
    return _getDefault(_nonExtensionInfoByIndex(index)) as T;
  }

Now this will behave differently whether as T is there or not. Though it's really the same typing error detected at runtime to guarantee soundness. Can we let the VM behave the same way in both cases?

I'd probably prefer that people not catch either though, at least not in a catch and continue sense.

I agree. One option would be to add a hint to dartanalyzer which recommends people to only catch on Exception, which will then not catch subtypes of Errors.

[vsmenon]

Actually, I'd prefer a debugging mode where all Errors are hard failures or otherwise highlighted (see #34060 for context). So, to answer @mkustermann 's question, I don't care much about the distinction between TypeError and CastError, just between Error and Exception (and other non-Error types).

Re the web, the main point is that Dart2JS provides aggressive options that do not preserve behavior in the presence of a runtime Error (but does for Exception and other thrown types). Our debugging tools should warn developers accordingly.

Per @leafpetersen's point, it's true today that Dart2JS treats TypeError and CastError differently in aggressive mode (it allows omitting the former). That's an accident of history though.

[mkustermann]

Thanks to all for adding their view on the topic!

After talking also to @lrhn about this, it appears there are not very strong opinions on what the VM will do here, so we might want to find a way to unify these two mechanisms into one, possibly by throwing an object which implements both interfaces.

[leafpetersen]

Per discussion, I'm fine with unifying these errors.

[mkustermann]

We've unified the implementation of implicit/explicit as checks in the VM to make them perform equally fast. We decided to retain the two different exceptions being thrown without extra penalty.

[alexmarkov]

With NNBD implicit casts are disallowed, so we need to replace them with explicit casts when migrating code. The difference in exceptions between explicit and implicit casts makes such changes breaking.

For example,

sdk/sdk/lib/collection/hash_set.dart

Lines 96 to 102 in fd992e4

	factory HashSet.from(Iterable elements) {
	HashSet<E> result = HashSet<E>();
	for (final e in elements) {
	result.add(e);
	}
	return result;
	}

was migrated to

sdk/sdk_nnbd/lib/collection/hash_set.dart

Lines 94 to 100 in fd992e4

	factory HashSet.from(Iterable<dynamic> elements) {
	HashSet<E> result = HashSet<E>();
	for (final e in elements) {
	result.add(e as E);
	}
	return result;
	}

This effectively changed the exception which is thrown if elements are not assignable to E.
This is flagged by failing co19_2/LibTest/collection/HashSet/HashSet.from_A02_t01 with unforked SDK.

/cc @lrhn @leafpetersen

[a-siva]

This issue has been marked as being required for unforking the SDK, what work remains to be done here?

[alexmarkov]

@a-siva I marked this issue as vm-nnbd-unfork-sdk because there were test failures on the unfork CL due to discrepancies between CastError and TypeError:

co19_2/LibTest/collection/HashSet/HashSet.from_A02_t01
co19_2/LibTest/collection/ListQueue/ListQueue.from_A03_t01
co19_2/LibTest/math/Point/operator_mult_A02_t01

Do you know if those tests are still failing? It is possible that this has been fixed already (cc @leafpetersen ).

[a-siva]

This issue seems to have been fixed in the co19 tests see
dart-lang/co19#527

[leafpetersen]

Tl;DR I believe this is now a non-issue.

All SDK implementation objects implementing TypeError or CastError were changed to implement both in https://dart-review.googlesource.com/c/sdk/+/138670 , and this has rolled out as a breaking change. This means that any test which expects a TypeError will continue to pass if a CastError implementation object is generated, and vice versa. Eventually we plan to remove CastError, and the co19 tests have been fixed up in preparation for that: dart-lang/co19#527 (thanks @eernstg for filing that). I'm not sure if those have rolled in (@athomas would know), but I don't think it matters.

I filed this issue to track the additional work required to get the VM to stop throwing CastErrorImpl and instead always throw TypeErrorImpl.

[athomas]

The dart-lang/co19#527 fixes have rolled into the SDK a long time ago (188c8fb).