[analysis_server] Support occurrences (Document Highlights) with different lengths
+ split unnamed constructors from classes
Fixes https://github.com/Dart-Code/Dart-Code/issues/4454
Change-Id: I42e7346b7c22348cf6424d15c5c35a24e31c85b3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406822
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analysis_server/lib/src/domains/analysis/occurrences_dart.dart b/pkg/analysis_server/lib/src/domains/analysis/occurrences_dart.dart
index f9eb1a3..1c269e0 100644
--- a/pkg/analysis_server/lib/src/domains/analysis/occurrences_dart.dart
+++ b/pkg/analysis_server/lib/src/domains/analysis/occurrences_dart.dart
@@ -5,30 +5,44 @@
import 'package:analysis_server/plugin/analysis/occurrences/occurrences_core.dart';
import 'package:analysis_server/src/protocol_server.dart' as protocol;
import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element2.dart';
import 'package:analyzer/src/dart/ast/extensions.dart';
import 'package:analyzer/src/dart/element/element.dart';
void addDartOccurrences(OccurrencesCollector collector, CompilationUnit unit) {
- var visitor = _DartUnitOccurrencesComputerVisitor();
+ var visitor = DartUnitOccurrencesComputerVisitor();
unit.accept(visitor);
- visitor.elementsOffsets.forEach((engineElement, offsets) {
- var length = engineElement.name3?.length ?? 0;
+ visitor.elementsOffsetLengths.forEach((engineElement, offsetLengths) {
+ // For legacy protocol, we only support occurrences with the same
+ // length, so we must filter the offset to only those that match the length
+ // from the element.
var serverElement = protocol.convertElement2(engineElement);
+ // Prefer the length from the mapped element over the element directly,
+ // because 'name3' may contain 'new' for constructors which doesn't match
+ // what is in the source.
+ var length =
+ serverElement.location?.length ?? engineElement.name3?.length ?? 0;
+ var offsets =
+ offsetLengths
+ .where((offsetLength) => offsetLength.$2 == length)
+ .map((offsetLength) => offsetLength.$1)
+ .toList();
+
var occurrences = protocol.Occurrences(serverElement, offsets, length);
collector.addOccurrences(occurrences);
});
}
-class _DartUnitOccurrencesComputerVisitor extends RecursiveAstVisitor<void> {
- final Map<Element2, List<int>> elementsOffsets = {};
+class DartUnitOccurrencesComputerVisitor extends RecursiveAstVisitor<void> {
+ final Map<Element2, List<(int, int)>> elementsOffsetLengths = {};
@override
void visitAssignedVariablePattern(AssignedVariablePattern node) {
var element = node.element2;
if (element != null) {
- _addOccurrence(element, node.name.offset);
+ _addOccurrence(element, node.name);
}
super.visitAssignedVariablePattern(node);
@@ -36,14 +50,14 @@
@override
void visitClassDeclaration(ClassDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitClassDeclaration(node);
}
@override
void visitClassTypeAlias(ClassTypeAlias node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitClassTypeAlias(node);
}
@@ -51,43 +65,64 @@
@override
void visitConstructorDeclaration(ConstructorDeclaration node) {
if (node.name case var name?) {
- _addOccurrence(node.declaredFragment!.element, name.offset);
+ _addOccurrence(node.declaredFragment!.element, name);
+ } else {
+ _addOccurrenceAt(
+ node.declaredFragment!.element,
+ node.returnType.offset,
+ node.returnType.length,
+ );
}
super.visitConstructorDeclaration(node);
}
@override
+ void visitConstructorName(ConstructorName node) {
+ // For unnamed constructors, we add an occurence for the constructor at
+ // the location of the returnType.
+ if (node.name == null) {
+ var element = node.element;
+ if (element != null) {
+ _addOccurrence(element, node.type.name2);
+ }
+ return; // skip visitNamedType.
+ }
+
+ super.visitConstructorName(node);
+ }
+
+ @override
void visitDeclaredIdentifier(DeclaredIdentifier node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitDeclaredIdentifier(node);
}
@override
void visitDeclaredVariablePattern(DeclaredVariablePattern node) {
- _addOccurrence(node.declaredElement2!, node.name.offset);
+ _addOccurrence(node.declaredElement2!, node.name);
super.visitDeclaredVariablePattern(node);
}
@override
void visitEnumConstantDeclaration(EnumConstantDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitEnumConstantDeclaration(node);
}
@override
void visitEnumDeclaration(EnumDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitEnumDeclaration(node);
}
@override
void visitExtensionTypeDeclaration(ExtensionTypeDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitExtensionTypeDeclaration(node);
}
@@ -98,7 +133,7 @@
if (declaredElement is FieldFormalParameterElement2) {
var field = declaredElement.field2;
if (field != null) {
- _addOccurrence(field, node.name.offset);
+ _addOccurrence(field, node.name);
}
}
@@ -107,40 +142,40 @@
@override
void visitFunctionDeclaration(FunctionDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitFunctionDeclaration(node);
}
@override
void visitFunctionTypeAlias(FunctionTypeAlias node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitFunctionTypeAlias(node);
}
@override
void visitGenericTypeAlias(GenericTypeAlias node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitGenericTypeAlias(node);
}
@override
void visitImportPrefixReference(ImportPrefixReference node) {
- _addOccurrence(node.element2!, node.offset);
+ _addOccurrence(node.element2!, node.name);
super.visitImportPrefixReference(node);
}
@override
void visitMethodDeclaration(MethodDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitMethodDeclaration(node);
}
@override
void visitMixinDeclaration(MixinDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitMixinDeclaration(node);
}
@@ -149,7 +184,7 @@
void visitNamedType(NamedType node) {
var element = node.element2;
if (element != null) {
- _addOccurrence(element, node.name2.offset);
+ _addOccurrence(element, node.name2);
}
super.visitNamedType(node);
@@ -165,7 +200,7 @@
? pattern.name
: node.name?.name;
if (element != null && name != null) {
- _addOccurrence(element, name.offset);
+ _addOccurrence(element, name);
}
super.visitPatternField(node);
}
@@ -173,10 +208,7 @@
@override
void visitRepresentationDeclaration(RepresentationDeclaration node) {
if (node.constructorName case var constructorName?) {
- _addOccurrence(
- node.constructorFragment!.element,
- constructorName.name.offset,
- );
+ _addOccurrence(node.constructorFragment!.element, constructorName.name);
}
super.visitRepresentationDeclaration(node);
@@ -186,7 +218,7 @@
void visitSimpleFormalParameter(SimpleFormalParameter node) {
var nameToken = node.name;
if (nameToken != null) {
- _addOccurrence(node.declaredFragment!.element, nameToken.offset);
+ _addOccurrence(node.declaredFragment!.element, nameToken);
}
super.visitSimpleFormalParameter(node);
@@ -194,36 +226,50 @@
@override
void visitSimpleIdentifier(SimpleIdentifier node) {
+ // For unnamed constructors, we don't want to add an occurrence for the
+ // class name here because visitConstructorDeclaration will have added one
+ // for the constructor (not the type).
+ if (node.parent case ConstructorDeclaration(
+ :var name,
+ :var returnType,
+ ) when name == null && node == returnType) {
+ return;
+ }
+
var element = node.writeOrReadElement2;
if (element != null) {
- _addOccurrence(element, node.offset);
+ _addOccurrence(element, node.token);
}
return super.visitSimpleIdentifier(node);
}
@override
void visitSuperFormalParameter(SuperFormalParameter node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitSuperFormalParameter(node);
}
@override
void visitVariableDeclaration(VariableDeclaration node) {
- _addOccurrence(node.declaredFragment!.element, node.name.offset);
+ _addOccurrence(node.declaredFragment!.element, node.name);
super.visitVariableDeclaration(node);
}
- void _addOccurrence(Element2 element, int offset) {
+ void _addOccurrence(Element2 element, Token token) {
+ _addOccurrenceAt(element, token.offset, token.length);
+ }
+
+ void _addOccurrenceAt(Element2 element, int offset, int length) {
var canonicalElement = _canonicalizeElement(element);
if (canonicalElement == null || element == DynamicElementImpl.instance) {
return;
}
- var offsets = elementsOffsets[canonicalElement];
- if (offsets == null) {
- offsets = <int>[];
- elementsOffsets[canonicalElement] = offsets;
+ var offsetLengths = elementsOffsetLengths[canonicalElement];
+ if (offsetLengths == null) {
+ offsetLengths = <(int, int)>[];
+ elementsOffsetLengths[canonicalElement] = offsetLengths;
}
- offsets.add(offset);
+ offsetLengths.add((offset, length));
}
Element2? _canonicalizeElement(Element2 element) {
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_document_highlights.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_document_highlights.dart
index 8026a69..9627da6 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_document_highlights.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_document_highlights.dart
@@ -3,12 +3,12 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:analysis_server/lsp_protocol/protocol.dart';
-import 'package:analysis_server/src/domains/analysis/occurrences.dart';
import 'package:analysis_server/src/domains/analysis/occurrences_dart.dart';
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:collection/collection.dart';
typedef StaticOptions = Either2<bool, DocumentHighlightOptions>;
@@ -45,8 +45,8 @@
var offset = unit.mapResultSync((unit) => toOffset(unit.lineInfo, pos));
return (unit, offset).mapResults((unit, requestedOffset) async {
- var collector = OccurrencesCollectorImpl();
- addDartOccurrences(collector, unit.unit);
+ var visitor = DartUnitOccurrencesComputerVisitor();
+ unit.unit.accept(visitor);
/// Checks whether an Occurrence offset/length spans the requested
/// offset.
@@ -54,23 +54,38 @@
/// It's possible multiple occurrences might match because some nodes
/// such as object destructuring might match multiple elements (for
/// example the object getter and a declared variable).
- bool spansRequestedPosition(int offset, int length) {
+ bool spansRequestedPosition((int, int) offsetLength) {
+ var (offset, length) = offsetLength;
return offset <= requestedOffset && offset + length >= requestedOffset;
}
- // Find an occurrence that has an instance that spans the position.
- var occurrences =
- collector.allOccurrences
+ // Find the groups (elements) of offset/lengths that contains an
+ // offset/length that spans the requested range. There may be multiple
+ // matches here if the source element is in multiple groups.
+ var matchingSet =
+ visitor.elementsOffsetLengths.values
.where(
- (occurrence) => occurrence.offsets.any(
- (offset) => spansRequestedPosition(offset, occurrence.length),
+ (offsetLengths) => offsetLengths.any(spansRequestedPosition),
+ )
+ .flattened
+ .toSet();
+
+ // No matches will return an empty list (not null) because that prevents
+ // the editor falling back to a text search.
+ var highlights =
+ matchingSet
+ .map(
+ (offsetLength) => DocumentHighlight(
+ range: toRange(
+ unit.lineInfo,
+ offsetLength.$1,
+ offsetLength.$2,
+ ),
),
)
.toList();
- // No matches will return an empty list (not null) because that prevents
- // the editor falling back to a text search.
- return success(toHighlights(unit.lineInfo, occurrences));
+ return success(highlights);
});
}
}
diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart
index 2790d9a..2ba55fa 100644
--- a/pkg/analysis_server/lib/src/lsp/mapping.dart
+++ b/pkg/analysis_server/lib/src/lsp/mapping.dart
@@ -1438,22 +1438,6 @@
}
}
-List<lsp.DocumentHighlight> toHighlights(
- server.LineInfo lineInfo,
- List<server.Occurrences> occurrences,
-) {
- return occurrences
- .map(
- (occurrence) => occurrence.offsets.map(
- (offset) => lsp.DocumentHighlight(
- range: toRange(lineInfo, offset, occurrence.length),
- ),
- ),
- )
- .flattenedToSet
- .toList();
-}
-
lsp.Location toLocation(
ClientUriConverter uriConverter,
server.Location location,
diff --git a/pkg/analysis_server/lib/src/protocol_server.dart b/pkg/analysis_server/lib/src/protocol_server.dart
index cd220a1..2cf4331 100644
--- a/pkg/analysis_server/lib/src/protocol_server.dart
+++ b/pkg/analysis_server/lib/src/protocol_server.dart
@@ -320,8 +320,15 @@
return null;
}
var fragment = element.firstFragment;
- var offset = fragment.nameOffset2 ?? 0;
- var length = fragment.name2?.length ?? 0;
+ var (offset, length) = switch (fragment) {
+ // For unnamed constructors, treat the type name as the element location
+ // instead of using 0,0.
+ engine.ConstructorFragment(:var typeNameOffset, :var typeName) =>
+ fragment.nameOffset2 != null
+ ? (fragment.nameOffset2 ?? 0, fragment.name2.length)
+ : (typeNameOffset ?? 0, typeName?.length ?? 0),
+ _ => (fragment.nameOffset2 ?? 0, fragment.name2?.length ?? 0),
+ };
var range = engine.SourceRange(offset, length);
return _locationForArgs2(fragment, range);
}
diff --git a/pkg/analysis_server/test/analysis/notification_occurrences_test.dart b/pkg/analysis_server/test/analysis/notification_occurrences_test.dart
index 354cf53..0fa59d6 100644
--- a/pkg/analysis_server/test/analysis/notification_occurrences_test.dart
+++ b/pkg/analysis_server/test/analysis/notification_occurrences_test.dart
@@ -50,7 +50,7 @@
expect(testOccurrences.element.name, elementName ?? range.text);
expect(
testOccurrences.offsets,
- containsAll(code.ranges.map((r) => r.sourceRange.offset)),
+ unorderedEquals(code.ranges.map((r) => r.sourceRange.offset)),
);
}
@@ -421,8 +421,8 @@
Future<void> test_pattern_cast_typeName() async {
await assertOccurrences(kind: ElementKind.CLASS, '''
-String f((num, /*[0*/String/*0]*/) record) {
- var (i as int, s as /*[1*/String/*1]*/) = record;
+/*[0*/String/*0]*/ f((num, /*[1*/String/*1]*/) record) {
+ var (i as int, s as /*[2*/String/*2]*/) = record;
}
);
''');
@@ -664,6 +664,38 @@
''');
}
+ Future<void> test_type_class_constructors() async {
+ await assertOccurrences(kind: ElementKind.CLASS, '''
+class /*[0*/A/*0]*/ {
+ A(); // Unnamed constructor is own entity
+ /*[1*/A/*1]*/.named();
+}
+
+/*[2*/A/*2]*/ a = A(); // Unnamed constructor is own entity
+var b = /*[3*/A/*3]*/.new();
+var c = /*[4*/A/*4]*/.new;
+ ''');
+ }
+
+ /// The type name in unnamed constructors are their own entity and not
+ /// part of the type name.
+ ///
+ /// For the legacy protocol, "new" is not treated as a reference to the
+ /// constructor because the protocol currently only supports same-length
+ /// occurrences.
+ Future<void> test_type_class_constructors_unnamed() async {
+ await assertOccurrences(kind: ElementKind.CONSTRUCTOR, '''
+class A {
+ /*[0*/A/*0]*/();
+ A.named();
+}
+
+A a = /*[1*/A/*1]*/();
+var b = A.new();
+var c = A.new;
+ ''');
+ }
+
Future<void> test_type_class_definition() async {
await assertOccurrences(kind: ElementKind.CLASS, '''
class /*[0*/A/*0]*/ {}
diff --git a/pkg/analysis_server/test/lsp/document_highlights_test.dart b/pkg/analysis_server/test/lsp/document_highlights_test.dart
index d875fb0..568a5f9 100644
--- a/pkg/analysis_server/test/lsp/document_highlights_test.dart
+++ b/pkg/analysis_server/test/lsp/document_highlights_test.dart
@@ -212,6 +212,34 @@
}
''');
+ Future<void> test_type_class_constructors() async {
+ await _testMarkedContent('''
+class /*[0*/A^/*0]*/ {
+ A(); // Unnamed constructor is own entity
+ /*[1*/A/*1]*/.named();
+}
+
+/*[2*/A/*2]*/ a = A(); // Unnamed constructor is own entity
+var b = /*[3*/A/*3]*/.new();
+var c = /*[4*/A/*4]*/.new;
+''');
+ }
+
+ /// The type name in unnamed constructors are their own entity and not
+ /// part of the type name.
+ Future<void> test_type_class_constructors_unnamed() async {
+ await _testMarkedContent('''
+class A {
+ /*[0*/A^/*0]*/();
+ A.named();
+}
+
+A a = /*[1*/A/*1]*/();
+var b = A./*[2*/new/*2]*/();
+var c = A./*[3*/new/*3]*/;
+''');
+ }
+
Future<void> test_typeAlias_class_declaration() => _testMarkedContent('''
class MyClass {}
mixin MyMixin {}
