Issue 22988. Improve checking for conflicts between parameters and local elements during extracting…

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

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // 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.
 
 library services.src.refactoring.extract_method;
 
 import 'dart:async';
 
 import 'package:analysis_server/src/protocol_server.dart' hide Element;
 import 'package:analysis_server/src/services/correction/name_suggestion.dart';
@@ skipping 59 matching lines @@
 
   String returnType;
   String name;
   bool extractAll = true;
   bool canCreateGetter = false;
   bool createGetter = false;
   final List<String> names = <String>[];
   final List<int> offsets = <int>[];
   final List<int> lengths = <int>[];
 
-  Set<String> _usedNames = new Set<String>();
+  /**
+   * The map of local names to their visibility ranges.
+   */
+  Map<String, List<SourceRange>> _localNames = <String, List<SourceRange>>{};
+
+  /**
+   * The set of names that are referenced without any qualifier.
+   */
+  Set<String> _unqualifiedNames = new Set<String>();
+
   Set<String> _excludedNames = new Set<String>();
   List<RefactoringMethodParameter> _parameters = <RefactoringMethodParameter>[];
   Map<String, RefactoringMethodParameter> _parametersMap =
       <String, RefactoringMethodParameter>{};
   Map<String, List<SourceRange>> _parameterReferencesMap =
       <String, List<SourceRange>>{};
   DartType _returnType;
   String _returnVariableName;
   AstNode _parentMember;
   Expression _selectionExpression;
