From c5365106c35454ef45fae15914dd72c439697a6d Mon Sep 17 00:00:00 2001
From: Mike Fairhurst <mfairhurst@google.com>
Date: Mon, 12 Aug 2019 20:20:10 +0000
Subject: [PATCH] [nnbd_migration] polish previous postdominator code

Change-Id: Ic99389d331dfa9495c57c16f166610c7149e7fb9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112648
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
---
 pkg/analyzer/lib/src/dart/ast/ast.dart        |  1 +
 pkg/nnbd_migration/lib/src/edge_builder.dart  | 49 ++++++++-----------
 .../test/edge_builder_test.dart               | 39 +++++++++++++++
 3 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/pkg/analyzer/lib/src/dart/ast/ast.dart b/pkg/analyzer/lib/src/dart/ast/ast.dart
index 87d899b95583..747ecf1fb34b 100644
--- a/pkg/analyzer/lib/src/dart/ast/ast.dart
+++ b/pkg/analyzer/lib/src/dart/ast/ast.dart
@@ -4661,6 +4661,7 @@ class ForPartsWithDeclarationsImpl extends ForPartsImpl
 
   @override
   Token get beginToken => _variableList?.beginToken ?? super.beginToken;
+
   @override
   Iterable<SyntacticEntity> get childEntities => new ChildEntities()
     ..add(_variableList)
diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart
index 9bdae8a144dc..d28c10be615e 100644
--- a/pkg/nnbd_migration/lib/src/edge_builder.dart
+++ b/pkg/nnbd_migration/lib/src/edge_builder.dart
@@ -124,7 +124,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
   final _guards = <NullabilityNode>[];
 
   /// The scope of locals (parameters, variables) that are post-dominated by the
-  /// current node as we walk the AST. We use a [ScopedSet] so that outer
+  /// current node as we walk the AST. We use a [_ScopedLocalSet] so that outer
   /// scopes may track their post-dominators separately from inner scopes.
   ///
   /// Note that this is not guaranteed to be complete. It is used to make hard
@@ -499,36 +499,27 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
   DecoratedType visitForStatement(ForStatement node) {
     // TODO do special condition handling
     // TODO do create true/false guards?
-    // Create a scope of for new initializers etc, which includes previous
-    // post-dominators.
-    _postDominatedLocals.doScoped(
-        copyCurrent: true,
-        action: () {
-          final parts = node.forLoopParts;
-          if (parts is ForParts) {
-            if (parts is ForPartsWithDeclarations) {
-              parts.variables.accept(this);
-            }
-            if (parts is ForPartsWithExpression) {
-              parts.initialization.accept(this);
-            }
-            parts.condition.accept(this);
-          }
-          if (parts is ForEachParts) {
-            parts.iterable.accept(this);
-          }
-
-          // The condition may fail/iterable may be empty, so the body does not
-          // post-dominate the parts, or the outer scope.
-          _postDominatedLocals.popScope();
-          _postDominatedLocals.pushScope();
+    final parts = node.forLoopParts;
+    if (parts is ForParts) {
+      if (parts is ForPartsWithDeclarations) {
+        parts.variables?.accept(this);
+      } else if (parts is ForPartsWithExpression) {
+        parts.initialization?.accept(this);
+      }
+      parts.condition?.accept(this);
+    } else if (parts is ForEachParts) {
+      parts.iterable.accept(this);
+    }
 
-          if (parts is ForParts) {
-            parts.updaters.accept(this);
-          }
+    // The condition may fail/iterable may be empty, so the body gets a new
+    // post-dominator scope.
+    _postDominatedLocals.doScoped(action: () {
+      node.body.accept(this);
 
-          node.body.accept(this);
-        });
+      if (parts is ForParts) {
+        parts.updaters.accept(this);
+      }
+    });
     return null;
   }
 
diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart
index 015e956535e2..aaf18bfb2aac 100644
--- a/pkg/nnbd_migration/test/edge_builder_test.dart
+++ b/pkg/nnbd_migration/test/edge_builder_test.dart
@@ -1432,6 +1432,17 @@ main() {
     // No assertions; just checking that it doesn't crash.
   }
 
+  test_forStatement_empty() async {
+    await analyze('''
+
+void test() {
+  for (; ; ) {
+    return;
+  }
+}
+''');
+  }
+
   test_function_assignment() async {
     await analyze('''
 class C {
@@ -2626,6 +2637,34 @@ void test(List<C> l, C c1, C c2) {
         assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false));
   }
 
+  test_postDominators_forStatement_conditional() async {
+    await analyze('''
+
+class C {
+  void m() {}
+}
+void test(bool b1, C c1, C c2, C c3) {
+  for (; b1/*check*/; c2.m()) {
+    C c4 = c1;
+    c4.m();
+    return;
+  }
+
+  c3.m();
+}
+''');
+
+    //TODO(mfairhurst): enable this check
+    //assertNullCheck(checkExpression('b1/*check*/'),
+    //    assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true));
+    assertNullCheck(checkExpression('c4.m'),
+        assertEdge(decoratedTypeAnnotation('C c4').node, never, hard: true));
+    assertNullCheck(checkExpression('c2.m'),
+        assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false));
+    assertNullCheck(checkExpression('c3.m'),
+        assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false));
+  }
+
   test_postDominators_forStatement_unconditional() async {
     await analyze('''