From 4a714b27d60c461c17c9b6f4b63ebc93dc9d543d Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 2 Aug 2021 14:33:09 +0000 Subject: [PATCH] Reland "[analyzer] Use "pub outdated" to get version numbers for pubspec completion" This is a reland of 9cbc497d0ada365617f29fd923ad11066ea5a4ea Original change's description: > [analyzer] Use "pub outdated" to get version numbers for pubspec completion > > Change-Id: Ic8ef9514946070d590fc4594db4d8474912b40ff > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/207760 > Reviewed-by: Brian Wilkerson > Commit-Queue: Brian Wilkerson Change-Id: I8acd224c7982312c48039a1cb439323e040b3859 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208324 Reviewed-by: Brian Wilkerson Commit-Queue: Brian Wilkerson --- .../lib/src/analysis_server.dart | 23 ++- .../lib/src/analysis_server_abstract.dart | 18 +- .../lib/src/context_manager.dart | 23 ++- .../lib/src/lsp/lsp_analysis_server.dart | 23 ++- pkg/analysis_server/lib/src/lsp/mapping.dart | 28 ++- .../services/completion/yaml/producer.dart | 6 +- .../completion/yaml/pubspec_generator.dart | 29 +-- .../lib/src/services/pub/pub_command.dart | 149 +++++++++++++++ .../src/services/pub/pub_package_service.dart | 137 ++++++++++++-- .../lib/src/utilities/process.dart | 32 ++++ pkg/analysis_server/test/lsp/completion.dart | 12 +- .../test/lsp/completion_yaml_test.dart | 161 +++++++++++++++- .../test/lsp/pub_package_service_test.dart | 173 +++++++++++++++++- .../test/lsp/server_abstract.dart | 5 +- pkg/analysis_server/test/mocks.dart | 29 +++ .../yaml/pubspec_generator_test.dart | 50 ++++- 16 files changed, 848 insertions(+), 50 deletions(-) create mode 100644 pkg/analysis_server/lib/src/services/pub/pub_command.dart create mode 100644 pkg/analysis_server/lib/src/utilities/process.dart diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart index e6b028ba87f..389690580a5 100644 --- a/pkg/analysis_server/lib/src/analysis_server.dart +++ b/pkg/analysis_server/lib/src/analysis_server.dart @@ -40,6 +40,7 @@ import 'package:analysis_server/src/server/error_notifier.dart'; import 'package:analysis_server/src/server/features.dart'; import 'package:analysis_server/src/server/sdk_configuration.dart'; import 'package:analysis_server/src/services/flutter/widget_descriptions.dart'; +import 'package:analysis_server/src/utilities/process.dart'; import 'package:analysis_server/src/utilities/request_statistics.dart'; import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/dart/ast/ast.dart'; @@ -130,6 +131,7 @@ class AnalysisServer extends AbstractAnalysisServer { CrashReportingAttachmentsBuilder crashReportingAttachmentsBuilder, InstrumentationService instrumentationService, { http.Client? httpClient, + ProcessRunner? processRunner, RequestStatisticsHelper? requestStatistics, DiagnosticServer? diagnosticServer, this.detachableFileSystemManager, @@ -143,6 +145,7 @@ class AnalysisServer extends AbstractAnalysisServer { baseResourceProvider, instrumentationService, httpClient, + processRunner, NotificationManager(channel, baseResourceProvider.pathContext), requestStatistics: requestStatistics, enableBazelWatcher: enableBazelWatcher, @@ -429,9 +432,11 @@ class AnalysisServer extends AbstractAnalysisServer { bool isPubspec(String filePath) => file_paths.isPubspecYaml(resourceProvider.pathContext, filePath); - // When a pubspec is opened, trigger package name caching for completion. - if (!pubPackageService.isRunning && files.any(isPubspec)) { - pubPackageService.beginPackageNamePreload(); + // When pubspecs are opened, trigger pre-loading of pub package names and + // versions. + final pubspecs = files.where(isPubspec).toList(); + if (pubspecs.isNotEmpty) { + pubPackageService.beginCachePreloads(pubspecs); } priorityFiles.clear(); @@ -687,6 +692,18 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks { analysisDriver.priorityFiles = analysisServer.priorityFiles.toList(); } + @override + void pubspecChanged(String pubspecPath) { + analysisServer.pubPackageService.fetchPackageVersionsViaPubOutdated( + pubspecPath, + pubspecWasModified: true); + } + + @override + void pubspecRemoved(String pubspecPath) { + analysisServer.pubPackageService.flushPackageCaches(pubspecPath); + } + @override void recordAnalysisErrors(String path, List errors) { filesToFlush.add(path); diff --git a/pkg/analysis_server/lib/src/analysis_server_abstract.dart b/pkg/analysis_server/lib/src/analysis_server_abstract.dart index 6a351b11249..4b28f5cc073 100644 --- a/pkg/analysis_server/lib/src/analysis_server_abstract.dart +++ b/pkg/analysis_server/lib/src/analysis_server_abstract.dart @@ -19,12 +19,14 @@ import 'package:analysis_server/src/services/completion/dart/documentation_cache import 'package:analysis_server/src/services/completion/dart/extension_cache.dart'; import 'package:analysis_server/src/services/correction/namespace.dart'; import 'package:analysis_server/src/services/pub/pub_api.dart'; +import 'package:analysis_server/src/services/pub/pub_command.dart'; import 'package:analysis_server/src/services/pub/pub_package_service.dart'; import 'package:analysis_server/src/services/search/element_visitors.dart'; import 'package:analysis_server/src/services/search/search_engine.dart'; import 'package:analysis_server/src/services/search/search_engine_internal.dart'; import 'package:analysis_server/src/utilities/file_string_sink.dart'; import 'package:analysis_server/src/utilities/null_string_sink.dart'; +import 'package:analysis_server/src/utilities/process.dart'; import 'package:analysis_server/src/utilities/request_statistics.dart'; import 'package:analysis_server/src/utilities/tee_string_sink.dart'; import 'package:analyzer/dart/analysis/analysis_context.dart'; @@ -152,14 +154,26 @@ abstract class AbstractAnalysisServer { ResourceProvider baseResourceProvider, this.instrumentationService, http.Client? httpClient, + ProcessRunner? processRunner, this.notificationManager, { this.requestStatistics, bool enableBazelWatcher = false, }) : resourceProvider = OverlayResourceProvider(baseResourceProvider), pubApi = PubApi(instrumentationService, httpClient, Platform.environment['PUB_HOSTED_URL']) { - pubPackageService = - PubPackageService(instrumentationService, baseResourceProvider, pubApi); + // We can only spawn processes (eg. to run pub commands) when backed by + // a real file system, otherwise we may try to run commands in folders that + // don't really exist. If processRunner was supplied, it's likely a mock + // from a test in which case the pub command should still be created. + if (baseResourceProvider is PhysicalResourceProvider) { + processRunner ??= ProcessRunner(); + } + final pubCommand = processRunner != null + ? PubCommand(instrumentationService, processRunner) + : null; + + pubPackageService = PubPackageService( + instrumentationService, baseResourceProvider, pubApi, pubCommand); performance = performanceDuringStartup; pluginManager = PluginManager( diff --git a/pkg/analysis_server/lib/src/context_manager.dart b/pkg/analysis_server/lib/src/context_manager.dart index 721f37cbeab..28509f0f274 100644 --- a/pkg/analysis_server/lib/src/context_manager.dart +++ b/pkg/analysis_server/lib/src/context_manager.dart @@ -122,6 +122,12 @@ abstract class ContextManagerCallbacks { /// TODO(scheglov) Just pass results in here? void listenAnalysisDriver(AnalysisDriver driver); + /// The `pubspec.yaml` at [path] was added/modified. + void pubspecChanged(String path); + + /// The `pubspec.yaml` at [path] was removed. + void pubspecRemoved(String path); + /// Record error information for the file with the given [path]. void recordAnalysisErrors(String path, List errors); } @@ -566,12 +572,21 @@ class ContextManagerImpl implements ContextManager { _instrumentationService.logWatchEvent('', path, type.toString()); + final isPubpsec = file_paths.isPubspecYaml(pathContext, path); if (file_paths.isAnalysisOptionsYaml(pathContext, path) || file_paths.isDotPackages(pathContext, path) || file_paths.isPackageConfigJson(pathContext, path) || - file_paths.isPubspecYaml(pathContext, path) || + isPubpsec || false) { _createAnalysisContexts(); + + if (isPubpsec) { + if (type == ChangeType.REMOVE) { + callbacks.pubspecRemoved(path); + } else { + callbacks.pubspecChanged(path); + } + } return; } @@ -726,6 +741,12 @@ class NoopContextManagerCallbacks implements ContextManagerCallbacks { @override void listenAnalysisDriver(AnalysisDriver driver) {} + @override + void pubspecChanged(String pubspecPath) {} + + @override + void pubspecRemoved(String pubspecPath) {} + @override void recordAnalysisErrors(String path, List errors) {} } diff --git a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart index cac56654c96..2dcd219a992 100644 --- a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart +++ b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart @@ -35,6 +35,7 @@ import 'package:analysis_server/src/server/error_notifier.dart'; import 'package:analysis_server/src/services/completion/completion_performance.dart' show CompletionPerformance; import 'package:analysis_server/src/services/refactoring/refactoring.dart'; +import 'package:analysis_server/src/utilities/process.dart'; import 'package:analyzer/dart/analysis/context_locator.dart'; import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/error/error.dart'; @@ -126,6 +127,7 @@ class LspAnalysisServer extends AbstractAnalysisServer { CrashReportingAttachmentsBuilder crashReportingAttachmentsBuilder, InstrumentationService instrumentationService, { http.Client? httpClient, + ProcessRunner? processRunner, DiagnosticServer? diagnosticServer, // Disable to avoid using this in unit tests. bool enableBazelWatcher = false, @@ -137,6 +139,7 @@ class LspAnalysisServer extends AbstractAnalysisServer { baseResourceProvider, instrumentationService, httpClient, + processRunner, LspNotificationManager(channel, baseResourceProvider.pathContext), enableBazelWatcher: enableBazelWatcher, ) { @@ -184,10 +187,10 @@ class LspAnalysisServer extends AbstractAnalysisServer { RefactoringWorkspace(driverMap.values, searchEngine); void addPriorityFile(String filePath) { - // When a pubspec is opened, trigger package name caching for completion. - if (!pubPackageService.isRunning && - file_paths.isPubspecYaml(resourceProvider.pathContext, filePath)) { - pubPackageService.beginPackageNamePreload(); + // When pubspecs are opened, trigger pre-loading of pub package names and + // versions. + if (file_paths.isPubspecYaml(resourceProvider.pathContext, filePath)) { + pubPackageService.beginCachePreloads([filePath]); } final didAdd = priorityFiles.add(filePath); @@ -851,6 +854,18 @@ class LspServerContextManagerCallbacks extends ContextManagerCallbacks { analysisDriver.priorityFiles = analysisServer.priorityFiles.toList(); } + @override + void pubspecChanged(String pubspecPath) { + analysisServer.pubPackageService.fetchPackageVersionsViaPubOutdated( + pubspecPath, + pubspecWasModified: true); + } + + @override + void pubspecRemoved(String pubspecPath) { + analysisServer.pubPackageService.flushPackageCaches(pubspecPath); + } + @override void recordAnalysisErrors(String path, List errors) { final errorsToSend = errors.where(_shouldSendError).toList(); diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart index 03be64d408b..da9bb2a89fa 100644 --- a/pkg/analysis_server/lib/src/lsp/mapping.dart +++ b/pkg/analysis_server/lib/src/lsp/mapping.dart @@ -49,6 +49,15 @@ final diagnosticTagsForErrorCode = >{ ], }; +/// Pattern for docComplete text on completion items that can be upgraded to +/// the "detail" field so that it can be shown more prominently by clients. +/// +/// This is typically used for labels like _latest compatible_ and _latest_ in +/// the pubspec version items. These go into docComplete so that they appear +/// reasonably for non-LSP clients where there is no equivalent of the detail +/// field. +final _upgradableDocCompletePattern = RegExp(r'^_([\w ]{0,20})_$'); + lsp.Either2 asStringOrMarkupContent( Set? preferredFormats, String content) { return preferredFormats == null @@ -967,7 +976,21 @@ lsp.CompletionItem toCompletionItem( final insertText = insertTextInfo.first; final insertTextFormat = insertTextInfo.last; final isMultilineCompletion = insertText.contains('\n'); - final cleanedDoc = cleanDartdoc(suggestion.docComplete); + + var cleanedDoc = cleanDartdoc(suggestion.docComplete); + var detail = getCompletionDetail(suggestion, completionKind, + supportsCompletionDeprecatedFlag || supportsDeprecatedTag); + + // To improve the display of some items (like pubspec version numbers), + // short labels in the format `_foo_` in docComplete are "upgraded" to the + // detail field. + final labelMatch = cleanedDoc != null + ? _upgradableDocCompletePattern.firstMatch(cleanedDoc) + : null; + if (labelMatch != null) { + cleanedDoc = null; + detail = labelMatch.group(1); + } // Because we potentially send thousands of these items, we should minimise // the generated JSON as much as possible - for example using nulls in place @@ -982,8 +1005,7 @@ lsp.CompletionItem toCompletionItem( commitCharacters: includeCommitCharacters ? dartCompletionCommitCharacters : null, data: resolutionData, - detail: getCompletionDetail(suggestion, completionKind, - supportsCompletionDeprecatedFlag || supportsDeprecatedTag), + detail: detail, documentation: cleanedDoc != null ? asStringOrMarkupContent(formats, cleanedDoc) : null, diff --git a/pkg/analysis_server/lib/src/services/completion/yaml/producer.dart b/pkg/analysis_server/lib/src/services/completion/yaml/producer.dart index 960346f9230..5d7f6813416 100644 --- a/pkg/analysis_server/lib/src/services/completion/yaml/producer.dart +++ b/pkg/analysis_server/lib/src/services/completion/yaml/producer.dart @@ -176,9 +176,11 @@ abstract class Producer { const Producer(); /// A utility method used to create a suggestion for the [identifier]. - CompletionSuggestion identifier(String identifier, {int relevance = 1000}) => + CompletionSuggestion identifier(String identifier, + {int relevance = 1000, String? docComplete}) => CompletionSuggestion(CompletionSuggestionKind.IDENTIFIER, relevance, - identifier, identifier.length, 0, false, false); + identifier, identifier.length, 0, false, false, + docComplete: docComplete); /// A utility method used to create a suggestion for the package [packageName]. CompletionSuggestion packageName(String packageName, diff --git a/pkg/analysis_server/lib/src/services/completion/yaml/pubspec_generator.dart b/pkg/analysis_server/lib/src/services/completion/yaml/pubspec_generator.dart index 724bb6fde77..a34d7913f7e 100644 --- a/pkg/analysis_server/lib/src/services/completion/yaml/pubspec_generator.dart +++ b/pkg/analysis_server/lib/src/services/completion/yaml/pubspec_generator.dart @@ -37,18 +37,25 @@ class PubPackageVersionProducer extends Producer { @override Iterable suggestions( YamlCompletionRequest request) sync* { - // TOOD(dantup): Consider supporting async completion requests so this - // could call packageDetails() (with a short timeout, and pub retries - // disabled). A user that explicitly invokes completion in the location - // of a version may be prepared to wait a short period for a web request - // to get completion versions (this is also the only way for non-LSP - // clients to get them, since there are no resolve calls). + final versions = request.pubPackageService + ?.cachedPubOutdatedVersions(request.filePath, package); + final resolvable = versions?.resolvableVersion; + var latest = versions?.latestVersion; + + // If we didn't get a latest version from the "pub outdated" results, we can + // use the result from the Pub API if we've called it (this will usually + // only be the case for LSP where a resolve() call was sent). // - // Supporting this will require making the completion async further up. - final details = request.pubPackageService?.cachedPackageDetails(package); - final version = details?.latestVersion; - if (version != null) { - yield identifier('^$version'); + // This allows us (in some cases) to still show version numbers even if the + // package was newly added to pubspec and not saved, so not yet in the + // "pub outdated" results. + latest ??= request.pubPackageService?.cachedPubApiLatestVersion(package); + + if (resolvable != null && resolvable != latest) { + yield identifier('^$resolvable', docComplete: '_latest compatible_'); + } + if (latest != null) { + yield identifier('^$latest', docComplete: '_latest_'); } } } diff --git a/pkg/analysis_server/lib/src/services/pub/pub_command.dart b/pkg/analysis_server/lib/src/services/pub/pub_command.dart new file mode 100644 index 00000000000..a649d6549c8 --- /dev/null +++ b/pkg/analysis_server/lib/src/services/pub/pub_command.dart @@ -0,0 +1,149 @@ +// Copyright (c) 2021, 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 'dart:async'; +import 'dart:convert'; +import 'dart:io'; + +import 'package:analysis_server/src/utilities/process.dart'; +import 'package:analyzer/instrumentation/service.dart'; +import 'package:path/path.dart' as path; + +/// A class for interacting with the `pub` command. +/// +/// `pub` commands will be queued and not run concurrently. +class PubCommand { + static const String _pubEnvironmentKey = 'PUB_ENVIRONMENT'; + final InstrumentationService _instrumentationService; + late final ProcessRunner _processRunner; + late final String _pubPath; + late final String _pubEnvironmentValue; + + /// Tracks the last queued command to avoid overlapping because pub does not + /// do its own locking when accessing the cache. + /// + /// https://github.com/dart-lang/pub/issues/1178 + /// + /// This does not prevent running concurrently with commands spawned by other + /// tools (such as the IDE). + var _lastQueuedCommand = Future.value(); + + PubCommand(this._instrumentationService, this._processRunner) { + _pubPath = path.join( + path.dirname(Platform.resolvedExecutable), + Platform.isWindows ? 'pub.bat' : 'pub', + ); + + // When calling the `pub` command, we must add an identifier to the + // PUB_ENVIRONMENT environment variable (joined with colons). + const _pubEnvString = 'analysis_server.pub_api'; + final existingPubEnv = Platform.environment[_pubEnvironmentKey]; + _pubEnvironmentValue = [ + if (existingPubEnv?.isNotEmpty ?? false) existingPubEnv, + _pubEnvString, + ].join(':'); + } + + /// Runs `pub outdated --show-all` and returns the results. + /// + /// If any error occurs executing the command, returns an empty list. + Future> outdatedVersions( + String pubspecPath) async { + final packageDirectory = path.dirname(pubspecPath); + final result = await _runPubJsonCommand( + ['outdated', '--show-all', '--json'], + workingDirectory: packageDirectory); + + if (result == null) { + return []; + } + + final packages = + (result['packages'] as List?)?.cast>(); + if (packages == null) { + return []; + } + + return packages + .map( + (json) => PubOutdatedPackageDetails( + json['package'] as String, + currentVersion: _version(json, 'current'), + latestVersion: _version(json, 'latest'), + resolvableVersion: _version(json, 'resolvable'), + upgradableVersion: _version(json, 'upgradable'), + ), + ) + .toList(); + } + + /// Runs a pub command and decodes JSON from `stdout`. + /// + /// Returns null if: + /// - exit code is non-zero + /// - returned text cannot be decoded as JSON + Future?> _runPubJsonCommand(List args, + {required String workingDirectory}) async { + // Atomically replace the lastQueuedCommand future with our own to ensure + // only one command waits on any previous commands future. + final completer = Completer(); + final lastCommand = _lastQueuedCommand; + _lastQueuedCommand = completer.future; + // And wait for that previous command to finish. + await lastCommand.catchError((_) {}); + + try { + final command = [_pubPath, ...args]; + + _instrumentationService.logInfo('Running pub command $command'); + final result = await _processRunner.run(_pubPath, args, + workingDirectory: workingDirectory, + environment: {_pubEnvironmentKey: _pubEnvironmentValue}); + + if (result.exitCode != 0) { + _instrumentationService.logError( + 'pub command returned ${result.exitCode} exit code: ${result.stderr}.'); + return null; + } + + try { + final results = jsonDecode(result.stdout); + _instrumentationService.logInfo('pub command completed successfully'); + return results; + } catch (e) { + _instrumentationService + .logError('pub command returned invalid JSON: $e.'); + return null; + } + } catch (e) { + _instrumentationService.logError('pub command failed to run: $e.'); + return null; + } finally { + completer.complete(); + } + } + + String? _version(Map json, String type) { + final versionType = json[type] as Map?; + final version = + versionType != null ? versionType['version'] as String? : null; + return version; + } +} + +class PubOutdatedPackageDetails { + final String packageName; + final String? currentVersion; + final String? latestVersion; + final String? resolvableVersion; + final String? upgradableVersion; + + PubOutdatedPackageDetails( + this.packageName, { + required this.currentVersion, + required this.latestVersion, + required this.resolvableVersion, + required this.upgradableVersion, + }); +} diff --git a/pkg/analysis_server/lib/src/services/pub/pub_package_service.dart b/pkg/analysis_server/lib/src/services/pub/pub_package_service.dart index d0990db67fe..4b6ef3a847b 100644 --- a/pkg/analysis_server/lib/src/services/pub/pub_package_service.dart +++ b/pkg/analysis_server/lib/src/services/pub/pub_package_service.dart @@ -6,10 +6,11 @@ import 'dart:async'; import 'dart:convert'; import 'package:analysis_server/src/services/pub/pub_api.dart'; +import 'package:analysis_server/src/services/pub/pub_command.dart'; import 'package:analyzer/file_system/file_system.dart'; -import 'package:analyzer/file_system/physical_file_system.dart'; import 'package:analyzer/instrumentation/service.dart'; import 'package:meta/meta.dart'; +import 'package:path/path.dart' as path; /// Information about Pub packages that can be converted to/from JSON and /// cached to disk. @@ -130,16 +131,26 @@ class PubPackage { /// A service for providing Pub package information. /// -/// Uses a [PubApi] to communicate with Pub and caches to disk using [cacheResourceProvider]. +/// Uses a [PubApi] to communicate with the Pub API and a [PubCommand] to +/// interact with the local `pub` command. +/// +/// Expensive results are cached to disk using [resourceProvider]. class PubPackageService { final InstrumentationService _instrumentationService; final PubApi _api; + + /// A wrapper over the "pub" command line too. + /// + /// This can be null when not running on a real file system because it may + /// try to interact with folders that don't really exist. + final PubCommand? _command; + Timer? _nextPackageNameListRequestTimer; Timer? _nextWriteDiskCacheTimer; - /// [ResourceProvider] used for caching. This should generally be a - /// [PhysicalResourceProvider] outside of tests. - final ResourceProvider cacheResourceProvider; + /// [ResourceProvider] used for accessing the disk for caches and checking + /// project types. This will be a [PhysicalResourceProvider] outside of tests. + final ResourceProvider resourceProvider; /// The current cache of package information. Initially `null`, but /// overwritten after first read of cache from disk or fetch from the API. @@ -148,25 +159,50 @@ class PubPackageService { int _packageDetailsRequestsInFlight = 0; - PubPackageService( - this._instrumentationService, this.cacheResourceProvider, this._api); + /// A cache of version numbers from running the "pub outdated" command used + /// for completion in pubspec.yaml. + final _pubspecPackageVersions = + >{}; - /// Gets the last set of package results or an empty List if no results. + PubPackageService(this._instrumentationService, this.resourceProvider, + this._api, this._command); + + /// Gets the last set of package results from the Pub API or an empty List if + /// no results. + /// + /// This data is used for completion of package names in pubspec.yaml + /// and for clients that support lazy resolution of completion items may also + /// include their descriptions and/or version numbers. List get cachedPackages => packageCache?.packages.values.toList() ?? []; - bool get isRunning => _nextPackageNameListRequestTimer != null; + @visibleForTesting + bool get isPackageNamesTimerRunning => + _nextPackageNameListRequestTimer != null; @visibleForTesting File get packageCacheFile { - final cacheFolder = cacheResourceProvider + final cacheFolder = resourceProvider .getStateLocation('.pub-package-details-cache')! ..create(); return cacheFolder.getChildAssumingFile('packages.json'); } - /// Begin a request to pre-load the package name list. + /// Begins preloading caches for package names and pub versions. + void beginCachePreloads(List pubspecs) { + beginPackageNamePreload(); + for (final pubspec in pubspecs) { + fetchPackageVersionsViaPubOutdated(pubspec, pubspecWasModified: false); + } + } + + /// Begin a timer to pre-load and update the package name list if one has not + /// already been started. void beginPackageNamePreload() { + if (isPackageNamesTimerRunning) { + return; + } + // If first time, try to read from disk. var cache = packageCache; if (cache == null) { @@ -179,11 +215,69 @@ class PubPackageService { Timer(cache.cacheTimeRemaining, _fetchFromServer); } - /// Gets the cached package details for package [packageName]. + /// Gets the latest cached package version fetched from the Pub API for the + /// package [packageName]. + String? cachedPubApiLatestVersion(String packageName) => + packageCache?.packages[packageName]?.latestVersion; + + /// Gets the package versions cached using "pub outdated" for the package + /// [packageName] for the project using [pubspecPath]. /// - /// Returns null if no package details are cached. - PubPackage? cachedPackageDetails(String packageName) => - packageCache?.packages[packageName]; + /// Versions in here might only be available for packages that are in the + /// pubspec on disk. Newly-added packages in the overlay might not be + /// available. + PubOutdatedPackageDetails? cachedPubOutdatedVersions( + String pubspecPath, String packageName) { + final pubspecCache = _pubspecPackageVersions[pubspecPath]; + return pubspecCache != null ? pubspecCache[packageName] : null; + } + + /// Begin a request to pre-load package versions using the "pub outdated" + /// command. + /// + /// If [pubspecWasModified] is true, the command will always be run. Otherwise it + /// will only be run if data is not already cached. + Future fetchPackageVersionsViaPubOutdated(String pubspecPath, + {required bool pubspecWasModified}) async { + final pubCommand = _command; + if (pubCommand == null) { + return; + } + + // If we already have a cache for the file and it was not modified (only + // opened) we do not need to re-run the command. + if (!pubspecWasModified && + _pubspecPackageVersions.containsKey(pubspecPath)) { + return; + } + + // Check if this pubspec is inside a DEPS-managed folder, and if so + // just cache an empty set of results since Pub is not managing + // dependencies. + if (_hasAncestorDEPSFile(pubspecPath)) { + _pubspecPackageVersions.putIfAbsent(pubspecPath, () => {}); + return; + } + + final results = await pubCommand.outdatedVersions(pubspecPath); + final cache = _pubspecPackageVersions.putIfAbsent(pubspecPath, () => {}); + for (final package in results) { + // We use the versions from the "pub outdated" results but only cache them + // in-memory for this specific pubspec, as the resolved version may be + // restricted by constraints/dependencies in the pubspec. The "pub" + // command does caching of the JSON versions to make "pub outdated" fast. + cache[package.packageName] = package; + } + } + + /// Clears package caches for [pubspecPath]. + /// + /// Does not remove other caches that are not pubspec-specific (for example + /// the latest version pulled directly from the Pub API independant of + /// pubspec). + Future flushPackageCaches(String pubspecPath) async { + _pubspecPackageVersions.remove(pubspecPath); + } /// Gets package details for package [packageName]. /// @@ -267,6 +361,19 @@ class PubPackageService { } } + /// Checks whether there is a DEPS file in any folder walking up from the + /// pubspec at [pubspecPath]. + bool _hasAncestorDEPSFile(String pubspecPath) { + var folder = path.dirname(pubspecPath); + do { + if (resourceProvider.getFile(path.join(folder, 'DEPS')).exists) { + return true; + } + folder = path.dirname(folder); + } while (folder != path.dirname(folder)); + return false; + } + /// Writes the package cache to disk after /// [PackageDetailsCache._writeCacheDebounceDuration] has elapsed, restarting /// the timer each time this method is called. diff --git a/pkg/analysis_server/lib/src/utilities/process.dart b/pkg/analysis_server/lib/src/utilities/process.dart new file mode 100644 index 00000000000..70ea179ec77 --- /dev/null +++ b/pkg/analysis_server/lib/src/utilities/process.dart @@ -0,0 +1,32 @@ +// Copyright (c) 2021, 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 'dart:async'; +import 'dart:convert'; +import 'dart:io'; + +/// An abstraction over [Process] from 'dart:io' to allow mocking in tests. +class ProcessRunner { + Future run( + String executable, + List arguments, { + String? workingDirectory, + Map? environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding? stdoutEncoding = systemEncoding, + Encoding? stderrEncoding = systemEncoding, + }) async { + return Process.run( + executable, + arguments, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } +} diff --git a/pkg/analysis_server/test/lsp/completion.dart b/pkg/analysis_server/test/lsp/completion.dart index 75c85b172b1..6d70a264e99 100644 --- a/pkg/analysis_server/test/lsp/completion.dart +++ b/pkg/analysis_server/test/lsp/completion.dart @@ -8,6 +8,9 @@ import 'package:test/test.dart'; import 'server_abstract.dart'; mixin CompletionTestMixin on AbstractLspAnalysisServerTest { + /// The last set of completion results fetched. + List completionResults = []; + int sortTextSorter(CompletionItem item1, CompletionItem item2) => (item1.sortText ?? item1.label).compareTo(item2.sortText ?? item2.label); @@ -40,23 +43,24 @@ mixin CompletionTestMixin on AbstractLspAnalysisServerTest { if (openCloseFile) { await openFile(fileUri, withoutMarkers(content)); } - final res = await getCompletion(fileUri, positionFromMarker(content)); + completionResults = + await getCompletion(fileUri, positionFromMarker(content)); if (openCloseFile) { await closeFile(fileUri); } // Sort the completions by sortText and filter to those we expect, so the ordering // can be compared. - final sortedResults = res + final sortedResults = completionResults .where((r) => expectCompletions.contains(r.label)) .toList() - ..sort(sortTextSorter); + ..sort(sortTextSorter); expect(sortedResults.map((item) => item.label), equals(expectCompletions)); // Check the edits apply correctly. if (applyEditsFor != null) { - var item = res.singleWhere((c) => c.label == applyEditsFor); + var item = completionResults.singleWhere((c) => c.label == applyEditsFor); final insertFormat = item.insertTextFormat; if (resolve) { diff --git a/pkg/analysis_server/test/lsp/completion_yaml_test.dart b/pkg/analysis_server/test/lsp/completion_yaml_test.dart index 9d26755438a..52bf479e5da 100644 --- a/pkg/analysis_server/test/lsp/completion_yaml_test.dart +++ b/pkg/analysis_server/test/lsp/completion_yaml_test.dart @@ -2,6 +2,8 @@ // 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 'dart:io'; + import 'package:analysis_server/src/services/pub/pub_api.dart'; import 'package:http/http.dart'; import 'package:linter/src/rules.dart'; @@ -367,7 +369,7 @@ dependencies: ); } - Future test_package_version() async { + Future test_package_versions_fromApi() async { httpClient.sendHandler = (BaseRequest request) async { if (request.url.path.startsWith(PubApi.packageNameListPath)) { return Response(samplePackageList, 200); @@ -418,6 +420,163 @@ dependencies: ); } + Future test_package_versions_fromPubOutdated() async { + final json = r''' + { + "packages": [ + { + "package": "one", + "latest": { "version": "3.2.1" }, + "resolvable": { "version": "1.2.4" } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, json, ''); + + final content = ''' +name: foo +version: 1.0.0 + +dependencies: + one: ^'''; + + final expected = ''' +name: foo +version: 1.0.0 + +dependencies: + one: ^1.2.4'''; + + await initialize(); + await openFile(pubspecFileUri, withoutMarkers(content)); + await pumpEventQueue(times: 500); + + await verifyCompletions( + pubspecFileUri, + content, + expectCompletions: ['^1.2.4', '^3.2.1'], + applyEditsFor: '^1.2.4', + expectedContent: expected, + openCloseFile: false, + ); + } + + Future test_package_versions_fromPubOutdated_afterChange() async { + final initialJson = r''' + { + "packages": [ + { + "package": "one", + "latest": { "version": "3.2.1" }, + "resolvable": { "version": "1.2.3" } + } + ] + } + '''; + final updatedJson = r''' + { + "packages": [ + { + "package": "one", + "latest": { "version": "2.1.0" }, + "resolvable": { "version": "2.3.4" } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, initialJson, ''); + + final content = ''' +name: foo +version: 1.0.0 + +dependencies: + one: ^'''; + + final expected = ''' +name: foo +version: 1.0.0 + +dependencies: + one: ^2.3.4'''; + + newFile(pubspecFilePath, content: content); + await initialize(); + await openFile(pubspecFileUri, withoutMarkers(content)); + await pumpEventQueue(times: 500); + + // Modify the underlying file which should trigger an update of the + // cached data. + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, updatedJson, ''); + modifyFile(pubspecFilePath, '$content# trailing comment'); + await pumpEventQueue(times: 500); + + await verifyCompletions( + pubspecFileUri, + content, + expectCompletions: ['^2.3.4', '^2.1.0'], + applyEditsFor: '^2.3.4', + expectedContent: expected, + openCloseFile: false, + ); + + // Also veryify the detail fields were populated as expected. + expect( + completionResults.singleWhere((c) => c.label == '^2.3.4').detail, + equals('latest compatible'), + ); + expect( + completionResults.singleWhere((c) => c.label == '^2.1.0').detail, + equals('latest'), + ); + } + + Future test_package_versions_fromPubOutdated_afterDelete() async { + final initialJson = r''' + { + "packages": [ + { + "package": "one", + "latest": { "version": "3.2.1" }, + "resolvable": { "version": "1.2.3" } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, initialJson, ''); + + final content = ''' +name: foo +version: 1.0.0 + +dependencies: + one: ^'''; + + newFile(pubspecFilePath, content: content); + await initialize(); + await openFile(pubspecFileUri, withoutMarkers(content)); + await pumpEventQueue(times: 500); + + // Delete the underlying file which should trigger eviction of the cache. + deleteFile(pubspecFilePath); + await pumpEventQueue(times: 500); + + await verifyCompletions( + pubspecFileUri, + content, + expectCompletions: [], + openCloseFile: false, + ); + + // There should have been no version numbers. + expect(completionResults, isEmpty); + } + Future test_topLevel() async { final content = ''' version: 1.0.0 diff --git a/pkg/analysis_server/test/lsp/pub_package_service_test.dart b/pkg/analysis_server/test/lsp/pub_package_service_test.dart index b00f28540dc..c22a0f50ab2 100644 --- a/pkg/analysis_server/test/lsp/pub_package_service_test.dart +++ b/pkg/analysis_server/test/lsp/pub_package_service_test.dart @@ -2,10 +2,16 @@ // 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 'dart:io'; + import 'package:analysis_server/src/services/pub/pub_api.dart'; +import 'package:analysis_server/src/services/pub/pub_command.dart'; import 'package:analysis_server/src/services/pub/pub_package_service.dart'; import 'package:analyzer/instrumentation/service.dart'; +import 'package:analyzer/src/test_utilities/resource_provider_mixin.dart'; +import 'package:collection/collection.dart'; import 'package:http/http.dart'; +import 'package:path/path.dart' as path; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -15,6 +21,7 @@ import 'server_abstract.dart'; void main() { defineReflectiveSuite(() { defineReflectiveTests(PubApiTest); + defineReflectiveTests(PubCommandTest); defineReflectiveTests(PubPackageServiceTest); }); } @@ -77,6 +84,168 @@ class PubApiTest { } } +@reflectiveTest +class PubCommandTest with ResourceProviderMixin { + late MockProcessRunner processRunner; + late PubCommand pubCommand; + late String pubspecPath, pubspec2Path; + + void setUp() { + pubspecPath = convertPath('/home/project/pubspec.yaml'); + pubspec2Path = convertPath('/home/project2/pubspec.yaml'); + processRunner = MockProcessRunner(); + pubCommand = PubCommand(InstrumentationService.NULL_SERVICE, processRunner); + } + + Future test_doesNotRunConcurrently() async { + var isRunning = false; + processRunner.runHandler = (executable, args, {dir, env}) async { + expect(isRunning, isFalse, + reason: 'pub commands should not run concurrently'); + isRunning = true; + await pumpEventQueue(times: 500); + isRunning = false; + return ProcessResult(0, 0, '', ''); + }; + await Future.wait([ + pubCommand.outdatedVersions(pubspecPath), + pubCommand.outdatedVersions(pubspecPath), + ]); + } + + Future test_outdated_args() async { + processRunner.runHandler = (executable, args, {dir, env}) { + var expectedPubPath = path.join( + path.dirname(Platform.resolvedExecutable), + Platform.isWindows ? 'pub.bat' : 'pub', + ); + expect(executable, equals(expectedPubPath)); + expect( + args, + equals([ + 'outdated', + '--show-all', + '--json', + ])); + expect(dir, equals(convertPath('/home/project'))); + expect( + env!['PUB_ENVIRONMENT'], + anyOf(equals('analysis_server.pub_api'), + endsWith(':analysis_server.pub_api'))); + return ProcessResult(0, 0, '', ''); + }; + await pubCommand.outdatedVersions(pubspecPath); + } + + Future test_outdated_invalidJson() async { + processRunner.runHandler = (String executable, List args, + {dir, env}) => + ProcessResult(1, 0, 'NOT VALID JSON', ''); + final result = await pubCommand.outdatedVersions(pubspecPath); + expect(result, isEmpty); + } + + Future test_outdated_missingFields() async { + final validJson = r''' + { + "packages": [ + { + "package": "foo", + "current": { "version": "1.0.0" }, + "upgradable": { "version": "2.0.0" }, + "resolvable": { } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, validJson, ''); + final result = await pubCommand.outdatedVersions(pubspecPath); + expect(result, hasLength(1)); + final package = result.first; + expect(package.packageName, equals('foo')); + expect(package.currentVersion, equals('1.0.0')); + expect(package.upgradableVersion, equals('2.0.0')); + expect(package.resolvableVersion, isNull); + expect(package.latestVersion, isNull); + } + + Future test_outdated_multiplePubspecs() async { + final pubspecJson1 = r''' + { + "packages": [ + { + "package": "foo", + "resolvable": { "version": "1.1.1" } + } + ] + } + '''; + final pubspecJson2 = r''' + { + "packages": [ + { + "package": "foo", + "resolvable": { "version": "2.2.2" } + } + ] + } + '''; + + processRunner.runHandler = (executable, args, {dir, env}) { + // Return different json based on the directory we were invoked in. + final json = + dir == path.dirname(pubspecPath) ? pubspecJson1 : pubspecJson2; + return ProcessResult(1, 0, json, ''); + }; + final result1 = await pubCommand.outdatedVersions(pubspecPath); + final result2 = await pubCommand.outdatedVersions(pubspec2Path); + expect(result1.first.resolvableVersion, equals('1.1.1')); + expect(result2.first.resolvableVersion, equals('2.2.2')); + } + + Future test_outdated_nonZeroExitCode() async { + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 123, '{}', ''); + final result = await pubCommand.outdatedVersions(pubspecPath); + expect(result, isEmpty); + } + + Future test_validJson() async { + final validJson = r''' + { + "packages": [ + { + "package": "foo", + "current": { "version": "1.0.0" }, + "upgradable": { "version": "2.0.0" }, + "resolvable": { "version": "3.0.0" }, + "latest": { "version": "4.0.0" } + }, + { + "package": "bar", + "current": { "version": "1.0.0" }, + "upgradable": { "version": "2.0.0" }, + "resolvable": { "version": "3.0.0" }, + "latest": { "version": "4.0.0" } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, validJson, ''); + final result = await pubCommand.outdatedVersions(pubspecPath); + expect(result, hasLength(2)); + result.forEachIndexed((index, package) { + expect(package.packageName, equals(index == 0 ? 'foo' : 'bar')); + expect(package.currentVersion, equals('1.0.0')); + expect(package.upgradableVersion, equals('2.0.0')); + expect(package.resolvableVersion, equals('3.0.0')); + expect(package.latestVersion, equals('4.0.0')); + }); + } +} + @reflectiveTest class PubPackageServiceTest extends AbstractLspAnalysisServerTest { /// A sample API response for package names. This should match the JSON served @@ -194,13 +363,13 @@ class PubPackageServiceTest extends AbstractLspAnalysisServerTest { Future test_packageCache_initializesOnPubspecOpen() async { await initialize(); - expect(server.pubPackageService.isRunning, isFalse); + expect(server.pubPackageService.isPackageNamesTimerRunning, isFalse); expect(server.pubPackageService.packageCache, isNull); expectPackages([]); await openFile(pubspecFileUri, ''); await pumpEventQueue(); - expect(server.pubPackageService.isRunning, isTrue); + expect(server.pubPackageService.isPackageNamesTimerRunning, isTrue); expect(server.pubPackageService.packageCache, isNotNull); expectPackages([]); } diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart index 963542a167e..2e49ae58705 100644 --- a/pkg/analysis_server/test/lsp/server_abstract.dart +++ b/pkg/analysis_server/test/lsp/server_abstract.dart @@ -48,6 +48,7 @@ abstract class AbstractLspAnalysisServerTest late MockLspServerChannel channel; late TestPluginManager pluginManager; late LspAnalysisServer server; + late MockProcessRunner processRunner; late MockHttpClient httpClient; /// The number of context builds that had already occurred the last time @@ -164,6 +165,7 @@ abstract class AbstractLspAnalysisServerTest void setUp() { httpClient = MockHttpClient(); + processRunner = MockProcessRunner(); channel = MockLspServerChannel(debugPrintCommunication); // Create an SDK in the mock file system. MockSdk(resourceProvider: resourceProvider); @@ -175,7 +177,8 @@ abstract class AbstractLspAnalysisServerTest DartSdkManager(convertPath('/sdk')), CrashReportingAttachmentsBuilder.empty, InstrumentationService.NULL_SERVICE, - httpClient: httpClient); + httpClient: httpClient, + processRunner: processRunner); server.pluginManager = pluginManager; projectFolderPath = convertPath('/home/test'); diff --git a/pkg/analysis_server/test/mocks.dart b/pkg/analysis_server/test/mocks.dart index d6cfa2c71c8..e1f3ecd2d12 100644 --- a/pkg/analysis_server/test/mocks.dart +++ b/pkg/analysis_server/test/mocks.dart @@ -3,9 +3,12 @@ // BSD-style license that can be found in the LICENSE file. import 'dart:async'; +import 'dart:convert'; +import 'dart:io'; import 'package:analysis_server/protocol/protocol.dart'; import 'package:analysis_server/protocol/protocol_generated.dart'; +import 'package:analysis_server/src/utilities/process.dart'; import 'package:analyzer/src/generated/source.dart'; import 'package:http/http.dart' as http; import 'package:test/test.dart'; @@ -47,6 +50,32 @@ class MockHttpClient extends http.BaseClient { } } +class MockProcessRunner implements ProcessRunner { + FutureOr Function(String executable, List arguments, + {String? dir, Map? env})? runHandler = + (executable, arguments, {dir, env}) => throw UnimplementedError(); + + @override + dynamic noSuchMethod(Invocation invocation) { + return super.noSuchMethod(invocation); + } + + @override + Future run( + String executable, + List arguments, { + String? workingDirectory, + Map? environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding? stdoutEncoding = systemEncoding, + Encoding? stderrEncoding = systemEncoding, + }) async { + return runHandler!(executable, arguments, + dir: workingDirectory, env: environment); + } +} + class MockSource implements Source { @override final String fullName; diff --git a/pkg/analysis_server/test/src/services/completion/yaml/pubspec_generator_test.dart b/pkg/analysis_server/test/src/services/completion/yaml/pubspec_generator_test.dart index 4f4923fbe68..4eab94d23dc 100644 --- a/pkg/analysis_server/test/src/services/completion/yaml/pubspec_generator_test.dart +++ b/pkg/analysis_server/test/src/services/completion/yaml/pubspec_generator_test.dart @@ -2,8 +2,11 @@ // 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 'dart:io'; + import 'package:analysis_server/src/services/completion/yaml/pubspec_generator.dart'; import 'package:analysis_server/src/services/pub/pub_api.dart'; +import 'package:analysis_server/src/services/pub/pub_command.dart'; import 'package:analysis_server/src/services/pub/pub_package_service.dart'; import 'package:analyzer/instrumentation/service.dart'; import 'package:http/http.dart'; @@ -22,6 +25,7 @@ void main() { @reflectiveTest class PubspecGeneratorTest extends YamlGeneratorTest { late MockHttpClient httpClient; + late MockProcessRunner processRunner; late PubPackageService pubPackageService; @@ -34,10 +38,12 @@ class PubspecGeneratorTest extends YamlGeneratorTest { void setUp() { httpClient = MockHttpClient(); + processRunner = MockProcessRunner(); pubPackageService = PubPackageService( InstrumentationService.NULL_SERVICE, resourceProvider, - PubApi(InstrumentationService.NULL_SERVICE, httpClient, null)); + PubApi(InstrumentationService.NULL_SERVICE, httpClient, null), + PubCommand(InstrumentationService.NULL_SERVICE, processRunner)); } void tearDown() { @@ -339,4 +345,46 @@ dependencies: '''); assertSuggestion('two: '); } + + void test_packageVersion() async { + final json = r''' + { + "packages": [ + { + "package": "one", + "latest": { "version": "3.2.1" }, + "resolvable": { "version": "1.2.4" } + } + ] + } + '''; + processRunner.runHandler = + (executable, args, {dir, env}) => ProcessResult(1, 0, json, ''); + + pubPackageService.beginCachePreloads([convertPath('/home/test/$fileName')]); + await pumpEventQueue(times: 500); + + getCompletions(''' +dependencies: + one: ^ +'''); + assertSuggestion('^1.2.4'); + assertSuggestion('^3.2.1'); + } + + /// Ensure in a repo with a DEPS file like the SDK, we do not run pub + /// processes to cache the version numbers. + void test_packageVersion_withDEPSfile() async { + var didRun = false; + processRunner.runHandler = (executable, args, {dir, env}) { + didRun = true; + return ProcessResult(1, 0, '', ''); + }; + + newFile('/home/DEPS'); + pubPackageService.beginCachePreloads([convertPath('/home/test/$fileName')]); + await pumpEventQueue(times: 500); + + expect(didRun, isFalse); + } }