Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null Safety: Sound non-nullable types with incremental migration (NNBD) #110

Closed
14 tasks done
leafpetersen opened this issue Nov 28, 2018 · 96 comments
Closed
14 tasks done
Assignees
Labels
feature Proposed language feature that solves one or more problems nnbd NNBD related issues

Comments

@leafpetersen
Copy link
Member

leafpetersen commented Nov 28, 2018

This is a solution proposal for #71. The proposal covers the intended final language design, and the language, tooling, and ecosystem support required for the migration.

Topic specific discussion issues for resolution:

@leafpetersen leafpetersen added the feature Proposed language feature that solves one or more problems label Nov 28, 2018
@leafpetersen leafpetersen self-assigned this Nov 28, 2018
@mit-mit mit-mit added this to Being discussed in Language funnel Nov 29, 2018
@mit-mit mit-mit moved this from Being discussed to Being spec'ed in Language funnel Nov 30, 2018
@leafpetersen
Copy link
Member Author

I've added a draft of the roadmap for this proposal, linked from the header comment.

@yjbanov
Copy link

yjbanov commented Dec 8, 2018

@jmesserly I'm not sure how to constructively move the NNBD discussion from #125 here, because the my suggestion is all about linking the two issues. But I'll try:

I suggest that, as a first step, we introduce sound non-nullable types as part of immutable types (#125). This has several advantages:

  • It would be a non-breaking change: immutable types is a new feature so having them NNBD doesn't break any existing code.
  • It could have the biggest performance impact: if non-nullability is implemented such that many highly-used types can be inlined, such as double, int, bool, enum, String, we could significantly improve memory layout efficiency.
  • It would give us a chance to get the syntax and semantics right and educate users before rolling NNBD to the "normal" classes, which does require a bigger migration.

@jmesserly
Copy link

IMO, linking the two proposals would be counterproductive. We are working hard on NNBD, there is a lot of design & planning work going on. There aren't any major open questions on syntax or desired semantics. Linking the two would prevent us from pushing forward on NNBD, because immutable types has a lot of open design questions, and we'd have to delay work on NNBD until that's resolved.

(Also it could be confusing if different parts of the language treat int as having different non-nullability. It would be really nice if int always means non-null, and int? always means mean nullable. The NNBD proposal achieves that.)

@yjbanov
Copy link

yjbanov commented Dec 8, 2018

That's fine too, it was just an idea. I don't have enough context to know how best to proceed, so if you believe that NNBD and immutables should proceed completely independently, then they should :)

@jmesserly
Copy link

Makes sense :). Yeah, we seem to be gathering a lot of momentum behind Leaf's proposal. It's really exciting! I think we'll be able to apply the many lessons of Strong Mode to NNBD, to make the migration as easy & rewarding as possible. (e.g. ideally it's non-breaking for code that isn't migrated. Until you turn on NNBD, you don't break. Once you turn it on, you have to fix some errors, but you gain some degree of null safety, even if everything else isn't migrated yet. Once everything in an app is migrated, then it has sound null safety. It's pretty neat.)

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 11, 2018
... which in this CL will always null.
This is the first step when adding NNDB support as outlined in
dart-lang/language#110

Change-Id: If3810bcaf1b73e70924f09d619e2a84e7d5ba8d6
Reviewed-on: https://dart-review.googlesource.com/c/86860
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 11, 2018
This adds code to ignore trailing `?` in `as` and `is` expressions
because the trailing `?` may be part of the larger expression.
At the moment, this change is a no-op, but will become active
when a subsequent CL lands support for nullable types as part of
dart-lang/language#110

Change-Id: I829d6aee0f11957ca9b5e143000005031649449f
Reviewed-on: https://dart-review.googlesource.com/c/86960
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 11, 2018
... as part of adding NNBD as outlined in
dart-lang/language#110

This only supports parsing simple nullable types
such as int? and List<int>? while subsequent CLs
will add support for parsing more complex types.

Change-Id: I3cc5c65d20bf3732a39cab0e823b2f7332342866
Reviewed-on: https://dart-review.googlesource.com/c/86961
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 12, 2018
... which in this CL will always null.
This is another step when adding NNDB support as outlined in
dart-lang/language#110

Change-Id: I3974fcb885c63be4af9d1007258383f3f8191ae0
Reviewed-on: https://dart-review.googlesource.com/c/87080
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 12, 2018
…tion type

This only supports nullable return values of the form

<identifier> '?' 'Function' '(' ...

