Version 3.6.0-82.0.dev

Merge 867611b into dev

pkg/analysis_server/lib/src/lsp/handlers/handler_inlay_hint.dart

@@ -8,13 +8,13 @@ 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/lsp/registration/feature_registration.dart';
-import 'package:analyzer/dart/analysis/results.dart';
 
 typedef StaticOptions
     = Either3<bool, InlayHintOptions, InlayHintRegistrationOptions>;
 
 class InlayHintHandler
-    extends LspMessageHandler<InlayHintParams, List<InlayHint>> {
+    extends LspMessageHandler<InlayHintParams, List<InlayHint>>
+    with LspHandlerHelperMixin {
   InlayHintHandler(super.server);
   @override
   Method get handlesMessage => Method.textDocument_inlayHint;
@@ -26,34 +26,38 @@ class InlayHintHandler
   @override
   Future<ErrorOr<List<InlayHint>>> handle(InlayHintParams params,
       MessageInfo message, CancellationToken token) async {
-    if (!isDartDocument(params.textDocument)) {
+    var textDocument = params.textDocument;
+    if (!isDartDocument(textDocument)) {
       return success([]);
     }
 
-    var path = pathOfDoc(params.textDocument);
+    var path = pathOfDoc(textDocument);
+    return path.mapResult((path) async {
+      // Capture the document version so we can verify it hasn't changed after
+      // we've got a resolved unit (which is async and may wait for context
+      // rebuilds).
+      var docIdentifier = extractDocumentVersion(textDocument, path);
 
-    // It's particularly important to provide results consistent with the
-    // document in the client in this handler to avoid inlay hints "jumping
-    // around" while the user types, so ensure no other requests (content
-    // updates) are processed while we do async work to get the resolved unit.
-    late ErrorOr<ResolvedUnitResult> result;
-    await server.lockRequestsWhile(() async {
-      result = await path.mapResult(requireResolvedUnit);
-    });
+      var result = await requireResolvedUnit(path);
 
-    if (token.isCancellationRequested) {
-      return cancelled();
-    }
+      if (fileHasBeenModified(path, docIdentifier.version)) {
+        return fileModifiedError;
+      }
 
-    return result.mapResult((result) async {
-      if (!result.exists) {
-        return success([]);
+      if (token.isCancellationRequested) {
+        return cancelled();
       }
 
-      var computer = DartInlayHintComputer(pathContext, result);
-      var hints = computer.compute();
+      return result.mapResult((result) async {
+        if (!result.exists) {
+          return success([]);
+        }
+
+        var computer = DartInlayHintComputer(pathContext, result);
+        var hints = computer.compute();
 
-      return success(hints);
+        return success(hints);
+      });
     });
   }
 }

pkg/analysis_server/lib/src/lsp/handlers/handler_rename.dart

@@ -127,18 +127,10 @@ class RenameHandler extends LspMessageHandler<RenameParams, WorkspaceEdit?>
     var pos = params.position;
     var textDocument = params.textDocument;
     var path = pathOfDoc(params.textDocument);
-    // If the client provided us a version doc identifier, we'll use it to ensure
-    // we're not computing a rename for an old document. If not, we'll just assume
-    // the version the server had at the time of receiving the request is valid
-    // and then use it to verify the document hadn't changed again before we
-    // send the edits.
-    var docIdentifier = path.mapResultSync((path) => success(
-        textDocument is OptionalVersionedTextDocumentIdentifier
-            ? textDocument
-            : textDocument is VersionedTextDocumentIdentifier
-                ? OptionalVersionedTextDocumentIdentifier(
-                    uri: textDocument.uri, version: textDocument.version)
-                : server.getVersionedDocumentIdentifier(path)));
+    // Capture the document version so we can verify it hasn't changed after
+    // we've computed the rename.
+    var docIdentifier = path.mapResultSync(
+        (path) => success(extractDocumentVersion(textDocument, path)));
 
     var unit = await path.mapResult(requireResolvedUnit);
     var offset = unit.mapResultSync((unit) => toOffset(unit.lineInfo, pos));

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

@@ -264,6 +264,22 @@ mixin HandlerHelperMixin<S extends AnalysisServer> {
 mixin LspHandlerHelperMixin {
   LspAnalysisServer get server;
 
+  /// Extracts the current document version from [textDocument] if available,
+  /// or uses the version that the server has via
+  /// [LspAnalysisServer.getVersionedDocumentIdentifier].
+  OptionalVersionedTextDocumentIdentifier extractDocumentVersion(
+    TextDocumentIdentifier textDocument,
+    String path,
+  ) {
+    return switch (textDocument) {
+      OptionalVersionedTextDocumentIdentifier() => textDocument,
+      VersionedTextDocumentIdentifier() =>
+        OptionalVersionedTextDocumentIdentifier(
+            uri: textDocument.uri, version: textDocument.version),
+      _ => server.getVersionedDocumentIdentifier(path),
+    };
+  }
+
   bool fileHasBeenModified(String path, int? clientVersion) {
     var serverDocumentVersion = server.getDocumentVersion(path);
     return clientVersion != null && clientVersion != serverDocumentVersion;

pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart

@@ -357,14 +357,22 @@ class LspAnalysisServer extends AnalysisServer {
   @override
   int? getDocumentVersion(String path) => documentVersions[path]?.version;
 
-  /// Gets the version of a document known to the server, returning a
-  /// [OptionalVersionedTextDocumentIdentifier] with a version of `null` if the
-  /// document version is not known.
+  /// Gets the current identifier/version of a document known to the server,
+  /// returning an [OptionalVersionedTextDocumentIdentifier] with a version of
+  /// `null` if the document version is not known.
+  ///
+  /// Prefer using [LspHandlerHelperMixin.extractDocumentVersion] when you
+  /// already have a [TextDocumentIdentifier] from the client because it is
+  /// guaranteed to be what the client expected and not just the current version
+  /// the server has.
   @override
   OptionalVersionedTextDocumentIdentifier getVersionedDocumentIdentifier(
-      String path) {
+    String path,
+  ) {
     return OptionalVersionedTextDocumentIdentifier(
-        uri: uriConverter.toClientUri(path), version: getDocumentVersion(path));
+      uri: uriConverter.toClientUri(path),
+      version: getDocumentVersion(path),
+    );
   }
 
   @override
@@ -531,6 +539,15 @@ class LspAnalysisServer extends AnalysisServer {
   /// operation, handles should generally check the cancellation flag
   /// immediately after this function returns.
   Future<T> lockRequestsWhile<T>(FutureOr<T> Function() operation) async {
+    // TODO(dantup): Prevent this method from locking responses from the client
+    //  because this can lead to deadlocks if called during initialization where
+    //  the server may wait for something (configuration) from the client. This
+    //  might fit in with potential upcoming scheduler changes.
+    //
+    // This is currently used by Completion+FixAll (which are less likely, but
+    // possible to be called during init).
+    //
+    // https://github.com/dart-lang/sdk/issues/56311#issuecomment-2250089185
     var completer = Completer<void>();
 
     // Pause handling incoming messages until `operation` completes.

pkg/analysis_server/test/lsp/code_actions_fixes_test.dart

@@ -34,7 +34,7 @@ class DeprecatedCamelCaseTypes extends LintRule {
   DeprecatedCamelCaseTypes()
       : super(
           name: 'camel_case_types',
-          categories: {Category.style},
+          categories: {LintRuleCategory.style},
           state: State.deprecated(),
           description: '',
           details: '',

pkg/analysis_server/test/lsp/inlay_hint_test.dart

@@ -594,6 +594,38 @@ final (Type:(int, {String a})) noComma3 = (1, a: '');
     await _expectHints(content, expected);
   }
 
+  /// During initialization the server asks the client for configuration.
+  /// Verify that if we send an inlay hint request before the client responds
+  /// to that, we do not fail to respond due to a deadlock.
+  ///
+  /// https://github.com/dart-lang/sdk/issues/56311
+  Future<void> test_requestBeforeInitializationConfigurationComplete() async {
+    var initializedComplete = Completer<void>();
+    var configResponse = Completer<Map<String, Object?>>();
+
+    // Initialize server (and wait for initialized), but don't respond to
+    // the config request yet.
+    unawaited(provideConfig(() async {
+      await initialize();
+      initializedComplete.complete();
+    }, configResponse.future));
+    await initializedComplete.future;
+
+    // Send a request for inlay hints (which will stall because server isn't
+    // ready until we've replied to config).
+    unawaited(openFile(mainFileUri, ''));
+    var hintsFuture = getInlayHints(
+      mainFileUri,
+      startOfDocRange,
+    );
+    await pumpEventQueue(times: 5000);
+
+    // Finally, respond to config and ensure inlay hints completes because
+    // the server completed initialization.
+    configResponse.complete({});
+    expect(await hintsFuture, isEmpty);
+  }
+
   Future<void> test_setter() async {
     var content = '''
 set f(int i) {}

pkg/analysis_server/test/src/services/completion/yaml/analysis_options_generator_test.dart

@@ -263,7 +263,7 @@ class InternalRule extends LintRule {
   InternalRule()
       : super(
           name: 'internal_lint',
-          categories: {Category.style},
+          categories: {LintRuleCategory.style},
           state: State.internal(),
           description: '',
           details: '',

pkg/analysis_server/test/src/services/correction/fix/analysis_options/remove_lint_test.dart

@@ -25,7 +25,7 @@ class DeprecatedRule extends LintRule {
           name: 'deprecated_rule',
           description: '',
           details: '...',
-          categories: {Category.errors},
+          categories: {LintRuleCategory.errors},
           state: State.deprecated(since: dart2_12),
         );
 
@@ -42,7 +42,7 @@ class RemovedRule extends LintRule {
             name: 'removed_rule',
             description: '',
             details: '...',
-            categories: {Category.errors},
+            categories: {LintRuleCategory.errors},
             state: State.removed());
 
   @override

pkg/analyzer/lib/src/lint/linter.dart

@@ -37,61 +37,6 @@ export 'package:analyzer/src/lint/linter_visitor.dart' show NodeLintRegistry;
 export 'package:analyzer/src/lint/state.dart'
     show dart2_12, dart3, dart3_3, State;
 
-abstract final class Category {
-  /// A category of rules that help to minimize binary size.
-  static const String binarySize = 'binary size';
-
-  /// A category of rules that encourage brevity in the source code.
-  static const String brevity = 'brevity';
-
-  /// A category of rules that help to maintain documentation comments.
-  static const String documentationCommentMaintenance =
-      'documentation comment maintenance';
-
-  /// A category of rules that align with the Effective Dart style guide.
-  static const String effectiveDart = 'effective dart';
-
-  /// A category representing possible coding errors.
-  // TODO(srawlins): Hopefully deprecate this (or just rename `error_prone`
-  // back to this one).
-  static const String errors = 'errors';
-
-  /// A category of rules that protect against error-prone code.
-  static const String errorProne = 'error-prone';
-
-  /// A category of rules that help to write Flutter code.
-  static const String flutter = 'flutter';
-
-  /// A category of rules that promote language feature usage.
-  static const String languageFeatureUsage = 'language feature usage';
-
-  /// A category of rules that protect against possibly memory-leaking code.
-  static const String memoryLeaks = 'memory leaks';
-
-  /// A category of rules that protect against non-performant code.
-  static const String nonPerformant = 'non-performant';
-
-  /// A category representing Pub-related rules.
-  static const String pub = 'pub';
-
-  /// A category of rules that promote a healthy public interface.
-  static const String publicInterface = 'public interface';
-
-  /// A category representing matters of style, largely derived from Effective
-  /// Dart.
-  static const String style = 'style';
-
-  /// A category of rules that protect against code that probably doesn't do
-  /// what you think it does, or that shouldn't be used as it is.
-  static const String unintentional = 'unintentional';
-
-  /// A category of rules that protect against unused code.
-  static const String unusedCode = 'unused code';
-
-  /// A category of rules that help to write code deployed to the web.
-  static const String web = 'web';
-}
-
 /// The result of attempting to evaluate an expression as a constant.
 final class LinterConstantEvaluationResult {
   /// The value of the expression, or `null` if has [errors].
@@ -381,6 +326,61 @@ abstract class LintRule {
   }
 }
 
+abstract final class LintRuleCategory {
+  /// A category of rules that help to minimize binary size.
+  static const String binarySize = 'binary size';
+
+  /// A category of rules that encourage brevity in the source code.
+  static const String brevity = 'brevity';
+
+  /// A category of rules that help to maintain documentation comments.
+  static const String documentationCommentMaintenance =
+      'documentation comment maintenance';
+
+  /// A category of rules that align with the Effective Dart style guide.
+  static const String effectiveDart = 'effective dart';
+
+  /// A category representing possible coding errors.
+  // TODO(srawlins): Hopefully deprecate this (or just rename `error_prone`
+  // back to this one).
+  static const String errors = 'errors';
+
+  /// A category of rules that protect against error-prone code.
+  static const String errorProne = 'error-prone';
+
+  /// A category of rules that help to write Flutter code.
+  static const String flutter = 'flutter';
+
+  /// A category of rules that promote language feature usage.
+  static const String languageFeatureUsage = 'language feature usage';
+
+  /// A category of rules that protect against possibly memory-leaking code.
+  static const String memoryLeaks = 'memory leaks';
+
+  /// A category of rules that protect against non-performant code.
+  static const String nonPerformant = 'non-performant';
+
+  /// A category representing Pub-related rules.
+  static const String pub = 'pub';
+
+  /// A category of rules that promote a healthy public interface.
+  static const String publicInterface = 'public interface';
+
+  /// A category representing matters of style, largely derived from Effective
+  /// Dart.
+  static const String style = 'style';
+
+  /// A category of rules that protect against code that probably doesn't do
+  /// what you think it does, or that shouldn't be used as it is.
+  static const String unintentional = 'unintentional';
+
+  /// A category of rules that protect against unused code.
+  static const String unusedCode = 'unused code';
+
+  /// A category of rules that help to write code deployed to the web.
+  static const String web = 'web';
+}
+
 /// Provides access to information needed by lint rules that is not available
 /// from AST nodes or the element model.
 class LintRuleUnitContext {

pkg/analyzer/test/src/diagnostics/removed_lint_use_test.dart

@@ -19,7 +19,7 @@ class RemovedLint extends LintRule {
   RemovedLint()
       : super(
           name: 'removed_lint',
-          categories: {Category.style},
+          categories: {LintRuleCategory.style},
           state: State.removed(since: dart3),
           description: '',
           details: '',

pkg/analyzer/test/src/diagnostics/replaced_lint_use_test.dart

@@ -19,7 +19,7 @@ class RemovedLint extends LintRule {
   RemovedLint()
       : super(
           name: 'removed_lint',
-          categories: {Category.style},
+          categories: {LintRuleCategory.style},
           state: State.removed(since: dart3, replacedBy: 'replacing_lint'),
           description: '',
           details: '',
@@ -65,7 +65,7 @@ class ReplacingLint extends LintRule {
   ReplacingLint()
       : super(
           name: 'replacing_lint',
-          categories: {Category.style},
+          categories: {LintRuleCategory.style},
           state: State.removed(since: dart3),
           description: '',
           details: '',

pkg/analyzer/test/src/diagnostics/unignorable_ignore_test.dart

@@ -80,7 +80,7 @@ class _AvoidIntRule extends LintRule {
           name: 'avoid_int',
           description: '',
           details: '',
-          categories: {Category.errors},
+          categories: {LintRuleCategory.errors},
         );
 
   @override