[analysis_server] Only mark positional arguments as editable if they …

…wouldn't require inserting additional values

We can _edit_ all existing arguments because we just replace their values.
We can also _add_ a new argument for a positional parameter as long as there are no missing positional arguments that would need to come before it (because we can't insert them).

Change-Id: Ia66e49f940c463511f472405e90ee46d3b5f6177
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/396841
Reviewed-by: Elliott Brooks <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

pkg/analysis_server/lib/src/lsp/handlers/custom/handler_editable_arguments.dart

@@ -9,6 +9,7 @@ import 'package:analysis_server/src/lsp/constants.dart';
 import 'package:analysis_server/src/lsp/error_or.dart';
 import 'package:analysis_server/src/lsp/handlers/handlers.dart';
 import 'package:analysis_server/src/lsp/mapping.dart';
+import 'package:analysis_server/src/utilities/extensions/numeric.dart';
 import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/constant/value.dart';
 import 'package:analyzer/dart/element/element2.dart';
@@ -99,22 +100,53 @@ class EditableArgumentsHandler
 
     var textDocument = server.getVersionedDocumentIdentifier(result.path);
 
-    // Build a map of the parameters that have matching arguments.
+    // Build a map of parameters to their positional index so we can tell
+    // whether a parameter that doesn't already have an argument will be
+    // editable (positionals can only be added if all previous positionals
+    // exist).
+    var currentParameterIndex = 0;
+    var parametersWithIndex = parameters.map(
+      (parameter) => (
+        parameter,
+        parameter.isPositional ? currentParameterIndex++ : null,
+      ),
+    );
+    var numPositionals = parameters.where((p) => p.isPositional).length;
+    var numSuppliedPositionals =
+        argumentList.arguments
+            .where((argument) => argument is! NamedExpression)
+            .length;
+
+    // Build a map of the parameters that have arguments so we can put them
+    // first.
     var parametersWithArguments = {
       for (var argument in argumentList.arguments)
         argument.correspondingParameter: argument,
     };
 
+    // Build the complete list of editable arguments.
     var editableArguments = [
-      // First include the arguments in the order they were specified.
+      // First arguments that exist in the order they were specified.
       for (var MapEntry(key: parameter, value: argument)
           in parametersWithArguments.entries)
-        if (parameter != null) _toEditableArgument(parameter, argument),
-      // Then the remaining parameters.
-      for (var parameter in parameters.where(
-        (p) => !parametersWithArguments.containsKey(p),
+        if (parameter != null)
+          _toEditableArgument(
+            parameter,
+            argument,
+            numPositionals: numPositionals,
+            numSuppliedPositionals: numSuppliedPositionals,
+          ),
+      // Then the remaining parameters that don't have existing arguments.
+      for (var (parameter, index) in parametersWithIndex.where(
+        (p) => !parametersWithArguments.containsKey(p.$1),
       ))
-        _toEditableArgument(parameter, null),
+        _toEditableArgument(
+          parameter,
+          null,
+          positionalIndex: index,
+          numPositionals: numPositionals,
+          numSuppliedPositionals: numSuppliedPositionals,
+        ),
     ];
 
     return EditableArguments(
@@ -151,14 +183,37 @@ class EditableArgumentsHandler
 
   /// Checks whether [argument] is editable and if not, returns a human-readable
   /// description why.
-  String? _getNotEditableReason(Expression argument) {
-    return switch (argument) {
-      AdjacentStrings() => "Adjacent strings can't be edited",
-      StringInterpolation() => "Interpolated strings can't be edited",
-      SimpleStringLiteral() when argument.value.contains('\n') =>
-        "Strings containing newlines can't be edited",
-      _ => null,
-    };
+  String? _getNotEditableReason({
+    required Expression? argument,
+    required int? positionalIndex,
+    required int numPositionals,
+    required int numSuppliedPositionals,
+  }) {
+    // If the argument has an existing value, editability is based only on that
+    // value.
+    if (argument != null) {
+      return switch (argument) {
+        AdjacentStrings() => "Adjacent strings can't be edited",
+        StringInterpolation() => "Interpolated strings can't be edited",
+        SimpleStringLiteral() when argument.value.contains('\n') =>
+          "Strings containing newlines can't be edited",
+        _ => null,
+      };
+    }
+
+    // If we are missing positionals, we can only add this one if it is the next
+    // (first missing) one.
+    if (positionalIndex != null && numSuppliedPositionals < numPositionals) {
+      // To be allowed, we must be the next one. Eg. our index is equal to the
+      // length/count of the existing ones.
+      if (positionalIndex != numSuppliedPositionals) {
+        return 'A value for the ${(positionalIndex + 1).toStringWithSuffix()} '
+            "parameter can't be added until a value for all preceding "
+            'positional parameters have been added.';
+      }
+    }
+
+    return null;
   }
 
   /// Computes the values for a parameter and argument and returns them along
@@ -194,8 +249,11 @@ class EditableArgumentsHandler
   /// is an argument that can be edited.
   EditableArgument? _toEditableArgument(
     FormalParameterElement parameter,
-    Expression? argument,
-  ) {
+    Expression? argument, {
+    int? positionalIndex,
+    required int numPositionals,
+    required int numSuppliedPositionals,
+  }) {
     var valueExpression =
         argument is NamedExpression ? argument.expression : argument;
 
@@ -206,10 +264,13 @@ class EditableArgumentsHandler
     Object? value;
     List<String>? options;
 
-    // Check whether this value may be editable (for example is not an
-    // interpolated string).
-    var notEditableReason =
-        valueExpression != null ? _getNotEditableReason(valueExpression) : null;
+    // Determine whether a value for this parameter is editable.
+    var notEditableReason = _getNotEditableReason(
+      argument: valueExpression,
+      positionalIndex: positionalIndex,
+      numPositionals: numPositionals,
+      numSuppliedPositionals: numSuppliedPositionals,
+    );
 
     if (parameter.type.isDartCoreDouble) {
       type = 'double';

pkg/analysis_server/lib/src/utilities/extensions/numeric.dart

@@ -7,3 +7,24 @@ extension DoubleExtensions on double {
   /// inclusive.
   bool between(double lower, double upper) => lower <= this && this <= upper;
 }
+
+extension IntExtensions on int {
+  /// Returns a string representation of this number with a suffix like 'th',
+  /// 'nd' or 'rd'.
+  ///
+  ///     1 -> 1st
+  ///     2 -> 2nd
+  ///     11 -> 11th
+  ///     21 -> 21st
+  String toStringWithSuffix() {
+    return switch (this % 100) {
+      >= 11 && <= 13 => '${this}th',
+      _ => switch (this % 10) {
+        1 => '${this}st',
+        2 => '${this}nd',
+        3 => '${this}rd',
+        _ => '${this}th',
+      },
+    };
+  }
+}

pkg/analysis_server/test/lsp/editable_arguments_test.dart

@@ -147,6 +147,88 @@ class MyWidget extends StatelessWidget {
     );
   }
 
+  test_isEditable_false_positional_optional() async {
+    var result = await getEditableArgumentsFor(r'''
+class MyWidget extends StatelessWidget {
+  const MyWidget([int? a, int? b, int? c]);
+
+  @override
+  Widget build(BuildContext context) => MyW^idget(1);
+}
+''');
+    expect(
+      result,
+      hasArgs(
+        orderedEquals([
+          isArg('a', isEditable: true),
+          isArg('b', isEditable: true),
+          isArg(
+            'c',
+            // c is not editable because it is not guaranteed that we can insert
+            // a default value for b (it could be a private value or require
+            // imports).
+            isEditable: false,
+            notEditableReason:
+                "A value for the 3rd parameter can't be added until a value "
+                'for all preceding positional parameters have been added.',
+          ),
+        ]),
+      ),
+    );
+  }
+
+  test_isEditable_false_positional_required1() async {
+    failTestOnErrorDiagnostic = false;
+    var result = await getEditableArgumentsFor(r'''
+class MyWidget extends StatelessWidget {
+  const MyWidget(int a, int b);
+
+  @override
+  Widget build(BuildContext context) => MyW^idget();
+}
+''');
+    expect(
+      result,
+      hasArg(
+        // b is not editable because there are missing required previous
+        // arguments (a).
+        isArg(
+          'b',
+          isEditable: false,
+          notEditableReason:
+              "A value for the 2nd parameter can't be added until a value "
+              'for all preceding positional parameters have been added.',
+        ),
+      ),
+    );
+  }
+
+  test_isEditable_false_positional_required2() async {
+    failTestOnErrorDiagnostic = false;
+    var result = await getEditableArgumentsFor(r'''
+class MyWidget extends StatelessWidget {
+  const MyWidget(int a, int b, int c);
+
+  @override
+  Widget build(BuildContext context) => MyW^idget(1);
+}
+''');
+    expect(
+      result,
+      hasArg(
+        // c is not editable because there are missing required previous
+        // arguments (b).
+        isArg(
+          'c',
+          isEditable: false,
+          notEditableReason:
+              "A value for the 3rd parameter can't be added until a value "
+              'for all preceding positional parameters have been added.',
+        ),
+      ),
+    );
+  }
+
   test_isEditable_false_string_adjacent() async {
     var result = await getEditableArgumentsFor(r'''
 class MyWidget extends StatelessWidget {
@@ -238,6 +320,50 @@ b
     );
   }
 
+  test_isEditable_true_named() async {
+    var result = await getEditableArgumentsFor(r'''
+class MyWidget extends StatelessWidget {
+  const MyWidget({int? a, int? b, int? c});
+
+  @override
+  Widget build(BuildContext context) => MyW^idget(a: 1);
+}
+''');
+    expect(
+      result,
+      hasArgs(
+        orderedEquals([
+          isArg('a', isEditable: true),
+          isArg('b', isEditable: true),
+          isArg('c', isEditable: true),
+        ]),
+      ),
+    );
+  }
+
+  test_isEditable_true_positional_required() async {
+    failTestOnErrorDiagnostic = false;
+    var result = await getEditableArgumentsFor(r'''
+class MyWidget extends StatelessWidget {
+  const MyWidget(int a, int b);
+
+  @override
+  Widget build(BuildContext context) => MyW^idget(1);
+}
+''');
+    expect(
+      result,
+      hasArg(
+        isArg(
+          'b',
+          // b is editable because it's the next argument and we don't need
+          // to add anything additional.
+          isEditable: true,
+        ),
+      ),
+    );
+  }
+
   test_isEditable_true_string_dollar_escaped() async {
     var result = await getEditableArgumentsFor(r'''
 class MyWidget extends StatelessWidget {

pkg/analysis_server/test/src/utilities/extensions/numeric_test.dart

@@ -0,0 +1,50 @@
+// Copyright (c) 2024, 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:analysis_server/src/utilities/extensions/numeric.dart';
+import 'package:test/expect.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+void main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(NumericTest);
+  });
+}
+
+@reflectiveTest
+class NumericTest {
+  Future<void> test_asStringWithSuffix() async {
+    var expectedResults = {
+      1: '1st',
+      2: '2nd',
+      3: '3rd',
+      4: '4th',
+      5: '5th',
+      6: '6th',
+      7: '7th',
+      8: '8th',
+      9: '9th',
+      10: '10th',
+      11: '11th',
+      12: '12th',
+      13: '13th',
+      14: '14th',
+      15: '15th',
+      20: '20th',
+      21: '21st',
+      22: '22nd',
+      23: '23rd',
+      24: '24th',
+      88: '88th',
+      100: '100th',
+      101: '101st',
+      111: '111th',
+      121: '121st',
+    };
+
+    for (var MapEntry(:key, :value) in expectedResults.entries) {
+      expect(key.toStringWithSuffix(), value);
+    }
+  }
+}

pkg/analysis_server/test/src/utilities/extensions/test_all.dart

@@ -5,12 +5,14 @@
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import 'ast_test.dart' as ast;
+import 'numeric_test.dart' as numeric;
 import 'range_factory_test.dart' as range_factory;
 import 'string_test.dart' as string;
 
 void main() {
   defineReflectiveSuite(() {
     ast.main();
+    numeric.main();
     range_factory.main();
     string.main();
   });