This is an increment CL in the ongoing effort to add nullable type support
as outlined in dart-lang/language#110

Change-Id: I42febae9f88f7e4d8b05907988deab97c7a7425c
Reviewed-on: https://dart-review.googlesource.com/c/87081
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 13, 2018
This adds support for nullable types of the form

<identifier> '.' <identifier> '?'

and

<identifier> '.' <identifier> '?' 'Function' '(' ...

This is an increment CL in the ongoing effort to add nullable type support
as outlined in dart-lang/language#110

Change-Id: I526aecbe64bacbd442cea0b4c52d36ff23b0443b
Reviewed-on: https://dart-review.googlesource.com/c/87083
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 14, 2018
This reverts commit 7720689.

Reason for revert: Breaks parsing less common conditionals (e.g. b ? c = true : g();)

Original change's description:
> Add support for prefixed nullable type
> 
> This adds support for nullable types of the form
> 
> <identifier> '.' <identifier> '?'
> 
> and
> 
> <identifier> '.' <identifier> '?' 'Function' '(' ...
> 
> This is an increment CL in the ongoing effort to add nullable type support
> as outlined in dart-lang/language#110
> 
> Change-Id: I526aecbe64bacbd442cea0b4c52d36ff23b0443b
> Reviewed-on: https://dart-review.googlesource.com/c/87083
> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
> Commit-Queue: Dan Rubel <danrubel@google.com>

TBR=brianwilkerson@google.com,danrubel@google.com

Change-Id: Ib5e74b4aad239f561a33eae9d95dffa2693037f7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/87282
Reviewed-by: Dan Rubel <danrubel@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 14, 2018
…unction type

This reverts commit 11d081d

Reason for revert: Breaks parsing less common conditionals (e.g. b ? c = true : g();)

Original change's description:
> Add support for simple nullable type return value in generalized function type
>
> This only supports nullable return values of the form
>
> <identifier> '?' 'Function' '(' ...
>
> This is an increment CL in the ongoing effort to add nullable type support
> as outlined in dart-lang/language#110

Change-Id: I99bce29619d4e448193e3c81fa86a982791b1f77
Reviewed-on: https://dart-review.googlesource.com/c/87283
Reviewed-by: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 14, 2018
This reverts commit bc8c7cf

Reason for revert: Breaks parsing less common conditionals (e.g. b ? c = true : g();)

Original change's description:
> Add support for parsing simple nullable types
>
> ... as part of adding NNBD as outlined in
> dart-lang/language#110
>
> This only supports parsing simple nullable types
> such as int? and List<int>? while subsequent CLs
> will add support for parsing more complex types.

Change-Id: I49a21a85dca19241e3b23ed5c9fb6084e70f2000
Reviewed-on: https://dart-review.googlesource.com/c/87284
Reviewed-by: Dan Rubel <danrubel@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
@leafpetersen
Copy link
Member Author

I've updated the header with some sub-issues for discussing specific topics.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 18, 2018
... as part of adding NNBD as outlined in
dart-lang/language#110

This only supports parsing simple nullable types
such as int? and List<int>? while subsequent CLs
will add support for parsing more complex types.

Change-Id: I144858946cb115755af437299899c2631105bf8c
Reviewed-on: https://dart-review.googlesource.com/c/87501
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 18, 2018
This reverts commit c5fd11b.

Reason for revert: Conditional expression lookahead may modify the token stream

Original change's description:
> Add support for parsing simple nullable types
> 
> ... as part of adding NNBD as outlined in
> dart-lang/language#110
> 
> This only supports parsing simple nullable types
> such as int? and List<int>? while subsequent CLs
> will add support for parsing more complex types.
> 
> Change-Id: I144858946cb115755af437299899c2631105bf8c
> Reviewed-on: https://dart-review.googlesource.com/c/87501
> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
> Commit-Queue: Dan Rubel <danrubel@google.com>

TBR=brianwilkerson@google.com,danrubel@google.com

Change-Id: Ie1f47e7384ff51159aa2ebeb21561455b8e6287f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/87620
Reviewed-by: Dan Rubel <danrubel@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 18, 2018
... as part of adding NNBD as outlined in
dart-lang/language#110

This CL updates the parser to recognize two specialized comments at the
beginning of a library of the form '//@NNBD' and '//@NNBD*' for use
by the analyzer to aid developers when converting their libraries
to be non-nullable by default.

In addition, the AstBuilder now populates the generated analyzer AST
with nullable type information.

Change-Id: I80d221dd138973aa32f05bde631245d9ac6ee10f
Reviewed-on: https://dart-review.googlesource.com/c/87540
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 21, 2018
... as part of adding NNBD as outlined in
dart-lang/language#110

This only supports parsing simple nullable types
such as int? and List<int>? while subsequent CLs
will add support for parsing more complex types.

Due to current parser limitations, it also only supports
nullable types in a limited number of statements such as

T? t;
if (x is String?) {}

In addition, address comments in
* https://dart-review.googlesource.com/c/sdk/+/87880
* https://dart-review.googlesource.com/c/sdk/+/87881

Change-Id: Ife4afadc0b72906fc0c4e9b1977ad838041c2a84
Reviewed-on: https://dart-review.googlesource.com/c/87920
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>
@leafpetersen
Copy link
Member Author

Added links to two new discussion issues in the header comment.

@Levi-Lesches
Copy link

Levi-Lesches commented Jan 5, 2021

And then, they will just assume the analyzer is broken and add exclamation marks everywhere when using “foo”.

A very good point. I saw on some other issue that someone proposed to fix this by putting implicit !s during compilation, which defeats the purpose of null safety. I, for one, am a fan of #1201 as it entirely solves this problem.

@leafpetersen
Copy link
Member Author

So, even with sealed classes, the cases where we could choose to promote an instance getter would still be fairly restricted — declared as a (final only?) field in the same class (or at least in the same library, so any breakage will still be immediately visible), and only using this. access

I don't think the this. access is necessary, if the class is sealed. In any case, while I agree that this is a limited case, I am still struck by the fact that this is pretty much exactly the case that Kotlin allows, and people don't seem to find this painful there.

@Timmmm
Copy link

Timmmm commented Jan 6, 2021

I, for one, am a fan of "if-vars" as it entirely solves this problem.

I don't think it entirely solves the problem. How do you use if-vars for code like this?

if (field == null) {
  throw ...
}
var x = field.foo;

@Levi-Lesches
Copy link

Levi-Lesches commented Jan 6, 2021

The author of that proposal included a "negative if-var", which covers exactly that case. It would look exactly like you wrote:

if (var field == null) {
  throw "Error, not allowed to be null";
}
var x = field.foo;

The gist is that when checking to see if an if-var is null (or when using is!), instead of creating a new variable in scope of the curly brackets, it creates a new variable outside the scope of the if statement, and requires that the "then" block exit abnormally (throws, returns, break, continue, etc.). It's an interesting idea, even though I have trouble getting over the non-intuitive scope. See my comment there for more about that.

@derolf
Copy link

derolf commented Jan 6, 2021

Honestly, I think the language should be designed in a way to avoid using the ! whenever possible!

If devs are forced into using ! at "strange" places and to need to scratch their head, they will scratch their head once or twice and then just put ! whenever the compiler complains.

This is why I wrote a linter for a bigger TypeScript project that disallowed the usage of ! completely.

@Timmmm
Copy link

Timmmm commented Jan 6, 2021

I have trouble getting over the non-intuitive scope

Yeah that is confusing. And var field == null is quite close to var field = null which also seems confusing. I think I'd still rather give up field/getter symmetry.

This is why I wrote a linter for a bigger TypeScript project that disallowed the usage of ! completely.

I'd say you wasted some time writing a linter! In any case this isn't a problem in Typescript because Typescript's null checking isn't sound. They just ignore this problem. For example the following code type checks fine but will throw a type error at runtime.

let i = 1;

class Foo {
    public get maybeString(): string | null {
        return ++i % 2 === 0 ? "Dave" : null;
    }
}

function maybeStringLen(f: Foo): number {
    if (f.maybeString === null) {
        return 0;
    }
    return f.maybeString.length;
}

let x = maybeStringLen(new Foo());

@jodinathan
Copy link

I, for one, am a fan of "if-vars" as it entirely solves this problem.

I don't think it entirely solves the problem. How do you use if-vars for code like this?

if (field == null) {
  throw ...
}
var x = field.foo;

I really think that this is a case for a new local variable:

final notNullField = field;

if (notNullField == null) {
  throw ...
}
var x = notNullField.foo;

I am also in favor for simple type and non-null scoped var:

// not null scoped var
if (field != null use notNullField) {
  var x = notNullField.foo;
}

// type scoped var
if (field is SomeClass use fieldAsSomeClass) {
  print(fieldAsSomeClass.foo);
}

