Skip to content

Flow analysis doesn't work after Null check operator #42021

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

Closed
sgrekhov opened this issue May 22, 2020 · 7 comments
Closed

Flow analysis doesn't work after Null check operator #42021

sgrekhov opened this issue May 22, 2020 · 7 comments
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P4

Comments

@sgrekhov
Copy link
Contributor

The following test fails

main () {
  late int i;
  String? s = "";
  if (s! == null) {
    i = 42;  // Variable is initialized in a dead code. This leaves it definitely unassigned
  }
  i;  // It is an error to read a local late variable when it is definitely unassigned.
//^
// [analyzer] unspecified
// [cfe] unspecified
}

There must be a compile time error but in fact analyzer produces hints only

Analyzing reachability_A06_t01.dart...
hint - The operand can't be null, so the condition is always false. - reachability_A06_t01.dart:22:10 - unnecessary_null_comparison               
1 hint found.

CFE also produces no error but LateInitializationError at a runtime only

If to modify the test and replace s! == null by false the test starts to work as expected (produces a compile time error)

main () {
  late int i;
  String? s = "";
  if (false) {
    i = 42;  // Variable is initialized in a dead code. This leaves it definitely unassigned
  }
  i;  // There is an expected error here
//^
// [analyzer] unspecified
// [cfe] unspecified
}

Dart VM version: 2.9.0-10.0.dev (dev) (Tue May 19 15:16:48 2020 +0200) on "windows_x64"

@lrhn
Copy link
Member

lrhn commented May 26, 2020

I'm not sure we recognize "definitely false" checks as causing dead code in the specified definite-assignment algorithm. The analyzer might be doing more than required here when it recognizes the value as always false (that's why it's only a hint), and in that case, it can't change the language semantics.

@lrhn lrhn added the legacy-area-analyzer Use area-devexp instead. label May 26, 2020
@srawlins srawlins added the dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec label Jun 16, 2020
@srawlins
Copy link
Member

I think then this goes back to the flow analysis spec. @stereotype441

@srawlins srawlins added NNBD Issues related to NNBD Release P4 labels Jan 23, 2021
@stereotype441
Copy link
Member

Yeah, this behavior is surprising, but believe it or not it is working as intended. Here's the subtle reason why we need flow analysis to behave this way. When a mixed version program is run with unsound null safety, it's possible for a variable whose type is (non-nullable) String to nonetheless contain a null value, for example:

// foo.dart
// @dart = 2.9
import 'bar.dart';
main() {
  // It's valid to pass `null` to `bar` because null safety is disabled in this file.
  bar(null);
}
// bar.dart
// @dart = 2.12
bar(String s) {
  if (s == null) {
    // It might seem like this code should be unreachable, because this file has
    // null safety enabled, and `s` has a non-nullable type.  But actually it's
    // reachable because of the non-null-safe code in `foo.dart`.
  }
}

We want flow analysis to behave the same regardless of whether the program is being compiled with sound or unsound null safety (because we don't want people to start getting compile errors if they switch modes), so that means that flow analysis has to treat the "if" block as reachable even for fully sound programs.

Similarly, we want flow analysis to be based on the types of expressions; we don't want it to perform extra reasoning steps that go beyond the type system. So for consistency, we need it to treat any if (<expr> == null) block as reachable, even if <expr> has a non-nullable type.

So, returning to the original example:

main () {
  late int i;
  String? s = "";
  if (s! == null) {
    i = 42;
  }
  i;
}

Even though we can see from human reasoning that i = 42; is dead code, flow analysis can't see that; all it sees is that s! has type String, and it reasons that an expression having type String might still be null due when run with unsound null safety. So it considers i = 42; to be live, and therefore it doesn't issue an error at i;.

You can find more information at dart-lang/language#1143 about why we decided to make things work this way. Hope that helps!

@diego-lipinski-de-castro

So how do I fix this? this seems unlogical. It says the operand cant be null and I cannot understand why when I explicit declared it as nullable
Captura de Tela 2021-04-13 às 16 03 04

@stereotype441
Copy link
Member

@diego-lipinski-de-castro The short answer is that I think you want this:

Delivery? driverDelivery = driverDeliveries.firstWhereOrNull((d) => d.id == deliveryId);

Note that firstWhereOrNull is defined in an extension so you may have add an import statement like this to your file:

import 'package:collection/collection.dart' show IterableExtension;

And you may have to upgrade your pubspec to ensure that you get a version of package:collection that includes this extension (anything after 1.15.0-nullsafety.4 should be fine).

The longer answer is that orElse: null is probably not doing what you thought it was doing. It doesn't mean "if nothing can be found, return null". It means "I'm not supplying an orElse function, so if nothing can be found, please throw an exception". So that's why the analyzer is saying that driverDelivery can't be null when you check it later.

Before null safety, if you wanted the behavior "if nothing can be found, return null", the way to ask for it was like this:

Delivery? driverDelivery = driverDeliveries.firstWhere((d) => d.id == deliveryId, orElse: () => null);

(Note the () => part, which means that you're passing a function in for orElse, that function will get called if nothing can be found.)

Unfortunately, this doesn't work with null safety, because the return type of the orElse function has to match the element type of whatever you're iterating over (which in your case I believe is the non-nullable type Delivery). You can't use a null where a non-nullable Delivery is expected, so you can't do orElse: () => null. We added the firstWhereOrNull method precisely to address this situation.

@diego-lipinski-de-castro

Thanks, I got what I was doing wrong due to your return, is this documented somewhere? Especially the firstWhereOrNull part, I dont think I would find this elsewhere

@stereotype441
Copy link
Member

Thanks, I got what I was doing wrong due to your return, is this documented somewhere? Especially the firstWhereOrNull part, I dont think I would find this elsewhere

Good point. It should be better documented. I filed #45680 to track this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P4
Projects
None yet
Development

No branches or pull requests

5 participants