[analysis_server] Provide better values for active parameters in signature help

We were previously using a -1 because we don't support this, but it turns out the LSP spec changed from "number" to "uinteger" in v3.16, meaning -1 would violate some assumptions clients made.

For the case where we don't have a value, we now just use the `length` of the `parameters` list (eg. the first invalid value, which is valid according to the spec and can indicate to the client that there is no active parameter*), however I thought we could also do better by providing values for some cases (named params that we can match up, and positional params by counting only the positional params+args and matching them up).

\* The spec says clients can treat out-of-range values as 0, however VS Code doesn't highlight any parameters in that case, which is better IMO.

Fixes #58577

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

Author: DanTup

pkg/analysis_server/lib/src/cider/signature_help.dart

@@ -53,7 +53,7 @@ class CiderSignatureHelpComputer {
       if (signature != null) {
         return SignatureHelpResponse(
           toSignatureHelp(formats, signature),
-          lineInfo.getLocation(computer.argumentList.offset + 1),
+          lineInfo.getLocation(signature.argumentList.offset + 1),
         );
       }
     }

pkg/analysis_server/lib/src/computer/computer_signature.dart

@@ -9,36 +9,34 @@ import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element2.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/ast/element_locator.dart';
-import 'package:analyzer/src/dart/ast/utilities.dart';
 import 'package:analyzer/src/dartdoc/dartdoc_directive_info.dart';
