decide and release null-safety related breaking changes

[sigmundch]

This is a meta issue to track and decide about several possible breaking changes for the null safety feature.

Background

Our goal is to make opt-out code continue to work as close as possible to how it was. However we have changes in the type system that could be breaking and some breaking changes in the core libraries. We are using this bug to review them all and decide which ones should be included and which ones we should either avoid or hide until users opt-in (if possible).

For those breaking changes that can't be hidden, we want to ship them before we unfork.

Open question: should we have a stable channel push with the breaking changes before we unfork?

Known breaking changes

Language changes

• type normalization (see nnbd: can we make opt-out null safety == disabled null safety? language#807): @leafpetersen suggests we move forward with this breaking change upfront
• mutual subtyping of bounds in generic function type parameters (nnbd: can we make opt-out null safety == disabled null safety? language#807): ditto
• change of static fields error semantics: given how this is local to each library, my inclination is to defer this until users opt-in.

Implementation bugs

• Promoted types are no longer inferred for local variables: Analyzer infers promoted type on locals #40413

API changes

• RuneIterator.rawIndex/current return -1 instead of null when reaching the end of the iteration
• runZoned from dart:async may now throw synchronously if the return type is non-nullable and
  the code throws.
• String.fromEnvironment/int.fromEnvironment can no longer return null.
• DoubleLinkedQueue.firstEntry() and DoubleLinkedQueue.lastEntry() used to return null if queue is empty. With NNBD core libraries they throw an error.
• Changes from TypeError to CastError due to removal of implicit casts is sometimes visible (e.g. HashSet.from) See: #34097 (comment)
• StreamSubscription.cancel changed from Future to Future<void> this introduced compile-time errors in package async/test/subscription_stream_test. The test was abusing that onCancel callbacks in StreamController have a dynamic return type.
• dart:io Socket.getRawOption on some error scenarios throws instead of returning null (similar to behavior on other existing properties)
• dart:io properties on detached processes: request [Breaking Change Request] Throw StateError in meaningless getters on detached processes #40483
• Missing null checks: many APIs changed to use non-null argument types, if they were checking for null in the body and no longer do, this may trip people that incorrectly use the API in weak mode. Option 1: add back null checks to each API by hand, option 2: ask all or some implementations to add a null check (this is not emitted today by DDC, for instance)

Summary

Breaking change	kind	Issue #	Ship before unfork (Yes/No)
type normalization	language	lang#807	?
bound subtyping	language	lang#807	?
static field errors	language		No
don't infer promoted types	language	#40483	Yes
RuneIterator.rawIndex/current changes	API		?
runZoned can throw synchronously	API		?
StreamSubscription.cancel() no longer returns values	API		?
*.fromEnvironment may throw	API		?
Queue.first/lastEntry() now throw	API		?
TypeError may now be CastError	both	#34097 (comment)	?
detached processes	API	#40413	?
Socket.getRawOption may throw	API		?
missing null checks	API		?
List.indexOf arg changed from Object to E	API		?
null errors are not replaced with NullThrownError in Completer.completeError, Future.error and StreamController.addError	API		?

[sigmundch]

@vsmenon @lrhn - I'm starting this bug to collect all we know about existing breaking changes and make sure we track the work of releasing them (breaking change announcements etc) before we unfork. Please simply edit the original issue above to add more items to the list.

[lrhn]

Static fields are already lazy, and they will only change behavior in cases where they currently throw a cyclic initialization error. It might not be a problematic change.

[sigmundch]

Static fields are already lazy,

Oops, I just reworded it to be more explicit about error semantics which is what I wanted to highlight.

... they will only change behavior in cases where they currently throw a cyclic initialization error. It might not be a problematic change.

I was more worried about the default value when an error is thrown (that when an initializer throws for any reason, the value becomes null and the initialization is not run again, but now that's not the case). I don't believe this happens much in practice either, but given how local the change is, I'm more ambivalent about making the breaking change upfront.

[vsmenon]

Can we track all potentially visible breaking-in-legacy-mode changes in the top comment here? Shipping these before unfork sgtm.

E.g., @lrhn @sortie - are you planning to rework / reland? (It appears to have also broken the flutter roll.)

https://dart-review.googlesource.com/c/sdk/+/134361

If so, can you update here?

Also, how shall we handle this with respect to the breaking change process? ( @franklinyow @mit-mit )

[sortie]

While migrating dart:io, I encountered two cases where making slight changes to edge cases in the API considerably improved usability or where it was impractical to preserve the current semantics under NNBD. After talking to the language team, we believed these changes to be low risk, and that it would be prudent to land them prior to the unfork to avoid problems at that time. These are:

• [nnbd] Reland backporting process detach semantics from NNBD dart:io. (back in review, pending a determination of whether it can reland)
  • The Process getters exitCode, stdin, stdout, and stderr were all useless (null) for detached processes. We assumed code wouldn't access them for detached processes since it didn't make sense to use them. Therefore all legitimate uses of the getters will always be non-null, and throwing a StateError in that case lets all legitimate uses avoid a null check.
  • However, this turned out to break bazel_worker as it wasn't using detached processes properly. The issue has just been fixed in bazel_worker-0.1.23+1. Flutter tests were affected by the bazel_worker breakage. The change can reland once bazel_worker rolls into Flutter.
  • Since making this change did indeed break something, we probably want to send this change through the breaking change process. No other breakage is known to have happened besides bazel_worker at this time.
• [nnbd] Reland backporting socket option semantics from NNBD dart:io. (relanded)
  • Socket.getRawOption has an edge case where the socket has been called with .destroy() or it has been upgraded to a secure socket (and otherwise doesn't make sense to keep being used), where it can return null. This behavior is undocumented. However RawSocket.getRawOption will never return null. Instead the behavior from the port, address, remotePort, and remoteAddress getters is adopted where it throws a SocketException.closed(), making the API consistent and useful in the general case.
  • No breakage is known to have been caused since it's otherwise a mistake to use a Socket as such after upgrading it to a SecureSocket.

I had already relanded the socket option semantics since it didn't cause any breakage and changes undocumented inconsistent behavior.

The detached process change is in review for relanding, we need to decide how to proceed with it. It can be relanded once bazel_worker rolls into Flutter, but we can send out a breaking change request.

[sortie]

I have made breaking change request #40483 about meaningless getters on detached processes.

[johnniwinther]

Promoted types are nolonger inferred for local variables: #40413

[alexmarkov]

DoubleLinkedQueue.firstEntry() and DoubleLinkedQueue.lastEntry() used to return null if queue is empty. With NNBD core libraries they throw an error.

[alexmarkov]

Implicit casts are not allowed in NNBD, but replacing implicit casts with explicit casts changes the exception which is thrown from TypeError to CastError. This is a breaking change for certain core libraries methods, like HashSet.from. More details are here: #34097 (comment)

[vsmenon]

fyi @davidmorgan - we'd like to backport changes here as much as possible and test/roll internally to sanity check. Any thoughts on process?

[vsmenon]

I'm bumping this to P0 as we need to get this rolling quickly.

Is the table in the initial comment complete?

Can we backport / test all breaking changes in flutter and internally ASAP?

[vsmenon]

If you see something missing, please update the table in the initial comment.

[alexmarkov]

Completer.completeError, Future.error and StreamController.addError no longer replace error with NullThrownError if error is null.

This is flagged by failing tests co19_2/LibTest/async/Completer/completeError_A02_t01, co19_2/LibTest/async/Future/Future.error_A01_t01 and co19_2/LibTest/async/StreamController/addError_A02_t01.

[liamappelbe]

List<E>.indexOf and List<E>.lastIndexOf were updated here to take an E rather than Object. So now if you pass a different type it throws an error rather than returning -1.

[mit-mit]

I started a new meta-issue for the public communication of the breaking changes related to null safety: #40686

[franklinyow]

I'm closing this since we've decided which one to port and has the record in #40686