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

Use of uninitialized late variables should be a linter error #46759

Closed
gaaclarke opened this issue Jul 29, 2021 · 9 comments
Closed

Use of uninitialized late variables should be a linter error #46759

gaaclarke opened this issue Jul 29, 2021 · 9 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.

Comments

@gaaclarke
Copy link
Contributor

gaaclarke commented Jul 29, 2021

Example:

String foo(int x) {
  late String result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

The linter should verify that every branch through the code has the potential to set result before it is actually used. It can't guarantee that it will bet set but static analysis can detect this code is problematic.

@bwilkerson
Copy link
Member

The compiler will do that checking for you if you don't make the local variable late:

String foo(int x) {
  String result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

Would that solve your need?

@lrhn
Copy link
Member

lrhn commented Jul 29, 2021

As @bwilkerson says, this is actually expected and intended behavior.

Non-late variables requires that they are definitely assigned before they are read. They are safe.

Late variables are introduced in order to loosen that requirement for situations where the compiler cannot determine that the variable is actually always assigned, but the programmer knows that it is. (Actually, late is used for multiple things, but this is one of them). For example:

late String floo;
if (someTest) { 
  floo = "bad";
}
compute(...);
if (someTest) {
  state += floo;  // Compiler only knows that `floo` is potentially assigned.
}

Here the author knows that floo is initialized in state += floo because it's guarded by the same test as the assignment to floo. The compiler is not that smart. If the variable had been non-late, the compiler would have complained because floo was not definitely assigned. Because the variable is late, the compiler shuts up and trusts the author (it's like dynamic in that sense - it removes static safety checks and replaces them with dynamic checks instead).

The compiler will still complain if there is no way for floo to be initialized, if it's definitely unassigned, but if there is a way for the variable to be assigned, it then trusts the author to know that it is in fact so. (Then it checks again when the variable is actually read, and throws if the variable wasn't really assigned).

@gaaclarke
Copy link
Contributor Author

The compiler will do that checking for you if you don't make the local variable late:

String foo(int x) {
  String result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

Would that solve your need?

In my exact case it wouldn't since my variable is nullable and not initializing a variable is the idiomatic way to make null variables.

So it's more like this:

String? foo(int x) {
  late String? result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

@gaaclarke
Copy link
Contributor Author

gaaclarke commented Jul 29, 2021

The compiler will still complain if there is no way for floo to be initialized, if it's definitely unassigned, but if there is a way for the variable to be assigned,

Yep, I'm suggesting we make that logic a bit smarter by examining the branches through the code, it's technically possible to do. I don't know if it is feasible given how the code is written.

String foo(int x) {
  late String result;
  return result;  // currently we correctly error here
}

@a-siva a-siva added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-linter Issues with the analyzer's support for the linter package labels Jul 29, 2021
@lrhn
Copy link
Member

lrhn commented Jul 30, 2021

The current behavior is not accidentally being less smart than possible, it's entirely deliberate that it's not flagging the use you have here, and adding late is in fact meant to ensure that you get that behavior.

You can write the same code as:

String? foo(int x) {
  String? result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

Since the variable is nullable, it's implicitly initialized to null.

Or, you can write it as:

String? foo(int x) {
  String result;
  if (x == 1) {
    result = 'one';
  }
  return result;
}

In that case, you do get an error because the result is uninitialized at the return result;.
You can add late to suppress that error, and there is no obvious other reason to add late to the declaration in this example.

In most cases, you don't need to add late to a variable, so why did you choose to make the variable late to begin with in this example?

@gaaclarke
Copy link
Contributor Author

gaaclarke commented Jul 30, 2021

The second case you mention with the uninitialized String does give me what I want, but it doesn't work in my case since I want to return null. Let my try to describe even more clear how I arrived to wanting this. I keep trying to simplify my use case and it is confusing the point.

I have code like this:

String? foo(int x) {
  if (x > 0) {
    if ( x == 1) {
      return 'one';
    } else {
      return 'bigger than one';
    }
  } else {
    return null;
  }
}

The reviewer of my code doesn't like the if-block with multiple returns. I like it because the compiler forces the maintainer to return an answer in each branch. So, I considered condensing it to one return statement but if I do that I want the same guarantee that I have with the branching return statements. I want to guarantee statically that each branch will set that variable we are returning.

I refactored it to

String? foo(int x) {
  late String? result;
  if (x > 0) {
    if ( x == 1) {
      result = 'one';
    } else {
      result = 'bigger than one';
    }
  } else {
    result = null;
  }
  return result;
}

This however doesn't give me the same level of static checks that the multiple return statements has. As someone is editing the code they could easily forget to to set result. I don't really understand why we wouldn't want to at least warn the developer they may be using an initialized late variable.

I could also use nested ternary operators to get what I want, but in the actual code that would be pretty messy.

@jcollins-g
Copy link
Contributor

Closing as WAI.

@gaaclarke
Copy link
Contributor Author

@jcollins-g, respectfully, the question wasn't wether this works as intended or not. The question is wether the intention was correct and if this is an improvement worth pursuing. When people make suggestions on how to improve Dart, we shouldn't close the issues as WAI because thats dismissing feedback to improve the language from users that have valuable real world usage of Dart.

A decision should be made if this is something worth doing or the issue should be filed as a proposal where it can be decided on in the future, not just pocket vetoed. If the team doesn't like the suggestion, say so.

@Levi-Lesches
Copy link

@gaaclarke, the issue is you're using late, which, like @lrhn said, is like dynamic in how it signals to the compiler deliberately not to error. So to say that the compiler should be made to error anyway would mean late uses its usefulness.

I don't see what's wrong with using return on every branch. You say

I want to guarantee statically that each branch will set that variable we are returning.

Using return is a great way to ensure that you return a value.

If returning null wasn't an option, making a non-nullable would fix this issue. You say you don't want the code to "fall through"; that you always want it to return something, even if it's null. Well, Dart implicitly returns null for you.

Regarding structure, I don't know your full code, but maybe you can "flatten" some of your ifs? So instead of

String? foo(int x) {
  late String? result;
  if (x > 0) {
    if ( x == 1) {
      result = 'one';
    } else {
      result = 'bigger than one';
    }
  } else {
    result = null;
  }
  return result;
}

you can do:

String? foo(int x) {
  String? result; 
  if (x <= 0) result = null;
  else if (x == 1) result = "one";
  else result = "bigger than one";
  
  return result;
}

This way, it's obvious that the final else really carries all other possible branches, and then you can feel more comfortable about switching using return on every line

Finally, if you really want to go this way, you can abuse semantics a bit by using a non-nullable variable and returning null explicitly:

String? foo(int x) {
  String result;
  if (x <= 0) return null;
  else if (x == 1) { /* forgot to assign here */ }
  else result = "bigger than one";
  
  return result;  // Error: 'result' must be assigned before it can be used
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.
Projects
None yet
Development

No branches or pull requests

6 participants