[analyzer] Format signatures in LSP hovers over multiple lines

Fixes https://github.com/Dart-Code/Dart-Code/issues/3357.

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

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

@@ -20,8 +20,14 @@ class DartUnitHoverComputer {
   final DartdocDirectiveInfo _dartdocInfo;
   final CompilationUnit _unit;
   final int _offset;
+  final bool multilineElementDescriptions;
 
-  DartUnitHoverComputer(this._dartdocInfo, this._unit, this._offset);
+  DartUnitHoverComputer(
+    this._dartdocInfo,
+    this._unit,
+    this._offset, {
+    this.multilineElementDescriptions = false,
+  });
 
   /// Returns the computed hover, maybe `null`.
   HoverInformation? compute() {
@@ -133,6 +139,7 @@ class DartUnitHoverComputer {
   String? _elementDisplayString(Element? element) {
     return element?.getDisplayString(
       withNullability: _unit.isNonNullableByDefault,
+      multiline: multilineElementDescriptions,
     );
   }
 

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

@@ -87,9 +87,13 @@ class HoverHandler extends MessageHandler<TextDocumentPositionParams, Hover?> {
       return success(null);
     }
 
-    final hover = DartUnitHoverComputer(
-            server.getDartdocDirectiveInfoFor(unit), compilationUnit, offset)
-        .compute();
+    final computer = DartUnitHoverComputer(
+      server.getDartdocDirectiveInfoFor(unit),
+      compilationUnit,
+      offset,
+      multilineElementDescriptions: true,
+    );
+    final hover = computer.compute();
     return success(toHover(unit.lineInfo, hover));
   }
 }

pkg/analysis_server/test/lsp/hover_test.dart

@@ -174,6 +174,52 @@ print();
     expect(hover!.range, equals(rangeFromMarkers(content)));
   }
 
+  Future<void> test_signatureFormatting_multiLine() async {
+    final content = '''
+    class Foo {
+      Foo(String arg1, String arg2, [String arg3]);
+    }
+
+    main() {
+      var a = Fo^o();
+    }
+    ''';
+
+    await initialize();
+    await openFile(mainFileUri, withoutMarkers(content));
+    final hover = await getHover(mainFileUri, positionFromMarker(content));
+    final contents = _getStringContents(hover!);
+    expect(contents, startsWith('''
+```dart
+(new) Foo Foo(
+  String arg1,
+  String arg2, [
+  String arg3,
+])
+```'''));
+  }
+
+  Future<void> test_signatureFormatting_singleLine() async {
+    final content = '''
+    class Foo {
+      Foo(String a, String b);
+    }
+
+    main() {
+      var a = Fo^o();
+    }
+    ''';
+
+    await initialize();
+    await openFile(mainFileUri, withoutMarkers(content));
+    final hover = await getHover(mainFileUri, positionFromMarker(content));
+    final contents = _getStringContents(hover!);
+    expect(contents, startsWith('''
+```dart
+(new) Foo Foo(String a, String b)
+```'''));
+  }
+
   Future<void> test_string_noDocComment() async {
     final content = '''
     String [[a^bc]];

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

@@ -686,9 +686,16 @@ abstract class Element implements AnalysisTarget {
   /// If [withNullability] is `false`, nullability suffixes will not be
   /// included into the presentation.
   ///
+  /// If [multiline] is `true`, the string may be wrapped over multiple lines
+  /// with newlines to improve formatting. For example function signatures may
+  /// be formatted as if they had trailing commas.
+  ///
   /// Clients should not depend on the content of the returned value as it will
   /// be changed if doing so would improve the UX.
-  String getDisplayString({required bool withNullability});
+  String getDisplayString({
+    required bool withNullability,
+    bool multiline = false,
+  });
 
   /// Return a display name for the given element that includes the path to the
   /// compilation unit in which the type is defined. If [shortName] is `null`

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

@@ -16,10 +16,12 @@ class ElementDisplayStringBuilder {
 
   final bool skipAllDynamicArguments;
   final bool withNullability;
+  final bool multiline;
 
   ElementDisplayStringBuilder({
     required this.skipAllDynamicArguments,
     required this.withNullability,
+    this.multiline = false,
   });
 
   @override
@@ -61,7 +63,11 @@ class ElementDisplayStringBuilder {
       _write(element.displayName);
     }
 
-    _writeFormalParameters(element.parameters, forElement: true);
+    _writeFormalParameters(
+      element.parameters,
+      forElement: true,
+      allowMultiline: true,
+    );
   }
 
   void writeDynamicType() {
@@ -81,7 +87,11 @@ class ElementDisplayStringBuilder {
 
     if (element.kind != ElementKind.GETTER) {
       _writeTypeParameters(element.typeParameters);
-      _writeFormalParameters(element.parameters, forElement: true);
+      _writeFormalParameters(
+        element.parameters,
+        forElement: true,
+        allowMultiline: true,
+      );
     }
   }
 
@@ -222,15 +232,34 @@ class ElementDisplayStringBuilder {
   void _writeFormalParameters(
     List<ParameterElement> parameters, {
     required bool forElement,
+    bool allowMultiline = false,
   }) {
+    // Assume the display string looks better wrapped when there are at least
+    // three parameters. This avoids having to pre-compute the single-line
+    // version and know the length of the function name/return type.
+    var multiline = allowMultiline && this.multiline && parameters.length >= 3;
+
+    // The prefix for open groups is included in seperator for single-line but
+    // not for multline so must be added explicitly.
+    var openGroupPrefix = multiline ? ' ' : '';
+    var separator = multiline ? ',' : ', ';
+    var trailingComma = multiline ? ',\n' : '';
+    var parameterPrefix = multiline ? '\n  ' : '';
+
     _write('(');
 
-    var lastKind = _WriteFormalParameterKind.requiredPositional;
+    _WriteFormalParameterKind? lastKind;
     var lastClose = '';
 
     void openGroup(_WriteFormalParameterKind kind, String open, String close) {
       if (lastKind != kind) {
         _write(lastClose);
+        if (lastKind != null) {
+          // We only need to include the space before the open group if there
+          // was a previous parameter, otherwise it goes immediately after the
+          // open paren.
+          _write(openGroupPrefix);
+        }
         _write(open);
         lastKind = kind;
         lastClose = close;
@@ -239,7 +268,7 @@ class ElementDisplayStringBuilder {
 
     for (var i = 0; i < parameters.length; i++) {
       if (i != 0) {
-        _write(', ');
+        _write(separator);
       }
 
       var parameter = parameters[i];
@@ -250,9 +279,11 @@ class ElementDisplayStringBuilder {
       } else if (parameter.isNamed) {
         openGroup(_WriteFormalParameterKind.named, '{', '}');
       }
+      _write(parameterPrefix);
       _writeWithoutDelimiters(parameter, forElement: forElement);
     }
 
+    _write(trailingComma);
     _write(lastClose);
     _write(')');
   }

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

@@ -2390,10 +2390,14 @@ abstract class ElementImpl implements Element {
   }
 
   @override
-  String getDisplayString({required bool withNullability}) {
+  String getDisplayString({
+    required bool withNullability,
+    bool multiline = false,
+  }) {
     var builder = ElementDisplayStringBuilder(
       skipAllDynamicArguments: false,
       withNullability: withNullability,
+      multiline: multiline,
     );
     appendTo(builder);
     return builder.toString();
@@ -4405,7 +4409,10 @@ class MultiplyDefinedElementImpl implements MultiplyDefinedElement {
       visitor.visitMultiplyDefinedElement(this);
 
   @override
-  String getDisplayString({required bool withNullability}) {
+  String getDisplayString({
+    required bool withNullability,
+    bool multiline = false,
+  }) {
     var elementsStr = conflictingElements.map((e) {
       return e.getDisplayString(
         withNullability: withNullability,

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

@@ -576,10 +576,14 @@ abstract class Member implements Element {
   void appendTo(ElementDisplayStringBuilder builder);
 
   @override
-  String getDisplayString({required bool withNullability}) {
+  String getDisplayString({
+    required bool withNullability,
+    bool multiline = false,
+  }) {
     var builder = ElementDisplayStringBuilder(
       skipAllDynamicArguments: false,
       withNullability: withNullability,
+      multiline: multiline,
     );
     appendTo(builder);
     return builder.toString();

pkg/analyzer/test/generated/elements_types_mixin.dart

@@ -496,11 +496,13 @@ mixin ElementsTypesMixin {
     String? name,
     required DartType type,
     bool isCovariant = false,
+    String? defaultValueCode,
   }) {
     var parameter = ParameterElementImpl(name ?? '', 0);
     parameter.parameterKind = ParameterKind.POSITIONAL;
     parameter.type = type;
     parameter.isExplicitlyCovariant = isCovariant;
+    parameter.defaultValueCode = defaultValueCode;
     return parameter;
   }
 

pkg/analyzer/test/src/dart/element/display_string_test.dart

@@ -0,0 +1,126 @@
+// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:analyzer/dart/element/type_provider.dart';
+import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/dart/element/type_system.dart';
+import 'package:test/test.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import '../../../generated/elements_types_mixin.dart';
+import '../../../generated/test_analysis_context.dart';
+
+main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(ElementDisplayStringTest);
+  });
+}
+
+@reflectiveTest
+class ElementDisplayStringTest with ElementsTypesMixin {
+  late final TestAnalysisContext _analysisContext;
+
+  @override
+  late final LibraryElementImpl testLibrary;
+
+  @override
+  late final TypeProvider typeProvider;
+
+  late final TypeSystemImpl typeSystem;
+
+  void setUp() {
+    _analysisContext = TestAnalysisContext();
+    typeProvider = _analysisContext.typeProviderLegacy;
+    typeSystem = _analysisContext.typeSystemLegacy;
+
+    testLibrary = library_(
+      uriStr: 'package:test/test.dart',
+      analysisContext: _analysisContext,
+      analysisSession: _analysisContext.analysisSession,
+      typeSystem: typeSystem,
+    );
+  }
+
+  void test_longMethod() {
+    final methodA = method(
+      'longMethodName',
+      stringQuestion,
+      parameters: [
+        requiredParameter(name: 'aaa', type: stringQuestion),
+        positionalParameter(
+            name: 'bbb', type: stringQuestion, defaultValueCode: "'a'"),
+        positionalParameter(name: 'ccc', type: stringQuestion),
+      ],
+    );
+
+    final singleLine = methodA.getDisplayString(withNullability: true);
+    expect(singleLine, '''
+String? longMethodName(String? aaa, [String? bbb = 'a', String? ccc])''');
+
+    final multiLine =
+        methodA.getDisplayString(withNullability: true, multiline: true);
+    expect(multiLine, '''
+String? longMethodName(
+  String? aaa, [
+  String? bbb = 'a',
+  String? ccc,
+])''');
+  }
+
+  void test_longMethod_functionType() {
+    // Function types are always kept on one line, even nested within multiline
+    // signatures.
+    final methodA = method(
+      'longMethodName',
+      stringQuestion,
+      parameters: [
+        requiredParameter(name: 'aaa', type: stringQuestion),
+        positionalParameter(
+            name: 'bbb',
+            type: functionTypeNone(
+              parameters: [
+                requiredParameter(name: 'xxx', type: stringQuestion),
+                requiredParameter(name: 'yyy', type: stringQuestion),
+                requiredParameter(name: 'zzz', type: stringQuestion),
+              ],
+              returnType: stringQuestion,
+            )),
+        positionalParameter(name: 'ccc', type: stringQuestion),
+      ],
+    );
+
+    final singleLine = methodA.getDisplayString(withNullability: true);
+    expect(singleLine, '''
+String? longMethodName(String? aaa, [String? Function(String?, String?, String?) bbb, String? ccc])''');
+
+    final multiLine =
+        methodA.getDisplayString(withNullability: true, multiline: true);
+    expect(multiLine, '''
+String? longMethodName(
+  String? aaa, [
+  String? Function(String?, String?, String?) bbb,
+  String? ccc,
+])''');
+  }
+
+  void test_shortMethod() {
+    final methodA = method(
+      'm',
+      stringQuestion,
+      parameters: [
+        requiredParameter(name: 'a', type: stringQuestion),
+        positionalParameter(name: 'b', type: stringQuestion),
+      ],
+    );
+
+    final singleLine = methodA.getDisplayString(withNullability: true);
+    expect(singleLine, 'String? m(String? a, [String? b])');
+
+    final multiLine =
+        methodA.getDisplayString(withNullability: true, multiline: true);
+    // The signature is short enough that it remains on one line even for
+    // multiline: true.
+    expect(multiLine, 'String? m(String? a, [String? b])');
+  }
+}

pkg/analyzer/test/src/dart/element/test_all.dart

@@ -5,6 +5,7 @@
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import 'class_hierarchy_test.dart' as class_hierarchy;
+import 'display_string_test.dart' as display_string;
 import 'element_test.dart' as element;
 import 'factor_type_test.dart' as factor_type;
 import 'flatten_type_test.dart' as flatten_type;
@@ -34,6 +35,7 @@ import 'upper_lower_bound_test.dart' as upper_bound;
 main() {
   defineReflectiveSuite(() {
     class_hierarchy.main();
+    display_string.main();
     element.main();
     factor_type.main();
     flatten_type.main();