Sdk: NNBD introduced breaking change to Isolate.spawn functions

[gaaclarke]

Projects that executed fine have been broken with the introduction of NNBD without any changes. This appears to happen because Isolate.spawnUri doesn't inherit the settings from the calling context so a non-NNBD context spawning an isolate will attempt to make a NNBD isolate unless the called code has a language version annotation.

Examples:

• https://github.com/flutter/flutter/issues/70355 (this issue could be addressed by just adding a language annotation to the executing code)
• https://cirrus-ci.com/task/5943556349100032 (this one had to be fixed by upgrading the test package)

Expected result:
I expect Isolate.spawn functions to default the spawned isolate to the same settings as the spawning isolate unless otherwise specified.

Dart version:
2.12.0 (build 2.12.0-31.0.dev)

[alexmarkov]

Related: https://github.com/dart-lang/sdk/issues/40632, https://github.com/dart-lang/sdk/issues/41207.
@jakemac53 @natebosch @leafpetersen @lrhn

[lrhn]

If I read this correctly, it's Isolate.spawnUri which opts its spawned isolate into sound null safety based on the entry-point library.
This is dangerous if the library may depend on non-null-safe libraries, and there is no way to override the behavior.
(Or rather, one will have to create a non-null-safe entry point which forwards to the real entry-point library, and use that until all dependencies have been migrated).

It's only a problem for out-of-order migration.

[natebosch]

It's only a problem for out-of-order migration.

The problem we are hitting is that it's a problem for users who upgrade their SDK but don't care about null safety yet.

The reasoning to default to opt-in if the VM doesn't know the language version is the same as the reasoning to default the overall VM to opt-in if you run a script that isn't in a package - we want the default to be null safety because it's the best default in the long term. In the short term during the SDK switch it does cause some pain.

[gaaclarke]

If I read this correctly, it's Isolate.spawnUri which opts its spawned isolate into sound null safety based on the entry-point library.

With the Pigeon issue mentioned in the description, the spawning context was non-NNBD but it was attempting to spawn an isolate (non-NNBD before upgrade, NNBD after). The code worked fine until there was an upgrade of Dart and it stopped working. I had to fix it by adding a language annotation to the spawned code.

Something like this this could would be broken with a Dart upgrade (see pigeon for a real example):

void main() {
  final String code = """
import 'package:non_nnbd/non_nnbd.dart';

void main(List<String> args, SendPort sendPort) async {
  print('hello');
}
""";

  String path = tempFilePath();
  File(path).writeAsString(code);
  Isolate.spawnUri(Uri.file(path), null, null);
}

[jakemac53]

Yes this is a breaking change but it is exactly the same breaking change that is being made for all dart code.

If you have some code which is not a part of a package (including code in a data uri), it will be opted in to null safety. That is the default language version. If you then run that code, it will default to sound null safety.

It is imo correct for both Isolate.spawnUri(<uri>) and dart <uri> to have the same behavior, and they do today, which is what you are seeing.

We should likely call this out explicitly in the breaking change announcements for NNBD though, if it isn't already.

We should also ideally have an option on Isolate.spawnUri to disable sound null safety, just like the command line argument to the dart vm. You can work around this instead by adding a dart language comment to opt out the file // @dart=2.9 for instance.

[mit-mit]

We should likely call this out explicitly in the breaking change announcements for NNBD though, if it isn't already.

@leafpetersen have we actually sent a general breaking change notification for null safety overall? With it going to beta, and dart <file name> getting null safety by-default, that might be a good idea.

[leafpetersen]

@leafpetersen have we actually sent a general breaking change notification for null safety overall?

No. I can do so.

[nex3]

When you first announced null safety, you made a big point of saying "Null safety is backwards compatible". This is a clear backwards incompatibility, and one that's already affecting real users—Dart Sass's CI has been broken for six days because of it, and we have no recourse but to wait for an upstream fix. And that's not counting the numerous changes we've had to make to a number of other packages we use to work around the underlying VM-default issue.

I understand the desire to move everyone onto null safety, and to have the VM default to the language version that's ultimately going to be universal in the future. But you can't just go about making breaking changes left and right, especially when you've publicly promised not to do so. Just because the new behavior is better doesn't negate the pain, frustration, and lost productivity that those breakages cause.

In Sass, when we need to make a breaking change, we follow a process for doing so that's designed to minimize user pain. First, we mark the old behavior as deprecated, and produce some kind of message or warning when it happens. (This is also how, for example, what Node.js did when they transitioned unhandled promises from being silently ignored to crashing the program.) We allow that deprecation to exist in the ecosystem for a substantial length of time—usually a year or more, but sometimes less if the breaking change is urgently necessary to support a CSS feature—and only once we're confident everyone has had a chance to migrate away from the old behavior do we add the new behavior.

I would suggest a similar pattern here. In Dart 2.12, continue to treat unmarked scripts as being the lowest possible language version, but if they have null-unsafe dependencies produce a warning message that indicates that their version should be explicitly marked. Then in Dart 2.15 or whatever, once null safety is more or less universal, switch the default with way lower risk of breakage.

[gaaclarke]

@nex3 I agree Dart should have avoided this breaking change. Seems like they have enough contextual information to move people to NNBD and not break people in this case, but in the meantime I was able to work around the issues by:
1) Adding language specifications to the top of my scripts
2) Upgrading the test package

Hopefully you can do that, too, to unblock your CI.

[franklinyow]

@leafpetersen I'm assigning this to you, please assign a priority, Thx!