Skip to content

Flow analysis should treat 's != null' as true if s is non-nullable #41985

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.

Sign up for GitHub

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

Jump to bottom
Closed
sgrekhov opened this issue May 20, 2020 · 0 comments
Closed

Flow analysis should treat 's != null' as true if s is non-nullable #41985

sgrekhov opened this issue May 20, 2020 · 0 comments
Assignees
@stereotype441
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. NNBD Issues related to NNBD Release

Comments

@sgrekhov
Copy link
Contributor

This issue is similar to #41981. The following code produces an error in both analyzer and CFE

main() {
  int i;
  String s = "";
  if (s != null) {
    i = 42;
  }
  i; //  Error: Non-nullable variable 'i' must be assigned before it can be used.
}

Here s is non-nullable so s != null is always true and therefore i became definitely assigned. It's not an error to read local non nullable variable if it is definitely assigned. Note that code below works

main() {
  int i;
  String s = "";
  if (true) {
    i = 42;
  }
  i; // No error in this case

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

@a-siva a-siva added legacy-area-front-end Legacy: Use area-dart-model instead. legacy-area-analyzer Use area-devexp instead. labels May 20, 2020
@kevmoo kevmoo changed the title Flow analysis should treat 's != null' as true is s is non-nullable Flow analysis should treat 's != null' as true if s is non-nullable May 20, 2020
@srawlins srawlins added NNBD Issues related to NNBD Release dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec labels Jun 16, 2020
@stereotype441 stereotype441 self-assigned this Jul 24, 2020
stereotype441 added a commit to stereotype441/language that referenced this issue Aug 5, 2020
@stereotype441
Rework flow analysis of == and !=.
39b4759 
Previously, we specified flow states `null()` and `nonNull()` (similar
to `true()` and `false()`), and the special behaviors of `== null` and
`!= null` checks fell out of a general analysis of `E1 == E2`.  But it
didn't work in practice, because it required doing joins like
`join(null(E1), null(E2))`, which isn't sound in general (because it
carries the flow control implication that either E1 or E2 is evaluated
but not both).  Because of these problems, the implementation never
matched this specification, and instead used ad-hoc pattern matching
to recognize `== null` and `!= null`.

