How should we hint/lint about "unnecessary" null checks during the migration period?

[stereotype441]

While working on migrating dart:ui, @yjbanov noticed a lot of instances of the analyzer hint unnecessary_null_comparison hint, especially for code like this:

void f(C c) {
  assert(c != null);
  ...
}

(this arose because the migration tool could not find any code that passed a null value into c, so it made its type non-nullable). At first glance, it might seem sensible to remove the assertion on the grounds that c can no longer be null, but that's only true in strong mode. Flutter will have a long tail of users running in weak mode until Dart 3.0, and assertions like these are necessary to help prevent them from passing nulls to Flutter APIs. It's going to require some discipline on the part of the Flutter team to remember not to delete these assertions, and that discipline will be made a lot more difficult by hints like this one.

Maintainers of Dart packages are going to run into a similar problem during the migration period. This makes me wonder if we should:

(a) remove the unnecessary_null_comparison hint so that users aren't fooled by it into removing assertions like these?

(b) exclude certain patterns from firing the unnecessary_null_comparison hint, such as assert(x != null) and if (x == null) { throw ... }?

(c) add a lint suggesting that users add assert(x != null) for non-nullable function parameters?

[bwilkerson]

Flutter will have a long tail of users running in weak mode until Dart 3.0 ...

Do we have any guess as to when 3.0 will ship? Are we talking months or years?

... and assertions like these are necessary to help prevent them from passing nulls to Flutter APIs.

I thought that the semantics were that a null check would be automatically inserted when invoking a function in a library that is opted-in to Null Safety is invoked from an opted-out library. @leafpetersen I'm guessing, based on this issue, that that's wrong. Did we consider that alternative? It seems like it would make transitioning much easier on package authors.

In any case, I don't understand how assertions help. Because most apps are run with assertions disabled, these assertions won't actually prevent anything unless clients (a) have tests that pass null in whenever a null could be passed in during production and (b) they run tests with assertions enabled.

(a) remove the unnecessary_null_comparison hint so that users aren't fooled by it into removing assertions like these?

That seems like a really big hammer, and I'd advise against it. There are plenty of places where the hint could still be useful to users (see below for my reasoning).

(b) exclude certain patterns from firing the unnecessary_null_comparison hint, such as assert(x != null) and if (x == null) { throw ... }?

I think this approach has some promise, but I think you have the wrong patterns. I think the real pattern we're looking for here is any place that a value from an opted-out library can be passed to an opted-in library. In other words, parameters in public APIs. In those cases we'd need to assume that the value might be null if it were invoked from an opted-out library and hence conclude that the test isn't unnecessary.

At some point we're going to want to be able to stop ignoring such places. Maybe that point is when we ship 3.0, but I'll suggest an alternative in case it hasn't been thought of before. The question is: when can the analyzer safely assume that a given library will not be invoked from opted-out libraries? For packages that aren't published, that might be the day they're converted to having all of their own libraries and depended on packages opted-in. For others that might be much later. If we can't automatically guard against null values, it might be useful to have a way for package authors to specify that a given version of their package can't be used in weak mode. That would allow a package author to decide when to stop supporting weak mode rather than forcing them to support weak mode until 3.0 ships (or beyond if some clients will want to be able to use newer versions of the package while still running on pre-3.0 SDKs).

(c) add a lint suggesting that users add assert(x != null) for non-nullable function parameters?

This seems like an orthogonal question to me. As stated above, I don't know what value there is in having these assertions, but adding the lint without addressing the hint would only create a situation where users can't avoid having one diagnostic or the other.

[jonahwilliams]

The value of assert(x != null) is the instead of hitting an NPE somewhere deep in the flutter engine code (which users may not understand), we show something as close as possible to the call site. In a non-null safe world this seems like a pretty reasonable compromise for Flutter's workflow where most development is done with assertions enabled.

[leafpetersen]

@jonahwilliams There's no question that there is value in having null checks on non-nullable variables in the interim. In the Dart SDK, we migrated our existing defensive null checks to use ArgumentError.checkNotNull with the plan to delete all uses of that when we convert to Dart 3. We discussed in the language meeting this morning the possibility of adding this as public API, with the intent to deprecate it in Dart 3. That way we could point people at it (possibly even in text of the hint above), as the correct way to check for null in the interim.

Another thing we've discussed is auto-inserting assert(x != null) for every method parameter of non-nullable type when running in unsound mode. The issue with this is that it's technically breaking, and we've promised not to break people. It's a bit squishy, since it only breaks when asserts are enabled. If flutter would want to turn this on its development, we could probably still provide this.

[lrhn]

For SDK-internal code, we use checkNotNullable from dart:_internal for checking the null-ness of non-nullable variables. Easier to find and remove than ArgumentError.checkNotNull.

(I'm all for adding asserts to check null safe code parameters! I'd add real checks if it could be done cheaply enough!)

[jonahwilliams]

The Argument.nonNull would work well for functions, but isn't usable in const constructors which limits how widely we could use it.

Anyway, you're right this this might only be an issue for Flutter. Applications likely won't care, and at least in the flutter_tool we'd probably prefer removing the null checks early. But I think the usability issues will ultimately depend on how long the migration period is, which I don't really know

[leafpetersen]

Another thing we've discussed is auto-inserting assert(x != null) for every method parameter of non-nullable type when running in unsound mode

This is probably still an option, but it's not something we'd want to do unless we were fairly confident that you guys would want to turn this on. Technically it makes your migration more breaking (at least in compatibility mode). You may migrate an API to take a non-nullable type, but someone may be using with null it in a way that "just works". Auto-adding these inserts would break that user even in legacy mode (albeit only when asserts were turned on).

cc @Hixie

FWIW, this is implemented in DDC, so you could fairly easily test it out in Flutter Web (it currently only prints a warning in the console, but that's easily adjusted).

[Hixie]

I thought that the semantics were that a null check would be automatically inserted when invoking a function in a library that is opted-in to Null Safety is invoked from an opted-out library.

I also thought this. It doesn't seem to make any sense to do anything else? Why would you ever want to pass a null into an API that specifically doesn't accept nulls?

Technically it makes your migration more breaking (at least in compatibility mode). You may migrate an API to take a non-nullable type, but someone may be using with null it in a way that "just works".

That's good. Breaking such usages is the whole point. These are bugs, the whole point of NNBD is to catch bugs like this.

[leafpetersen]

I also thought this. It doesn't seem to make any sense to do anything else? Why would you ever want to pass a null into an API that specifically doesn't accept nulls?

Only that we're pushing hard to make this as strictly non-breaking for un-migrated users as possible.

That's good. Breaking such usages is the whole point. These are bugs, the whole point of NNBD is to catch bugs like this.

Ok, I'll see if I can make this happen.

[davidmorgan]

FWIW, for google3 the consideration is pretty much the opposite: we will be going through a long incremental migration, and we can't afford lots of small changes to runtime behaviour along the way. We want the runtime changes in one go when we enable strong mode.

Using assert for the checks is a plausible path since we can turn off asserts if/when we need to. (I think, we mostly have them already turned off everywhere).

Thanks.