the semantics don't bother me as long as I have some simple and intuitive as above

@Timmmm
Copy link

Timmmm commented Jan 6, 2021

I really think that this is a case for a new local variable

But that is what we have to do at the moment. The point of this thread is to find better solutions.

if (field != null use notNullField) {
  var x = notNullField.foo;
}

Another case if-vars don't solve is multiple chained conditions. E.g. in Typescript you can easily do this:

if (field !== null && field.subfield !== null && field.subfield.subsubfield !== null) {
  doStuff(field.subfield.subsubfield);
}

Would you advocate this?

if (field != null use a && a.subfield != null use b && b.subsubfield != null use c) {
  doStuff(c);
}

Surely we can do better than that!

@jodinathan
Copy link

jodinathan commented Jan 6, 2021

I really think that this is a case for a new local variable

But that is what we have to do at the moment. The point of this thread is to find better solutions.

if (field != null use notNullField) {
  var x = notNullField.foo;
}

Another case if-vars don't solve is multiple chained conditions. E.g. in Typescript you can easily do this:

if (field !== null && field.subfield !== null && field.subfield.subsubfield !== null) {
  doStuff(field.subfield.subsubfield);
}

Would you advocate this?

if (field != null use a && a.subfield != null use b && b.subsubfield != null use c) {
  doStuff(c);
}

Surely we can do better than that!

TS is a great language and a very good evolution of JS stuff, but I don't think we should try to strictly follow it.

your example could be simple as:

if (field?.subfield?.subsubfield != null use c) {
  doStuff(c);
}

@Levi-Lesches
Copy link

And with if-vars (positive if-vars -- the thread seems to agree that negative if-vars are too messy for the moment), that would be:

if (var field?.subfield?.subfield != null) {
  doStuff(subfield);
}

Also,

This has now launched in tech preview

