Support 'catastrophic' errors

[vsmenon]

Runtime soundness checks inserted by DDC should be catastrophic - i.e., they should not be recoverable (at least not transparently). E.g., they should not be 'accidentally' suppressed in a try-catch along with other exceptions.

This includes most (if not all) of the throw operations in runtime_dart.js. There's an open question of what we want this failure to look like. It should be something developer friendly.

[jmesserly]

hmmm. It seems the existing Error type covers this?
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:core.Error

Error objects thrown in the case of a program failure.

An Error object represents a program failure that the programmer should have avoided.

Examples include calling a function with invalid arguments, or even with the wrong number of arguments, or calling it at a time when it is not allowed.

These are not errors that a caller should expect or catch - if they occur, the program is erroneous, and terminating the program may be the safest response.

When deciding that a function throws an error, the conditions where it happens should be clearly described, and they should be detectable and predictable, so the programmer using the function can avoid triggering the error.

Such descriptions often uses words like "must" or "must not" to describe the condition, and if you see words like that in a function's documentation, then not satisfying the requirement is very likely to cause an error to be thrown.

Yes, you could technically catch it, but generally are not supposed to for most code. It's sometimes useful, though, e.g. debugging. Occasionally, framework code needs to deal with it in a special way. Consider a test framework, that might catch all exceptions but send them to another iframe.

I'm not sure we need to do anything here but make our exceptions Errors. Then, I can see an option to make all Errors an immediate fail (of the isolate?), although I'm not sure it's worth enough, given the occasional valid use cases for catching all errors.

[vsmenon]

It looks like untyped catches:

try {
  ...
} catch (e) {
  ...
}

are pretty common in existing Dart code - I see them all over the place in Angular. I don't think catastrophic errors should be this easy to catch / suppress.

Our subset argument relies on that - we don't promise to match checked mode / production mode once a catastrophic error is hit.

Good point on working with test frameworks though!

[jmesserly]

hmmm. Maybe that should be a lint message. But, I presume those catches are doing something with the error, not just ignoring it?

Our subset argument relies on that - we don't promise to match checked mode / production mode once a catastrophic error is hit.

Sure. But our type errors aren't any different from existing checked mode errors. And the code is choosing to catch catastrophic errors, so it should do that.

[vsmenon]

And the code is choosing to catch catastrophic errors, so it should do that.

Programmers are probably not really intending this - it's just that we make it easier to catch everything than just catch Exception.

One possibility is (to have a flag) to treat:

try {
  ...
} catch (e) {
  ...
}

like this:

try {
  ...
} on Exception catch (e) {
  ...
}

I.e., any non-Exception throw is effectively catastrophic, but test frameworks can explicitly choose to catch Object.

[jmesserly]

yeah, that's interesting. I can definitely get behind something like that -- we seem to have too many catch(e)'s that should be on Exception catch(e) ... The challenge here is broader Dart ecosystem adoption. Maybe the linter can nudge folks to be more explicit in catches?

[leafpetersen]

Is there anything wrong with just having an internal exception class (internal to JS, not a subtype of dart objects), and generating untyped catch blocks to look like:

try {
// Code for try block here....
} catch(e) {
 if (e instanceof CatastrophicError) throw e;
 // Code for catch block here....
}

[jmesserly]

Right @leafpetersen ... technically we can do it. Implementation is super simple. My concerns are whether it is the right thing for users. Specifically:

• JS interop: hiding these exceptions from Dart catch does nothing to hide them from JS
• readability: every catch(e) needs this extra code. Granted, once can easily add a type annotation to avoid it. But it's yet another quirk for people writing Dart code intended for a JS audience
• frameworks, tests, logging: uncatchable errors make some patterns impossible to express, when you're running code that may have errors, but you want to continue anyway.
• special cases: why are DDC type Errors different from any other *Errors, including existing checked mode errors? Those are all severe. See API doc for Error, explaining why you shouldn't catch them without very good reason.

I understand the theoretical purity argument for DDC uncatchable errors, but I'm not convinced it's the right thing to do in practice.

Compatibility is always more complex. Consider breaking changes: is it breaking to remove an exception case, like an "UnimplementedError"? Technically, code could be relying on that exception. In practice, such code is considered to be broken, and it is allowed to remove these exceptions without treating it as a "breaking change". Speaking from experience here in both CLR and Dart.

For that reason, I'm somewhat skeptical that we want uncatchable exceptions. I could see a case for making all Errors uncatchable, but that's a challenge for the broader ecosystem, and you have to come up with a plan for frameworks that legitimately need to catch everything.

A perhaps workable proposal:

• catch(e) means on Exception catch(e)
• on Object catch (e) means catch(e)

That makes people opt-in to catching Error.

Note, I'm not personally advocating that proposal... my point is just, this is a way broader issue than DDC, it applies to all Errors.

[jmesserly]

Also, not sure I buy this as a "soundness" issue. We're in no worse shape than existing checked mode (and index out of range error, and null errors, etc).

[leafpetersen]