+import 'package:analyzer/src/utilities/extensions/ast.dart';
 
 /// A computer for the signature at the specified offset of a Dart
 /// [CompilationUnit].
 class DartUnitSignatureComputer {
   final AstNode? _node;
-  late ArgumentList _argumentList;
+  final int _offset;
   final DocumentationPreference documentationPreference;
   final DartDocumentationComputer _documentationComputer;
 
   DartUnitSignatureComputer(
     DartdocDirectiveInfo dartdocInfo,
     CompilationUnit unit,
-    int offset, {
+    this._offset, {
     this.documentationPreference = DocumentationPreference.full,
   }) : _documentationComputer = DartDocumentationComputer(dartdocInfo),
-       _node = NodeLocator(offset).searchWithin(unit);
-
-  /// The [ArgumentList] node located by [compute].
-  ArgumentList get argumentList => _argumentList;
+       _node = unit.nodeCovering(offset: _offset);
 
   bool get offsetIsValid => _node != null;
 
   /// Returns the computed signature information, maybe `null`.
-  AnalysisGetSignatureResult? compute() {
-    var argumentList = _findArgumentList();
-    if (argumentList == null) {
+  SignatureInformation? compute() {
+    var argumentAndList = _findArgumentAndList();
+    if (argumentAndList == null) {
       return null;
     }
+    var (argumentList, argument) = argumentAndList;
     String? name;
     Element2? element;
     List<FormalParameterElement>? parameters;
@@ -77,16 +75,105 @@ class DartUnitSignatureComputer {
       return null;
     }
 
-    _argumentList = argumentList;
-    var convertedParameters = parameters.map((p) => _convertParam(p)).toList();
+    // Try to compute the active parameter so the IDE can highlight it.
+    int? activeParameterIndex;
+    if (argument case Expression(:var correspondingParameter?)) {
+      // If we know the active parameter, use its index.
+      activeParameterIndex = parameters.indexOf(correspondingParameter);
+    } else if (argument is! NamedExpression) {
+      // If we're not a named expression, then we can count how many positional
+      // parameters there are before us, and then find the index of the same
+      // index positional parameter.
+      var positionalArgsToSkip =
+          argumentList.arguments
+              .where((argument) => argument is! NamedExpression)
+              .takeWhile((argument) => argument.end < _offset)
+              .length;
+      for (var i = 0; i < parameters.length; i++) {
+        if (parameters[i].isPositional) {
+          // This is the first positional parameter after our skips, so this is
+          // the active parameter.
+          if (positionalArgsToSkip == 0) {
+            activeParameterIndex = i;
+            break;
+          }
+          positionalArgsToSkip--;
+        }
+      }
+    }
+
     var dartdoc = _documentationComputer.computePreferred2(
       element,
       documentationPreference,
     );
 
+    return SignatureInformation(
+      name: name,
+      parameters: parameters,
+      argumentList: argumentList,
+      activeParameterIndex: activeParameterIndex,
+      dartdoc: dartdoc,
+    );
+  }
+
+  /// Return the closest argument list surrounding the [_node] and the node for
+  /// the active argument (if there is one).
+  (ArgumentList, AstNode?)? _findArgumentAndList() {
+    var node = _node;
+    while (node != null) {
+      // Certain nodes don't make sense to search above for an argument list
+      // (for example when inside a function expression).
+      if (node is FunctionExpression) {
+        return null;
+      }
+
+      if (node is ArgumentList) {
+        return (node, null);
+      }
+      if (node.parent case ArgumentList list) {
+        return (list, node);
+      }
+
+      node = node.parent;
+    }
+    return null;
+  }
+}
+
+/// Information about a function signature.
+class SignatureInformation {
+  /// The name of the function/method.
+  final String name;
+
+  /// The parameters for the function/method.
+  final List<FormalParameterElement> parameters;
+
+  /// The current argument list at the invocation site.
+  final ArgumentList argumentList;
+
+  /// Documentation for the function/method.
+  final String? dartdoc;
+
+  /// The index in [parameters] for the parameter that matches where the offset
+  /// was in the invocation list.
+  ///
+  /// This is only supplied when it can be computed. Positional arguments past
+  /// the number of positional parameters or named arguments with no matching
+  /// name will not be returned.
+  final int? activeParameterIndex;
+
+  SignatureInformation({
+    required this.name,
+    required this.parameters,
+    required this.argumentList,
+    required this.activeParameterIndex,
+    required this.dartdoc,
+  });
+
+  AnalysisGetSignatureResult toLegacyProtocol() {
     return AnalysisGetSignatureResult(
       name,
-      convertedParameters,
+      parameters.map(_convertParam).toList(),
       dartdoc: dartdoc,
     );
   }
@@ -105,18 +192,4 @@ class DartUnitSignatureComputer {
       defaultValue: param.defaultValueCode,
     );
   }
-
-  /// Return the closest argument list surrounding the [_node].
-  ArgumentList? _findArgumentList() {
-    var node = _node;
-    while (node != null && node is! ArgumentList) {
-      // Certain nodes don't make sense to search above for an argument list
-      // (for example when inside a function expression).
-      if (node is FunctionExpression) {
-        return null;
-      }
-      node = node.parent;
-    }
-    return node as ArgumentList?;
-  }
 }

pkg/analysis_server/lib/src/computer/computer_type_arguments_signature.dart

@@ -97,24 +97,23 @@ class DartTypeArgumentsSignatureComputer {
             )
             .toList();
 
-    var signatures = [
-      lsp.SignatureInformation(
-        label: label,
-        documentation:
-            documentation != null
-                ? asMarkupContentOrString(preferredFormats, documentation)
-                : null,
-        parameters: parameters,
-      ),
-    ];
+    var signature = lsp.SignatureInformation(
+      label: label,
+      documentation:
+          documentation != null
+              ? asMarkupContentOrString(preferredFormats, documentation)
+              : null,
+      parameters: parameters,
+    );
 
     return lsp.SignatureHelp(
-      signatures: signatures,
+      signatures: [signature],
       activeSignature: 0,
-      // As with [toSignatureHelp], we don't currently use this, but need to set
-      // it to something that doesn't match a parameter to avoid one being
-      // selected.
-      activeParameter: -1,
+      // This must be a unsigned integer but can be out of range. Since we don't
+      // currently support this, just provide the first out-of-bounds value and
+      // allow the client to decide what to do (the LSP spec says it can be
+      // treated as 0, but VS Code will not highlight any, which is preferred).
+      activeParameter: signature.parameters?.length ?? 0,
     );
   }
 }

pkg/analysis_server/lib/src/handler/legacy/analysis_get_signature.dart

@@ -59,6 +59,6 @@ class AnalysisGetSignatureHandler extends LegacyHandler {
       return;
     }
 
-    sendResult(signature);
+    sendResult(signature.toLegacyProtocol());
   }
 }

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

@@ -97,7 +97,7 @@ class SignatureHelpHandler
       // argument list.
       // The ArgumentList's offset is before the paren, but the request offset
       // will be after.
