[analysis_server] Treat field formal parameters and fields equally when finding references in LSP

This changes LSP's Find References to treat fields and field formal parameters as a single entity (matching what Document Highlights (Occurences) and Rename refactor does).

It does this in one direction by resolving the initial element to a field if it's a FieldFormalParameter, and in the other by including FieldFormalParameters when collecting the set of elements in the hierarchy to find references to.

Fixes Dart-Code/Dart-Code#4868

Change-Id: I33626a8d58f35d0c3bda574078f32bf98f4bc87c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364120
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/lsp/handlers/handler_references.dart

@@ -7,7 +7,6 @@ import 'package:analysis_server/src/lsp/error_or.dart';
 import 'package:analysis_server/src/lsp/handlers/handlers.dart';
 import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/lsp/registration/feature_registration.dart';
-import 'package:analysis_server/src/protocol_server.dart' show NavigationTarget;
 import 'package:analysis_server/src/search/element_references.dart';
 import 'package:analysis_server/src/services/search/search_engine.dart'
     show SearchMatch;
@@ -16,8 +15,6 @@ import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/src/dart/ast/utilities.dart';
 import 'package:analyzer/src/util/performance/operation_performance.dart';
-import 'package:analyzer_plugin/src/utilities/navigation/navigation.dart';
-import 'package:analyzer_plugin/utilities/navigation/navigation_dart.dart';
 
 typedef StaticOptions = Either2<bool, ReferenceOptions>;
 
@@ -48,19 +45,20 @@ class ReferencesHandler
             _getReferences(unit, offset, params, performance)));
   }
 
-  List<Location> _getDeclarations(ParsedUnitResult result, int offset) {
-    var collector = NavigationCollectorImpl();
-    computeDartNavigation(
-        server.resourceProvider, collector, result, offset, 0);
-
-    return convert(collector.targets, (NavigationTarget target) {
-      var targetFilePath = collector.files[target.fileIndex];
-      var targetFileUri = uriConverter.toClientUri(targetFilePath);
-      var lineInfo = server.getLineInfo(targetFilePath);
-      return lineInfo != null
-          ? navigationTargetToLocation(targetFileUri, target, lineInfo)
-          : null;
-    }).nonNulls.toList();
+  List<Location> _getDeclarations(Element element) {
+    element = element.nonSynthetic;
+    var unitElement = element.thisOrAncestorOfType<CompilationUnitElement>();
+    if (unitElement == null) {
+      return [];
+    }
+
+    return [
+      Location(
+        uri: uriConverter.toClientUri(unitElement.source.fullName),
+        range: toRange(
+            unitElement.lineInfo, element.nameOffset, element.nameLength),
+      )
+    ];
   }
 
   Future<ErrorOr<List<Location>?>> _getReferences(
@@ -72,6 +70,7 @@ class ReferencesHandler
     node = _getReferenceTargetNode(node);
 
     var element = switch (server.getElementOfNode(node)) {
+      FieldFormalParameterElement(:var field?) => field,
       PropertyAccessorElement(:var variable2?) => variable2,
       (var element) => element,
     };
@@ -106,9 +105,9 @@ class ReferencesHandler
         'convert', (_) => convert(results, toLocation).nonNulls.toList());
 
     if (params.context.includeDeclaration == true) {
-      // Also include the definition for the symbol at this location.
+      // Also include the definition for the resolved element.
       referenceResults.addAll(performance.run(
-          '_getDeclarations', (_) => _getDeclarations(result, offset)));
+          '_getDeclarations', (_) => _getDeclarations(element)));
     }
 
     return success(referenceResults);

pkg/analysis_server/lib/src/search/element_references.dart