Fair enough, but I do think there is something more than just the usual compatibility concerns here. Our runtime errors are supposed to turn places where we would otherwise diverge in semantics from dart2js and into error conditions: users are supposed to fix these errors before deploying. If these errors can get masked by catch-all blocks, then a user can see an apparently perfectly reasonable execution under DDC, deploy with dart2js, and see an also apparently perfectly reasonable execution but with different results. It's possible that this is a mostly theoretical concern, but I'm not entirely sure that I buy it: I'm worried about something like (e.g.) Angular wrapping up some code with a try/catch and just silently discarding the error.

An alternative would be to do something like pop up a dialog and hang the script: that has no overhead on the try/catch end, but is definitely more intrusive to the user.

Perhaps a combination of logging to the console and throwing an Error exception (catchable) might be a reasonable compromise: at least there is some error trail left behind to indicate that something bad happened.

[jmesserly]

Angular wrapping up some code with a try/catch and just silently discarding the error.

if this is happening, it's a bug in Angular. Are we sure about it? If so, let's send a fix their way. Silently ignoring errors is really bad. They're hurting their own users by doing that.

I'd guess that isn't what they are doing, though. Consider:

try {
  // ... call some maybe buggy user code ...
  // after all, user could still be developing, maybe they want the rest
  // of the app to work instead of seeing a blank page.
} catch (e, s) {
  // throw the error asynchronously, so it still bubbles up as a top-level exception 
  new Completer().completeError(e, s);
}

We had that exact pattern in package:observe for Polymer:
https://github.com/dart-lang/observe/blob/2739589267cc8e0ac8e7c111a22e7a862d605930/lib/src/path_observer.dart#L786

Perhaps a combination of logging to the console and throwing an Error exception (catchable) might be a reasonable compromise: at least there is some error trail left behind to indicate that something bad happened.

yeah, that sounds good to me.

I'm still not seeing how this is different from Dartium checked mode+dart2js production deployment. If anything, the difference there is much larger.

[leafpetersen]

Not sure at all about Angular, except that I believe that something like this was involved with the recent google3 breakage that Dan dealt with - I think it was an NSM exception that Angular was catching (due to a refactoring removing a method), and then silently retrying?

From a quick scan, there are a few places in dart:html and a few other sdk libraries that assume that they know what caused an exception and either do nothing, or do a default action. A couple of examples.

      try {
        e._initCustomEvent(type, canBubble, cancelable, detail);
      } catch(_) {
        e._initCustomEvent(type, canBubble, cancelable, null);
      }

    try {
      r['socket'] = _socket._toJSON(true);
    } catch (_) {
      r['socket'] = {
        'id': _servicePath,
        'type': '@Socket',
        'name': 'UserSocket',
        'user_name': 'UserSocket',
      };
    }

It's true that this isn't really any different from the checked mode issue.

[vsmenon]

Perhaps step 1 here is to use a common type - e.g., StrongModeError - for these. A coercion error due to an implicit strong mode check should throw that or a subtype of that instead of CastError as it does today.

Then we can experiment with how to surface these (or other Errors for that matter).

I agree on the checked mode point, though I think that's unfortunate as well.

[vsmenon]

How do we want to solve the test infra case? If an individual test throws a StrongModeError, we want our infra to report that - we don't want the test to be able to catch / suppress that. OTOH, we'd like the infra to be able to catch / handle.

We'd like this to work with existing tests that weren't written with strong mode in mind.

E.g., we should be able to run through all the existing lang and co19 tests and determine which ones ever throw a StrongModeError.

[alan-knight]

Presumably a well-behaved test framework catches the error and then reports a test failure because of a thrown exception, so you could just look at the output. And it's our test framework, so if it isn't well-behaved we could make it so.

[vsmenon]

The trick is to get the error to the framework in the first place. :-)

But maybe printing StrongModeError to the console and looking for that is good enough to start.

[jmesserly]

exactly what @alan-knight said ... if it's a normal error, test framework just catches it. we get correct handling for free.

[jmesserly]

(and it works through futures, async tests, etc.)

[vsmenon]

Just to clarify, I'm mainly worried about (poorly written) tests like this:

try {
  void access(List<int> list) => list[list.length];
  var list = [1, 2, 3];
  access(list);
  fail("shouldn't reach here");
} catch(e) {
}

This test is supposed to be checking for a range error, but DDC should generate a strong mode error. We should be able to report that a StrongModeError happened on this test even though that never gets thrown to the framework.

[jmesserly]

yeah, that test is super problematic though. FWIW, at least in tests/language, they always check for specific specified exception types for this very reason. My recollection of co19 is the same. Here's an example:
https://github.com/dart-lang/co19/blob/master/LibTest/core/List/List_A01_t01.dart#L34

[vsmenon]

I will start generating StrongModeErrors as appropriate in our backend while I'm also looking at #236

[jmesserly]

This strikes me related to another bug, that we could drop into the debugger if you hit a strong mode error.

[jmesserly]

merging this into #509