dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Analyzer is missing an error case for "if (null)" #36193

Closed lukepighetti closed 4 years ago

lukepighetti commented 5 years ago

Consider the following:

final String foo = null;

void main()  {
  if(foo?.isNotEmpty){
    print("true");
  } else {
    print("not true");
  }
}

Even though this executes and prints not true, there is a compiler warning that states:

The value of the '?.' operator can be 'null', which isn't appropriate in a condition.

However, if we change it to explicitly null, there is no warning!

void main()  {
  if(null){
    print("true");
  } else {
    print("not true");
  }
}

I would argue that if(foo?.isNotEmpty) should not produce a compiler warning since the conditional checks if the value is true. It doesn't check if the value is not false.

A common developer solution to this problem seems to be the following:


final String foo = null;

void main()  {
  if(foo?.isNotEmpty ?? false){
    print("true");
  } else {
    print("not true");
  }
}

I think this little lint/warning promotes codegolf when it could silently fail without issue.

matanlurey commented 5 years ago

Sorry, this warning is not unnecessary, as Dart does not allow null in conditionals since Dart 2:

void main() {
  if (null) {
    print('Hello');
  }
}
$ dart /tmp/null.dart

Unhandled exception:
Failed assertion: boolean expression must not be null
#0      main (file:///tmp/null.dart)
#1      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:300:19)
#2      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:171:12)

If you are not seeing this error, it is a bug. Analyzer should be reporting an error as well.

lukepighetti commented 5 years ago

For reference:

Screen Shot 2019-03-13 at 7 17 25 PM
eernstg commented 5 years ago

This will be a bug at the point where the plain type bool does no longer mean bool | Null (that is, when we have introduced non-null-by-default), but currently the static analysis requires only that

It is a compile-time error if the type of the expression $b$
may not be assigned to \code{bool}.

where $b$ is the condition of an if statement. Null is assignable to bool today, so there is no compile-time error (not even with --no-implicit-casts, because this is an upcast).

However, it has always been a dynamic error for such a condition to be null at run time (in Dart 1 checked mode, and in Dart 2), so there is a bug here: dart2js compiled code prints 'Are you sure?', and that is a bug. As @matanlurey mentioned, the VM raises a dynamic error and prints 'boolean expression must not be null'. So the bug does not exist in the VM.

I'll change the labels of this issue such that it targets dart2js and not the analyzer.

We're working hard on support for non-null types, so it will be a compile-time error as well, just not immediately. Of course, it could be the target of a lint in case we think it's sufficiently common.

lrhn commented 5 years ago

There is alread a open issue for dart2js, so let's keep this issue open as an analyzer feature request to do better.

The spec (here from the if statement condition) says that

It is a compile-time error if the type of the expression b may not be assigned to bool.

but it is actaully guaraneteed to be a runtime error if the type of the condition is not a supertype of bool. The analyzer could give a warning in that case, even if the specification allows it as written.

eernstg commented 5 years ago

I added a reference to this issue in #34147, such that this known dart2js bug and a couple of similar ones aren't forgotten due to the 'area-analyzer' label here.

srawlins commented 4 years ago

Good news, dartanalyzer --enable-experiment=non-nullable reports:

error • Conditions must have a static type of 'bool'. • 36193.dart:2:6 • non_bool_condition