In the new formulation, we remove the flow states `null()` and
`nonNull()` and retain the ad-hoc pattern matching for `== null` and
`!= null`, consistent with the current implementation.  However, we
also add cases to recognize when an equality test is guaranteed to
evaluate to `true` or `false`; this addresses
dart-lang/sdk#41985 and ensures that we
recognize dead code associated with unnecessary null checks.
@stereotype441 stereotype441 mentioned this issue Aug 5, 2020
Merged
stereotype441 added a commit to stereotype441/flutter that referenced this issue Aug 5, 2020
@stereotype441
Ignore dead_code hints for weak-only null checks.
8c89a33 
When dart-lang/sdk#41985 is fixed, the
analyzer will begin detecting dead code due to "unnecessary" null
checks.  Since we want to retain null checks in Flutter even when they
appear unnecessary (in order to preserve runtime behavior in weak
mode), we need to suppress these dead code hints.
This was referenced Aug 5, 2020
Merged
Closed
stereotype441 added a commit to flutter/flutter that referenced this issue Aug 11, 2020
@stereotype441
Ignore dead_code hints for weak-only null checks. (#63007)
a4254c7 
* Ignore dead_code hints for weak-only null checks.

When dart-lang/sdk#41985 is fixed, the
analyzer will begin detecting dead code due to "unnecessary" null
checks.  Since we want to retain null checks in Flutter even when they
appear unnecessary (in order to preserve runtime behavior in weak
mode), we need to suppress these dead code hints.

* Add comments explaining why we're ignoring dead_code hints
stereotype441 added a commit to dart-lang/language that referenced this issue Aug 12, 2020
@stereotype441
Rework flow analysis of == and !=. (#1134)
b901765 
Previously, we specified flow states `null()` and `nonNull()` (similar
to `true()` and `false()`), and the special behaviors of `== null` and
`!= null` checks fell out of a general analysis of `E1 == E2`.  But it
didn't work in practice, because it required doing joins like
`join(null(E1), null(E2))`, which isn't sound in general (because it
carries the flow control implication that either E1 or E2 is evaluated
but not both).  Because of these problems, the implementation never
matched this specification, and instead used ad-hoc pattern matching
to recognize `== null` and `!= null`.

In the new formulation, we remove the flow states `null()` and
`nonNull()` and retain the ad-hoc pattern matching for `== null` and
`!= null`, consistent with the current implementation.  However, we
also add cases to recognize when an equality test is guaranteed to
evaluate to `true` or `false`; this addresses
dart-lang/sdk#41985 and ensures that we
recognize dead code associated with unnecessary null checks.
stereotype441 added a commit to stereotype441/flutter that referenced this issue Aug 12, 2020
@stereotype441
Ignore another dead_code hint for a weak-only null check.
cf75605 
When dart-lang/sdk#41985 is fixed, the
analyzer will begin detecting dead code due to "unnecessary" null
checks.  Since we want to retain null checks in Flutter even when they
appear unnecessary (in order to preserve runtime behavior in weak
mode), we need to suppress these dead code hints.
@stereotype441 stereotype441 mentioned this issue Aug 12, 2020
Merged
13 tasks
stereotype441 added a commit to stereotype441/flutter that referenced this issue Aug 14, 2020
@stereotype441
Ignore/fix dead_code checks for weak-only null checks.
df142fb 
Now that dart-lang/sdk#41985 is fixed, the
analyzer detects dead code due to "unnecessary" null checks.  Since we
want to retain null checks in Flutter even when they appear
unnecessary (in order to preserve runtime behavior in weak mode), we
need to suppress these dead code hints.

Note that one assertion is removed by this PR (`heightFactor != null
|| (heightFactor == 1.0 && heightDelta == 0.0)`).  This is because
`heightFactor == 1.0 && heightDelta == 0.0` can only be true if
`heightFactor != null`, so this assertion is equivalent to simply
asserting that `heightFactor != null`, which is already asserted two
lines above.
@stereotype441 stereotype441 mentioned this issue Aug 14, 2020
Merged
13 tasks
stereotype441 added a commit to flutter/flutter that referenced this issue Aug 14, 2020
@stereotype441
Ignore/fix dead_code checks for weak-only null checks. (#63794)
d80aa48 
Now that dart-lang/sdk#41985 is fixed, the
analyzer detects dead code due to "unnecessary" null checks.  Since we
want to retain null checks in Flutter even when they appear
unnecessary (in order to preserve runtime behavior in weak mode), we
need to suppress these dead code hints.

Note that one assertion is removed by this PR (`heightFactor != null
|| (heightFactor == 1.0 && heightDelta == 0.0)`).  This is because
`heightFactor == 1.0 && heightDelta == 0.0` can only be true if
`heightFactor != null`, so this assertion is equivalent to simply
asserting that `heightFactor != null`, which is already asserted two
lines above.
smadey pushed a commit to smadey/flutter that referenced this issue Aug 27, 2020
@stereotype441 @smadey
Ignore/fix dead_code checks for weak-only null checks. (flutter#63794)
64a5bcb 
Now that dart-lang/sdk#41985 is fixed, the
analyzer detects dead code due to "unnecessary" null checks.  Since we
want to retain null checks in Flutter even when they appear
unnecessary (in order to preserve runtime behavior in weak mode), we
need to suppress these dead code hints.

Note that one assertion is removed by this PR (`heightFactor != null
|| (heightFactor == 1.0 && heightDelta == 0.0)`).  This is because
`heightFactor == 1.0 && heightDelta == 0.0` can only be true if
`heightFactor != null`, so this assertion is equivalent to simply
asserting that `heightFactor != null`, which is already asserted two
lines above.
mingwandroid pushed a commit to mingwandroid/flutter that referenced this issue Sep 6, 2020
@stereotype441 @mingwandroid
Ignore dead_code hints for weak-only null checks. (flutter#63007)
2884788 
* Ignore dead_code hints for weak-only null checks.

When dart-lang/sdk#41985 is fixed, the
analyzer will begin detecting dead code due to "unnecessary" null
checks.  Since we want to retain null checks in Flutter even when they
appear unnecessary (in order to preserve runtime behavior in weak
mode), we need to suppress these dead code hints.

* Add comments explaining why we're ignoring dead_code hints
mingwandroid pushed a commit to mingwandroid/flutter that referenced this issue Sep 6, 2020
@stereotype441 @mingwandroid
Ignore/fix dead_code checks for weak-only null checks. (flutter#63794)
32b5498 
Now that dart-lang/sdk#41985 is fixed, the
analyzer detects dead code due to "unnecessary" null checks.  Since we
want to retain null checks in Flutter even when they appear
unnecessary (in order to preserve runtime behavior in weak mode), we
need to suppress these dead code hints.

Note that one assertion is removed by this PR (`heightFactor != null
|| (heightFactor == 1.0 && heightDelta == 0.0)`).  This is because
`heightFactor == 1.0 && heightDelta == 0.0` can only be true if
`heightFactor != null`, so this assertion is equivalent to simply
asserting that `heightFactor != null`, which is already asserted two
lines above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stereotype441 stereotype441

Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. NNBD Issues related to NNBD Release
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@srawlins @stereotype441 @sgrekhov @a-siva