[analysis_server] Preserve existing quote kinds when updating string arguments
The original code tried to predict the "best" kind of quotes based on the string value (for example changing to double quotes when the string contained a single quote).
Since, we decided it would be better to try to preserve the existing quotes the user picked (this results in less of the string changing, and preserves user preferences/lints).
In some cases we can't preserve the exact delimeters, because if the string is a raw string but the new value contains the delimeter, we can't escape it, so we drop the `r` and escape anything that requires it.
Change-Id: I9a224ade5edd5bd558310b78be132fc9addd3ed3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398885
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Elliott Brooks <elliottbrooks@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart b/pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart
index fb41e16..70160c0 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart
@@ -101,9 +101,13 @@
);
}
+ var argument = parameterArguments[parameter];
+ var valueExpression =
+ argument is NamedExpression ? argument.expression : argument;
+
// Determine whether a value for this parameter is editable.
var notEditableReason = getNotEditableReason(
- argument: parameterArguments[parameter],
+ argument: valueExpression,
positionalIndex: positionalParameterIndexes[parameter],
numPositionals: numPositionals,
numSuppliedPositionals: numSuppliedPositionals,
@@ -119,7 +123,11 @@
}
// Compute the new expression for this argument.
- var newValueCode = _computeValueCode(parameter, params.edit);
+ var newValueCode = _computeValueCode(
+ parameter,
+ valueExpression,
+ params.edit,
+ );
// Build the edit and send it to the client.
var workspaceEdit = await newValueCode.mapResult(
@@ -139,35 +147,6 @@
});
}
- /// Computes the string of Dart code (including quotes) for [value].
- String _computeStringValueCode(String value) {
- const singleQuote = "'";
- const doubleQuote = '"';
-
- // Use single quotes unless the string contains a single quote and no
- // double quotes.
- var surroundingQuote =
- value.contains(singleQuote) && !value.contains(doubleQuote)
- ? doubleQuote
- : singleQuote;
-
- // TODO(dantup): Determine if we already have reusable code for this
- // anywhere.
- // TODO(dantup): Decide whether we should write multiline and/or raw strings
- // for some cases.
- var escaped = value
- .replaceAll(r'\', r'\\') // Escape backslashes
- .replaceAll(
- surroundingQuote,
- '\\$surroundingQuote',
- ) // Escape surrounding quotes we'll use
- .replaceAll('\r', r'\r')
- .replaceAll('\n', r'\n')
- .replaceAll(r'$', r'\$');
-
- return '$surroundingQuote$escaped$surroundingQuote';
- }
-
/// Computes the string of Dart code that should be used as the new value
/// for this argument.
///
@@ -175,6 +154,7 @@
/// the parameter name or any commas that may be required.
ErrorOr<String> _computeValueCode(
FormalParameterElement parameter,
+ Expression? argument,
ArgumentEdit edit,
) {
// TODO(dantup): Should we accept arbitrary strings for all values? For
@@ -202,9 +182,17 @@
return success(value.toString());
} else if (type.isDartCoreBool && value is bool) {
return success(value.toString());
- } else if (type.isDartCoreString && value is String) {
- return success(_computeStringValueCode(value));
- } else if (type case InterfaceType(
+ } else if (parameter.type.isDartCoreString && value is String) {
+ var simpleString = argument is SimpleStringLiteral ? argument : null;
+ return success(
+ computeStringValueCode(
+ value,
+ preferSingleQuotes: simpleString?.isSingleQuoted ?? true,
+ preferMultiline: simpleString?.isMultiline ?? false,
+ preferRaw: simpleString?.isRaw ?? false,
+ ),
+ );
+ } else if (parameter.type case InterfaceType(
:EnumElement2 element3,
) when value is String?) {
var allowedValues = getQualifiedEnumConstantNames(element3);
@@ -398,4 +386,47 @@
newCode.toString(),
);
}
+
+ /// Computes the string of Dart code (including quotes) for the String
+ /// [value].
+ ///
+ /// [preferSingleQuotes], [preferMultiline] and [preferRaw] are used to
+ /// control the kinds of delimeters used for the string but are not
+ /// guaranteed because the contents of the strings might prevent some
+ /// delimeters (for example raw strings can't be used where there need to be
+ /// escape sequences).
+ static String computeStringValueCode(
+ String value, {
+ bool preferSingleQuotes = true,
+ bool preferMultiline = false,
+ bool preferRaw = false,
+ }) {
+ var quoteCharacter = preferSingleQuotes ? "'" : '"';
+ var useMultiline = preferMultiline /* && value.contains('\n') ??? */;
+ var numQuotes = useMultiline ? 3 : 1;
+ var surroundingQuote = quoteCharacter * numQuotes;
+ // Only use raw if requested _and_ the string doesn't contain the
+ // quotes that'll be used to surround it or newlines.
+ var useRaw =
+ preferRaw &&
+ !value.contains(surroundingQuote) &&
+ !value.contains('\r') &&
+ !value.contains('\n');
+
+ // Escape non-quote characters.
+ if (!useRaw) {
+ value = value
+ .replaceAll(r'\', r'\\') // Escape backslashes
+ .replaceAll('\r', r'\r')
+ .replaceAll('\n', r'\n')
+ .replaceAll(r'$', r'\$');
+ }
+
+ // Escape quotes.
+ var escapedSurroundingQuote = '\\$quoteCharacter' * numQuotes;
+ value = value.replaceAll(surroundingQuote, escapedSurroundingQuote);
+
+ var prefix = useRaw ? 'r' : '';
+ return '$prefix$surroundingQuote$value$surroundingQuote';
+ }
}
diff --git a/pkg/analysis_server/test/lsp/edit_argument_test.dart b/pkg/analysis_server/test/lsp/edit_argument_test.dart
index 2b115d2..beef9af 100644
--- a/pkg/analysis_server/test/lsp/edit_argument_test.dart
+++ b/pkg/analysis_server/test/lsp/edit_argument_test.dart
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:analysis_server/lsp_protocol/protocol.dart';
+import 'package:analysis_server/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart';
import 'package:analyzer/src/test_utilities/test_code_format.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -11,13 +12,212 @@
import '../utils/test_code_extensions.dart';
import 'server_abstract.dart';
+// ignore_for_file: prefer_single_quotes
+
void main() {
defineReflectiveSuite(() {
defineReflectiveTests(EditArgumentTest);
+ defineReflectiveTests(ComputeStringValueTest);
});
}
@reflectiveTest
+class ComputeStringValueTest {
+ test_doubleQuote_multi_notRaw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (r""" ' """, r'''""" ' """'''),
+ // Double quotes (not escaped).
+ (r""" " """, r'''""" " """'''),
+ // Dollars (escaped).
+ (r""" $ """, r'''""" \$ """'''),
+ // Newlines (escaped).
+ (""" \r\n """, r'''""" \r\n """'''),
+ ],
+ single: false,
+ multi: true,
+ raw: false,
+ );
+ }
+
+ test_doubleQuote_multi_raw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (" ' ", r'''r""" ' """'''),
+ // Double quotes (not escaped).
+ (' " ', r'''r""" " """'''),
+ // Three Single quotes (not escaped, backslashes are to nest quotes here).
+ (" ''' ", '''r""" \''' """'''),
+ // Three Double quotes (escaped, changed to non-raw because quotes in string).
+ (' """ ', r'""" \"\"\" """'),
+ // Dollars (not escaped).
+ (r' $ ', r'r""" $ """'),
+ // Newlines (escaped, changed to non-raw because newlines in string).
+ (' \r\n ', r'""" \r\n """'),
+ ],
+ single: false,
+ multi: true,
+ raw: true,
+ );
+ }
+
+ test_doubleQuote_notMulti_notRaw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (" ' ", r'''" ' "'''),
+ // Double quotes (escaped).
+ (' " ', r'''" \" "'''),
+ // Three Single quotes (not escaped, backslashes are to nest quotes here).
+ (" ''' ", '''" \'\'\' "'''),
+ // Three Double quotes (escaped).
+ (' """ ', r'" \"\"\" "'),
+ // Dollars (escaped).
+ (r' $ ', r'" \$ "'),
+ // Newlines (escaped).
+ (' \r\n ', r'" \r\n "'),
+ ],
+ single: false,
+ multi: false,
+ raw: false,
+ );
+ }
+
+ test_doubleQuote_notMulti_raw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (" ' ", r'''r" ' "'''),
+ // Double quotes (escaped, changed to non-raw because quotes in string).
+ (' " ', r'" \" "'),
+ // Three Single quotes (not escaped, backslashes are to nest quotes here).
+ (" ''' ", '''r" \''' "'''),
+ // Three Double quotes (escaped, changed to non-raw because quotes in string).
+ (' """ ', r'" \"\"\" "'),
+ // Dollars (not escaped).
+ (r' $ ', r'r" $ "'),
+ // Newlines (escaped, changed to non-raw because newlines in string).
+ (' \r\n ', r'" \r\n "'),
+ ],
+ single: false,
+ multi: false,
+ raw: true,
+ );
+ }
+
+ test_singleQuote_multi_notRaw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (r""" ' """, r"""''' ' '''"""),
+ // Double quotes (not escaped).
+ (r""" " """, r"""''' " '''"""),
+ // Dollars (escaped).
+ (r""" $ """, r"""''' \$ '''"""),
+ // Newlines (escaped).
+ (""" \r\n """, r"""''' \r\n '''"""),
+ ],
+ single: true,
+ multi: true,
+ raw: false,
+ );
+ }
+
+ test_singleQuote_multi_raw() async {
+ verifyStrings(
+ [
+ // Single quotes (not escaped).
+ (" ' ", r"r''' ' '''"),
+ // Double quotes (not escaped).
+ (' " ', r"""r''' " '''"""),
+ // Three Single quotes (escaped, changed to non-raw because quotes in string).
+ (" ''' ", r"''' \'\'\' '''"),
+ // Three Double quotes (not escaped, backslashes are to nest quotes here).
+ (' """ ', """r''' \""" '''"""),
+ // Dollars (not escaped).
+ (r' $ ', r"r''' $ '''"),
+ // Newlines (escaped, changed to non-raw because newlines in string).
+ (' \r\n ', r"''' \r\n '''"),
+ ],
+ single: true,
+ multi: true,
+ raw: true,
+ );
+ }
+
+ test_singleQuote_notMulti_notRaw() async {
+ verifyStrings(
+ [
+ // Single quotes (escaped).
+ (" ' ", r"' \' '"),
+ // Double quotes (not escaped).
+ (' " ', r"""' " '"""),
+ // Three Single quotes (escaped).
+ (" ''' ", r"' \'\'\' '"),
+ // Three Double quotes (not escaped, backslashes are to nest quotes here).
+ (' """ ', """' \""" '"""),
+ // Dollars (escaped).
+ (r' $ ', r"' \$ '"),
+ // Newlines (escaped).
+ (' \r\n ', r"' \r\n '"),
+ ],
+ single: true,
+ multi: false,
+ raw: false,
+ );
+ }
+
+ test_singleQuote_notMulti_raw() async {
+ verifyStrings(
+ [
+ // Single quotes (escaped, changed to non-raw because quotes in string).
+ (" ' ", r"' \' '"),
+ // Double quotes (not escaped).
+ (' " ', r"""r' " '"""),
+ // Three Single quotes (escaped, changed to non-raw because quotes in string).
+ (" ''' ", r"' \'\'\' '"),
+ // Three Double quotes (not escaped, backslashes are to nest quotes here).
+ (' """ ', """r' \""" '"""),
+ // Dollars (not escaped).
+ (r' $ ', r"r' $ '"),
+ // Newlines (escaped, changed to non-raw because newlines in string).
+ (' \r\n ', r"' \r\n '"),
+ ],
+ single: true,
+ multi: false,
+ raw: true,
+ );
+ }
+
+ /// Verifies a set of strings in [tests] are written correctly as literal
+ /// Dart strings.
+ void verifyStrings(
+ List<(String, String)> tests, {
+ required bool single,
+ required bool multi,
+ required bool raw,
+ }) {
+ for (var (input, expected) in tests) {
+ var result = EditArgumentHandler.computeStringValueCode(
+ input,
+ preferSingleQuotes: single,
+ preferMultiline: multi,
+ preferRaw: raw,
+ );
+
+ expect(
+ result,
+ expected,
+ reason:
+ '[$input] should be represented by the literal Dart code [$expected] but was [$result]',
+ );
+ }
+ }
+}
+
+@reflectiveTest
class EditArgumentTest extends AbstractLspAnalysisServerTest {
late TestCode code;
@@ -454,7 +654,7 @@
params: '({ String? x })',
originalArgs: "(x: 'a')",
edit: ArgumentEdit(name: 'x', newValue: "a'b"),
- expectedArgs: '''(x: "a'b")''',
+ expectedArgs: r'''(x: 'a\'b')''',
);
}
@@ -494,6 +694,60 @@
);
}
+ test_type_string_quotes_dollar_escapedNonRaw() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: "(x: '')",
+ edit: ArgumentEdit(name: 'x', newValue: r'$'),
+ expectedArgs: r"(x: '\$')",
+ );
+ }
+
+ test_type_string_quotes_dollar_notEscapedRaw() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: "(x: r'')",
+ edit: ArgumentEdit(name: 'x', newValue: r'$'),
+ expectedArgs: r"(x: r'$')",
+ );
+ }
+
+ test_type_string_quotes_usesExistingDouble() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: '(x: "a")',
+ edit: ArgumentEdit(name: 'x', newValue: 'a'),
+ expectedArgs: '(x: "a")',
+ );
+ }
+
+ test_type_string_quotes_usesExistingSingle() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: "(x: 'a')",
+ edit: ArgumentEdit(name: 'x', newValue: 'a'),
+ expectedArgs: "(x: 'a')",
+ );
+ }
+
+ test_type_string_quotes_usesExistingTripleDouble() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: '(x: """a""")',
+ edit: ArgumentEdit(name: 'x', newValue: 'a'),
+ expectedArgs: '(x: """a""")',
+ );
+ }
+
+ test_type_string_quotes_usesExistingTripleSingle() async {
+ await _expectSimpleArgumentEdit(
+ params: '({ String? x })',
+ originalArgs: "(x: '''a''')",
+ edit: ArgumentEdit(name: 'x', newValue: 'a'),
+ expectedArgs: "(x: '''a''')",
+ );
+ }
+
test_type_string_replaceLiteral() async {
await _expectSimpleArgumentEdit(
params: '({ String? x })',
@@ -508,7 +762,7 @@
params: '({ String? x })',
originalArgs: "(x: r'a')",
edit: ArgumentEdit(name: 'x', newValue: 'b'),
- expectedArgs: "(x: 'b')",
+ expectedArgs: "(x: r'b')",
);
}
@@ -517,7 +771,7 @@
params: '({ String? x })',
originalArgs: "(x: '''a''')",
edit: ArgumentEdit(name: 'x', newValue: 'b'),
- expectedArgs: "(x: 'b')",
+ expectedArgs: "(x: '''b''')",
);
}
