Scope tweaks. Report REFERENCED_BEFORE_DECLARATION in more places.

Change-Id: Iac395adf15c3f636e3e7e7241e0573f83ab61372
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155304
Reviewed-by: Brian Wilkerson <[email protected]>

Author: scheglov

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

@@ -14,13 +14,17 @@ import 'package:analyzer/src/generated/engine.dart';
 
 /// The scope defined by a block.
 class BlockScope {
-  /// Return the elements that are declared directly in the given [block]. This
-  /// does not include elements declared in nested blocks.
-  static Iterable<Element> elementsInBlock(Block block) sync* {
-    NodeList<Statement> statements = block.statements;
+  /// Return the elements that are declared directly in the given [statements].
+  /// This does not include elements declared in nested blocks.
+  static Iterable<Element> elementsInStatements(
+    List<Statement> statements,
+  ) sync* {
     int statementCount = statements.length;
     for (int i = 0; i < statementCount; i++) {
       Statement statement = statements[i];
+      if (statement is LabeledStatement) {
+        statement = (statement as LabeledStatement).statement;
+      }
       if (statement is VariableDeclarationStatement) {
         NodeList<VariableDeclaration> variables = statement.variables.variables;
         int variableCount = variables.length;

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

@@ -359,13 +359,10 @@ class ErrorVerifier extends RecursiveAstVisitor<void> {
 
   @override
   void visitBlock(Block node) {
-    _hiddenElements = HiddenElements(_hiddenElements, node);
-    try {
+    _withHiddenElements(node.statements, () {
       _duplicateDefinitionVerifier.checkStatements(node.statements);
       super.visitBlock(node);
-    } finally {
-      _hiddenElements = _hiddenElements.outerElements;
-    }
+    });
   }
 
   @override
@@ -1111,14 +1108,18 @@ class ErrorVerifier extends RecursiveAstVisitor<void> {
 
   @override
   void visitSwitchCase(SwitchCase node) {
-    _duplicateDefinitionVerifier.checkStatements(node.statements);
-    super.visitSwitchCase(node);
+    _withHiddenElements(node.statements, () {
+      _duplicateDefinitionVerifier.checkStatements(node.statements);
+      super.visitSwitchCase(node);
+    });
   }
 
   @override
   void visitSwitchDefault(SwitchDefault node) {
-    _duplicateDefinitionVerifier.checkStatements(node.statements);
-    super.visitSwitchDefault(node);
+    _withHiddenElements(node.statements, () {
+      _duplicateDefinitionVerifier.checkStatements(node.statements);
+      super.visitSwitchDefault(node);
+    });
   }
 
   @override
@@ -5124,6 +5125,15 @@ class ErrorVerifier extends RecursiveAstVisitor<void> {
     }
   }
 
+  void _withHiddenElements(List<Statement> statements, void Function() f) {
+    _hiddenElements = HiddenElements(_hiddenElements, statements);
+    try {
+      f();
+    } finally {
+      _hiddenElements = _hiddenElements.outerElements;
+    }
+  }
+
   /// Return [FieldElement]s that are declared in the [ClassDeclaration] with
   /// the given [constructor], but are not initialized.
   static List<FieldElement> computeNotInitializedFields(
@@ -5212,9 +5222,9 @@ class HiddenElements {
 
   /// Initialize a newly created set of hidden elements to include all of the
   /// elements defined in the set of [outerElements] and all of the elements
-  /// declared in the given [block].
-  HiddenElements(this.outerElements, Block block) {
-    _initializeElements(block);
+  /// declared in the given [statements].
+  HiddenElements(this.outerElements, List<Statement> statements) {
+    _initializeElements(statements);
   }
 
   /// Return `true` if this set of elements contains the given [element].
@@ -5234,9 +5244,9 @@ class HiddenElements {
   }
 
   /// Initialize the list of elements that are not yet declared to be all of the
-  /// elements declared somewhere in the given [block].
-  void _initializeElements(Block block) {
-    _elements.addAll(BlockScope.elementsInBlock(block));
+  /// elements declared somewhere in the given [statements].
+  void _initializeElements(List<Statement> statements) {
+    _elements.addAll(BlockScope.elementsInStatements(statements));
   }
 }
 

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

@@ -2154,16 +2154,9 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
 
   @override
   void visitBlock(Block node) {
-    Scope outerScope = nameScope;
-    try {
-      var enclosedScope = LocalScope(nameScope);
-      BlockScope.elementsInBlock(node).forEach(enclosedScope.add);
-      nameScope = enclosedScope;
-      _setNodeNameScope(node, nameScope);
+    _withDeclaredLocals(node, node.statements, () {
       super.visitBlock(node);
-    } finally {
-      nameScope = outerScope;
-    }
+    });
   }
 
   @override
@@ -2296,11 +2289,7 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
 
   @override
   void visitDeclaredIdentifier(DeclaredIdentifier node) {
-    VariableElement element = node.declaredElement;
-    // TODO(scheglov) Do we need this?
-    if (element != null) {
-      _define(element);
-    }
+    _define(node.declaredElement);
     super.visitDeclaredIdentifier(node);
   }
 
@@ -2467,15 +2456,9 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
 
   @override
   void visitFunctionDeclaration(FunctionDeclaration node) {
-    ExecutableElement element = node.declaredElement;
-    // TODO(scheglov) Do we need this?
-    if (element != null &&
-        element.enclosingElement is! CompilationUnitElement) {
-      _define(element);
-    }
-
     Scope outerScope = nameScope;
     try {
+      var element = node.declaredElement;
       nameScope = TypeParameterScope(
         nameScope,
         element.typeParameters,
@@ -2678,26 +2661,17 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
   @override
   void visitSwitchCase(SwitchCase node) {
     node.expression.accept(this);
-    Scope outerNameScope = nameScope;
-    try {
-      nameScope = LocalScope(nameScope);
-      _setNodeNameScope(node, nameScope);
+
+    _withDeclaredLocals(node, node.statements, () {
       node.statements.accept(this);
-    } finally {
-      nameScope = outerNameScope;
-    }
+    });
   }
 
   @override
   void visitSwitchDefault(SwitchDefault node) {
-    Scope outerNameScope = nameScope;
-    try {
-      nameScope = LocalScope(nameScope);
-      _setNodeNameScope(node, nameScope);
+    _withDeclaredLocals(node, node.statements, () {
       node.statements.accept(this);
-    } finally {
-      nameScope = outerNameScope;
-    }
+    });
   }
 
   @override
@@ -2728,13 +2702,9 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
   @override
   void visitVariableDeclaration(VariableDeclaration node) {
     super.visitVariableDeclaration(node);
-    if (node.parent.parent is! TopLevelVariableDeclaration &&
-        node.parent.parent is! FieldDeclaration) {
-      // TODO(scheglov) Do we need this?
-      VariableElement element = node.declaredElement;
-      if (element != null) {
-        _define(element);
-      }
+
+    if (node.parent.parent is ForParts) {
+      _define(node.declaredElement);
     }
   }
 
@@ -2769,6 +2739,25 @@ abstract class ScopedVisitor extends UnifyingAstVisitor<void> {
     (nameScope as LocalScope).add(element);
   }
 
+  void _withDeclaredLocals(
+    AstNode node,
+    List<Statement> statements,
+    void Function() f,
+  ) {
+    var outerScope = nameScope;
+    try {
+      var enclosedScope = LocalScope(nameScope);
+      BlockScope.elementsInStatements(statements).forEach(enclosedScope.add);
+
+      nameScope = enclosedScope;
+      _setNodeNameScope(node, nameScope);
+
+      f();
+    } finally {
+      nameScope = outerScope;
+    }
+  }
+
   /// Return the [Scope] to use while resolving inside the [node].
   ///
   /// Not every node has the scope set, for example we set the scopes for

pkg/analyzer/test/generated/non_error_resolver_test.dart

@@ -1324,27 +1324,6 @@ class A {
 ''');
   }
 
-  test_for_in_scope() async {
-    await assertNoErrorsInCode('''
-main() {
-  List<List<int>> x = [[1]];
-  for (int x in x.first) {
-    print(x.isEven);
-  }
-}
-''');
-  }
-
-  test_forEach_genericFunctionType() async {
-    await assertNoErrorsInCode(r'''
-main() {
-  for (Null Function<T>(T, Null) e in <dynamic>[]) {
-    e;
-  }
-}
-''');
-  }
-
   test_functionDeclaration_scope_returnType() async {
     await assertNoErrorsInCode('''
 int f(int) { return 0; }

pkg/analyzer/test/generated/resolver_test.dart

@@ -350,7 +350,6 @@ int f(num n) {
     ]);
   }
 
-//  @failingTest
   test_for() async {
     await assertErrorsInCode(r'''
 int f(List<int> list) {

pkg/analyzer/test/src/dart/resolution/for_statement_test.dart

@@ -2,8 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-import 'package:analyzer/src/error/codes.dart';
-import 'package:test/test.dart';
+import 'package:analyzer/src/dart/error/syntactic_errors.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import 'driver_resolution.dart';
@@ -15,28 +14,67 @@ main() {
   });
 }
 
+/// TODO(scheglov) Move other for-in tests here.
 @reflectiveTest
 class ForEachStatementResolutionTest extends DriverResolutionTest {
-  test_importPrefix_asIterable() async {
-    // TODO(scheglov) Remove this test (already tested as import prefix).
-    // TODO(scheglov) Move other for-in tests here.
+  test_iterable_missing() async {
     await assertErrorsInCode(r'''
-import 'dart:async' as p;
-
-main() {
-  for (var x in p) {}
+void f() {
+  for (var v in) {
+    v;
+  }
 }
 ''', [
-      error(HintCode.UNUSED_LOCAL_VARIABLE, 47, 1),
-      error(CompileTimeErrorCode.PREFIX_IDENTIFIER_NOT_FOLLOWED_BY_DOT, 52, 1),
+      error(ParserErrorCode.MISSING_IDENTIFIER, 26, 1),
     ]);
 
-    var xRef = findNode.simple('x in');
-    expect(xRef.staticElement, isNotNull);
+    assertType(findElement.localVar('v').type, 'dynamic');
+    assertType(findNode.simple('v;'), 'dynamic');
+  }
+
+  /// Test that the parameter `x` is in the scope of the iterable.
+  /// But the declared identifier `x` is in the scope of the body.
+  test_scope() async {
+    await assertNoErrorsInCode('''
+void f(List<List<int>> x) {
+  for (int x in x.first) {
+    x.isEven;
+  }
+}
+''');
+
+    assertElement(
+      findNode.simple('x) {'),
+      findElement.parameter('x'),
+    );
+
+    assertElement(
+      findNode.simple('x.isEven'),
+      findElement.localVar('x'),
+    );
+  }
+
+  test_type_genericFunctionType() async {
+    await assertNoErrorsInCode(r'''
+void f() {
+  for (Null Function<T>(T, Null) e in <dynamic>[]) {
+    e;
+  }
+}
+''');
+  }
+
+  test_type_inferred() async {
+    await assertNoErrorsInCode(r'''
+void f(List<int> a) {
+  for (var v in a) {
+    v;
+  }
+}
+''');
 
-    var pRef = findNode.simple('p) {}');
-    assertElement(pRef, findElement.prefix('p'));
-    assertTypeDynamic(pRef);
+    assertType(findElement.localVar('v').type, 'int');
+    assertType(findNode.simple('v;'), 'int');
   }
 }