Resolve typing treatment of throw/catch with NNBD

[leafpetersen]

In comments on the NNBD feature spec the question came up of what errors and warnings to provide on throw catch. Currently, Dart makes it a runtime error to throw null. Apparently the reasoning given at the time was that null would match any catch site, and users were unlikely to be prepared to receive null. With NNBD it's possible to distinguish which catch sites are prepared to accept null, so this reason goes away.

There are three questions we should decide:

• Should we change the semantics to allow null to be thrown?
• Should we allow potentially nullable things to be thrown?
• Should we allow the on clause to specify a potentially nullable type?

If we don't change the semantics to allow null to be thrown, then there is no point in having a nullable type in an on clause. However, if you want to catch something of generic type you're out of luck:

foo<T>(Object o) { 
  try { 
     if (o != null) throw o;
  } on T catch (e) {
     Expect.identical(o, e);
  }
}

If we don't change the semantics of throwing null, then it seems somewhat unfriendly to allow the user to throw a nullable type, since it will always be an error if the value is actually null. The same issue with generic types arises, but can be worked around by casting to Object.

The options that seem consistent to me are:

1. Leave it as a runtime error to throw null, make it an error to throw a potentially nullable type, and allow a potentially nullable type in the on clause.

• You can throw something of potentially nullable type by casting it to Object
• You can catch something of potentially nullable type

2. Leave it as a runtime error to throw null, make it an error to throw a potentially nullable type, make it an error to have a potentially nullable type in the on clause.

• You can throw something of potentially nullable type by casting it to Object
• You can't catch something of potentially nullable type: you need to catch Object? and then rethrow based on an is check

3. Make throwing null not an error, allow throwing potentially nullable types, allowing potentially nullable types in the on clause.

• You can throw something of potentially nullable type.
• You can catch something of potentially nullable type, and it may be null.

The current feature spec specified behavior 2. Should we change this?

cc @lrhn @munificent @eernstg @stereotype441

[rrousselGit]

I don't see 1. and 2. as mutually exclusive.

We could have:

on Foo? catch (e) {
  if (e is Foo) rethrow;
}

[leafpetersen]

@rrousselGit In 2 your code is a static error, in 1 it is not. That's the difference. In neither of them, can e every actually be null (but in 3 it can).

[rrousselGit]

Where would be the static error? The on Foo??

I don't see any reason to have a compile error here. As long as re/throw matches the runtime error behavior, allowing or not on Foo? doesn't make the code any safer.

While on Foo? is pointless in itself, it may come in handy with different features such as typedefs, extensions, or union types (if we ever have some).

[leafpetersen]

The on Foo??

yes. "make it an error to have a potentially nullable type in the on clause"

[lrhn]

