Skip to content

Assignment expressions in if-statements don't correctly promote nullable variables #3658

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

Open
skylon07 opened this issue Mar 18, 2024 · 5 comments
Labels
flow-analysis Discussions about possible future improvements to flow analysis request Requests to resolve a particular developer problem

Comments

@skylon07
Copy link

skylon07 commented Mar 18, 2024

This program errors:

void main() {
  String? getString() => "asdf";

  String? someString;
  if ((someString = getString()) != null) {
    // errors:
    // "The property 'isNotEmpty' can't be unconditionally accessed because the receiver can be 'null'."
    print(someString.isNotEmpty);
  }
}

This one does not:

void main() {
  String? getString() => "asdf";

  String? someString;
  someString = getString();
  if (someString != null) {
    // does not error
    print(someString.isNotEmpty);
  }
}

I feel like the first program shouldn't error... Doesn't it guarantee that someString is not nullable just as much as the second program does? I might be wrong here -- I don't usually use assignment syntax in if-statements like this. (If you're curious, I'm using this syntax to store the results of a chain of nullable RegExp matches, which looks neater than having several assignments put between unchained if-statements.)

Infos

  • Dart 3.3.1 (stable) (Wed Mar 6 13:09:19 2024 +0000) on "macos_arm64"
  • on macos / Version 14.0 (Build 23A344)
  • locale is en-US
@eernstg
Copy link
Member

eernstg commented Mar 18, 2024

There is a request for generalisation of the flow analysis in #1224, in order to get a larger set of reasons for promoting a local variable. In that issue the topic is that if (foo?.something == somethingNotNull) ... could promote foo to have a non-nullable type in the true continuation (because the condition would necessarily be false when foo == null).

This issue is similar in that it is a request for detecting that x can't be null in the true continuation of (x = e) != null.

I'll move this issue to the language repository and label it in a way which is similar to the existing issue.

@eernstg eernstg transferred this issue from dart-lang/sdk Mar 18, 2024
@eernstg eernstg added flow-analysis Discussions about possible future improvements to flow analysis request Requests to resolve a particular developer problem labels Mar 18, 2024
@eernstg
Copy link
Member

eernstg commented Mar 18, 2024

@stereotype441, @johnniwinther, it seems that we may have a group of somewhat similar requests in this area (at least, #1224 seems related). Would you prefer to make it a single issue covering all the variants? What's the best labeling?

@lrhn
Copy link
Member

lrhn commented Mar 19, 2024

A single meta-issue with links to individual issues for concrete enhancement ideas is probably better than combining different ideas into one issue, in case we choose to do some, but not all, of them.

@eernstg
Copy link
Member

eernstg commented Mar 19, 2024

Maybe we'd just create a new label (e.g., enhanced-promotion) and then use that for proposals about new ways to establish that a given promotion is justified.

@stereotype441
Copy link
Member

This came up in some internal code recently. Here's a stripped-down version of what the code looked like (with the identifier names and types changed to protect IP):

void main() {
  int? x;
  [
    for (var y in [1, 2, null, 4, 5])
      if ((x = y) != null) x!.isEven, // It would be nice if the `!` in `x!` were unnecessary
  ];
}

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 10, 2024
…statements.

This CL adds language tests for this issue and makes sure the feature isn't enabled if `inference-update-4` feature flag isn't enabled.

Companion CL for https://dart-review.googlesource.com/c/sdk/+/388903 which adds the behaviour for promoting in assignment expressions of if-statements.

Bug: dart-lang/language#3658
Change-Id: I6c8d7f922b4c50d4655202e61cebaf8891daee0f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389223
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 17, 2024
Assignments in the condition of an if statements don't store the promoted information (for the write of that variable). This fix stores the promotion of that expression for when we use it to null check or otherwise.

Part 1 because there's still some holes with postfix operators and null asserts that need to be fixed, but this behaviour stands on its own at the moment.

Everything is behind the flag so we'll iterate.

Bug: dart-lang/language#3658
Change-Id: I8663f089a451468651efccadeb3991b34a37d899
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/388903
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 31, 2024
This is a small change that allows forwarding let expressions for IfNullSets in the CFE.

Since we now store the promotion information for a written expression in an if-statement, the CFE builds a let expression and it's not recognized by flow analysis. To fix this, we add a forward so flow analysis recognizes that it's the same expression that it's trying to find.

language/inference_update_4/assignment_promotion_in_if_statement_test passes from this change.

Bug: dart-lang/language#3658
Change-Id: I4ec99de036498869b0d9c5c652ca748d9f5e570a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/391861
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 31, 2024
…fix inc/dec expressions.

Postfix increment and decrement expressions should not be saving any promotion information.

An example of where saving the promotion information after the write is unsafe is:
```
class A {
  A operator +(int i) {
    return new B();
  }
}

class B extends A {}

main() {
  A x = A();
  if ((x++) is B) {
    // x should not be B
  }
}
```

This change is only for the analyzer because the CFE does something different (converts the postfix increment/decrement into a let expression).

Bug: dart-lang/language#3658
Change-Id: Ic22f69bf79da66965908ade80bdf70399f0bcaa3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/391494
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Nov 5, 2024
Add Flow Analysis unit tests for FlowAnalysis.postIncDec() which should be used for postfix increment and decrement operations.

Bug: dart-lang/language#3658
Change-Id: If77b6fbb5fc80d5f5d014ec0516d77578446dced
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393441
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flow-analysis Discussions about possible future improvements to flow analysis request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

4 participants