From 453647022f640126025e5cefc7dfb17a2ee252db Mon Sep 17 00:00:00 2001
From: Danny Tuppeny <danny@tuppeny.com>
Date: Mon, 17 Apr 2023 17:11:03 +0000
Subject: [PATCH] [analysis_server] Don't fail on invalid/complex setters in
 LSP code completion

Fixes https://github.com/dart-lang/sdk/issues/52050.

Change-Id: I6cf5420d5e017b501481cb436a7d1b17f46c0e44
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/295542
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
---
 pkg/analysis_server/lib/src/lsp/mapping.dart  | 24 +++++++-----
 .../test/lsp/completion_dart_test.dart        | 39 ++++++++++++++++++-
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart
index 20b55ad67d1..3184e2ad297 100644
--- a/pkg/analysis_server/lib/src/lsp/mapping.dart
+++ b/pkg/analysis_server/lib/src/lsp/mapping.dart
@@ -50,6 +50,10 @@ final diagnosticTagsForErrorCode = <String, List<lsp.DiagnosticTag>>{
   ],
 };
 
+/// A regex to extract the type name from the parameter string of a setter
+/// completion item.
+final _completionSetterTypePattern = RegExp(r'^\((\S+)\s+\S+\)$');
+
 /// Pattern for docComplete text on completion items that can be upgraded to
 /// the "detail" field so that it can be shown more prominently by clients.
 ///
@@ -396,15 +400,17 @@ String? getCompletionDetail(
     // appear in the completion list, so displaying them as setters is misleading.
     // To avoid this, always show only the return type, whether it's a getter
     // or a setter.
-    return prefix +
-        (element?.kind == server.ElementKind.GETTER
-            ? (returnType ?? '')
-            // Don't assume setters always have parameters
-            // See https://github.com/dart-lang/sdk/issues/27747
-            : parameters != null && parameters.isNotEmpty
-                // Extract the type part from '(MyType value)`
-                ? parameters.substring(1, parameters.lastIndexOf(' '))
-                : '');
+    if (element?.kind == server.ElementKind.GETTER) {
+      return prefix + (returnType ?? '');
+    } else if (parameters == null) {
+      return prefix;
+    } else {
+      // TODO(dantup): Consider having the type of a setter available on
+      //  CompletionSuggestion (or changing it to a protocol-agnostic class that
+      //  includes it).
+      return prefix +
+          (_completionSetterTypePattern.firstMatch(parameters)?.group(1) ?? '');
+    }
   } else if (hasParameters && hasReturnType) {
     return '$prefix$parameters → $returnType';
   } else if (hasReturnType) {
diff --git a/pkg/analysis_server/test/lsp/completion_dart_test.dart b/pkg/analysis_server/test/lsp/completion_dart_test.dart
index 4eabfb1283b..7d776dbea41 100644
--- a/pkg/analysis_server/test/lsp/completion_dart_test.dart
+++ b/pkg/analysis_server/test/lsp/completion_dart_test.dart
@@ -1345,9 +1345,9 @@ void f() {
     final content = '''
     class MyClass {
       String get justGetter => '';
-      String set justSetter(String value) {}
+      set justSetter(String value) {}
       String get getterAndSetter => '';
-      String set getterAndSetter(String value) {}
+      set getterAndSetter(String value) {}
     }
 
     void f() {
@@ -2210,6 +2210,41 @@ void f() { }
     expect(res.any((c) => c.label == 'UniqueNamedClassForLspThree'), isTrue);
   }
 
+  Future<void> test_setters() async {
+    final content = '''
+    class MyClass {
+      set stringSetter(String a) {}
+      set noArgSetter() {}
+      set multiArgSetter(a, b) {}
+      set functionSetter(String Function(int a, int b) foo) {}
+    }
+
+    void f() {
+      MyClass a;
+      a.^
+    }
+    ''';
+
+    await initialize();
+    await openFile(mainFileUri, withoutMarkers(content));
+    final res = await getCompletion(mainFileUri, positionFromMarker(content));
+    final setters = res
+        .where((c) => c.label.endsWith('Setter'))
+        .map((c) => '${c.label} (${c.detail})')
+        .toList();
+    expect(
+      setters,
+      [
+        'stringSetter (String)',
+        'noArgSetter ()',
+        'multiArgSetter ()',
+        // Because of how we extract the type name, we don't currently support
+        // this.
+        'functionSetter ()',
+      ],
+    );
+  }
+
   Future<void> test_unimportedSymbols() async {
     newFile(
       join(projectFolderPath, 'other_file.dart'),