commit db07eef77d537bafe05a8546860dcc0e64ef4d3d
author Sam Rawlins <srawlins@google.com> Wed Oct 27 01:43:21 2021 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Wed Oct 27 01:43:21 2021 +0000
tree 38cd1151a3865ddffdd8e9d37416fe28e981ba9a
parent c46baff9c9a7d10b55d9039f0c75c11ecc41c32a

analyzer: visit local variables in resolution more safely

I'm preparing for the situation where an expression of (almost) any kind
could be wrapped with a generic function instantiation, and I was very
worried that there were a lot of cases like this:

    var localVariable = node.childNode;
    localVariable.accept(_resolver);
    // Do more with localVariable.

This is dangerous because localVariable may no longer be the same node
as `node.childNode`. I searched eerywhere I could for dangerous cases
like this. In some cases, I follow the middle statement with
`localVariable = node.childNode`, which is a common practice. In other
cases, the local variable was only referenced 2 or 3 times, and was
unnecessary. In other cases, I saw that the node strictly referred to
statements or other types of nodes, like a CatchClause. I am
reassured though that there really wasn't too much code like this.

I'd love to figure out how to enforce this statically, but haven't
found a good solution.

Change-Id: If38124dc2036b6f04879a065d66bf3ea73e68977
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/218184
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>

pkg/analyzer/lib/src/generated/resolver.dart

diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index 202769a..8b33178 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -1224,21 +1224,20 @@
     boolExpressionVerifier.checkForNonBoolCondition(condition,
         whyNotPromoted: whyNotPromoted);
 
-    Expression thenExpression = node.thenExpression;
-    InferenceContext.setTypeFromNode(thenExpression, node);
+    InferenceContext.setTypeFromNode(node.thenExpression, node);
 
     if (flow != null) {
       flow.conditional_thenBegin(condition, node);
-      checkUnreachableNode(thenExpression);
+      checkUnreachableNode(node.thenExpression);
     }
-    thenExpression.accept(this);
-    nullSafetyDeadCodeVerifier.flowEnd(thenExpression);
+    node.thenExpression.accept(this);
+    nullSafetyDeadCodeVerifier.flowEnd(node.thenExpression);
 
     Expression elseExpression = node.elseExpression;
     InferenceContext.setTypeFromNode(elseExpression, node);
 
     if (flow != null) {
-      flow.conditional_elseBegin(thenExpression);
+      flow.conditional_elseBegin(node.thenExpression);
       checkUnreachableNode(elseExpression);
       elseExpression.accept(this);
       flow.conditional_end(node, elseExpression);
@@ -1246,6 +1245,7 @@
     } else {
       elseExpression.accept(this);
     }
+    elseExpression = node.elseExpression;
 
     node.accept(elementResolver);
     node.accept(typeAnalyzer);
@@ -1300,6 +1300,7 @@
     var expression = node.expression;
     InferenceContext.setType(expression, fieldType);
     expression.accept(this);
+    expression = node.expression;
     var whyNotPromoted = flowAnalysis.flow?.whyNotPromoted(expression);
     node.accept(elementResolver);
     node.accept(typeAnalyzer);
@@ -1356,11 +1357,10 @@
   void visitDoStatement(DoStatement node) {
     checkUnreachableNode(node);
 
-    var body = node.body;
     var condition = node.condition;
 
     flowAnalysis.flow?.doStatement_bodyBegin(node);
-    body.accept(this);
+    node.body.accept(this);
 
     flowAnalysis.flow?.doStatement_conditionBegin();
     InferenceContext.setType(condition, typeProvider.boolType);
@@ -1599,10 +1599,9 @@
     boolExpressionVerifier.checkForNonBoolCondition(condition,
         whyNotPromoted: whyNotPromoted);
 
-    CollectionElement thenElement = node.thenElement;
     flowAnalysis.flow?.ifStatement_thenBegin(condition, node);
-    thenElement.accept(this);
-    nullSafetyDeadCodeVerifier.flowEnd(thenElement);
+    node.thenElement.accept(this);
+    nullSafetyDeadCodeVerifier.flowEnd(node.thenElement);
 
     var elseElement = node.elseElement;
     if (elseElement != null) {
@@ -1632,10 +1631,9 @@
     boolExpressionVerifier.checkForNonBoolCondition(condition,
         whyNotPromoted: whyNotPromoted);
 
-    Statement thenStatement = node.thenStatement;
     flowAnalysis.flow?.ifStatement_thenBegin(condition, node);
-    thenStatement.accept(this);
-    nullSafetyDeadCodeVerifier.flowEnd(thenStatement);
+    node.thenStatement.accept(this);
+    nullSafetyDeadCodeVerifier.flowEnd(node.thenStatement);
 
     var elseStatement = node.elseStatement;
     if (elseStatement != null) {
@@ -1760,6 +1758,7 @@
     var whyNotPromotedList = <Map<DartType, NonPromotionReason> Function()>[];
     var target = node.target;
     target?.accept(this);
+    target = node.target;
 
     if (_migratableAstInfoProvider.isMethodInvocationNullAware(node)) {
       var flow = flowAnalysis.flow;
@@ -2141,9 +2140,8 @@
     boolExpressionVerifier.checkForNonBoolCondition(node.condition,
         whyNotPromoted: whyNotPromoted);
 
-    Statement body = node.body;
     flowAnalysis.flow?.whileStatement_bodyBegin(node, condition);
-    body.accept(this);
+    node.body.accept(this);
     flowAnalysis.flow?.whileStatement_end();
     nullSafetyDeadCodeVerifier.flowEnd(node.body);
     // TODO(brianwilkerson) If the loop can only be exited because the condition