From 336ad5217922c9b8a8542307b66cfa6522d33e9a Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 1 Jun 2026 12:43:19 -0700 Subject: [PATCH] [analysis_server] Fix handling of default values in unanswered Interactive Forms This fixes a bug where an interactive form where all fields have defaults would be considered complete immediately, so we'd never present the fields to the client. It also includes some minor refactoring extracted from a future CL that implements command/resolve and supports Interactive Forms in refactors in an attempt to reduce the size of that change to aid reviewing. Change-Id: I176fe25dbb0b610d69617fa04562b0d3ce571642 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/508220 Reviewed-by: Brian Wilkerson Reviewed-by: Samuel Rawlins --- .../benchmark/perf/memory_tests.dart | 5 +- .../support/integration_tests.dart | 17 ++++-- pkg/analysis_server/lib/src/lsp/mapping.dart | 6 +-- .../interactive_forms/interactive_forms.dart | 21 +++++--- .../framework/refactoring_processor.dart | 43 ++++++++++----- .../framework/refactoring_producer.dart | 6 +++ .../lib/src/utilities/extensions/list.dart | 8 +++ .../test/lsp/request_helpers_mixin.dart | 5 +- .../interactive_forms_test.dart | 52 ++++++++++++++++--- .../move_top_level_to_file_test.dart | 2 +- .../refactoring/refactoring_test_support.dart | 24 +++++---- .../test/support/interactive_forms.dart | 12 +++++ pkg/analysis_server/tool/lsp_spec/README.md | 17 +++--- 13 files changed, 157 insertions(+), 61 deletions(-) create mode 100644 pkg/analysis_server/lib/src/utilities/extensions/list.dart create mode 100644 pkg/analysis_server/test/support/interactive_forms.dart diff --git a/pkg/analysis_server/benchmark/perf/memory_tests.dart b/pkg/analysis_server/benchmark/perf/memory_tests.dart index 5d122583993..0005e5fa4f7 100644 --- a/pkg/analysis_server/benchmark/perf/memory_tests.dart +++ b/pkg/analysis_server/benchmark/perf/memory_tests.dart @@ -216,8 +216,9 @@ class LspAnalysisServerMemoryUsageTest Map> currentAnalysisErrors = {}; @override - void expect(Object? actual, Matcher matcher, {String? reason}) => - outOfTestExpect(actual, matcher, reason: reason); + void expect(Object? actual, Object? matcher, {String? reason}) { + outOfTestExpect(actual, matcher, reason: reason); + } /// The server is automatically started before every test. @override diff --git a/pkg/analysis_server/integration_test/support/integration_tests.dart b/pkg/analysis_server/integration_test/support/integration_tests.dart index 984ae6d86fc..e6e484f521e 100644 --- a/pkg/analysis_server/integration_test/support/integration_tests.dart +++ b/pkg/analysis_server/integration_test/support/integration_tests.dart @@ -56,11 +56,15 @@ Matcher isOneOf(List choiceMatchers) => _OneOf(choiceMatchers); /// Assert that [actual] matches [matcher]. void outOfTestExpect( Object? actual, - Matcher matcher, { + Object? matcherOrValue, { String? reason, skip, bool verbose = false, }) { + var matcher = matcherOrValue is Matcher + ? matcherOrValue + : equals(matcherOrValue); + var matchState = {}; try { if (matcher.matches(actual, matchState)) return; @@ -95,12 +99,15 @@ String _defaultFailFormatter( typedef MatcherCreator = Matcher Function(); /// Type of closures used by MatchesJsonObject to record field mismatches. -typedef MismatchDescriber = - Description Function(Description mismatchDescription); +typedef MismatchDescriber = Description Function( + Description mismatchDescription, +); /// Type of callbacks used to process notifications. -typedef NotificationProcessor = - void Function(String event, Map params); +typedef NotificationProcessor = void Function( + String event, + Map params, +); /// Type of callbacks used to process reverse-requests. typedef ReverseRequestProcessor = void Function(Request request); diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart index fdf9ff86a48..9ef05933dab 100644 --- a/pkg/analysis_server/lib/src/lsp/mapping.dart +++ b/pkg/analysis_server/lib/src/lsp/mapping.dart @@ -22,6 +22,7 @@ import 'package:analysis_server/src/protocol_server.dart' import 'package:analysis_server/src/services/completion/dart/dart_completion_suggestion.dart'; import 'package:analysis_server/src/services/completion/dart/feature_computer.dart'; import 'package:analysis_server/src/services/snippets/snippet.dart'; +import 'package:analysis_server/src/utilities/extensions/list.dart'; import 'package:analysis_server/src/utilities/extensions/string.dart'; import 'package:analyzer/dart/analysis/results.dart' as server; import 'package:analyzer/dart/element/element.dart'; @@ -1899,8 +1900,3 @@ extension CompletionLabelExtension on CompletionItemLabelDetails { CompletionItemLabelDetails? get nullIfEmpty => detail != null || description != null ? this : null; } - -extension _ListExtensions on List { - /// Returns `null` if this list is empty, otherwise `this`. - List? get nullIfEmpty => isEmpty ? null : this; -} diff --git a/pkg/analysis_server/lib/src/services/interactive_forms/interactive_forms.dart b/pkg/analysis_server/lib/src/services/interactive_forms/interactive_forms.dart index 4802bf14a05..8a1c8307a8a 100644 --- a/pkg/analysis_server/lib/src/services/interactive_forms/interactive_forms.dart +++ b/pkg/analysis_server/lib/src/services/interactive_forms/interactive_forms.dart @@ -132,11 +132,13 @@ class InteractiveForm { _isComplete = true; // Default until we see validation errors. clientFields.clear(); for (var field in _fieldMap.values) { - // Use the default value if no answer was supplied by the client, since - // this allows us to have unsupported form fields as long as they have - // defaults. - var answerValue = answerById[field.id]?.value ?? field.defaultValue; - var errorMessage = _validateAnswer(field, answerValue); + var answerValue = answerById[field.id]?.value; + var errorMessage = _validateAnswer( + field, + // For validation, we can use the default value if none was provided. + // This allows unsupported fields with defaults to pass validation. + answerValue ?? field.defaultValue, + ); var isValid = errorMessage == null; // Record the current answer and validation state so it can be used by @@ -152,7 +154,14 @@ class InteractiveForm { } // Update form completion state. - _isComplete = _isComplete && isValid; + if (!isValid) { + // User has given an invalid answer and must be shown an error. + _isComplete = false; + } else if (_isSupported(field) && field.required && answerValue == null) { + // A supported, required field does not have an answer so the form must + // still be presented again. + _isComplete = false; + } } // If the form is complete, no fields go back to the client. diff --git a/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_processor.dart b/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_processor.dart index 9cf706c572a..61dc8e41597 100644 --- a/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_processor.dart +++ b/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_processor.dart @@ -18,8 +18,9 @@ import 'package:language_server_protocol/protocol_custom_generated.dart'; import 'package:language_server_protocol/protocol_generated.dart'; /// A function that can be executed to create a refactoring producer. -typedef RefactoringProducerGenerator = - RefactoringProducer Function(RefactoringContext); +typedef RefactoringProducerGenerator = RefactoringProducer Function( + RefactoringContext, +); class RefactoringProcessor { /// A list of the generators used to produce refactorings. @@ -101,16 +102,10 @@ class RefactoringProcessor { command: Command( command: command, title: producer.title, - arguments: [ - { - 'filePath': context.resolvedUnitResult.path, - 'selectionOffset': context.selectionOffset, - 'selectionLength': context.selectionLength, - 'arguments': parameters - .map((param) => param.defaultValue) - .toList(), - }, - ], + arguments: buildCommandArguments( + context, + parameters.map((param) => param.defaultValue).toList(), + ), ), data: {'parameters': parameters}, ), @@ -134,4 +129,28 @@ class RefactoringProcessor { _performance?.computeTime = _timer.elapsed; return refactorings; } + + /// Builds the command arguments that go to the client, which include the + /// values required to rebuild the refactoring context, and the arguments + /// specific to the refactor. + /// + /// We always use a single argument that is a map so all values are named, + /// with the refactor-specific arguments being in the `arguments` field of + /// that map. + /// + /// This is the opposite of [extractRefactorArguments] which extracts the + /// refactor arguments back out of the command. + static List buildCommandArguments( + RefactoringContext context, + List refactorAguments, + ) { + return [ + { + 'filePath': context.resolvedUnitResult.path, + 'selectionOffset': context.selectionOffset, + 'selectionLength': context.selectionLength, + 'arguments': refactorAguments, + }, + ]; + } } diff --git a/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_producer.dart b/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_producer.dart index ac49f37d6f0..00588da0566 100644 --- a/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_producer.dart +++ b/pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_producer.dart @@ -3,6 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:analysis_server/src/services/refactoring/framework/refactoring_context.dart'; +import 'package:analysis_server/src/services/refactoring/framework/refactoring_processor.dart'; import 'package:analysis_server/src/services/search/search_engine.dart'; import 'package:analysis_server_plugin/edit/correction_utils.dart'; import 'package:analysis_server_plugin/src/utilities/selection.dart'; @@ -34,6 +35,11 @@ abstract class ParameterizedRefactoringProducer extends RefactoringProducer { /// Return a list of the parameters to send to the client. List get parameters; + + /// A convenience wrapper around [RefactoringProcessor.buildCommandArguments]. + List buildCommandArguments(List args) { + return RefactoringProcessor.buildCommandArguments(refactoringContext, args); + } } /// An object that can compute a refactoring in a Dart file. diff --git a/pkg/analysis_server/lib/src/utilities/extensions/list.dart b/pkg/analysis_server/lib/src/utilities/extensions/list.dart new file mode 100644 index 00000000000..c268968f72d --- /dev/null +++ b/pkg/analysis_server/lib/src/utilities/extensions/list.dart @@ -0,0 +1,8 @@ +// Copyright (c) 2026, 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. + +extension ListExtensions on List { + /// Returns `null` if this list is empty, otherwise `this`. + List? get nullIfEmpty => isEmpty ? null : this; +} diff --git a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart b/pkg/analysis_server/test/lsp/request_helpers_mixin.dart index f276d25ac7f..88d0bbcdafd 100644 --- a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart +++ b/pkg/analysis_server/test/lsp/request_helpers_mixin.dart @@ -301,8 +301,9 @@ mixin LspRequestHelpersMixin { ); } - void expect(Object? actual, Matcher matcher, {String? reason}) => - test.expect(actual, matcher, reason: reason); + void expect(Object? actual, Object? matcher, {String? reason}) { + test.expect(actual, matcher, reason: reason); + } Future expectSuccessfulResponseTo( RequestMessage request, diff --git a/pkg/analysis_server/test/src/services/interactive_forms/interactive_forms_test.dart b/pkg/analysis_server/test/src/services/interactive_forms/interactive_forms_test.dart index 833e022350e..605f073709e 100644 --- a/pkg/analysis_server/test/src/services/interactive_forms/interactive_forms_test.dart +++ b/pkg/analysis_server/test/src/services/interactive_forms/interactive_forms_test.dart @@ -7,6 +7,8 @@ import 'package:analysis_server/src/services/interactive_forms/interactive_forms import 'package:matcher/expect.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; +import '../../../support/interactive_forms.dart'; + void main() { defineReflectiveSuite(() { defineReflectiveTests(InteractiveFormsTest); @@ -15,6 +17,49 @@ void main() { @reflectiveTest class InteractiveFormsTest { + /// Default values are not treated the same as user answers. A form will not + /// be considered complete even if unanswered fields have defaults (as long + /// as they are supported). + test_defaults_doNotCompleteForm_answered() { + var fieldA = _stringField('a', defaultValue: 'aDefault'); + var fieldB = _stringField('b', defaultValue: 'bDefault'); + var fields = [fieldA, fieldB]; + + var form = InteractiveForm( + supportedInteractiveFormInputTypes: {'string'}, + fields: fields, + ); + + // Process empty answers. This makes no difference to the unanswered case + // above. + form.processResponse([]); + + // Because we never provided answers, we still have fields to complete. + expect(form.clientFields, [fieldA, fieldB]); + expect(form.clientAnswers, isEmpty); + expect(form.answers, ['aDefault', 'bDefault']); + } + + /// Default values are not treated the same as user answers. A form will not + /// be considered complete even if unanswered fields have defaults (as long + /// as they are supported). + test_defaults_doNotCompleteForm_unanswered() { + var fieldA = _stringField('a', defaultValue: 'aDefault'); + var fieldB = _stringField('b', defaultValue: 'bDefault'); + var fields = [fieldA, fieldB]; + + var form = InteractiveForm( + supportedInteractiveFormInputTypes: {'string'}, + fields: fields, + ); + + // Because we have never responded to the form, we still have fields to + // complete. + expect(form.clientFields, [fieldA, fieldB]); + expect(form.clientAnswers, isEmpty); + expect(form.answers, ['aDefault', 'bDefault']); + } + test_initialState() { var fieldA = _stringField('a', defaultValue: 'aDefault'); var fieldB = _stringField('b'); @@ -392,10 +437,3 @@ class InteractiveFormsTest { ); } } - -extension on FormField { - /// Returns a [FormAnswer] for this field with the answer [value]. - FormAnswer answer(Object? value) { - return FormAnswer(id: id, value: value); - } -} diff --git a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart index cc732d70074..f5f588a3121 100644 --- a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart +++ b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart @@ -32,7 +32,7 @@ class ^A {} /// Replaces the "Save URI" argument in [action]. void replaceSaveUriArgument(CodeAction action, Uri newFileUri) { - var arguments = getRefactorCommandArguments(action); + var arguments = getRefactorCommandArguments(action.command?.arguments); // The filename is the first item we prompt for so is first in the // arguments. arguments[0] = newFileUri.toString(); diff --git a/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart b/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart index 31d36bfd40b..85b85a74013 100644 --- a/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart +++ b/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart @@ -115,16 +115,12 @@ abstract class RefactoringTest extends AbstractLspAnalysisServerTest /// Unwraps the 'arguments' field from the arguments object (which is the /// single argument for the command). - List getRefactorCommandArguments(CodeAction action) { - var command = action.command!; - var commandArguments = command.arguments as List; + List getRefactorCommandArguments(List? commandArguments) { + // Our refactor commands use a single object in their arguments so we can + // have named fields instead of positional arguments. + var argsObject = commandArguments!.single as Map; - // Our refactor command uses a single object in its arguments so we can have - // named fields instead of having the client have to know which index - // corresponds to the parameters. - var argsObject = commandArguments.single as Map; - - // Within that object, the 'arguments' field is the List that + // Within the object, the 'arguments' field is the List that // contains the values for the parameters. var arguments = argsObject['arguments'] as List; @@ -136,8 +132,14 @@ abstract class RefactoringTest extends AbstractLspAnalysisServerTest /// Enables all required client capabilities for new refactors unless the /// corresponding flags are set to `false`. @override - Future initializeServer({bool experimentalOptInFlag = true}) async { - var config = {if (experimentalOptInFlag) 'experimentalRefactors': true}; + Future initializeServer({ + bool experimentalOptInFlag = true, + bool experimentalInteractiveForms = false, + }) async { + var config = { + if (experimentalOptInFlag) 'experimentalRefactors': true, + if (experimentalInteractiveForms) 'experimentalInteractiveForms': true, + }; await provideConfig(super.initializeServer, config); } diff --git a/pkg/analysis_server/test/support/interactive_forms.dart b/pkg/analysis_server/test/support/interactive_forms.dart new file mode 100644 index 00000000000..dc83f962d9a --- /dev/null +++ b/pkg/analysis_server/test/support/interactive_forms.dart @@ -0,0 +1,12 @@ +// Copyright (c) 2026, 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:language_server_protocol/protocol_custom_generated.dart'; + +extension FormFieldExtension on FormField { + /// Returns a [FormAnswer] for this field with the answer [value]. + FormAnswer answer(Object? value) { + return FormAnswer(id: id, value: value); + } +} diff --git a/pkg/analysis_server/tool/lsp_spec/README.md b/pkg/analysis_server/tool/lsp_spec/README.md index 32fed9a6324..24c27b9391e 100644 --- a/pkg/analysis_server/tool/lsp_spec/README.md +++ b/pkg/analysis_server/tool/lsp_spec/README.md @@ -60,17 +60,14 @@ Client workspace settings are requested with `workspace/configuration` during in Below is a list of LSP methods and their implementation status. - Method: The LSP method name -- Basic Impl: This method has an implementation but may assume some client capabilities -- Capabilities: Only types from the original spec or as advertised in client capabilities are returned -- Plugins: This functionality works with server plugins -- Tests: Has automated tests -- Tested Client: Has been manually tested in at least one LSP client editor +- Server: The method is supported by the Dart server. +- Plugins: This functionality can be extended by third party analyzer plugins. | Method | Server | Plugins | Notes | | - | - | - | - | -| initialize | ✅ | N/A | trace and other options NYI| +| initialize | ✅ | N/A | trace and other options NYI | | initialized | ✅ | N/A | | -| shutdown | ✅ | N/A | supported but does nothing| +| shutdown | ✅ | N/A | supported but does nothing | | exit | ✅ | N/A | | | $/cancelRequest | ✅ | | | | $/logTrace | | | | @@ -80,9 +77,9 @@ Below is a list of LSP methods and their implementation status. | client/unregisterCapability | ✅ | ✅ | | | notebookDocument/* | | | | | telemetry/event | | | | -| textDocument/codeAction (assists) | ✅ | ✅ | Only if the client advertises `codeActionLiteralSupport` with `Refactor`| +| textDocument/codeAction (assists) | ✅ | ✅ | Only if the client advertises `codeActionLiteralSupport` with `Refactor` | | textDocument/codeAction (fixAll) | ✅ | | | -| textDocument/codeAction (fixes) | ✅ | ✅ | Only if the client advertises `codeActionLiteralSupport` with `QuickFix`| +| textDocument/codeAction (fixes) | ✅ | ✅ | Only if the client advertises `codeActionLiteralSupport` with `QuickFix` | | textDocument/codeAction (organiseImports) | ✅ | | | | textDocument/codeAction (refactors) | ✅ | | | | textDocument/codeAction (sortMembers) | ✅ | | | @@ -146,7 +143,7 @@ Below is a list of LSP methods and their implementation status. | workspace/diagnostic | | | | | workspace/diagnostic/refresh | | | | | workspace/didChangeConfiguration | ✅ | | | -| workspace/didChangeWatchedFiles | | | unused, server does own watching| +| workspace/didChangeWatchedFiles | | | unused, server does own watching | | workspace/didChangeWorkspaceFolders | ✅ | ✅ | | | workspace/didCreateFiles | | | | | workspace/didDeleteFiles | | | |