Skip to content

[NNBD] No error in Analyzer when non-nullable function may return null #42568

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 Jul 3, 2020 · 5 comments
Closed
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Jul 3, 2020

The following code produces no issues in Analyzer but a compile error in CFE

import "dart:async";
main() {
  Future<int> f = new Future.value(42);
  f.then((_) {
    bool b = true;
    if (b) {
      return 42;
    }
  });
}

CFE output is

Error: A non-null value must be returned since the return type 'int' doesn't allow null.
f.then((_) {
       ^

There must be a compile error in Analyzer as well

Dart VM version: 2.9.0-20.0.dev (dev) (Wed Jul 1 11:29:16 2020 +0200) on "windows_x64"

@lrhn
Copy link
Member

lrhn commented Jul 3, 2020

The result of f.then(...) does seem to be Future<int> in the analyzer too, so it's not because it deduces that the function return type is int?.

@lrhn lrhn added legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jul 3, 2020
@eernstg
Copy link
Member

eernstg commented Jul 3, 2020

I believe this is concerned with function literal return type inference: If the invocation of then gets an explicit type argument <int> then the CFE and the analyzer agree that it is an error, because the function literal may return null. With type argument <int?>, only the CFE reports an error (and it's the same error), which would be consistent with the following difference:

  • The CFE computes int as the inferred return type of the function literal, and
  • the analyzer computes int?

The rules here for the computation of the 'actual returned type' do not take the normal completion (that is: we reach the end of the function) into account, so I believe the CFE is correct and the analyzer takes an extra step which is not specified (in adding Null to the actual returned type).

I also think it would make sense to do as the analyzer says, in which case we should add an extra case to the rules about computing the actual return type.

@eernstg
Copy link
Member

eernstg commented Jul 3, 2020

Second thought: We should stick to the current rules. ;-)

The local function f is a compile time error, but g is not:

void main() {
  var b = true;
  f() {
    if (b) return 1;
  }
  g() {
    if (b) return 1;
    if (!b) return null;
  }
}

The point is that we do allow an explicit return null to make the inferred return type nullable, but reaching the end of the function is not given the same powers: That situation is just noted (so we know that the semantics of the function allows for returning null), and the return type is inferred (ignoring that we may reach the end), and then it's an error to reach the end if and because the inferred return type is non-nullable.

The current rules are giving the same treatment to the function literal as we're giving local functions (and no other functions are subject to return type inference based on the body). So the CFE behavior is the consistent behavior.

Cf. dart-lang/language#1063.

@scheglov scheglov self-assigned this Jul 7, 2020
@scheglov
Copy link
Contributor

scheglov commented Jul 7, 2020

It seems that this is a moving target, as discussion is continues in dart-lang/language#1063.
FWIW, I like the idea of making returning Null explicit, so updating the spec for inference rules and analyzer.

@eernstg
Copy link
Member

eernstg commented Jul 17, 2020

@scheglov, @sgrekhov, with dart-lang/language#1063 closed I believe we can conclude that the analyzer works as intended: The normal completion gives rise to a nullable inferred return type for the function literal, and there is no error when the return statement is checked. So I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants