Looking for consistency in Null Safety core library migrations

[nshahan]

I'm seeking comments from the Language Team/Core Library Maintainers to give clarity and consistency to the wider team as we work though this migration.

We have been running into inconsistencies in the migrations of the core libraries for Null Safety.
In many cases when the migration changed an API to accept non-nullable arguments the explicit checks for null and thrown errors have been removed. In some cases casts or is checks are used to ensure there are still some failures if a null value sneaks in through a legacy library.

There are multiple tests (in the legacy _2 suites) that have pinned down the existing behavior where exceptions are thrown. These tests makes it obvious where the existing behavior will change when using the API from a legacy library.

• Should we add the null checks and thrown exceptions back into the core libraries for now until
  we ship Dart 3.0?
• Should we add new casts or is checks to the beginning of the methods to ensure at least some error is thrown?
• Should we weaken the tests (in the legacy _2 suites) to allow for different exceptions? For example TypeError vs CastError?
• Should we remove those test expectations altogether?
• Do we want to handle all these cases consistently or should we actually send all of these reviews
  to the maintainers to evaluate independently?

To help make this a little more concrete here is a great example of the situation we are running into. The migration removed the null check from the _StreamIterator constructor:
https://dart-review.googlesource.com/c/sdk/+/122786/7/sdk_nnbd/lib/async/stream_impl.dart#b973

This test is now failing with an observable change in behavior when called from a legacy library:
https://github.com/dart-lang/sdk/blob/master/tests/language_2/control_flow_collections/await_for_null_test.dart

@eernstg @leafpetersen @lrhn @munificent

Thanks!

cc @fishythefish @joshualitt @Markzipan @natebosch

[nshahan]

cc @sigmundch

[lrhn]

Please do allow both TypeError and CastError. See #787.
For the record, the language specification states that an as check throws a TypeError even though it currently throws a CastError.

The StreamIterator change is bad because that class is used internally in the language implementation for await for loops. That means that removing it is a language implementation bug (which is also why language implementation should never use public classes).

On the other hand, we are now in a situation where keeping the stream ?? (throw ArgumentError.notNull(...)) would cause a warning because stream is not nullable.
We could migrate it to stream as Stream<T> which would throw on null, but risk an "unnecessary cast" warning. There are definite limits to how many workarounds we should add in order to enable null check for non-nullable code. That's why just removing them is nice and easy (and completes the migration in one step), but it's not safe as long as the program contains legacy code.

[eernstg]

Until we have null-safety soundness (that is, until no more legacy code exists), the removal of manually written null checks would lower the "quality of service" for users of a library somewhat, so I'd recommend that we keep these checks in the most widely used libraries, and in particular in platform libraries.

Consider the situation where a method/function in our widely used library L is called, and an argument is null even though its type is non-nullable. (This will only happen because the static checks are unsound as long as any legacy code exists in the program, but with this unsoundness it may happen anywhere.)

With the manual checks, the caller will see a dynamic error that indicates a wrong argument was received. This is the well-known behavior. It may be surprising because the developer who gets to check out this situation will claim that this shouldn't ever happen, but we will then have to explain that the unsoundness will make it possible, during the transition to pure nnbd.

Without the manual checks, the caller will see a dynamic error that indicates something went wrong because of a null, somewhere potentially far away from the location where the null was wrongly received as an argument. In this case we will presumably need to debug our library L, or someone else will be asked to debug their code, because there is no reason why we would be able to redirect the suspicion to the actual cause.

Should we add the null checks and thrown exceptions back

I'd recommend that. Adding further checks may not be justified to the same extent, because that would be an improvement in an area where existing usages haven't revealed much of a need.

Do we want to handle all these cases consistently

I'd recommend that we consistently apply the rule "don't delete explicit null checks in widely used APIs that, due to unsoundness, may reveal actual nulls coming from code which is not in the same library, especially if the call site is not maintained by us."

But wouldn't the person who migrated the code be in the best position to go over the CL and find those checks that should not have been removed after all, and restore them?

In the situation where nnbd warnings are emitted because the null checks appear to be unnecessary, ArgumentError.checkNotNull(...) may be helpful.