Stop using ExecutableElement.localVariables in RenameClassMemberRefactoringImpl.

This is the only / last usage in Analysis Server.
And this change fixes 3 failing tests.

R=brianwilkerson@google.com
BUG=

Review-Url: https://codereview.chromium.org/2961363003 .

pkg/analysis_server/lib/src/edit/edit_domain.dart

@@ -830,8 +830,8 @@ class _RefactoringManager {
       CompilationUnit unit = await server.getResolvedCompilationUnit(file);
       if (unit != null) {
         _resetOnAnalysisStarted();
-        refactoring =
-            new ExtractMethodRefactoring(searchEngine, unit, offset, length);
+        refactoring = new ExtractMethodRefactoring(
+            searchEngine, server.getAstProvider(file), unit, offset, length);
         feedback = new ExtractMethodFeedback(offset, length, '', <String>[],
             false, <RefactoringMethodParameter>[], <int>[], <int>[]);
       }

pkg/analysis_server/lib/src/services/refactoring/extract_method.dart

@@ -24,6 +24,7 @@ import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/ast/utilities.dart';
+import 'package:analyzer/src/dart/element/ast_provider.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/java_core.dart';
 import 'package:analyzer/src/generated/resolver.dart' show ExitDetector;
@@ -72,6 +73,7 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl
       'execution flows exit. Semantics may not be preserved.';
 
   final SearchEngine searchEngine;
+  final AstProvider astProvider;
   final CompilationUnit unit;
   final int selectionOffset;
   final int selectionLength;
@@ -118,7 +120,7 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl
   List<_Occurrence> _occurrences = [];
   bool _staticContext = false;
 
-  ExtractMethodRefactoringImpl(this.searchEngine, this.unit,
+  ExtractMethodRefactoringImpl(this.searchEngine, this.astProvider, this.unit,
       this.selectionOffset, this.selectionLength) {
     unitElement = unit.element;
     libraryElement = unitElement.library;
@@ -418,7 +420,8 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl
     // method of class
     if (parent is ClassDeclaration) {
       ClassElement classElement = parent.element;
-      return validateCreateMethod(searchEngine, classElement, name);
+      return validateCreateMethod(
+          searchEngine, astProvider, classElement, name);
     }
     // OK
     return new Future<RefactoringStatus>.value(result);

pkg/analysis_server/lib/src/services/refactoring/refactoring.dart

@@ -137,10 +137,14 @@ abstract class ExtractMethodRefactoring implements Refactoring {
   /**
    * Returns a new [ExtractMethodRefactoring] instance.
    */
-  factory ExtractMethodRefactoring(SearchEngine searchEngine,
-      CompilationUnit unit, int selectionOffset, int selectionLength) {
+  factory ExtractMethodRefactoring(
+      SearchEngine searchEngine,
+      AstProvider astProvider,
+      CompilationUnit unit,
+      int selectionOffset,
+      int selectionLength) {
     return new ExtractMethodRefactoringImpl(
-        searchEngine, unit, selectionOffset, selectionLength);
+        searchEngine, astProvider, unit, selectionOffset, selectionLength);
   }
 
   /**
@@ -398,7 +402,8 @@ abstract class RenameRefactoring implements Refactoring {
       return new RenameLocalRefactoringImpl(searchEngine, astProvider, element);
     }
     if (element.enclosingElement is ClassElement) {
-      return new RenameClassMemberRefactoringImpl(searchEngine, element);
+      return new RenameClassMemberRefactoringImpl(
+          searchEngine, astProvider, element);
     }
     return null;
   }
@@ -449,9 +454,7 @@ class ResolvedUnitCache {
   }
 
   Future<CompilationUnit> getUnit(Element element) async {
-    CompilationUnitElement unitElement =
-        element.getAncestor((e) => e is CompilationUnitElement)
-            as CompilationUnitElement;
+    CompilationUnitElement unitElement = getUnitElement(element);
     CompilationUnit unit = _map[unitElement];
     if (unit == null) {
       unit = await _astProvider.getResolvedUnitForElement(element);
@@ -459,4 +462,9 @@ class ResolvedUnitCache {
     }
     return unit;
   }
+
+  CompilationUnitElement getUnitElement(Element element) {
+    return element.getAncestor((e) => e is CompilationUnitElement)
+        as CompilationUnitElement;
+  }
 }

pkg/analysis_server/lib/src/services/refactoring/rename_class_member.dart

@@ -14,17 +14,20 @@ import 'package:analysis_server/src/services/refactoring/refactoring_internal.da
 import 'package:analysis_server/src/services/refactoring/rename.dart';
 import 'package:analysis_server/src/services/search/hierarchy.dart';
 import 'package:analysis_server/src/services/search/search_engine.dart';
-import 'package:analyzer/dart/ast/ast.dart' show Identifier;
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/src/dart/element/ast_provider.dart';
 import 'package:analyzer/src/generated/java_core.dart';
 
 /**
  * Checks if creating a method with the given [name] in [classElement] will
  * cause any conflicts.
  */
-Future<RefactoringStatus> validateCreateMethod(
-    SearchEngine searchEngine, ClassElement classElement, String name) {
-  return new _ClassMemberValidator.forCreate(searchEngine, classElement, name)
+Future<RefactoringStatus> validateCreateMethod(SearchEngine searchEngine,
+    AstProvider astProvider, ClassElement classElement, String name) {
+  return new _ClassMemberValidator.forCreate(
+          searchEngine, astProvider, classElement, name)
       .validate();
 }
 
@@ -32,9 +35,12 @@ Future<RefactoringStatus> validateCreateMethod(
  * A [Refactoring] for renaming class member [Element]s.
  */
 class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
+  final AstProvider astProvider;
+
   _ClassMemberValidator _validator;
 
-  RenameClassMemberRefactoringImpl(SearchEngine searchEngine, Element element)
+  RenameClassMemberRefactoringImpl(
+      SearchEngine searchEngine, this.astProvider, Element element)
       : super(searchEngine, element);
 
   @override
@@ -50,8 +56,8 @@ class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
 
   @override
   Future<RefactoringStatus> checkFinalConditions() {
-    _validator =
-        new _ClassMemberValidator.forRename(searchEngine, element, newName);
+    _validator = new _ClassMemberValidator.forRename(
+        searchEngine, astProvider, element, newName);
     return _validator.validate();
   }
 
@@ -122,6 +128,7 @@ class RenameClassMemberRefactoringImpl extends RenameRefactoringImpl {
  */
 class _ClassMemberValidator {
   final SearchEngine searchEngine;
+  final ResolvedUnitCache unitCache;
   final LibraryElement library;
   final Element element;
   final ClassElement elementClass;
@@ -134,14 +141,17 @@ class _ClassMemberValidator {
   List<SearchMatch> references = <SearchMatch>[];
 
   _ClassMemberValidator.forCreate(
-      this.searchEngine, this.elementClass, this.name)
-      : isRename = false,
+      this.searchEngine, AstProvider astProvider, this.elementClass, this.name)
+      : unitCache = new ResolvedUnitCache(astProvider),
+        isRename = false,
         library = null,
         element = null,
         elementKind = ElementKind.METHOD;
 
-  _ClassMemberValidator.forRename(this.searchEngine, Element element, this.name)
-      : isRename = true,
+  _ClassMemberValidator.forRename(
+      this.searchEngine, AstProvider astProvider, Element element, this.name)
+      : unitCache = new ResolvedUnitCache(astProvider),
+        isRename = true,
         library = element.library,
         element = element,
         elementClass = element.enclosingElement,
@@ -188,7 +198,7 @@ class _ClassMemberValidator {
     }
     // usage of the renamed Element is shadowed by a local element
     {
-      _MatchShadowedByLocal conflict = _getShadowingLocalElement();
+      _MatchShadowedByLocal conflict = await _getShadowingLocalElement();
       if (conflict != null) {
         LocalElement localElement = conflict.localElement;
         result.addError(
@@ -237,25 +247,31 @@ class _ClassMemberValidator {
     return result;
   }
 
-  _MatchShadowedByLocal _getShadowingLocalElement() {
+  Future<_MatchShadowedByLocal> _getShadowingLocalElement() async {
+    var localElementMap = <CompilationUnitElement, List<LocalElement>>{};
+    Future<List<LocalElement>> getLocalElements(Element element) async {
+      var unitElement = unitCache.getUnitElement(element);
+      var localElements = localElementMap[unitElement];
+      if (localElements == null) {
+        var unit = await unitCache.getUnit(unitElement);
+        var collector = new _LocalElementsCollector(name);
+        unit.accept(collector);
+        localElements = collector.elements;
+        localElementMap[unitElement] = localElements;
+      }
+      return localElements;
+    }
+
     for (SearchMatch match in references) {
-      // qualified reference cannot be shadowed by a local element
+      // Qualified reference cannot be shadowed by local elements.
       if (match.isQualified) {
         continue;
       }
-      // check local elements of the enclosing executable
-      Element containingElement = match.element;
-      if (containingElement is ExecutableElement) {
-        Iterable<LocalElement> localElements = <Iterable<LocalElement>>[
-          containingElement.functions,
-          containingElement.localVariables,
-          containingElement.parameters
-        ].expand((Iterable<LocalElement> x) => x);
-        for (LocalElement localElement in localElements) {
-          if (localElement.displayName == name &&
-              localElement.visibleRange.intersects(match.sourceRange)) {
-            return new _MatchShadowedByLocal(match, localElement);
-          }
+      // Check local elements that might shadow the reference.
+      var localElements = await getLocalElements(match.element);
+      for (LocalElement localElement in localElements) {
+        if (localElement.visibleRange.intersects(match.sourceRange)) {
+          return new _MatchShadowedByLocal(match, localElement);
         }
       }
     }
@@ -307,6 +323,20 @@ class _ClassMemberValidator {
   }
 }
 
+class _LocalElementsCollector extends GeneralizingAstVisitor<Null> {
+  final String name;
+  final List<LocalElement> elements = [];
+
+  _LocalElementsCollector(this.name);
+
+  visitSimpleIdentifier(SimpleIdentifier node) {
+    Element element = node.staticElement;
+    if (element is LocalElement && element.name == name) {
+      elements.add(element);
+    }
+  }
+}
+
 class _MatchShadowedByLocal {
   final SearchMatch match;
   final LocalElement localElement;

pkg/analysis_server/test/services/refactoring/extract_method_test.dart

@@ -2841,8 +2841,8 @@ Future<int> newFuture() => null;
   }
 
   void _createRefactoring(int offset, int length) {
-    refactoring =
-        new ExtractMethodRefactoring(searchEngine, testUnit, offset, length);
+    refactoring = new ExtractMethodRefactoring(
+        searchEngine, astProvider, testUnit, offset, length);
     refactoring.name = 'res';
   }
 

pkg/analysis_server/test/services/refactoring/rename_class_member_test.dart

@@ -150,7 +150,6 @@ main(A a) {
         expectedMessage: "Renamed method will be invisible in 'my.lib'.");
   }
 
-  @failingTest
   test_checkFinalConditions_shadowed_byLocalFunction_inSameClass() async {
     await indexTestUnit('''
 class A {
@@ -171,7 +170,6 @@ class A {
         expectedContextSearch: 'test(); // marker');
   }
 
-  @failingTest
   test_checkFinalConditions_shadowed_byLocalVariable_inSameClass() async {
     await indexTestUnit('''
 class A {
@@ -192,7 +190,6 @@ class A {
         expectedContextSearch: 'test(); // marker');
   }
 
-  @failingTest
   test_checkFinalConditions_shadowed_byLocalVariable_inSubClass() async {
     await indexTestUnit('''
 class A {