dart-lang / dart-lang/language

Promotion of variables after assignment in try

Open
#1,976 6 comments 0 reactions 0 assignees View on GitHub
flow-analysis
Dominant language
TeX
Stars
2.9k
Forks
239
Avg merge
2d 18h
Merged PRs (30d)
14

Description

Take the following code:

```dart
import 'dart:math';

void main() {
String? s1;
String? s2;
Random r = new Random();
bool b1 = r.nextBool();
bool b2 = r.nextBool();
if (b1) {
s1 = giveMeString();
s2 = giveMeString();
}
if (s1 == null) {
print("s1 was null");
return;
}
if (s2 == null) {
print("s2 was null");
return;
}
// s1 and s2 are now non-null.
onlyAcceptsString(s1);
onlyAcceptsString(s2);

try {
// s1 = giveMeMaybeString(b2);
s1 = giveMeStringOrCrash(b2);
} catch (e) {
print("Crashed!");
}
if (!b2) {
s2 = giveMeStringOrCrash(b2);
}

// Now s1 is nullable.
onlyAcceptsString(s1);
onlyAcceptsString(s2);
}

void onlyAcceptsString(String s) {
print("Happy!");
}

String giveMeString() {
return "42!";
}

String? giveMeMaybeString(bool giveString) {
if (giveString) return "42!";
return null;
}

String giveMeStringOrCrash(bool crash) {
if (crash) {
throw "booh!";
}
return "42!!";
}
```

I'm puzzled by `s1` being nullable after the try. Either it's assigned a `String` and thus still a `String` or it isn't and it's also still a `String`. I understand that if I had uncommented the first line in the try and it had been
```dart
s1 = giveMeMaybeString(b2);
s1 = giveMeStringOrCrash(b2);
```
it could be null, but without it it can't.

Digging around in the flow analysis I see that in `tryCatchStatement_bodyEnd` it does a `conservativeJoin`, basically removing all promotion for written variables:

https://github.com/dart-lang/sdk/blob/c7625fc68dfb829d7ab9a3a6cbf50f069ad2cc84/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart#L4084-L4098

I've hacked around and noticed there's a `nonPromotionHistory` available. Utilizing that I've applied the following hack:

```dart
// Hack: Remove written variables that wasn't ever demoted.
Set written = {};
for (Variable v in info._written) {
VariableModel vModel =
afterBody.infoFor(info._written.first);
if (vModel.nonPromotionHistory != null) {
written.add(v);
}
}

FlowModel beforeCatch =
beforeBody.conservativeJoin(written, info._captured);
```

which seems to do what I'd want: Accept
```dart
// s1 = giveMeMaybeString(b2);
s1 = giveMeStringOrCrash(b2);
```

as non-nullable and mark

```dart
s1 = giveMeMaybeString(b2);
s1 = giveMeStringOrCrash(b2);
```

as nullable.

It also still demotes for instance
```dart
Object? foo;
foo = "Hello";
if (foo is String) {
try {
foo = 42;
} catch (e) {
print("Crashed!");
}
onlyAcceptsString(foo);
}
```

but is happy about
```dart
Object? foo;
foo = "Hello";
if (foo is String) {
try {
foo = "still string";
} catch (e) {
print("Crashed!");
}
onlyAcceptsString(foo);
}
```

Can we have something like this? Or is there some reason why this wouldn't work?
/cc @stereotype441 @lrhn @johnniwinther

Contributor guide

Open the contributing guide

Research direction

Start in pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart at tryCatchStatement_bodyEnd and read how conservativeJoin uses written variables. Compare the issue's s1 and foo examples, including nonPromotionHistory, and determine the intended promotion behavior before validating it against the existing flow-analysis logic.

Written by the indexing model from the issue text.

Assessment

Tech stack
dart
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.