[DAS] Renaming positional parameters to rename hierarchy equivalent

Fixes: #53187
Change-Id: Ie0aaea2e24667ad04a95ce6522fd687710501230
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/412520
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
Auto-Submit: Felipe Morschel <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>

Author: FMorschel

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

@@ -8,7 +8,9 @@ import 'package:analysis_server/src/services/correction/util.dart';
 import 'package:analysis_server/src/services/refactoring/legacy/refactoring.dart';
 import 'package:analysis_server/src/services/refactoring/legacy/refactoring_internal.dart';
 import 'package:analysis_server/src/services/search/search_engine.dart';
+import 'package:analyzer/dart/analysis/code_style_options.dart';
 import 'package:analyzer/dart/element/element2.dart';
+import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/src/dart/analysis/session_helper.dart';
 import 'package:analyzer/src/dart/element/element.dart';
 import 'package:analyzer/src/generated/java_core.dart';
@@ -154,6 +156,11 @@ abstract class RenameRefactoringImpl extends RefactoringImpl
   /// Adds individual edits to [change].
   Future<void> fillChange();
 
+  CodeStyleOptions getCodeStyleOptions(File file) =>
+      sessionHelper.session.analysisContext
+          .getAnalysisOptionsForFile(file)
+          .codeStyleOptions;
+
   static String _getOldName(Element2 element) {
     if (element is ConstructorElement2) {
       var name = element.name3;

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

@@ -17,6 +17,7 @@ import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element2.dart';
 import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/generated/java_core.dart';
 
 class ConflictValidatorVisitor extends RecursiveAstVisitor<void> {
   final RefactoringStatus result;
@@ -88,7 +89,12 @@ class ConflictValidatorVisitor extends RecursiveAstVisitor<void> {
       if (_isVisibleWithTarget(declaredElement)) {
         conflictingLocals.add(declaredElement);
         var nodeKind = declaredElement.kind.displayName;
-        var message = "Duplicate $nodeKind '$newName'.";
+        var message = format(
+          "Duplicate {0} of name '{1}'{2}.",
+          nodeKind,
+          newName,
+          declaredElement.declarationLocation,
+        );
         result.addError(message, newLocation_fromElement(declaredElement));
         return;
       }
@@ -200,3 +206,26 @@ class RenameLocalRefactoringImpl extends RenameRefactoringImpl {
     processor.addReferenceEdits(references);
   }
 }
+
+extension on Element2 {
+  String get declarationLocation {
+    var sourceName = firstFragment.libraryFragment!.source.shortName;
+    var executable = enclosingElement2;
+    String className = '';
+    String executableName = '';
+    if (executable is MethodElement2) {
+      var namescope = executable.enclosingElement2 as ClassElement2?;
+      className = namescope?.displayName ?? '';
+      if (className.isNotEmpty && executable.displayName.isNotEmpty) {
+        className += '.';
+      }
+      executableName = executable.displayName;
+    } else if (executable is TopLevelFunctionElement) {
+      executableName = executable.displayName;
+    }
+    if (executableName.isEmpty) {
+      return " in '$sourceName'";
+    }
+    return " at $className$executableName in '$sourceName'";
+  }
+}

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

@@ -11,10 +11,12 @@ import 'package:analysis_server/src/services/refactoring/legacy/visible_ranges_c
 import 'package:analysis_server/src/services/search/hierarchy.dart';
 import 'package:analyzer/dart/element/element2.dart';
 import 'package:analyzer/src/generated/java_core.dart';
+import 'package:analyzer_plugin/protocol/protocol_common.dart';
 
 /// A [Refactoring] for renaming [FormalParameterElement]s.
 class RenameParameterRefactoringImpl extends RenameRefactoringImpl {
   List<FormalParameterElement> elements = [];
+  bool _renameAllPositionalOccurences = false;
 
   RenameParameterRefactoringImpl(
     super.workspace,
@@ -33,6 +35,7 @@ class RenameParameterRefactoringImpl extends RenameRefactoringImpl {
   @override
   Future<RefactoringStatus> checkFinalConditions() async {
     var result = RefactoringStatus();
+    var conflictResult = RefactoringStatus();
     await _prepareElements();
     for (var element in elements) {
       if (newName.startsWith('_') && element.isNamed) {
@@ -45,16 +48,39 @@ class RenameParameterRefactoringImpl extends RenameRefactoringImpl {
         break;
       }
       var resolvedUnit = await sessionHelper.getResolvedUnitByElement(element);
-      var unit = resolvedUnit?.unit;
-      unit?.accept(
-        ConflictValidatorVisitor(
-          result,
-          newName,
-          element,
-          VisibleRangesComputer.forNode(unit),
+      if (resolvedUnit != null) {
+        // If any of the resolved units have the lint enabled, we should avoid
+        // renaming method parameters separately from the other implementations.
+        if (element.isPositional && !_renameAllPositionalOccurences) {
+          _renameAllPositionalOccurences |=
+              getCodeStyleOptions(
+                resolvedUnit.file,
+              ).avoidRenamingMethodParameters;
+        }
+
+        var unit = resolvedUnit.unit;
+        unit.accept(
+          ConflictValidatorVisitor(
+            conflictResult,
+            newName,
+            element,
+            VisibleRangesComputer.forNode(unit),
+          ),
+        );
+      }
+    }
+    if (_renameAllPositionalOccurences && elements.length > 1) {
+      result.addStatus(
+        _RefactoringStatusExt.from(
+          'This will also rename all related positional parameters '
+          'to the same name.',
+          conflictResult,
         ),
       );
     }
+    if (result.problem == null) {
+      return conflictResult;
+    }
     return result;
   }
 
@@ -69,6 +95,11 @@ class RenameParameterRefactoringImpl extends RenameRefactoringImpl {
   Future<void> fillChange() async {
     var processor = RenameProcessor(workspace, sessionHelper, change, newName);
     for (var element in elements) {
+      if (element != this.element &&
+          element.isPositional &&
+          !_renameAllPositionalOccurences) {
+        continue;
+      }
       var fieldRenamed = false;
       if (element is FieldFormalParameterElement2) {
         var field = element.field2;
@@ -104,10 +135,31 @@ class RenameParameterRefactoringImpl extends RenameRefactoringImpl {
   Future<void> _prepareElements() async {
     var element = this.element;
     if (element.isNamed) {
-      elements =
-          (await getHierarchyNamedParameters(searchEngine, element)).toList();
-    } else {
-      elements = [element];
+      elements = await getHierarchyNamedParameters(searchEngine, element);
+    } else if (element.isPositional) {
+      elements = await getHierarchyPositionalParameters(searchEngine, element);
     }
   }
 }
+
+extension _RefactoringStatusExt on RefactoringStatus {
+  String get messagesAggregated {
+    if (problems.isEmpty) {
+      return '';
+    }
+    if (problems.length == 1) {
+      return '\n${problems.first.message}';
+    }
+    return '\n${problems.first.message} And ${problems.length - 1} more '
+        'error${problems.length > 1 ? 's' : ''}.';
+  }
+
+  static RefactoringStatus from(String message, RefactoringStatus result) {
+    var constructor = switch (result.severity) {
+      RefactoringProblemSeverity.ERROR => RefactoringStatus.error,
+      RefactoringProblemSeverity.FATAL => RefactoringStatus.fatal,
+      _ => RefactoringStatus.warning,
+    };
+    return constructor(message + result.messagesAggregated);
+  }
+}

pkg/analysis_server/lib/src/services/search/hierarchy.dart

@@ -221,6 +221,37 @@ Future<List<FormalParameterElement>> getHierarchyNamedParameters(
   return [element];
 }
 
+Future<List<FormalParameterElement>> getHierarchyPositionalParameters(
+  SearchEngine searchEngine,
+  FormalParameterElement element,
+) async {
+  if (element.isPositional) {
+    var method = element.enclosingElement2;
+    if (method is MethodElement2) {
+      var index = method.parameterIndex(element);
+      // Should not ever happen but this means we can't find the index.
+      if (index == null) {
+        return [element];
+      }
+      var hierarchyParameters = <FormalParameterElement>[];
+      var hierarchyMembers = await getHierarchyMembers(searchEngine, method);
+      for (var hierarchyMethod in hierarchyMembers) {
+        if (hierarchyMethod is MethodElement2) {
+          for (var hierarchyParameter in hierarchyMethod.formalParameters) {
+            if (hierarchyParameter.isPositional &&
+                hierarchyMethod.parameterIndex(hierarchyParameter) == index) {
+              hierarchyParameters.add(hierarchyParameter);
+              break;
+            }
+          }
+        }
+      }
+      return hierarchyParameters;
+    }
+  }
+  return [element];
+}
+
 /// Returns non-synthetic members of the given [InterfaceElement2] and its super
 /// classes.
 ///
@@ -254,3 +285,18 @@ String? _getBaseName(Element2 element) {
   }
   return element.name3;
 }
+
+extension on MethodElement2 {
+  int? parameterIndex(FormalParameterElement parameter) {
+    var index = 0;
+    for (var positionalParameter in formalParameters.where(
+      (p) => p.isPositional,
+    )) {
+      if (positionalParameter == parameter) {
+        return index;
+      }
+      index++;
+    }
+    return null;
+  }
+}

pkg/analysis_server/test/edit/refactoring_test.dart

@@ -2739,7 +2739,7 @@ void f() {
       expect(problems, hasLength(1));
       assertResultProblemsError(
         problems,
-        "Duplicate local variable 'newName'.",
+        "Duplicate local variable of name 'newName' at f in 'test.dart'.",
       );
     });
   }