Skip to content

Commit

Permalink
[analysis_server] Treat field formal parameters and fields equally wh…
Browse files Browse the repository at this point in the history 
…en 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]>
  • Loading branch information
@DanTup
DanTup authored and Commit Queue committed Jun 13, 2024
1 parent 95f90f5 commit bf7f517
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 45 deletions.
35 changes: 17 additions & 18 deletions pkg/analysis_server/lib/src/lsp/handlers/handler_references.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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>;

Expand Down Expand Up @@ -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(
Expand All @@ -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,
};
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 14 additions & 7 deletions pkg/analysis_server/lib/src/search/element_references.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/analysis_server/lib/src/services/refactoring/legacy/convert_getter_to_method.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
60 changes: 49 additions & 11 deletions pkg/analysis_server/lib/src/services/search/hierarchy.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
93 changes: 85 additions & 8 deletions pkg/analysis_server/test/lsp/references_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
}
}

0 comments on commit bf7f517

Please sign in to comment.