Flow analysis: test that all variables are registered with add().

stereotype441 · commit-bot@chromium.org · commit ee5a90cb1b6a · 2019-07-22T20:27:08.000Z
This uncovered two more instances where we weren't properly registering variables: - Local function parameters - Variables introduced by "catch" syntax. - Variables introduced by "for (...; ...; ...)" syntax. Change-Id: Id5e8aa1b598962cca1bb19df6d4ede99a672f108 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109885 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
diff --git a/pkg/analyzer/lib/src/dart/resolver/flow_analysis.dart b/pkg/analyzer/lib/src/dart/resolver/flow_analysis.dart
@@ -42,7 +42,14 @@ class AssignedVariables<Statement, Variable> {
 }
 
 class FlowAnalysis<Statement, Expression, Variable, Type> {
+  static bool get _assertionsEnabled {
+    bool result = false;
+    assert(result = true);
+    return result;
+  }
+
   final _VariableSet<Variable> _emptySet;
+
   final State<Variable, Type> _identity;
 
   /// The [NodeOperations], used to manipulate expressions.
@@ -63,8 +70,8 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   /// states.
   final Map<Statement, int> _statementToStackIndex = {};
 
-  /// The list of all variables.
-  final List<Variable> _variables = [];
+  /// List of all variables passed to [add].
+  final List<Variable> _addedVariables = [];
 
   State<Variable, Type> _current;
 
@@ -77,6 +84,19 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   /// The state when [_condition] evaluates to `false`.
   State<Variable, Type> _conditionFalse;
 
+  /// If assertions are enabled, keeps track of all variables that have been
+  /// passed into the API (other than through a call to [add]).  The [finish]
+  /// method uses this to verify that the caller doesn't forget to pass a
+  /// variable to [add].
+  ///
+  /// Note: the reason we have to keep track of this set (rather than simply
+  /// checking each variable at the time it is passed into the API) is because
+  /// the client doesn't call `add` until a variable is declared, and in
+  /// erroneous code, it's possible that a variable might be used before its
+  /// declaration.
+  final Set<Variable> _referencedVariables =
+      _assertionsEnabled ? <Variable>{} : null;
+
   factory FlowAnalysis(
     NodeOperations<Expression> nodeOperations,
     TypeOperations<Variable, Type> typeOperations,
@@ -108,7 +128,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   /// Add a new [variable], which might be already [assigned].
   void add(Variable variable, {bool assigned: false}) {
-    _variables.add(variable);
+    _addedVariables.add(variable);
     _current = _current.add(variable, assigned: assigned);
   }
 
@@ -174,6 +194,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   /// The [binaryExpression] checks that the [variable] is equal to `null`.
   void conditionEqNull(Expression binaryExpression, Variable variable) {
+    _variableReferenced(variable);
     if (functionBody.isPotentiallyMutatedInClosure(variable)) {
       return;
     }
@@ -185,6 +206,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   /// The [binaryExpression] checks that the [variable] is not equal to `null`.
   void conditionNotEqNull(Expression binaryExpression, Variable variable) {
+    _variableReferenced(variable);
     if (functionBody.isPotentiallyMutatedInClosure(variable)) {
       return;
     }
@@ -196,6 +218,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   void doStatement_bodyBegin(
       Statement doStatement, Set<Variable> loopAssigned) {
+    _variablesReferenced(loopAssigned);
     _current = _current.removePromotedAll(loopAssigned);
 
     _statementToStackIndex[doStatement] = _stack.length;
@@ -221,7 +244,21 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
     _current = _join(falseCondition, breakState);
   }
 
+  /// This method should be called at the conclusion of flow analysis for a top
+  /// level function or method.  Performs assertion checks.
+  void finish() {
+    assert(_stack.isEmpty);
+    assert(() {
+      var variablesNotAdded =
+          _referencedVariables.difference(Set<Variable>.from(_addedVariables));
+      assert(variablesNotAdded.isEmpty,
+          'Variables not passed to add: $variablesNotAdded');
+      return true;
+    }());
+  }
+
   void forEachStatement_bodyBegin(Set<Variable> loopAssigned) {
+    _variablesReferenced(loopAssigned);
     _stack.add(_current);
     _current = _current.removePromotedAll(loopAssigned);
   }
@@ -245,6 +282,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   }
 
   void forStatement_conditionBegin(Set<Variable> loopAssigned) {
+    _variablesReferenced(loopAssigned);
     _current = _current.removePromotedAll(loopAssigned);
   }
 
@@ -346,11 +384,13 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   /// Return whether the [variable] is definitely assigned in the current state.
   bool isAssigned(Variable variable) {
+    _variableReferenced(variable);
     return !_current.notAssigned.contains(variable);
   }
 
   void isExpression_end(
       Expression isExpression, Variable variable, bool isNot, Type type) {
+    _variableReferenced(variable);
     if (functionBody.isPotentiallyMutatedInClosure(variable)) {
       return;
     }
@@ -474,6 +514,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   /// Retrieves the type that the [variable] is promoted to, if the [variable]
   /// is currently promoted.  Otherwise returns `null`.
   Type promotedType(Variable variable) {
+    _variableReferenced(variable);
     return _current.promoted[variable];
   }
 
@@ -482,6 +523,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   /// these variables might have different types and are "un-promoted" from
   /// the "afterExpression" state.
   void switchStatement_beginCase(Set<Variable> notPromoted) {
+    _variablesReferenced(notPromoted);
     _current = _stack.last.removePromotedAll(notPromoted);
   }
 
@@ -511,6 +553,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   }
 
   void tryCatchStatement_bodyEnd(Set<Variable> assignedInBody) {
+    _variablesReferenced(assignedInBody);
     var beforeBody = _stack.removeLast();
     var beforeCatch = beforeBody.removePromotedAll(assignedInBody);
     _stack.add(beforeCatch);
@@ -539,6 +582,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   }
 
   void tryFinallyStatement_end(Set<Variable> assignedInFinally) {
+    _variablesReferenced(assignedInFinally);
     var afterBody = _stack.removeLast();
     _current = _current.restrict(
       typeOperations,
@@ -549,16 +593,13 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   }
 
   void tryFinallyStatement_finallyBegin(Set<Variable> assignedInBody) {
+    _variablesReferenced(assignedInBody);
     var beforeTry = _stack.removeLast();
     var afterBody = _current;
     _stack.add(afterBody);
     _current = _join(afterBody, beforeTry.removePromotedAll(assignedInBody));
   }
 
-  void verifyStackEmpty() {
-    assert(_stack.isEmpty);
-  }
-
   void whileStatement_bodyBegin(
       Statement whileStatement, Expression condition) {
     _conditionalEnd(condition);
@@ -574,6 +615,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   }
 
   void whileStatement_conditionBegin(Set<Variable> loopAssigned) {
+    _variablesReferenced(loopAssigned);
     _current = _current.removePromotedAll(loopAssigned);
   }
 
@@ -587,6 +629,7 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
 
   /// Register write of the given [variable] in the current state.
   void write(Variable variable) {
+    _variableReferenced(variable);
     _current = _current.write(typeOperations, _emptySet, variable);
   }
 
@@ -623,6 +666,26 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
       newPromoted,
     );
   }
+
+  /// If assertions are enabled, records that the given variable has been
+  /// referenced.  The [finish] method will verify that all referenced variables
+  /// were eventually passed to [add].
+  void _variableReferenced(Variable variable) {
+    assert(() {
+      _referencedVariables.add(variable);
+      return true;
+    }());
+  }
+
+  /// If assertions are enabled, records that the given variables have been
+  /// referenced.  The [finish] method will verify that all referenced variables
+  /// were eventually passed to [add].
+  void _variablesReferenced(Iterable<Variable> variables) {
+    assert(() {
+      _referencedVariables.addAll(variables);
+      return true;
+    }());
+  }
 }
 
 /// Accessor for function body information.
diff --git a/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart b/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart
@@ -133,15 +133,14 @@ class FlowAnalysisHelper {
 
     if (_blockFunctionBodyLevel > 1) {
       assert(flow != null);
-      return;
+    } else {
+      flow = FlowAnalysis<Statement, Expression, VariableElement, DartType>(
+        _nodeOperations,
+        _typeOperations,
+        _FunctionBodyAccess(node),
+      );
     }
 
-    flow = FlowAnalysis<Statement, Expression, VariableElement, DartType>(
-      _nodeOperations,
-      _typeOperations,
-      _FunctionBodyAccess(node),
-    );
-
     var parameters = _enclosingExecutableParameters(node);
     if (parameters != null) {
       for (var parameter in parameters.parameters) {
@@ -157,12 +156,17 @@ class FlowAnalysisHelper {
       return;
     }
 
+    // Set this.flow to null before doing any clean-up so that if an exception
+    // is raised, the state is already updated correctly, and we don't have
+    // cascading failures.
+    var flow = this.flow;
+    this.flow = null;
+
     if (!flow.isReachable) {
       result.functionBodiesThatDontComplete.add(node);
     }
 
-    flow.verifyStackEmpty();
-    flow = null;
+    flow.finish();
   }
 
   void breakStatement(BreakStatement node) {
@@ -247,12 +251,14 @@ class FlowAnalysisHelper {
     }
   }
 
-  void variableDeclarationStatement(VariableDeclarationStatement node) {
-    var variables = node.variables.variables;
-    for (var i = 0; i < variables.length; ++i) {
-      var variable = variables[i];
-      flow.add(variable.declaredElement,
-          assigned: variable.initializer != null);
+  void variableDeclarationList(VariableDeclarationList node) {
+    if (flow != null) {
+      var variables = node.variables;
+      for (var i = 0; i < variables.length; ++i) {
+        var variable = variables[i];
+        flow.add(variable.declaredElement,
+            assigned: variable.initializer != null);
+      }
     }
   }
 
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -5118,6 +5118,12 @@ class ResolverVisitor extends ScopedVisitor {
     for (var i = 0; i < catchLength; ++i) {
       var catchClause = catchClauses[i];
       flow.tryCatchStatement_catchBegin();
+      if (catchClause.exceptionParameter != null) {
+        flow.add(catchClause.exceptionParameter.staticElement, assigned: true);
+      }
+      if (catchClause.stackTraceParameter != null) {
+        flow.add(catchClause.stackTraceParameter.staticElement, assigned: true);
+      }
       catchClause.accept(this);
       flow.tryCatchStatement_catchEnd();
     }
@@ -5159,6 +5165,7 @@ class ResolverVisitor extends ScopedVisitor {
 
   @override
   void visitVariableDeclarationList(VariableDeclarationList node) {
+    _flowAnalysis?.variableDeclarationList(node);
     for (VariableDeclaration decl in node.variables) {
       VariableElement variableElement =
           resolutionMap.elementDeclaredByVariableDeclaration(decl);
@@ -5167,12 +5174,6 @@ class ResolverVisitor extends ScopedVisitor {
     super.visitVariableDeclarationList(node);
   }
 
-  @override
-  void visitVariableDeclarationStatement(VariableDeclarationStatement node) {
-    _flowAnalysis?.variableDeclarationStatement(node);
-    super.visitVariableDeclarationStatement(node);
-  }
-
   @override
   void visitWhileStatement(WhileStatement node) {
     _flowAnalysis?.checkUnreachableNode(node);
diff --git a/pkg/analyzer/test/src/dart/resolution/flow_analysis_unit_test.dart b/pkg/analyzer/test/src/dart/resolution/flow_analysis_unit_test.dart
@@ -17,7 +17,7 @@ main() {
       h.flow.ifStatement_elseBegin();
       expect(h.flow.promotedType(x), isNull);
       h.flow.ifStatement_end(true);
-      h.flow.verifyStackEmpty();
+      h.flow.finish();
     });
 
     test('conditionEqNull promotes false branch', () {
@@ -30,7 +30,21 @@ main() {
       h.flow.ifStatement_elseBegin();
       expect(h.flow.promotedType(x).type, 'int');
       h.flow.ifStatement_end(true);
-      h.flow.verifyStackEmpty();
+      h.flow.finish();
+    });
+
+    test('finish checks proper nesting', () {
+      var h = _Harness();
+      var expr = _Expression();
+      h.flow.ifStatement_thenBegin(expr);
+      expect(() => h.flow.finish(), _asserts);
+    });
+
+    test('finish checks for un-added variables', () {
+      var h = _Harness();
+      var x = _Var('x', _Type('int'));
+      h.flow.isAssigned(x);
+      expect(() => h.flow.finish(), _asserts);
     });
 
     test('ifStatement_end(false) keeps else branch if then branch exits', () {
@@ -42,7 +56,7 @@ main() {
       h.flow.handleExit();
       h.flow.ifStatement_end(false);
       expect(h.flow.promotedType(x).type, 'int');
-      h.flow.verifyStackEmpty();
+      h.flow.finish();
     });
 
     void _checkIs(String declaredType, String tryPromoteType,
@@ -60,7 +74,7 @@ main() {
       h.flow.ifStatement_elseBegin();
       expect(h.flow.promotedType(x), isNull);
       h.flow.ifStatement_end(true);
-      h.flow.verifyStackEmpty();
+      h.flow.finish();
     }
 
     test('isExpression_end promotes to a subtype', () {
@@ -439,6 +453,18 @@ main() {
   });
 }
 
+/// Returns the appropriate matcher for expecting an assertion error to be
+/// thrown or not, based on whether assertions are enabled.
+Matcher get _asserts {
+  var matcher = throwsA(TypeMatcher<AssertionError>());
+  bool assertionsEnabled = false;
+  assert(assertionsEnabled = true);
+  if (!assertionsEnabled) {
+    matcher = isNot(matcher);
+  }
+  return matcher;
+}
+
 class _Expression {}
 
 class _Harness
diff --git a/pkg/analyzer/test/src/dart/resolution/type_promotion_test.dart b/pkg/analyzer/test/src/dart/resolution/type_promotion_test.dart
@@ -172,6 +172,16 @@ void f(bool b, Object x) {
 ''');
   }
 
+  test_for_declaredVar() async {
+    await resolveCode(r'''
+void f() {
+  for (Object x = g(); x is int; x = g()) {
+    /*int*/ x;
+  }
+}
+''');
+  }
+
   test_for_outerIsType() async {
     await resolveCode(r'''
 void f(bool b, Object x) {