I just noticed that all 14 tasks at the top of this issue are complete. (Well, one is not checked off yet but the issue it's linked to is closed.) Does that mean this issue is ready to be closed? Oh how far it's come.

@derolf
Copy link

derolf commented Jan 26, 2021

At least, for private final fields, it should be sound to propagate the null-safety:

class Foo {
  ...
  final Bar? _bar;
  ...
  void baz() {
    if (_bar != null) {
      _bar.somemethod(); <-- is always safe here
    }
  }
}

@jodinathan
Copy link

At least, for private final fields, it should be sound to propagate the null-safety:

class Foo {
  ...
  final Bar? _bar;
  ...
  void baz() {
    if (_bar != null) {
      _bar.somemethod(); <-- is always safe here
    }
  }
}

and if not overridden in any other class in the same file/part

@derolf
Copy link

derolf commented Jan 26, 2021

@jodinathan

and if not overridden in any other class in the same file/part

I know, it's not perfect. But, I am currently migrating a bigger Flutter project with 150k Dart LOC.

The most annoying piece are exactly these constructs where you have final nullable fields. Typical pattern is a widget wrapping some arguments:

class Bla extends StatelessWidget {
  Bla({this.text});
  final String? text;

  @override
  Widget build(BuildContext) => text == null ? SizedBox.shrink() : Text(text);
}

@jodinathan
Copy link

@jodinathan

and if not overridden in any other class in the same file/part

I know, it's not perfect. But, I am currently migrating a bigger Flutter project with 150k Dart LOC.

The most annoying piece are exactly these constructs where you have final nullable fields. Typical pattern is a widget wrapping some arguments:

class Bla extends StatelessWidget {
  Bla({this.text});
  final String? text;

  @override
  Widget build(BuildContext) => text == null ? SizedBox.shrink() : Text(text);
}

I agree.
One of the reasons that we are not migrating to NNBD is the non-null promotion issue

@rrousselGit
Copy link

rrousselGit commented Jan 26, 2021

Sealed classes or structures would make this a lot less painful

The problem exists because of inheritance. By being able to store state in an inheritance-free manner, the type-inference could be much better

In the end it isn't just about null-testing here. All kinds of testing suffer from the same issue, such as:

final Object something
...

if (something is MyClass) {
  (something as MyClass).doSomething();
}

@derolf
Copy link

derolf commented Jan 28, 2021

While migrating a 150k project, I run into the following problem:

///
void foo<T>(T bar) {
  switch (T) {
    case String:
      break;
    case double:
      break;
    default:
      throw Exception('Unsupported type $T');
  }
}

If T is nullable, it throws, so how can I add nullable Ts to the case?

@rrousselGit
Copy link

You can do:

Type _typeOf<T>() => T;

if (T == _typeOf<String?>())

That will require to not use a switch-case though

@lrhn
Copy link
Member

lrhn commented Jan 28, 2021

@derolf You cannot add nullable types to the switch. Only constants can be used as switch case expressions, and there is no syntax for creating constant nullable type literals.

You'll have to use a non-constant approach instead.
One alternative:

Type typeOf<T>() => T; // Helper function.
final Type _nullableString = typeOf<String?>();
final Type _nullableDouble = typeOf<String?>();
void foo<T>(T bar) {
  var type = T;
  if (type == String || type == _nullableString || type == double || type == _nullableDouble) return;
  throw Exception("Unsupported type $type");
}

This will allow you to check against any type.

I'd probably rewrite it to avoid the Type equality checks. If you want to support both String? and String, maybe just check that T is a subtype of String?. Then I'd do:

bool _isSubType<S, T>() => <S>[] is List<T>;
void foo<T>(T bar) {
  if ((_isSubtype<T, String?>() || _isSubtype<T, double?>()) &&
      !(_isSubtype<T, Null>()) {
    return;
  }
  throw ....
}

@derolf
Copy link

derolf commented Jan 28, 2021

@rrousselGit @lrhn thanks for the hints, I am using T.toString() and switch on it. After fixing ~4000 nullability issues, the app now runs. Btw I tried to use the "migration tool", but had to completely revert it because it was adding way too many "!" everywhere.

So, I am fixing the tests now.

Does anyone has an idea how to run tests from Visual Code while passing --no-sound-null-safety to Dart???

Options:

  • Adding "@Dart = 2.11" to all tests -> doesn't work because the tests themselves are null safe
  • Environment variable to turn on "no-sound-null-safety"?
  • Something in pubspec.yaml to turn on "no-sound-null-safety"?
  • Some option in the visual code plugin?

@derolf
Copy link

derolf commented Jan 28, 2021

.vscode/settings.json:

{
    "dart.flutterTestAdditionalArgs": [
        "--no-sound-null-safety"
    ],
}

@Levi-Lesches
Copy link

You cannot add nullable types to the switch. Only constants can be used as switch case expressions, and there is no syntax for creating constant nullable type literals.

Was there talk about having something like T is Object? a while ago? (I get that it still wouldn't fit in a switch-case, just curious).

@olfek
Copy link

olfek commented Feb 16, 2021

Hello, when is this feature being released?

@leafpetersen
Copy link
Member Author

This feature has been released in the beta channel and should land in stable shortly.

@olfek
Copy link

olfek commented Feb 17, 2021

@leafpetersen What is shortly? Few days? weeks? months? I am looking to start a new web service project in Dart.

@leafpetersen
Copy link
Member Author

@olfek I suspect that the PMs would be sad if I stole their thunder, so I think I'll leave it at shortly. :) The feature has been released in a beta build, and the general cadence of stable releases is public info, so you can draw your own conclusions.

That said, I would discourage you from starting a new project with null safety enabled unless you're fairly confident that you will not subsequently need to take on a dependency which is not yet null safe. We are strongly encouraging folks to migrate in "waterfall" order (dependencies first). Starting with a null safe app and then adding a non-null safe dependency will require you to jump through some hoops and switch back to unsound mode. So if you're expecting to not have many package dependencies (or you can validate that all of your expected dependencies already have null safe releases), then by all means, grab a beta-channel SDK and go for it, but otherwise I might suggest waiting until the package ecosystem has a bit more time to migrate.

@mit-mit
Copy link
Member

mit-mit commented Feb 22, 2021

Please see this blog post for our latest update on migrating to null safety:

https://medium.com/dartlang/preparing-the-dart-and-flutter-ecosystem-for-null-safety-e550ce72c010

@mit-mit
Copy link
Member

mit-mit commented Mar 3, 2021

As of Dart 2.12 -- launched to the stable channel today -- null safety is now available!
https://medium.com/dartlang/announcing-dart-2-12-499a6e689c87

@mit-mit mit-mit closed this as completed Mar 3, 2021
@mit-mit mit-mit moved this from Being implemented to Done in Language funnel Mar 3, 2021
@gmpassos
Copy link

gmpassos commented Mar 8, 2021

What a nice thread, 2+ years of hard work! I'm enjoying Dart Null Safety, it's the future of modern languages. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems nnbd NNBD related issues
Projects
Development

No branches or pull requests