From dfda06061aecdb6fef6db4a7fddfdc1c58fe8190 Mon Sep 17 00:00:00 2001
From: Paul Berry <paulberry@google.com>
Date: Thu, 14 Feb 2019 12:58:09 +0000
Subject: [PATCH] Restore state safely in migration visitors.

When the migration tool is run in permissive mode, its visitors try to
catch exceptions and continue running.  This means that when the
visitors make temporary state changes before recursive calls, they
have to restore the state inside `finally` blocks.

This should hopefully result in slightly saner behavior when running
the migration tool in permissive mode.

Change-Id: I46e07684526314befad860c4e065ec2c7f6d8b11
Reviewed-on: https://dart-review.googlesource.com/c/93127
Auto-Submit: Paul Berry <paulberry@google.com>
Reviewed-by: Dan Rubel <danrubel@google.com>
---
 .../src/nullability/constraint_gatherer.dart  | 72 ++++++++++++-------
 .../constraint_variable_gatherer.dart         |  7 +-
 2 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
index a9b229df824..8b23b2e536c 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
@@ -240,8 +240,11 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
     assert(_currentFunctionType == null);
     _currentFunctionType =
         _variables.decoratedElementType(node.declaredElement);
-    node.functionExpression.body.accept(this);
-    _currentFunctionType = null;
+    try {
+      node.functionExpression.body.accept(this);
+    } finally {
+      _currentFunctionType = null;
+    }
     return null;
   }
 
@@ -265,16 +268,22 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
     if (trueGuard != null) {
       _guards.add(trueGuard);
     }
-    node.thenStatement.accept(this);
-    if (trueGuard != null) {
-      _guards.removeLast();
+    try {
+      node.thenStatement.accept(this);
+    } finally {
+      if (trueGuard != null) {
+        _guards.removeLast();
+      }
     }
     if (falseGuard != null) {
       _guards.add(falseGuard);
     }
-    node.elseStatement?.accept(this);
-    if (falseGuard != null) {
-      _guards.removeLast();
+    try {
+      node.elseStatement?.accept(this);
+    } finally {
+      if (falseGuard != null) {
+        _guards.removeLast();
+      }
     }
     return null;
   }
@@ -290,8 +299,11 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
     assert(_currentFunctionType == null);
     _currentFunctionType =
         _variables.decoratedElementType(node.declaredElement);
-    node.body.accept(this);
-    _currentFunctionType = null;
+    try {
+      node.body.accept(this);
+    } finally {
+      _currentFunctionType = null;
+    }
     return null;
   }
 
@@ -407,23 +419,26 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
       Expression expression) {
     if (sourceType.nullable != null) {
       _guards.add(sourceType.nullable);
-      CheckExpression checkNotNull;
-      if (expression != null) {
-        checkNotNull = CheckExpression(expression);
-        _variables.recordExpressionChecks(
-            expression, ExpressionChecks(_source, checkNotNull));
-      }
-      // nullable_src => nullable_dst | check_expr
-      _recordFact(ConstraintVariable.or(
-          _constraints, destinationType.nullable, checkNotNull));
-      if (checkNotNull != null) {
-        // nullable_src & nonNullIntent_dst => check_expr
-        var nonNullIntent = destinationType.nonNullIntent;
-        if (nonNullIntent != null) {
-          _recordConstraint(nonNullIntent, checkNotNull);
+      try {
+        CheckExpression checkNotNull;
+        if (expression != null) {
+          checkNotNull = CheckExpression(expression);
+          _variables.recordExpressionChecks(
+              expression, ExpressionChecks(_source, checkNotNull));
         }
+        // nullable_src => nullable_dst | check_expr
+        _recordFact(ConstraintVariable.or(
+            _constraints, destinationType.nullable, checkNotNull));
+        if (checkNotNull != null) {
+          // nullable_src & nonNullIntent_dst => check_expr
+          var nonNullIntent = destinationType.nonNullIntent;
+          if (nonNullIntent != null) {
+            _recordConstraint(nonNullIntent, checkNotNull);
+          }
+        }
+      } finally {
+        _guards.removeLast();
       }
-      _guards.removeLast();
     }
     // TODO(paulberry): it's a cheat to pass in expression=null for the
     // recursive checks.  Really we want to unify all the checks in a single
@@ -508,8 +523,11 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
   void _recordConstraint(
       ConstraintVariable condition, ConstraintVariable consequence) {
     _guards.add(condition);
-    _recordFact(consequence);
-    _guards.removeLast();
+    try {
+      _recordFact(consequence);
+    } finally {
+      _guards.removeLast();
+    }
   }
 
   /// Records a constraint having [consequence] as its right hand side.  Any
diff --git a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
index d3a89538207..f722bada72e 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
@@ -172,8 +172,11 @@ class ConstraintVariableGatherer extends GeneralizingAstVisitor<DecoratedType> {
         namedParameters: {},
         namedParameterOptionalVariables: {});
     _currentFunctionType = functionType;
-    parameters.accept(this);
-    _currentFunctionType = previousFunctionType;
+    try {
+      parameters.accept(this);
+    } finally {
+      _currentFunctionType = previousFunctionType;
+    }
     _variables.recordDecoratedElementType(declaredElement, functionType);
   }
 }