(The reasoning at the time was not that it would catch Null, but that it would look like it should catch Null, and it would be confusing that it didn't. The syntax at the time was catch (SomeError e), which is equivalent to the later syntax on SomeError catch (e), and would not actually catch Null. It still looked like a type annotation, which were otherwise nullable. We have since (for Dart 2/strong mode) made Null a subtype of all types, but it's still not treated that way by is checks, and wouldn't have to be by on/catch checks.).

[lrhn]

I have previosuly been in favor of allowing you to throw null, for consistency reasons, but with NNBD we want to catch accidental nulls, and throwing null is likely an accident. So, I now say that we should not allow throwing null.
Since this is a proper NNBD non-nullable thing, the expression of a throw expression should get the same treatment as any other non-nullable context, which means making it a compile-time error to have a potentially nullable expression as the operand of throw.

There is no strong reason to require the on type of a catch to be non-nullable, you can assign a non-nullable value to a nullable variable without warning. Since there is no way to make a type variable non-nullable (no ! operator on types), code like:

try {
  T bailoutValue = ...;
  if (bailoutValue != null) throw bailoutValue;
  do something
} on T catch (bailoutValue) {
  // Did a bailout
]

would not have a way to type the catch.
It's silly code, and you should probably make T non-nullable and use T? bailoutValue = ... for cases where null means something specific, but the code, as written, is technically correct.

So my vote goes version 1. I'd probably be fine with disallowing potentially nullable types in the on clause too, but only from a style point-of-view. It's consistent and safe to allow nullable types.

If you write no on clause, the type of the catch variable still becomes dynamic. I'd prefer Object, but we probably have existing code expecting dynamic, which is another reason we cannot disallow a potentially nullable on type since it is the default, and I want you to be able to explicitly write the default.

A rethrow is always safe because we know that the actually caught value is non-null, no matter how the user-visible variable exposing it is typed.

I expect the analyzer to warn if you actually use a potentially nullable type in the on clause, because it's definitely not needed. If you use a nullable type, and a corresponding non-nullable one exists, you can use that instead. If you use a potentially nullable, but not nullable, type, then you would probably be better off rewriting the code.

The only question is whether we promote the catch variable to no-null on entry, because it was just assigned a definitely non-null value.

[stereotype441]

I'd also vote for version 1. And I like the idea of promoting the catch variable to non-null inside the catch block.

[eernstg]

I think approach 3 (allow throwing null, allow on T also for a T which is potentially nullable, and presumably also for a T which is plain nullable) is the cleanest approach: No exceptions, and hence no need to worry about corner cases. I'd recommend that approach (that we might call 'relaxed'), or alternatively a cleanly strict approach.

@lrhn mentions that it was confusing that the on type T was used in a type test (like x is T where x is the exception), where T has always been treated as a non-null type, whereas type annotations are otherwise subject to a type cast (like x as T), where T is (in pre-NNBD code) treated as nullable. But with NNBD there is no such conflict.

So the only remaining reason to go with approach 1 (or 2, but I also prefer 1 among those two) would be the one that @lrhn mentioned:

we want to catch accidental nulls

This does not fit very well with a construct which is essentially typeless anyway (expecting that we won't introduce Java-ish throws clauses on function signatures and function types). In the same vein, we aren't going to require that every throw expression throws something whose static type is a subtype of Error or Exception, because there's a lot of code which violates that rule (e.g., throwing strings).

So we are weighing the anomaly of treating 'potentially nullable' types unsoundly in this single case versus the temptation to kill some more nulls. With that, I prefer having one the following 2 options:

• Approach 3, allowing on T for any type T, nullable or not.

• A strict variant of approach 1 (we don't need to make it a run-time error to throw null): It's a compile-time error to throw null (in particular, a potentially nullable type needs a cast); the on clause admits nullable types (including potentially nullable ones), but behaves as if the thrown object o had passed a o != null test, such that the catch body need not re-test. Finally, throw d where d has static type dynamic could be allowed (following NNBD assignability) and could be defined to mean throw (d as Object).

Approach 3 consistently allows null for throwing, and the strict variant of approach 1 consistently prevents throwing null as well as testing for null. I think those options are the most meaningful ones for developers in the long run.

[leafpetersen]

Ok, let's go with 1. We can specify this as either of:

1a: It's an error to throw a potentially nullable type
1b: It's an error to throw a type which is not assignable to Object.

The latter allows throw d where d has type dynamic, as proposed by @eernstg . @lrhn preferences between the two?

[lrhn]

I think I prefer 1b. It is consistent with the operand expression having a context type of Object, which is how I would define it in the specification.
That will automatically require the expression to have a type which is a sub-type of Object, or which has an implicit downcast to Object (aka, being dynamic).

I think that everywhere the specification requires a specific type (bool test expressions, Iterable/Stream for-in loops, Object throw) it should act precisely the same as the type system requiring a specific type (say, the required argument type of a function). That will include allowing an impilcit downcast from to that required type, and with NNBD that still works, it just has to be from dynamic.

[leafpetersen]

SGTM.

[lrhn]

(For the record, it's now too late to change because all the async error handling APIs are expecting Object for the error).

[eernstg]

(Just curious, I think we're happy about the rule that it is a compile-time error to throw e unless the static type of e is assignable to Object; so what's the change that we might want but can't have now?)

[lrhn]

(We don't allow you to throw null; or throw e; where e has a potentially nullable type. Obviously we could allow it, and then still throw a NullThrownError if the value ends up being null, but that would be counter to the idea of null safety, so the real alternative would be to allow you to throw null as the actual error value. Which we can't now because Future.catchError accepts a Function(Object, StakTrace) which cannot handle the null.)