Skip to content

Flow analysis does not specify treatment of local variables in function literal bodies #1247

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
eernstg opened this issue Oct 2, 2020 · 12 comments
Assignees
Labels
flow-analysis Discussions about possible future improvements to flow analysis specification technical-debt Dealing with a part of the language which needs clarification or adjustments

Comments

@eernstg
Copy link
Member

eernstg commented Oct 2, 2020

Having looked around in flow-analysis.md, I think the following is not covered:

A local variable v declared in a function f which is evaluated in the body of a function literal l in the body of f is considered to have its declared type (even in the case where v has been promoted at the location where l occurs and v is never demoted).

(The case where v is assigned in l is covered: it is then writeCaptured, and promotion never applies.)

@stereotype441, do you agree that this is not covered today, and the rule could be approximately as stated above?

@stereotype441
Copy link
Member

stereotype441 commented Oct 7, 2020

The current behavior is actually more subtle than this. It depends whether an assignment to v exists anywhere. If it does, then promotion is cancelled within the body of any local functions*, e.g.:

f() {
  Object v = 0;
  bool Function() g;
  if (v is int) {
    g = () => v.isEven; // ERROR: v has type `Object`
  }
  v = 'bad';
  print(g());
}

However, if there is no assignment to v anywhere, then the promotion happens, e.g.:

f() {
  Object v = 0;
  bool Function() g;
  if (v is int) {
    g = () => v.isEven; // OK
  }
  print(g());
}

You're right that this is not specified. I believe we need a clause in the Expressions section of the flow analysis spec explaining what happens for function expressions. It should be something like this:

  • Function expression: If N is a function expression then:
    • If N is a block based function expression ((...arguments...) { ...statements... }), let N2 be { ...statements... }.
    • If N is an expression based function expression ((...arguments...) => E), let N2 be E.
    • Let F be the entire function for which flow analysis is presently being done.
    • Let before(N2) = conservativeJoin(before(N), written(F), captured(F)).
    • Let after(N) = conservativeJoin(before(N), [], written(N2)).

We need a similar rule for local (named) function declaration statements and for late variable initializers, both of which behave similar to function expressions.

(*Edit: added the text "within the body of any local functions" in response to #1247 (comment))

@renatoathaydes
Copy link

I believe the current implementation for final local variables is mistaken and does not match the description given by @stereotype441 .

Take this example:

main() {
  final num a;
  a = 1;
  if (a is int) {
    final isEven = a.isEven;
    return (i) => i == a.isEven;
  }
}

According to this sentence:

The current behavior is actually more subtle than this. It depends whether an assignment to v exists anywhere. If it does, then promotion is cancelled

The above code should fail on line final isEven = a.isEven; already. But it doesn't, meaning type promotion is occurring.

However, within the scope of the if block (where promotion has happened), the promotion is lost inside the inner scope of the lambda in the following line.

This can only be a mistake.

@eernstg
Copy link
Member Author

eernstg commented Jul 2, 2021

@renatoathaydes wrote:

Take this example:

main() {
  final num a;
  a = 1;
  if (a is int) {
    final isEven = a.isEven;
    return (i) => i == a.isEven;
  }
}

...
The above code should fail on line final isEven = a.isEven;

Actually, the promotion which is being cancelled is the promotion of the local variable in the nested function. You do get an error in line 6 (i == a.isEven), and that's because there is no promotion of a in this function body.

The reason why the promotion is lost is that the nested function can be executed anywhere (we don't try to detect statically where that function object goes, so any construct that runs unknown code, including basically any method call, could potentially run that function).

But the promotion in line 5 (final isEven = a.isEven;) does occur (that one can rely on the general flow analysis, and we know that a is int yielded true at that point), so you don't get an error in line 5. So I think it is working as intended.

@stereotype441
Copy link
Member

@renatoathaydes wrote:

The above code should fail on line final isEven = a.isEven; already. But it doesn't, meaning type promotion is occurring.

However, within the scope of the if block (where promotion has happened), the promotion is lost inside the inner scope of the lambda in the following line.

This can only be a mistake.

You're right. The mistake was in my sentence "If it does, then promotion is cancelled." I should have said "If it does, then promotion is cancelled within the body of any local functions". I've corrected my comment above (#1247 (comment)).

I believe that with this correction, my description is now consistent with what the implementation does, and with @eernstg's explanation from #1247 (comment).

@renatoathaydes
Copy link

But do you really think it makes sense for the promotion to happen on line 5 but not on line 6? Could a possibly change types between the two lines?

@renatoathaydes
Copy link

That lambda on line 6 is instantiated at a time when the value of local variable a is known (line 5 proves that). It's a final local variable. It gets captured by the lambda at a time it's been definitely assigned.

IMO it won't matter whether you pass the lambda reference to a million places , calls it a million more: a can never change value or type.

@eernstg
Copy link
Member Author

eernstg commented Jul 2, 2021

Here is an example which shows that it is not sound to assume that a can be promoted in the body of a function literal (line 4), even though it is guaranteed that a is an int when that line is reached:

bool f(num a) {
  bool Function() g;
  if (a is int) {
    g = () => a.isEven;
  } else {
    g = () => false;
  }
  a = 1.5;
  return g();
}

The point is that a might not be an int when the function literal is executed.

The fact that the variable is final in the original example makes it reasonable to say that it won't stop being an int, but I don't think the flow analysis will maintain that distinction in the case where the final variable is initialized after the declaration. That could happen in more than one location, with different types, as long as it is guaranteed to have happened in every location where it is used.

@stereotype441, do you think it would be possible to improve on the analysis for that kind of final local variables?

@renatoathaydes
Copy link

This is a different case completely. a is not final in your last example.

@renatoathaydes
Copy link

I don't think the flow analysis will maintain that distinction in the case where the final variable is initialized after the declaration.

There must be a simple way to find out that:

  • the local variable is final.
  • it has definitely been assigned.

The first one is trivial. The second one: the compiler already knows at any point if the variable has been assigned at a certain location, otherwise it would allow invalid code to be compiled.

@stereotype441
Copy link
Member

@renatoathaydes You're right. I'm filing a separate issue to track this.

@stereotype441
Copy link
Member

@renatoathaydes @eernstg since this issue is about the spec being an incomplete description of what was implemented, and the issue Renato raised is a suggested improvement to what was implemented, I think we should track them separately. I've filed #1721 to track Renato's issue.

@renatoathaydes
Copy link

Thanks @stereotype441 , really appreciate it.

@eernstg eernstg added the technical-debt Dealing with a part of the language which needs clarification or adjustments label Aug 6, 2021
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 specification technical-debt Dealing with a part of the language which needs clarification or adjustments
Projects
None yet
Development

No branches or pull requests

3 participants