Skip to content
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

Local variable is not promoted when checked that its value is a non-null literal #1726

Open
mateusfccp opened this issue Jul 5, 2021 · 9 comments
Labels
flow-analysis Discussions about possible future improvements to flow analysis request Requests to resolve a particular developer problem

Comments

@mateusfccp
Copy link
Contributor

mateusfccp commented Jul 5, 2021

Consider the following code:

enum Fruit {apple, banana, orange}

void printFruitIfAppleElsePrintGarbage(Fruit? fruit) {  
  if (fruit == Fruit.apple) {
    printFruit(fruit);
  } else {
    print('🗑');
  }
}

void printFruit(Fruit fruit) => print(fruit);

The code above won't compile, even if it is (seemingly) clear that, if fruit is Fruit.apple it won't possibly be null. The same happens if using a switch-case. The following won't compile:

void printFruitIfAppleElsePrintGarbage(Fruit? fruit) {
  switch (fruit) {
    case Fruit.apple: return printFruit(fruit);
    default: return print('🗑');
  }
}

Obviously, if I explicitly check for null (if (fruit != null && fruit == Fruit.apple)) it will be properly promoted.

Is this the intended behavior? If yes, why? Maybe I am missing something and there's some technical reason to why the compiler can't be sure that the value is not null in these cases?

@mateusfccp mateusfccp added the bug There is a mistake in the language specification or in an active document label Jul 5, 2021
@Levi-Lesches
Copy link

There was a commet somewhere else that explained it better, but I believe this is intended, and only explicit checks for null will promote.

I think it's simple to say that checking against other nullable expressions shouldn't be enough to promote, but a non-null literal may be possible.

@eernstg
Copy link
Member

eernstg commented Jul 6, 2021

Cool!

Note that #1224 has a similar topic, and the topic flow-analysis in general is related. I'll add the label 'flow-analysis' and remove the label 'bug' because the discussion is concerned with a language enhancement.

We can strengthen the rule proposed here a bit, though:

It seems to be a sound rule that v is non-null if v == e is true, where v is a promotable local variable of an arbitrary nullable type, and e is an expression whose type is non-nullable. Here's an argument:

When v == e yields true, the evaluation of e must have yielded a non-null object (by soundness, and because the type of e is non-nullable). If v were null then v == e would evaluate to false (without running any methods on the value of v); so it is guaranteed that v was non-null.

In other words, there's no way we can write a twisted implementation of operator == on any class and thereby make this promotion unsound, because we won't even call any operator == in the case where we would want to cheat. ;-)

@eernstg eernstg added flow-analysis Discussions about possible future improvements to flow analysis request Requests to resolve a particular developer problem and removed bug There is a mistake in the language specification or in an active document labels Jul 6, 2021
@lrhn
Copy link
Member

lrhn commented Jul 6, 2021

There is no end to the expressions we could potentially allow to promote. The following ones are ones we haven't chosen to promote on:

x is Null                        // if true, x is null
identical(x, nonNullableValue)   // if true, x is not null
identical(x, null)               // if true, x is null
x == nonNullableValue            // if true, x is not null
x?.foo == nonNullableValue       // if true, x is not null
(x ?? nullableValue) == null     // if true, x is null

We have to draw a line at some point, at we drew it early.
The only tests which promote are x == null, x != null (which is just sugar for !(x == null)) and x is NonNullableType. We also potentially promote on cast operations if they don't throw (x as int, x!).

@eernstg
Copy link
Member

eernstg commented Jul 6, 2021

Note, though, that

x?.foo == nonNullableValue

is in the title of #1224. So we may have drawn that line early, but it's still under consideration to move it.

@Levi-Lesches
Copy link

I agree with @eernstg here, that

x == nonNullableValue

and

x?.foo == nonNullableValue

are both used often enough that it would be nice if they're supported, but the rest don't have to be. I don't see identical and is Null or ?? being used the same way often enough. Plus, this isn't really about "every case that's possible", just the ones where it's obvious to the reader that the value should promote. The other cases, especially the ?? one, don't really convey that IMO.

@dbilgin
Copy link

dbilgin commented Jul 10, 2021

I'm not sure I get this I have had this problem with direct null checks many times such as:

      if (widget.onChanged != null) {
        widget.onChanged(query);
      }

This also gives me an error, shouldn't it be getting promoted?

@Levi-Lesches
Copy link

Levi-Lesches commented Jul 10, 2021

That's a slightly different issue because fields work differently than local variables. When you access a field, you're really calling a getter, and that getter is essentially a function, which can return any value. More importantly, it can return different values each time it's called. So just because widget.onChanged is not null the first time, doesn't mean it can't be null the second time:

/// This is probably what you have. 
class MyWidget extends StatefulWidget {
  final VoidCallback? onChanged;
  MyWidget(this.onChanged);
}

/// But this is valid as well
class MyTrickyWidget extends StatefulWidget {
  VoidCallback? get onChanged => Random.nextBool() ? () { print("Hello"); } : null;
}

Because of this difference, Dart can't promote fields. There is some syntax being proposed to get around it, like #1514 and #1201, but they all boil down to shortcuts for the following:

void onChanged() {
  // Because callback is local, it can't be overriden with a getter, so it's value won't change between checks
  final VoidCallback? callback = widget.onChanged;
  if (callback != null) {
    callback(query);  // promoted
  }
}

}

@dbilgin
Copy link

dbilgin commented Jul 10, 2021

@Levi-Lesches I see, thank you very much for the detailed explanation! I won't spam this thread with a different issue.

@mateusfccp mateusfccp changed the title Local variable is not promoted when checked that it's value is a non-null literal Local variable is not promoted when checked that its value is a non-null literal Jul 10, 2021
@lrhn
Copy link
Member

lrhn commented Jul 26, 2021

Also just want to promote null-aware invocation of the call method of the function: widget.onChanged?.call(query);

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

5 participants