[Breaking change request] Make stack traces not be null.

[lrhn]

Summary

We will replace a null stack trace supplied by users with a default stack trace.

History: dart-lang/language#747
CL: https://dart-review.googlesource.com/c/sdk/+/128660

What is changing:

A lot of async methods currently allow the user to optionally supply a stack trace. Examples include:

• Completer.completeError
• StreamController.addError
• AsyncError constructor

If the user actually calls these methods with no stack trace, or with an explicit null stack trace, then the system will dutifully propagate null all the way to the user error handler (like Future.catchError or an async method try/catch).

We change this so that if no stack trace is supplied, the system will instead insert a default stack trace.
That default stack trace is computed as either:

1. If the error object implements Error then read its .stackTrace.
2. If not, or if that is also null, then use a default stack trace, StackTrace.empty.

The default stack trace is similar to StackTrace.fromString(""). It is a unique object not equal to any other stack trace.

Why is this changing?

Because the current behavior means that in null-sound mode all error handling functions need to take StackTrace? (a nullable type) as argument, even though almost all stack traces are non-null in practice.
That's an unnecessary annoyance when writing error handlers (the experience of migrating the platform libraries had many situations of forgetting the ?).
Providing a nullable value is a tax on users of that value, and in this case there is no benefit from it.

So, we change the behavior in order to be able to have a better and more usable null-sound API.

Example

var c = Completer<int>();
var f = c.future;
var onError(Object error, StackTrace stack) {
  print("$error\n$stack");
}
var f2 = f.catchError(onError);
c.completeError("just the error");

This code would not be valid NNBD code because it lacks the ? after StackTrace. That's a small annoyance, but an unnecessary one.

With this change, the code becomes valid, and the $stack won't print null.

Expected impact

Most likely nothing will break.

If you have code which depends on a stack trace being null, then the code will stop working. That is not expected to happen outside of tests.

If you have code which assumes that a stack trace is not null, which is much more common, your code will now be sound.

There will be cases where the default stack trace will be different from the stack trace currently produced. For example, an await Future.error("no stack trace") will currently synthesize a stack trace at the await expression (which is why we haven't seen try ... catch (error, stack) have a null stack value). It will now get the empty stack trace instead.
Again this will likely not matter outside of tests.

The stack trace package may want to adapt to recognize the default empty stack trace.
It is not necessary, the package already works with a trace that has an empty toString, and it creates a trace with no frames.

[Hixie]

why StackTrace.empty, instead of StackTrace. current?

[lrhn]

Because StackTrace.current has a significant performance overhead.

If someone throws an exception (not Error) which is intended to be caught by the caller, then there is no need for allocating an expensive unused stack trace.

If it is an Error, then we do use the stack trace already stored on the Error object, if any.

[franklinyow]

cc @Hixie @matanlurey @dgrove for review and approval.

[matanlurey]

Seems fine

[vsmenon]

lgtm for dart

[Hixie]

I would prefer if we just make StackTrace.current cheap and used that, but I don't feel strongly about it.

[franklinyow]

Approved

[leafpetersen]

This has landed here.