Move PackageRestoreDataSource to Microsoft.VisualStudio.ProjectSystem.Managed#8850
Conversation
Currently the code to collect the information needed for NuGet restore operations lives in the Microsoft.VisualStudio.ProjectSystem.Managed layer, though the actual call to the VS NuGet APIs (specifically, `IVsSolutionRestoreService3.NominateProjectAsync`) only occurs in the Microsoft.VisualStudio.ProjectSystem.Managed.VS layer. Even though the *.Managed layer doesn't use that API, it does still have a dependency on the NuGet.VisualStudio NuGet package because the data structures it builds up implement a number of NuGet interfaces (`IVsReferenceProperties`, `IVsProjectRestoreInfo2`, `IVsReferenceItem`, etc.). The *.Managed layer sometimes makes use of these interfaces, but it doesn't _need_ to--it already knows what the concrete type is. We also have a number of collection types (`ProjectProperties`, `ReferenceItems`, etc.) that exist only to implement a NuGet collection interface (such as `IVsProjectProperties` or `IVsReferenceItems`) that allow access by either index or name. We don't need this functionality at the *.Managed layer, either, and can quite easily use an `ImmutableList<T>` instead of a custom collection type. This commit strips the NuGet interfaces from the data model in the *.Managed layer, and eliminates the unnecessary collection types. This in turn allows us to drop the reference to the NuGet.VisualStudio package. In the *.Managed.VS layer we add a number of wrapper types that exist to adapt the data model to the NuGet-specific interfaces. Where possible, the creation of the wrappers is done lazily; if NuGet never queries a particular property of the data model, no wrapper will be created. The most interesting part of this change is to the `ImmutablePropertyCollection` which is what allows us to create collections that support accesing items by index or name. It will take as inputs types of the data model from the *.Managed layer, but it needs to offer to the consumer (NuGet) objects with the expected NuGet interface. It now takes a transform function, allowing derived types to specify how to create an "output" object from an "input". A future change will move the `ImmutablePropertyCollection` type up to the *.Managed.VS layer as it is no longer used in *.Managed. There is more that could be done to simplify the data model. The `ProjectProperty` and `ReferenceProperty` types are identical and could be replaced by a `KeyValuePair<string, string>`, and all the data is originally in dictionaries in `IProjectRuleSnapshot` instances. We could pass those dictionaries around instead of converting them to a NuGet-specific object model which would reduce the number of types and avoid some object allocations, but it isn't necessary and would create too much churn at this point. We can come back to that at a later date if we wish.
Move this type to our VS-specific layer as it is no longer used in lower layers.
These tests can move down to Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests as they are no longer dependent on VS-specific functionality.
1. Define an `INuGetRestoreService` responsible for running NuGet restore operations on behalf of the `PackageRestoreDataSource`. The `INuGetRestoreService` abstracts away _how_ the restore is run, and the `PackageRestoreDataSource` is now just responsible for determining when a restore is needed. 2. Move a bunch of existing code (mostly from PackageRestoreDataSource.ProjectRestoreInfoSource.cs) into a new `NuGetRestoreService` type implementing `INuGetRestoreService`. This new type becomes the only place we call the VS NuGet APIs. The `NuGetRestoreService` is alsor responsible for tracking possible future restore nominations. 3. Add a couple of unit tests for `NuGetRestoreService`. Note the current implementation runs an async task in a "fire and forget" manner which makes it impossible to reliably unit test certain functionality. I've still added the unit tests (which are timing dependent and so _sometimes_ pass) but marked them as skipped. There are probably better approaches that do not require this "fire and forget" call and that's something we should look at for the future. A future change will update file names.
Rename PackageRestoreDataSource.ProjectRestoreInfoSource.cs to NuGetRestoreService.cs to reflect the contained class.
The `PackageRestoreDataSource` has a dependency on the VS info bar via the `IInfoBarService` which is of course defined in our VS layer. I'd like to move `PackageRestoreDataSource` down to MS.VS.PS.Managed, so it needs to be updated to use a similar service from that layer. As we currently don't have anything suitable at that layer, this change introduces the `INonModalNotificationService`. This service provides a way of showing simple messages, warnings, and errors that does not block the user (e.g. it does not pop up a modal dialog). The VS layer implementation of this delegates to the existin `IInfoBarService`.
Remove another VS-specific concept from `PackageRestoreDataSource` by moving a call to a `CodeMarker` to the corresponding location in `NuGetRestoreService`.
Move the use of the `FeatureFlagNames.EnableNuGetRestoreCycleDetection` feature flag to `IProjectSystemOptions`. It's not really an option, of course--at least I don't think the user can affect it--but it behaves like one.
Move `PackageRestoreDataSource` and `INuGetRestoreService` down to the Microsoft.VisualStudio.ProjectSystem.Managed layer as they no longer have VS dependencies. This also requires moving a resource that does not exist in the lower layer.
|
It may be useful to look at the commits one at a time. |
|
There's a bunch of usages of
|
You could inject a mock for the fault handler and access the task via that. That shouldn't be too tough. Currently it's buried in |
Oh, yes. That would be straightforward. A more invasive alternative did occur to me. We need the "fire and forget" only because we need to perform an async operation within the synchronous Aside: I'm starting to view the use of |
Rename the `*Wrapper` types used to implement the various `IVs*` interfaces defined by NuGet to `Vs*` instead. For example, `ProjectPropertyWrapper` implements `IVsProjectProperty` and here is renamed to `VsProjectProperty`. This emphasizes that the types are used to communicate information to a different part of VS rather than used internally. The files will be renamed in a subsequent commit.
Rename files to match the names of the types they contain.
`Assert.Single(...)` returns the single object (if there is indeed exactly one); we can use this is slightly simplify our unit tests.
`ImmutableArray` is the thinnest possible wrapper (just a struct) over a straight array, while `ImmutableList` is an object that will wrap at least one other object and possibly as much as one object per list element. `ImmutableList` is a better choice when we expect significant transformations to the list (lots of adding and removing items) as it may be able to reuse the underlying objects; `ImmutableArray` can't do that and will always end up allocating a new array of the necessary size. In the case of the NuGet restore information, however, we basically create it once and transform it relatively rarely if at all. In this case `ImmutableArray` is the better option as it will significantly reduce the number of object allocations.
This is an interface so we don't need access modifiers on the members.
Rename `INuGetRestoreService.UpdateComplete` to `NotifyComplete` and `UpdateFaulted` to `NotifyFaulted`. The new names better convey the intended purpose.
The `NuGetRestoreService` uses the `IProjectFaultHandlerService` to run some "fire and forget" work. Since the work is "forgotten" our unit tests can't wait for it to complete, which leads to a couple of flaky (and currently skipped) tests. Right now we're acquiring the service from `UnconfiguredProject.Services`, but by importing it directly in the constructor the unit tests can provide a mock that captures the otherwise "forgotten" `Task` and then await it.
| private bool _enabled; | ||
| private bool _restoring; | ||
| private bool _updatesCompleted; |
There was a problem hiding this comment.
Do we not follow the Is convention for booleans? Like,
- _isEnabled
- _isRestoring
- _areUpdatesCompleted
| if (_savedNominatedConfiguredVersion.Count == 1) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Looks like this should be checked first in the method since it is the easiest check to do. One thing that is difficult to ascertain is if the intent is that the first if-statement must be false before checking this statement. While it is arranged to operate that way, is that intentional?
Only reason I ask is because sequential boolean logic can sometimes be... tricky. The entire method can technically be a single expression but splitting it up makes it easier to read/scan. However, trying to identify intentionality can be a bit more... loose/unclear.
There was a problem hiding this comment.
No, this is in the right place. The first if statement checks if the current version of the active ConfiguredProject is newer than the version that corresponds to what we previously nominated. If so, things are out of date.
If we make it to the second if we know the active ConfiguredProject isn't out of date. If that's the only configuration we care about then we're done. In a multi-targeting project, however, we need to check the other configurations, which happens in the subsequent foreach loop.
Basically the first two if statements serve as a fast path through the common case, and we don't have to touch UnconfiguredProject.LoadedConfiguredProjects.
There was a problem hiding this comment.
(Also note this method is unchanged from the original code. So there might be bugs in it, but at least they are the same bugs. :-) )
| if (_savedNominatedConfiguredVersion.TryGetValue(loadedProject.ProjectConfiguration, out IComparable savedProjectVersion)) | ||
| { | ||
| if (loadedProject.ProjectVersion.IsLaterThan(savedProjectVersion)) | ||
| { |
There was a problem hiding this comment.
nit: Combine into single expression. There's no functional reason to be 2 if-statements.
| succeeded: succeeded && lastWriteTime != DateTime.MinValue); | ||
| } | ||
|
|
||
| protected virtual bool IsProjectConfigurationVersionOutOfDate(IReadOnlyCollection<PackageRestoreConfiguredInput> configuredInputs) |
There was a problem hiding this comment.
nit: The logic for this method is very similar to the IsSavedNominationOutOfDate method I commented on above. Any chance for consolidation would be welcomed.
| _wasSourceBlockContinuationSet = true; | ||
|
|
||
| // Inform the NuGet restore service when there will be no further updates so it can cancel any pending work. | ||
| _ = SourceBlock.Completion.ContinueWith(t => |
There was a problem hiding this comment.
Q: Is this a pattern now to assign a discard for methods that return a value instead of just calling them like a void method?
There was a problem hiding this comment.
It's a language feature, actually. You can assign to "_" without having to declare it, and this signals that you know a value is being returned but you've made a conscious choice to ignore it.
ghost
added this to the
3 participants
Most pieces of the dataflow chain that detect changes to NuGet-related items (like
PackageReference,PackageVersion, etc.) and "nominate" a package for restore are already in the Microsoft.VisualStudio.ProjectSystem.Managed project. The last major piece isPackageRestoreDataSourcewhich is responsible for detecting if changes have actually occurred in the set of relevant data, detecting cycles in the restore operation, and, most importantly, actually nominating the project.This PR moves
PackageRestoreDataSourcedown to Microsoft.VisualStudio.ProjectSystem.Managed. There are two major parts to this:INuGetRestoreService, to handle the nomination logic and tracking pending nominations.PackageRestoreDataSourcebut incidental to its core functionality (e.g., accessing feature flags, raising notifications to the user, etc.).Related to AB#1736319.
Microsoft Reviewers: Open in CodeFlow