[analyzer] Improve locations for diagnostic context messages when no names

Previously -1 was used as the offset for unnamed elements when building diagnostics, but this is never a valid location (and for VS Code, results in the diagnostic not appearing at all, even if the invalid location is only in a context message).

With this change we use the offset of the fragment instead.

Fixes #62955.

Change-Id: I6aec9b7a9a6d49b151b1cf3eaea4b684486b6d8d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/490121
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

Author: DanTup

pkg/analysis_server/test/lsp/diagnostic_test.dart

@@ -210,6 +210,37 @@ void f() {
     expect(diagnostic.relatedInformation, hasLength(1));
   }
 
+  Future<void> test_contextMessage_unnamedElement_issue62955() async {
+    var code = TestCode.parseNormalized('''
+void main() {
+  [].named;
+}
+
+/*[0*//*0]*/extension<T> on List<T> {
+  Iterable<T> get named => throw 0;
+}
+
+/*[1*//*1]*/extension<T> on List<T> {
+  T? named(String? name) => throw 0;
+}
+''');
+    newFile(mainFilePath, code.code);
+
+    var diagnosticsUpdate = waitForDiagnostics(mainFileUri);
+    await initialize();
+    var diagnostics = await diagnosticsUpdate;
+    var diagnostic = diagnostics!.singleWhere(
+      (diagnostic) => diagnostic.code == 'ambiguous_extension_member_access',
+    );
+
+    expect(diagnostic.relatedInformation, hasLength(2));
+    var [related1, related2] = diagnostic.relatedInformation!;
+    expect(related1.message, contains('<unnamed extension> is defined in'));
+    expect(related1.location.range, code.ranges[0].range);
+    expect(related2.message, contains('<unnamed extension> is defined in'));
+    expect(related2.location.range, code.ranges[1].range);
+  }
+
   Future<void> test_correction() async {
     newFile(mainFilePath, '''
 void f() {

pkg/analyzer/lib/src/dart/element/element.dart

@@ -1767,6 +1767,7 @@ abstract class ElementImpl implements Element {
       libraryFragment: _firstFragment.libraryFragment,
       name: _firstFragment.name,
       nameOffset: _firstFragment.nameOffset,
+      firstTokenOffset: _firstFragment.firstTokenOffset,
     );
   }
 
@@ -3131,11 +3132,13 @@ class FirstFragmentLocation {
   final LibraryFragmentImpl? libraryFragment;
   final String? name;
   final int? nameOffset;
+  final int? firstTokenOffset;
 
   FirstFragmentLocation({
     required this.libraryFragment,
     required this.name,
     required this.nameOffset,
+    required this.firstTokenOffset,
   });
 }
 

pkg/analyzer/lib/src/error/listener.dart

@@ -14,6 +14,7 @@ import 'package:analyzer/source/source.dart';
 import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer/src/dart/ast/ast.dart';
 import 'package:analyzer/src/dart/ast/extensions.dart';
+import 'package:analyzer/src/dart/element/element.dart';
 import 'package:analyzer/src/dart/element/extensions.dart';
 import 'package:analyzer/src/dart/element/type.dart';
 import 'package:analyzer/src/diagnostic/diagnostic_message.dart';
@@ -80,9 +81,12 @@ List<DiagnosticMessage> convertTypeNames(
       // context messages, remove the extra text added to the buffer.
       StringBuffer? buffer;
       for (var element in typeToConvert.allElements) {
-        var name = element.name;
-        name ??= element is ExtensionElement ? unnamedExtension : unnamed;
-        var sourcePath = element.firstFragment.libraryFragment!.source.fullName;
+        var nonSynthetic = element.nonSynthetic;
+        var name = nonSynthetic.name;
+        name ??= nonSynthetic is ExtensionElement ? unnamedExtension : unnamed;
+
+        var location = _DiagnosticLocation.forElement(nonSynthetic);
+        var sourcePath = location.sourcePath!;
         if (nameToElementMap[name]!.length > 1) {
           if (buffer == null) {
             buffer = StringBuffer();
@@ -95,9 +99,9 @@ List<DiagnosticMessage> convertTypeNames(
         messages.add(
           DiagnosticMessageImpl(
             filePath: sourcePath,
-            length: element.name?.length ?? 0,
             message: '$name is defined in $sourcePath',
-            offset: element.firstFragment.nameOffset ?? -1,
+            offset: location.offset,
+            length: location.length,
             url: null,
           ),
         );
@@ -203,10 +207,11 @@ class DiagnosticReporter {
     List<DiagnosticMessage>? contextMessages,
   }) {
     var nonSynthetic = element.nonSynthetic;
+    var location = _DiagnosticLocation.forElement(nonSynthetic);
     return atOffset(
       diagnosticCode: diagnosticCode,
-      offset: nonSynthetic.firstFragment.nameOffset ?? -1,
-      length: nonSynthetic.name?.length ?? 0,
+      offset: location.offset,
+      length: location.length,
       arguments: arguments,
       contextMessages: contextMessages,
     );
@@ -391,6 +396,49 @@ class DiagnosticReporter {
   }
 }
 
+/// The location to report a diagnostic at.
+class _DiagnosticLocation {
+  final int offset;
+  final int length;
+  final String? sourcePath;
+
+  _DiagnosticLocation(
+    this.sourcePath, {
+    required this.offset,
+    required this.length,
+  });
+
+  /// Computes the diagnostic location for a given element.
+  factory _DiagnosticLocation.forElement(Element element) {
+    var location = (element.nonSynthetic as ElementImpl).firstFragmentLocation;
+
+    var sourcePath = location.libraryFragment?.source.fullName;
+    var nameOffset = location.nameOffset;
+    var firstTokenOffset = location.firstTokenOffset;
+
+    int offset, length;
+    if (nameOffset != null) {
+      // Has name, use offset and length.
+      offset = nameOffset;
+      length = location.name?.length ?? 0;
+    } else if (firstTokenOffset != null) {
+      // No name, use zero-length offset at the first token,
+      offset = firstTokenOffset;
+      length = 0;
+    } else {
+      // We don't have any valid location to use. We currently use -1 as an
+      // indicator of this. We could consider using 0 (so the location is at
+      // least valid, even if not specific), but it might make it more difficult
+      // to track down bugs (for example the LSP mapping code has an assert that
+      // would flag this if it happened during a test/assert-enabled run).
+      offset = -1;
+      length = 0;
+    }
+
+    return _DiagnosticLocation(sourcePath, offset: offset, length: length);
+  }
+}
+
 /// Used by [convertTypeNames] to keep track of an error argument that is an
 /// [Element], that is being converted to a display string.
 class _ElementToConvert implements _ToConvert {

pkg/analyzer/test/error/error_reporter_test.dart

@@ -38,6 +38,7 @@ class ErrorReporterTest extends PubPackageResolutionTest {
 
   test_atElement_unnamed() async {
     await resolveTestCode(r'''
+// comment to prevent expected offset being 0
 extension on int {}
 ''');
     var element = findElement2.unnamedExtension();
@@ -50,7 +51,8 @@ extension on int {}
     reporter.atElement2(element, diag.castToNonType, arguments: ['A']);
 
     var diagnostic = listener.diagnostics[0];
-    expect(diagnostic.offset, -1);
+    // No name, so expect offset of declaration.
+    expect(diagnostic.offset, firstFragment.offset);
   }
 
   test_atNode_types_differentNames() async {

pkg/analyzer/test/src/diagnostics/ambiguous_extension_member_access_test.dart

@@ -238,7 +238,10 @@ extension on Iterable<Undef2> { void foo() {} }
                 "'extension on Iterable<InvalidType> "
                 "(where <unnamed extension> is defined in ${testFile.path})',",
           ],
-          contextMessages: [message(testFile, -1, 0), message(testFile, -1, 0)],
+          contextMessages: [
+            message(testFile, 75, 0),
+            message(testFile, 123, 0),
+          ],
         ),
         error(diag.nonTypeAsTypeArgument, 97, 6),
         error(diag.nonTypeAsTypeArgument, 145, 6),