@@ skipping 258 matching lines @@
     RefactoringStatus result = new RefactoringStatus();
     for (RefactoringMethodParameter parameter in _parameters) {
       result.addStatus(validateParameterName(parameter.name));
       for (RefactoringMethodParameter other in _parameters) {
         if (!identical(parameter, other) && other.name == parameter.name) {
           result.addError(
               format("Parameter '{0}' already exists", parameter.name));
           return result;
         }
       }
-      if (_usedNames.contains(parameter.name)) {
+      // TODO

[Brian Wilkerson, 2015/04/03 21:00:41]

Remove or expand (explain what needs to be done).

+      if (_isParameterNameConflictWithBody(parameter)) {
         result.addError(format(
             "'{0}' is already used as a name in the selected code",
             parameter.name));
         return result;
       }
+//      List<SourceRange> usedRanges = _usedNames[parameter.name];
+//      if (_usedNames.contains(parameter.name)) {
+//        result.addError(format(
+//            "'{0}' is already used as a name in the selected code",
+//            parameter.name));
+//        return result;
+//      }
     }
     return result;
   }
 
   /**
    * Checks if created method will shadow or will be shadowed by other elements.
    */
   Future<RefactoringStatus> _checkPossibleConflicts() async {
     RefactoringStatus result = new RefactoringStatus();
     AstNode parent = _parentMember.parent;
@@ skipping 246 matching lines @@
 
   /**
    * Checks if it is OK to extract the node with the given [SourceRange].
    */
   bool _isExtractable(SourceRange range) {
     _ExtractMethodAnalyzer analyzer = new _ExtractMethodAnalyzer(unit, range);
     utils.unit.accept(analyzer);
     return analyzer.status.isOK;
   }
 
+  bool _isParameterNameConflictWithBody(RefactoringMethodParameter parameter) {
+    String id = parameter.id;
+    String name = parameter.name;
+    // TODO

[Brian Wilkerson, 2015/04/03 21:00:41]

ditto

+    List<SourceRange> parameterRanges = _parameterReferencesMap[id];
+    List<SourceRange> otherRanges = _localNames[name];
+    for (SourceRange parameterRange in parameterRanges) {
+      if (otherRanges != null) {
+        for (SourceRange otherRange in otherRanges) {
+          if (parameterRange.intersects(otherRange)) {
+            return true;
+          }
+        }
+      }
+    }
+    if (_unqualifiedNames.contains(name)) {
+      return true;
+    }
+    return false;
+  }
+
   /**
    * Checks if [element] is referenced after [selectionRange].
    */
   bool _isUsedAfterSelection(VariableElement element) {
     var visitor = new _IsUsedAfterSelectionVisitor(this, element);
     _parentMember.accept(visitor);
     return visitor.result;
   }
 
   /**
@@ skipping 325 matching lines @@
       // next statement
       if (found) {
         beginStatementIndex += selectionCount;
       } else {
         beginStatementIndex++;
       }
     }
   }
 }
 
-class _InitializeParametersVisitor extends GeneralizingAstVisitor<Object> {
+class _InitializeParametersVisitor extends GeneralizingAstVisitor {
   final ExtractMethodRefactoringImpl ref;
   final List<VariableElement> assignedUsedVariables;
 
   _InitializeParametersVisitor(this.ref, this.assignedUsedVariables);
 
   @override
-  Object visitSimpleIdentifier(SimpleIdentifier node) {
+  void visitSimpleIdentifier(SimpleIdentifier node) {
     SourceRange nodeRange = rangeNode(node);
-    if (ref.selectionRange.covers(nodeRange)) {
-      // analyze local variable
-      VariableElement variableElement =
-          getLocalOrParameterVariableElement(node);
-      if (variableElement != null) {
-        // name of the named expression
-        if (isNamedExpressionName(node)) {
-          return null;
+    if (!ref.selectionRange.covers(nodeRange)) {
+      return;
+    }
+    String name = node.name;
+    // analyze local variable
+    VariableElement variableElement = getLocalOrParameterVariableElement(node);
+    if (variableElement != null) {
+      // name of the named expression
+      if (isNamedExpressionName(node)) {
+        return;
+      }
+      // if declared outside, add parameter
+      if (!ref._isDeclaredInSelection(variableElement)) {
+        // add parameter
+        RefactoringMethodParameter parameter = ref._parametersMap[name];
+        if (parameter == null) {
+          DartType parameterType = node.bestType;
+          String parameterTypeCode = ref._getTypeCode(parameterType);
+          parameter = new RefactoringMethodParameter(
+              RefactoringMethodParameterKind.REQUIRED, parameterTypeCode, name,
+              id: name);
+          ref._parameters.add(parameter);
+          ref._parametersMap[name] = parameter;
         }
-        // if declared outside, add parameter
-        if (!ref._isDeclaredInSelection(variableElement)) {
-          String variableName = variableElement.displayName;
-          // add parameter
-          RefactoringMethodParameter parameter =
-              ref._parametersMap[variableName];
-          if (parameter == null) {
-            DartType parameterType = node.bestType;
-            String parameterTypeCode = ref._getTypeCode(parameterType);
-            parameter = new RefactoringMethodParameter(
-                RefactoringMethodParameterKind.REQUIRED, parameterTypeCode,
-                variableName, id: variableName);
-            ref._parameters.add(parameter);
-            ref._parametersMap[variableName] = parameter;
-          }
-          // add reference to parameter
-          ref._addParameterReference(variableName, nodeRange);
-        }
-        // remember, if assigned and used after selection
-        if (isLeftHandOfAssignment(node) &&
-            ref._isUsedAfterSelection(variableElement)) {
-          if (!assignedUsedVariables.contains(variableElement)) {
-            assignedUsedVariables.add(variableElement);
-          }
+        // add reference to parameter
+        ref._addParameterReference(name, nodeRange);
+      }
+      // remember, if assigned and used after selection
+      if (isLeftHandOfAssignment(node) &&
+          ref._isUsedAfterSelection(variableElement)) {
+        if (!assignedUsedVariables.contains(variableElement)) {
+          assignedUsedVariables.add(variableElement);
         }
       }
-      // remember declaration names
+    }
+    // remember information for conflicts checking
+    if (variableElement is LocalElement) {
+      // declared local elements
+      LocalElement localElement = variableElement as LocalElement;
       if (node.inDeclarationContext()) {
-        ref._usedNames.add(node.name);
+        ref._localNames.putIfAbsent(name, () => <SourceRange>[]);
+        ref._localNames[name].add(localElement.visibleRange);
+      }
+    } else {
+      // unqualified non-local names
+      if (!node.isQualified) {
+        ref._unqualifiedNames.add(name);
       }
     }
-    return null;
   }
 }
 
 class _IsUsedAfterSelectionVisitor extends GeneralizingAstVisitor {
   final ExtractMethodRefactoringImpl ref;
   final VariableElement element;
   bool result = false;
 
   _IsUsedAfterSelectionVisitor(this.ref, this.element);
 
@@ skipping 71 matching lines @@
       return false;
     }
     for (int i = 0; i < parameterTypes.length; i++) {
       if (other.parameterTypes[i] != parameterTypes[i]) {
         return false;
       }
     }
     return true;
   }
 }