@@ -68,18 +68,25 @@ class ElementReferencesComputer {
   ///
   /// Otherwise, only references to [element] should be searched.
   Future<Iterable<Element>> _getRefElements(
-      Element element, OperationPerformanceImpl performance) {
+      Element element, OperationPerformanceImpl performance) async {
     if (element is ParameterElement && element.isNamed) {
-      return performance.runAsync('getHierarchyNamedParameters',
+      return await performance.runAsync('getHierarchyNamedParameters',
           (_) => getHierarchyNamedParameters(searchEngine, element));
     }
     if (element is ClassMemberElement) {
-      return performance.runAsync(
-          'getHierarchyMembers',
-          (performance) => getHierarchyMembers(searchEngine, element,
-              performance: performance));
+      var (members, parameters) = await performance.runAsync(
+        'getHierarchyMembers',
+        (performance) => getHierarchyMembersAndParameters(
+          searchEngine,
+          element,
+          performance: performance,
+          includeParametersForFields: true,
+        ),
+      );
+
+      return {...members, ...parameters};
     }
-    return Future.value([element]);
+    return [element];
   }
 
   static SearchResult toResult(SearchMatch match) {

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

@@ -59,7 +59,7 @@ class ConvertGetterToMethodRefactoringImpl extends RefactoringImpl
         (field.enclosingElement is InterfaceElement ||
             field.enclosingElement is ExtensionElement)) {
       var elements = await getHierarchyMembers(searchEngine, field);
-      await Future.forEach(elements, (ClassMemberElement member) async {
+      await Future.forEach(elements, (Element member) async {
         if (member is FieldElement) {
           var getter = member.getter;
           if (getter != null && !getter.isSynthetic) {

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

@@ -80,23 +80,49 @@ List<Element> getExtensionMembers(ExtensionElement extension, [String? name]) {
   return members;
 }
 
-/// Return all implementations of the given [member], its superclasses, and
-/// their subclasses.
+/// Return all implementations of the given [member], including in its
+/// superclasses and their subclasses.
+///
+/// If [includeParametersForFields] is true and [member] is a [FieldElement],
+/// any [FieldFormalParameterElement]s for the member will also be provided
+/// (otherwise, the parameter set will be empty in the result).
 Future<Set<ClassMemberElement>> getHierarchyMembers(
-    SearchEngine searchEngine, ClassMemberElement member,
-    {OperationPerformanceImpl? performance}) async {
+  SearchEngine searchEngine,
+  ClassMemberElement member, {
+  OperationPerformanceImpl? performance,
+}) async {
+  var (members, _) = await getHierarchyMembersAndParameters(
+      searchEngine, member,
+      performance: performance);
+  return members;
+}
+
+/// Return all implementations of the given [member], including in its
+/// superclasses and their subclasses.
+///
+/// If [includeParametersForFields] is true and [member] is a [FieldElement],
+/// any [FieldFormalParameterElement]s for the member will also be provided
+/// (otherwise, the parameter set will be empty in the result).
+Future<(Set<ClassMemberElement>, Set<ParameterElement>)>
+    getHierarchyMembersAndParameters(
+  SearchEngine searchEngine,
+  ClassMemberElement member, {
+  OperationPerformanceImpl? performance,
+  bool includeParametersForFields = false,
+}) async {
   performance ??= OperationPerformanceImpl('<root>');
-  Set<ClassMemberElement> result = HashSet<ClassMemberElement>();
+  Set<ClassMemberElement> members = HashSet<ClassMemberElement>();
+  Set<ParameterElement> parameters = HashSet<ParameterElement>();
   // extension member
   var enclosingElement = member.enclosingElement;
   if (enclosingElement is ExtensionElement) {
-    result.add(member);
-    return Future.value(result);
+    members.add(member);
+    return (members, parameters);
   }
   // static elements
   if (member.isStatic || member is ConstructorElement) {
-    result.add(member);
-    return result;
+    members.add(member);
+    return (members, parameters);
   }
   // method, field, etc
   if (enclosingElement is InterfaceElement) {
@@ -133,13 +159,25 @@ Future<Set<ClassMemberElement>> getHierarchyMembers(
       var subClassMembers = getChildren(subClass, name);
       for (var member in subClassMembers) {
         if (member is ClassMemberElement) {
-          result.add(member);
+          members.add(member);
+        }
+      }
+
+      if (includeParametersForFields && member is FieldElement) {
+        for (var constructor in subClass.constructors) {
+          for (var parameter in constructor.parameters) {
+            if (parameter is FieldFormalParameterElement &&
+                parameter.field == member) {
+              parameters.add(parameter);
+            }
+          }
         }
       }
     }
+    return (members, parameters);
   }
 
-  return result;
+  return (members, parameters);
 }
 
 /// If the [element] is a named parameter in a [MethodElement], return all

pkg/analysis_server/test/lsp/references_test.dart

@@ -96,15 +96,19 @@ void f() {
   }
 
   Future<void> test_field_decalaration_initializingFormal() async {
-    // References on the field should find both the initializing formal and the
-    // reference to the getter.
+    // References on the field should find the initializing formal, the
+    // reference to the getter and the constructor argument.
     var content = '''
 class AAA {
   final String? aa^a;
   const AAA({this./*[0*/aaa/*0]*/});
 }
 
-final a = AAA(aaa: '')./*[1*/aaa/*1]*/;
+class BBB extends AAA {
+  BBB({super./*[1*/aaa/*1]*/});
+}
+
+final a = AAA(/*[2*/aaa/*2]*/: '')./*[3*/aaa/*3]*/;
 ''';
 
     await _checkRanges(content);
@@ -209,15 +213,83 @@ imp^ort 'dart:async' as async;
     await _checkRanges(content);
   }
 
-  Future<void> test_initializingFormals() async {
-    // References on "this.aaa" should only find the matching named argument.
+  Future<void> test_initializingFormal_argument_withDeclaration() async {
+    // Find references on an initializing formal argument should include
+    // all references to the field too.
+    var content = '''
+class AAA {
+  String? /*[0*/aaa/*0]*/;
+  AAA({this./*[1*/aaa/*1]*/});
+}
+
+void f() {
+  final a = AAA(/*[2*/a^aa/*2]*/: '');
+  var x = a./*[3*/aaa/*3]*/;
+  a./*[4*/aaa/*4]*/ = '';
+}
+''';
+
+    await _checkRanges(content, includeDeclarations: true);
+  }
+
+  Future<void> test_initializingFormal_argument_withoutDeclaration() async {
+    // Find references on an initializing formal argument should include
+    // all references to the field too. The field is not included
+    // because we didn't request the declaration.
     var content = '''
 class AAA {
-  final String? aaa;
-  const AAA({this.aa^a});
+  String? aaa;
+  AAA({this./*[0*/aaa/*0]*/});
+}
+
+void f() {
+  final a = AAA(/*[1*/a^aa/*1]*/: '');
+  var x = a./*[2*/aaa/*2]*/;
+  a./*[3*/aaa/*3]*/ = '';
 }
+''';
+
+    await _checkRanges(content);
+  }
 
-final a = AAA([!aaa!]: '').aaa;
+  Future<void> test_initializingFormal_parameter_withDeclaration() async {
+    // Find references on an initializing formal parameter should include
+    // all references to the field too.
+    var content = '''
+class AAA {
+  String? /*[0*/aaa/*0]*/;
+  AAA({this./*[1*/aa^a/*1]*/});
+}
+
+void f() {
+  final a = AAA(/*[2*/aaa/*2]*/: '');
+  var x = a./*[3*/aaa/*3]*/;
+  a./*[4*/aaa/*4]*/ = '';
+}
+''';
+
+    await _checkRanges(content, includeDeclarations: true);
+  }
+
+  Future<void> test_initializingFormal_parameter_withoutDeclaration() async {
+    // Find references on an initializing formal parameter should include
+    // all references to the field too. The field is not included
+    // because we didn't request the declaration.
+    var content = '''
+class AAA {
+  String? aaa;
+  AAA({this./*[0*/aa^a/*0]*/});
+}
+
+class BBB extends AAA {
+  BBB({super./*[1*/aaa/*1]*/});
+}
+
+void f() {
+  final a = AAA(/*[2*/aaa/*2]*/: '');
+  var x = a./*[3*/aaa/*3]*/;
+  a./*[4*/aaa/*4]*/ = '';
+}
 ''';
 
     await _checkRanges(content);
@@ -410,6 +482,11 @@ class Aaa^<T> {}
           Location(uri: otherFileUri, range: range.range),
     ];
 
+    // Checking sets produces a better failure message than lists
+    // (it'll show which item is missing instead of just saying
+    // the lengths are different), so check that first.
+    expect(res.toSet(), expected.toSet());
+    // But also check the list in case there were unexpected duplicates.
     expect(res, unorderedEquals(expected));
   }
 }