-      if (autoTriggered && offset != computer.argumentList.offset + 1) {
+      if (autoTriggered && offset != signature.argumentList.offset + 1) {
         return success(null);
       }
 

pkg/analysis_server/lib/src/lsp/mapping.dart

@@ -7,6 +7,7 @@ import 'package:analysis_server/lsp_protocol/protocol.dart' hide Declaration;
 import 'package:analysis_server/src/analysis_server.dart';
 import 'package:analysis_server/src/collections.dart';
 import 'package:analysis_server/src/computer/computer_documentation.dart';
+import 'package:analysis_server/src/computer/computer_signature.dart' as server;
 import 'package:analysis_server/src/lsp/client_capabilities.dart';
 import 'package:analysis_server/src/lsp/constants.dart' as lsp;
 import 'package:analysis_server/src/lsp/constants.dart';
@@ -525,11 +526,7 @@ lsp.Location? fragmentToLocation(
 
   return lsp.Location(
     uri: uriConverter.toClientUri(sourcePath),
-    range: toRange(
-      libraryFragment.lineInfo,
-      nameOffset,
-      nameLength,
-    ),
+    range: toRange(libraryFragment.lineInfo, nameOffset, nameLength),
   );
 }
 
@@ -1519,40 +1516,29 @@ lsp.Range toRange(server.LineInfo lineInfo, int offset, int length) {
 
 lsp.SignatureHelp toSignatureHelp(
   Set<lsp.MarkupKind>? preferredFormats,
-  server.AnalysisGetSignatureResult signature,
+  server.SignatureInformation signature,
 ) {
   // For now, we only support returning one (though we may wish to use named
   // args. etc. to provide one for each possible "next" option when the cursor
   // is at the end ready to provide another argument).
 
   /// Gets the label for an individual parameter in the form
   ///     String s = 'foo'
-  String getParamLabel(server.ParameterInfo p) {
-    var def = p.defaultValue != null ? ' = ${p.defaultValue}' : '';
-    var prefix =
-        p.kind == server.ParameterKind.REQUIRED_NAMED ? 'required ' : '';
-    return '$prefix${p.type} ${p.name}$def';
+  String getParamLabel(FormalParameterElement p) {
+    var defaultCodeSuffix =
+        p.defaultValueCode != null ? ' = ${p.defaultValueCode}' : '';
+    var requiredPrefix = p.isRequiredNamed ? 'required ' : '';
+    return '$requiredPrefix${p.type} ${p.displayName}$defaultCodeSuffix';
   }
 
   /// Gets the full signature label in the form
   ///     foo(String s, int i, bool a = true)
-  String getSignatureLabel(server.AnalysisGetSignatureResult resp) {
+  String getSignatureLabel(server.SignatureInformation resp) {
     var positionalRequired =
-        signature.parameters
-            .where((p) => p.kind == server.ParameterKind.REQUIRED_POSITIONAL)
-            .toList();
+        signature.parameters.where((p) => p.isRequiredPositional).toList();
     var positionalOptional =
-        signature.parameters
-            .where((p) => p.kind == server.ParameterKind.OPTIONAL_POSITIONAL)
-            .toList();
-    var named =
-        signature.parameters
-            .where(
-              (p) =>
-                  p.kind == server.ParameterKind.OPTIONAL_NAMED ||
-                  p.kind == server.ParameterKind.REQUIRED_NAMED,
-            )
-            .toList();
+        signature.parameters.where((p) => p.isOptionalPositional).toList();
+    var named = signature.parameters.where((p) => p.isNamed).toList();
     var params = [];
     if (positionalRequired.isNotEmpty) {
       params.add(positionalRequired.map(getParamLabel).join(', '));
@@ -1566,7 +1552,7 @@ lsp.SignatureHelp toSignatureHelp(
     return '${resp.name}(${params.join(", ")})';
   }
 
-  lsp.ParameterInformation toParameterInfo(server.ParameterInfo param) {
+  lsp.ParameterInformation toParameterInfo(FormalParameterElement param) {
     // LSP 3.14.0 supports providing label offsets (to avoid clients having
     // to guess based on substrings). We should check the
     // signatureHelp.signatureInformation.parameterInformation.labelOffsetSupport
@@ -1588,15 +1574,15 @@ lsp.SignatureHelp toSignatureHelp(
       ),
     ],
     activeSignature: 0, // activeSignature
-    // TODO(dantup): The LSP spec says this value will default to 0 if it's
-    // not supplied or outside of the value range. However, setting -1 results
-    // in no parameters being selected in VS Code, whereas null/0 will select the first.
-    // We'd like for none to be selected (since we don't support this yet) so
-    // we send -1. I've made a request for LSP to support not selecting a parameter
-    // (because you could also be on param 5 of an invalid call to a function
-    // taking only 3 arguments) here:
-    // https://github.com/Microsoft/language-server-protocol/issues/456#issuecomment-452318297
-    activeParameter: -1, // activeParameter
+    // We must provide a unsigned integer here but it's possible there isn't
+    // a valid value (because the user might be in the 10th argument of an
+    // invocation that only takes 1). The LSP spec allows us to send an
+    // out-of-bounds value so send the first out-of-bound value (`.length`). The
+    // spec says this may be treated as 0, however VS Code will not highlight
+    // any parameter in this case (which is preferred and hopefully other
+    // clients may copy).
+    activeParameter:
+        signature.activeParameterIndex ?? signature.parameters.length,
   );
 }