From a635ccf2f5779c2f5ce0937fb6f0e22be808d97e Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 2 Dec 2024 19:41:37 +0000 Subject: [PATCH] [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 Reviewed-by: Brian Wilkerson Commit-Queue: Brian Wilkerson --- .../custom/handler_editable_arguments.dart | 103 +++++++++++--- .../lib/src/utilities/extensions/numeric.dart | 21 +++ .../test/lsp/editable_arguments_test.dart | 126 ++++++++++++++++++ .../utilities/extensions/numeric_test.dart | 50 +++++++ .../src/utilities/extensions/test_all.dart | 2 + 5 files changed, 281 insertions(+), 21 deletions(-) create mode 100644 pkg/analysis_server/test/src/utilities/extensions/numeric_test.dart diff --git a/pkg/analysis_server/lib/src/lsp/handlers/custom/handler_editable_arguments.dart b/pkg/analysis_server/lib/src/lsp/handlers/custom/handler_editable_arguments.dart index 75dd6bedbeaa..ed1839702e27 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/custom/handler_editable_arguments.dart +++ b/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? 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'; diff --git a/pkg/analysis_server/lib/src/utilities/extensions/numeric.dart b/pkg/analysis_server/lib/src/utilities/extensions/numeric.dart index 005763f997a2..21308d101de0 100644 --- a/pkg/analysis_server/lib/src/utilities/extensions/numeric.dart +++ b/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', + }, + }; + } +} diff --git a/pkg/analysis_server/test/lsp/editable_arguments_test.dart b/pkg/analysis_server/test/lsp/editable_arguments_test.dart index 97e7c89edf6d..a4f238505934 100644 --- a/pkg/analysis_server/test/lsp/editable_arguments_test.dart +++ b/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 { diff --git a/pkg/analysis_server/test/src/utilities/extensions/numeric_test.dart b/pkg/analysis_server/test/src/utilities/extensions/numeric_test.dart new file mode 100644 index 000000000000..fb6cc3659bed --- /dev/null +++ b/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 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); + } + } +} diff --git a/pkg/analysis_server/test/src/utilities/extensions/test_all.dart b/pkg/analysis_server/test/src/utilities/extensions/test_all.dart index 5d785532e33d..8bf2fbc61814 100644 --- a/pkg/analysis_server/test/src/utilities/extensions/test_all.dart +++ b/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(); });