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

Flow analysis is overly pessimistic about closures capturing for-in loop variables #43136

Closed
stereotype441 opened this issue Aug 20, 2020 · 5 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. type-enhancement A request for a change that isn't a bug

Comments

@stereotype441
Copy link
Member

The following code works in legacy mode:

List<void Function()> deferPrintingIsEven(List<num> nums) {
  List<void Function()> result = [];
  for (var n in nums) {
    if (n is int) {
      result.add(() {
        print(n.isEven);
      });
    }
  }
  return result;
}

main() {
  for (var f in deferPrintingIsEven([0, 0.5, 1])) {
    f();
  }
}

It prints:

true
false

It ought to work with the null safety experiment enabled as well, but instead it gives a compile error:

test.dart:6:17: Error: The getter 'isEven' isn't defined for the class 'num'.
Try correcting the name to the name of an existing getter, or defining a getter or field named 'isEven'.
        print(n.isEven);
                ^^^^^^

I believe what's happening is that flow analysis considers the variable n to be assigned to within the body of the method, so it cancels promotions on entry to the function expression. However, according to the spec, the for-in loop is treated as equivalent to:

  List<num> id1 = nums;
  var id2 = id1.iterator;
  while (id2.moveNext()) {
    var n = id2.current;
    {
      if (n is int) {
        result.add(() {
          print(n.isEven);
        });
      }
    }
  }

...which is accepted by flow analysis (because each initialization of n is in effect initializing a fresh variable, so there is no assignment that could clobber promotion).

Flow analysis should be changed so that the for-in form behaves the same as its desugared equivalent.

@stereotype441 stereotype441 added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Aug 20, 2020
@franklinyow franklinyow added the type-enhancement A request for a change that isn't a bug label Sep 3, 2020
@leafpetersen
Copy link
Member

Is this still planned?

@stereotype441
Copy link
Member Author

I would still like to do this, especially because it means that code that used to work (prior to null safety) will stop working when null safety is enabled. However, time is drawing short and I might not get to it in time.

stereotype441 added a commit to stereotype441/source_span that referenced this issue Sep 12, 2020
When dart-lang/sdk#43136 is fixed, two
non-null assertions in the source_span package will become
unnecessary, causing warnings which will break the build of the SDK.
This change adds "// ignore" comments to avoid the breakage.  Once the
fix is landed, I'll remove both the "// ignore" comments and the
unnecessary non-null assertions.
stereotype441 added a commit to stereotype441/source_span that referenced this issue Sep 12, 2020
Flow analysis currently has a bug preventing for-each loop variables
from being properly promoted in the presence of closures
(dart-lang/sdk#43136); as a result of this
bug the source_span package had two non-null assertions that ought to
have been unnecessary.

I'm preparing a fix for that bug, however if I land it as is, it will
cause the front end to emit errors saying "Operand of null-aware
operation '!' has type '_Highlight' which excludes null"; this in turn
will cause SDK bot breakages (since source_span is imported into the
SDK repo).

So, in order to land the fix, we need to first update the source_span
package to work around the bug; this change does that by allocating a
temporary variable (which *is* promoted correctly).

Once the fix for dart-lang/sdk#43136 lands,
I will make a follow-up change that deletes the temporary variable.
stereotype441 added a commit to stereotype441/language that referenced this issue Sep 12, 2020
stereotype441 added a commit to stereotype441/language that referenced this issue Sep 12, 2020
stereotype441 added a commit to stereotype441/language that referenced this issue Sep 12, 2020
stereotype441 added a commit to stereotype441/language that referenced this issue Sep 14, 2020
stereotype441 added a commit to dart-lang/source_span that referenced this issue Sep 14, 2020
Flow analysis currently has a bug preventing for-each loop variables
from being properly promoted in the presence of closures
(dart-lang/sdk#43136); as a result of this
bug the source_span package had two non-null assertions that ought to
have been unnecessary.

I'm preparing a fix for that bug, however if I land it as is, it will
cause the front end to emit errors saying "Operand of null-aware
operation '!' has type '_Highlight' which excludes null"; this in turn
will cause SDK bot breakages (since source_span is imported into the
SDK repo).

So, in order to land the fix, we need to first update the source_span
package to work around the bug; this change does that by allocating a
temporary variable (which *is* promoted correctly).

Once the fix for dart-lang/sdk#43136 lands,
I will make a follow-up change that deletes the temporary variable.
@stereotype441
Copy link
Member Author

Whoops, fix has not landed yet.

@stereotype441 stereotype441 reopened this Sep 15, 2020
dart-bot pushed a commit that referenced this issue Sep 16, 2020
…en to.

When performing flow analysis for a "for each" loop such as `for (var
x in ...) { ... }`, we don't need to consider the iterated value to be
"written to" the variable `x`, since the actual runtime semantics are
to create a fresh instance of the variable `x` each time through the
loop, initialized to the iterated value.

Fixes #43136.

Bug: #43136
Change-Id: I497c408c3efc26e93502de8ba0530bb5278e74c6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/162581
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@kevmoo
Copy link
Member

kevmoo commented Sep 16, 2020

@stereotype441 – I don't think you meant to have this closed by 27fc687 – right?

I think GitHubs auto-close logic caught you by mistake

@kevmoo kevmoo reopened this Sep 16, 2020
@stereotype441
Copy link
Member Author

@kevmoo thanks. This was fixed, but the SHA that fixed it should have been 53e707f.

stereotype441 added a commit to stereotype441/source_span that referenced this issue Sep 29, 2020
Now that dart-lang/sdk#43136 is fixed, the
workaround is no longer needed.
stereotype441 added a commit to stereotype441/csslib that referenced this issue Sep 29, 2020
This cast was working around
dart-lang/sdk#43136.  Now that it is fixed,
the cast is no longer needed.
jakemac53 pushed a commit to dart-lang/source_span that referenced this issue Sep 29, 2020
Now that dart-lang/sdk#43136 is fixed, the
workaround is no longer needed.
natebosch pushed a commit to dart-lang/csslib that referenced this issue Sep 29, 2020
This cast was working around
dart-lang/sdk#43136.  Now that it is fixed,
the cast is no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants