[DAS] Fixes regression in completion for unnamed parameters in closures

Fixes: #61518
Change-Id: Ia190a35188d2e9cf67fd7a6e75580cbcf2f9c6a2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/450180
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Auto-Submit: Felipe Morschel <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>

Author: FMorschel

pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart

@@ -22,6 +22,7 @@ import 'package:analyzer/dart/ast/syntactic_entity.dart';
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/dart/element/nullability_suffix.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer/src/dart/ast/ast.dart';
@@ -320,12 +321,26 @@ class InScopeCompletionPass extends SimpleAstVisitor<void> {
         // Only suggest expression keywords if it's possible that the user is
         // completing a positional argument.
         if (positionalArgumentCount < positionalParameterCount) {
-          _forExpression(parent, mustBeNonVoid: true);
           // This assumes that the positional parameters will always be first in
           // the list of parameters.
           var parameter = parameters[positionalArgumentCount];
           var parameterType = parameter.type;
-          if (parameterType is FunctionType) {
+          var isFunctionType = parameterType is FunctionType;
+          _forExpression(
+            parent,
+            mustBeNonVoid: true,
+            canBeNull:
+                parameterType.nullabilitySuffix != NullabilitySuffix.none ||
+                parameterType is DynamicType,
+            // TODO(FMorschel): Determine if the expected type is bool and only
+            // suggest `true` and `false` in that case.
+            canBeBool: !isFunctionType,
+            // TODO(FMorschel): Determine if the parameter type has a constant
+            // constructor.
+            // Function tear-offs and closures cannot be constant.
+            canSuggestConst: !isFunctionType,
+          );
+          if (isFunctionType) {
             Expression? argument;
             if (argumentIndex < arguments.length) {
               argument = arguments[argumentIndex];
@@ -3552,6 +3567,9 @@ class InScopeCompletionPass extends SimpleAstVisitor<void> {
     AstNode node, {
     bool mustBeAssignable = false,
     bool mustBeNonVoid = false,
+    bool canBeBool = true,
+    bool canBeNull = true,
+    bool canSuggestConst = true,
   }) {
     var mustBeConstant =
         node is Expression &&
@@ -3561,6 +3579,9 @@ class InScopeCompletionPass extends SimpleAstVisitor<void> {
       node,
       mustBeConstant: mustBeConstant,
       mustBeStatic: mustBeStatic,
+      canBeBool: canBeBool,
+      canBeNull: canBeNull,
+      canSuggestConst: canSuggestConst,
     );
     declarationHelper(
       mustBeAssignable: mustBeAssignable,

pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart

@@ -263,6 +263,9 @@ class KeywordHelper {
   /// keywords to include.
   void addExpressionKeywords(
     AstNode? node, {
+    bool canBeBool = true,
+    bool canBeNull = true,
+    bool canSuggestConst = true,
     bool mustBeConstant = false,
     bool mustBeStatic = false,
   }) {
@@ -311,11 +314,17 @@ class KeywordHelper {
       return true;
     }
 
-    addKeyword(Keyword.FALSE);
-    addKeyword(Keyword.NULL);
-    addKeyword(Keyword.TRUE);
+    if (canBeBool) {
+      addKeyword(Keyword.FALSE);
+    }
+    if (canBeNull) {
+      addKeyword(Keyword.NULL);
+    }
+    if (canBeBool) {
+      addKeyword(Keyword.TRUE);
+    }
     if (node != null) {
-      if (constIsValid(node)) {
+      if (canSuggestConst && constIsValid(node)) {
         addKeyword(Keyword.CONST);
       }
       if (!mustBeConstant && !mustBeStatic) {

pkg/analysis_server/lib/src/services/completion/dart/utilities.dart

@@ -42,7 +42,7 @@ String buildClosureParameters(
   var hasNamed = false;
   var hasOptionalPositional = false;
   var parameters = type.formalParameters;
-  var existingNames = parameters.map((p) => p.displayName).toSet();
+  var existingNames = parameters.map((p) => p.name).nonNulls.toSet();
   for (var i = 0; i < parameters.length; ++i) {
     var parameter = parameters[i];
     if (i != 0) {
@@ -59,7 +59,7 @@ String buildClosureParameters(
       buffer.write(parameter.type);
       buffer.write(' ');
     }
-    var name = parameter.displayName;
+    var name = parameter.name ?? '';
     if (name.isEmpty) {
       name = 'p$i';
       var index = 1;

pkg/analysis_server/test/client/completion_driver_test.dart

@@ -68,7 +68,7 @@ abstract class AbstractCompletionDriverTest
 
   /// Return `true` if closures (suggestions starting with a left paren) should
   /// be included in the text to be compared.
-  bool get includeClosures => false;
+  bool includeClosures = false;
 
   /// Return `true` if overrides should be included in the text to be compared.
   bool get includeOverrides => true;

pkg/analysis_server/test/lsp/completion.dart

@@ -15,7 +15,11 @@ mixin CompletionTestMixin on AbstractLspAnalysisServerTest {
   List<CompletionItem> completionResults = [];
 
   int sortTextSorter(CompletionItem item1, CompletionItem item2) =>
-      (item1.sortText ?? item1.label).compareTo(item2.sortText ?? item2.label);
+      (item1.sortText == item2.sortText)
+      ? item1.label.compareTo(item2.label)
+      : (item1.sortText ?? item1.label).compareTo(
+          item2.sortText ?? item2.label,
+        );
 
   Future<String?> verifyCompletions(
     Uri fileUri,

pkg/analysis_server/test/lsp/completion_dart_test.dart

@@ -1545,6 +1545,28 @@ void g() {
     );
   }
 
+  Future<void> test_closure_unnamedParameters() async {
+    var content = '''
+void f(void Function(int) x) {
+  f(^);
+}
+''';
+
+    var expectedContent = '''
+void f(void Function(int) x) {
+  f((p0) => ^,);
+}
+''';
+
+    await verifyCompletions(
+      mainFileUri,
+      content,
+      expectCompletions: ['(p0) =>', '(p0) {}'],
+      applyEditsFor: '(p0) =>',
+      expectedContent: expectedContent,
+    );
+  }
+
   Future<void> test_color_material() async {
     content = '''
 import 'package:flutter/material.dart';

pkg/analysis_server/test/lsp/completion_yaml_test.dart

@@ -530,7 +530,7 @@ dependencies:
     await verifyCompletions(
       pubspecFileUri,
       content,
-      expectCompletions: ['^2.3.4', '^2.1.0'],
+      expectCompletions: ['^2.1.0', '^2.3.4'],
       applyEditsFor: '^2.3.4',
       expectedContent: expected,
       openCloseFile: false,
@@ -630,7 +630,7 @@ name: ''';
     await verifyCompletions(
       pubspecFileUri,
       content,
-      expectCompletions: ['name: ', 'description: '],
+      expectCompletions: ['description: ', 'name: '],
       applyEditsFor: 'name: ',
       expectedContent: expected,
     );

pkg/analysis_server/test/services/completion/dart/completion_test.dart

@@ -4089,8 +4089,6 @@ replacement
 suggestions
   n
     kind: localVariable
-  null
-    kind: keyword
   const
     kind: keyword
 ''');
@@ -4985,8 +4983,6 @@ suggestions
     kind: keyword
   false
     kind: keyword
-  null
-    kind: keyword
   this
     kind: keyword
   const
@@ -5890,18 +5886,39 @@ void r() {
   x(^);
 }
 ''');
+    // Could be:
+    //
+    // ```dart
+    // switch (i) {
+    //   < 0 => (k) {},
+    //   _ => v,
+    // }
+    // ```
     assertResponse(r'''
 suggestions
   v
     kind: localVariable
-  true
-    kind: keyword
-  false
+  switch
     kind: keyword
+''');
+  }
+
+  Future<void> test_commentSnippets058_1_nullable() async {
+    allowedIdentifiers = {'v'};
+    await computeSuggestions('''
+typedef void callback(int k);
+void x(callback? q){}
+void r() {
+  callback v;
+  x(^);
+}
+''');
+    assertResponse(r'''
+suggestions
+  v
+    kind: localVariable
   null
     kind: keyword
-  const
-    kind: keyword
   switch
     kind: keyword
 ''');
@@ -7946,8 +7963,6 @@ suggestions
     kind: constructorInvocation
   A.second
     kind: constructorInvocation
-  null
-    kind: keyword
   false
     kind: keyword
   true

pkg/analysis_server/test/services/completion/dart/declaration/closure_test.dart

@@ -139,6 +139,25 @@ suggestions
 ''');
   }
 
+  Future<void> test_parameter_unnamed() async {
+    await computeSuggestions('''
+void f(void Function(int) x) {
+  f(^);
+}
+''');
+    assertResponse('''
+suggestions
+  (p0) => ^,
+    kind: invocation
+    displayText: (p0) =>
+  (p0) {
+    ^
+  },
+    kind: invocation
+    displayText: (p0) {}
+''');
+  }
+
   Future<void> test_parameters_optionalNamed() async {
     await computeSuggestions('''
 void f({void Function(int a, {int b, int c}) closure}) {}

pkg/analysis_server/test/services/completion/dart/location/argument_list_test.dart

@@ -71,8 +71,6 @@ suggestions
     kind: keyword
   false
     kind: keyword
-  null
-    kind: keyword
   const
     kind: keyword
   switch
@@ -98,8 +96,6 @@ replacement
 suggestions
   random
     kind: localVariable
-  null
-    kind: keyword
   false
     kind: keyword
   true
@@ -235,6 +231,83 @@ suggestions
     kind: keyword
   null
     kind: keyword
+''');
+  }
+
+  Future<void> test_closure_namedParameters() async {
+    includeClosures = true;
+    await computeSuggestions('''
+void f(void Function(int i) x) {
+  f(^);
+}
+''');
+    assertResponse('''
+suggestions
+  (i) => ^,
+    kind: invocation
+  (i) {
+    ^
+  },
+    kind: invocation
+  switch
+    kind: keyword
+''');
+  }
+
+  Future<void> test_closure_unnamedParameters() async {
+    includeClosures = true;
+    await computeSuggestions('''
+void f(void Function(int) x) {
+  f(^);
+}
+''');
+    assertResponse('''
+suggestions
+  (p0) => ^,
+    kind: invocation
+  (p0) {
+    ^
+  },
+    kind: invocation
+  switch
+    kind: keyword
+''');
+  }
+
+  Future<void> test_closure_unnamedParameters_equal() async {
+    includeClosures = true;
+    await computeSuggestions('''
+void f(void Function(int p2, int p2_1, int) x) {
+  f(^);
+}
+''');
+    // We expect it to try and use `p2` but since it is already in use, it
+    // should try `p2_1`, and then `p2_2`.
+    assertResponse('''
+suggestions
+  (p2, p2_1, p2_2) => ^,
+    kind: invocation
+  (p2, p2_1, p2_2) {
+    ^
+  },
+    kind: invocation
+  switch
+    kind: keyword
+''');
+  }
+
+  Future<void> test_nullableClosure() async {
+    await computeSuggestions('''
+void f(void Function(int i)? x) {
+  f(^);
+}
+''');
+    assertResponse('''
+suggestions
+  null
+    kind: keyword
+  switch
+    kind: keyword
 ''');
   }
 }