-
Notifications
You must be signed in to change notification settings - Fork 391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite dependencies tree #9008
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drewnoakes
added
Tenet-Performance
This issue affects the "Performance" tenet.
Feature-Dependency-Node
"Dependencies" node in Solution Explorer that display project, binary & package references
Performance-Scenario-Solution-Open
This issue affects solution open performance.
Performance-Scenario-Solution-Explorer
This issue affects solution explorer performance.
labels
May 10, 2023
ghost
added
Feature-Language-Service
Populating the Roslyn workspace with references, source files, analyzers, etc
Feature-Multi-Targeting
Targeting and building for multiple frameworks.
Feature-NuGet
NuGet integration including pushing it properties, project and package references, and Pack support.
Tenet-User Friendly
This issue affects the "User Friendly" tenet; UI usability, accessibility or high-DPI related.
labels
May 10, 2023
drewnoakes
removed
Feature-Language-Service
Populating the Roslyn workspace with references, source files, analyzers, etc
Feature-NuGet
NuGet integration including pushing it properties, project and package references, and Pack support.
labels
May 10, 2023
drewnoakes
force-pushed
the
dep-node-rewrite
branch
3 times, most recently
from
May 10, 2023 08:08
dd21fae
to
efdbada
Compare
ghost
added
Feature-Language-Service
Populating the Roslyn workspace with references, source files, analyzers, etc
Feature-NuGet
NuGet integration including pushing it properties, project and package references, and Pack support.
labels
May 10, 2023
drewnoakes
force-pushed
the
dep-node-rewrite
branch
from
May 10, 2023 08:51
efdbada
to
a6c982a
Compare
We will want this method for later use in the dependencies tree.
There are two changes here: 1. Use `JointRuleDataflowLinkOptions` where possible. When subscribing to `JointRule` data sources, we were previously using `StandardRuleDataflowLinkOptions` to pass the set of rule names. This would cause the data source to internally divide the rules into those that come from evaluation and those that come from build. By passing `JointRuleDataflowLinkOptions` instead, we can do that categorization at design-time and avoid any runtime overhead during project load. 2. Have components model the set of rule names using `ImmutableHashSet<string>` directly to avoid conversion during solution load (from `string[]` or `IEnumerable<string>`).
Allows use of C# records and init properties when targeting .NET Framework.
This rewrite brings the dependencies tree in line with the current best practices for modelling project data. Specifically: - Using standard dataflow blocks for merging data across configurations (replacing the previous `AggregateCrossTargetProjectContext` and related types). - Using "slices" to simplify data handling and correctly support switching the active configuration. - Making a snapshot of dependencies available at various levels, simplifying certain behaviours (previously we'd only have deltas, so components needed to track their own state independently). The dependencies tree now populates much sooner, and no longer shows yellow triangles during project load for projects that are already restored, which greatly improves perceived performance. Fewer tree updates also improve actual performance. A simpler representation of dependencies is now made. Instead of `IDependencyModel` which had many properties, we now have `IDependency` with only a few. If a dependency exposes a browse object, it may also implement `IDependencyWithBrowseObject` which adds a few extra properties. If we expose additional capabilities in future, we can add more interfaces to support that, which providers may opt in to. The rewrite introduces the distinction between "configured" and "unconfigured" dependencies. For example, all dependencies coming from MSBuild are configured, while for a TypeScript/JavaScript project NPM packages are unconfigured (they have no concept of MSBuild configurations). Unconfigured dependencies are displayed at the top level, while configured dependencies are displayed beneath a node reflecting their configuration (when multi-targeting). We also introduce an abstraction for MSBuild properties that come from project/joint rule sources. There is no common representation for dependencies in MSBuild, so a "factory" abstraction is introduced to take care of mapping between items and their metadata to a common representation. A new `DependencyGroupType` type was added to model information common to all dependencies of a given type. Other miscellaneous changes in this rewrite: - Remove all explicit handling of target frameworks from the code. Everything is now performed in terms of "slices" which include target frameworks and potentially other implicit configuration dimensions. - Tree building now occurs directly on `IDependency`, removing the need for the additional `IDependencyViewModel` object and the copying associated with that. - Improve tree update logic to avoid collapsing expanded nodes during updates. - Move all "legacy" extensibility code into its own folder, and implement in terms of the new APIs in `LegacyDependencySubscriber`. - Added static class `KnownProjectImageMonikers` which caches `ProjectImageMoniker` instances, avoiding the locking and work done within `ToProjectSystemType()` on each use of an image moniker at run time. - Consolidation of namespaces. Originally the dependencies tree was in the VS layer. In 16.x much of the code was moved to the host agnostic layer, however namespaces could not be updated. This rewrite moves us towards addressing that, where possible, by replacing the previous.
drewnoakes
force-pushed
the
dep-node-rewrite
branch
from
May 11, 2023 03:25
a6c982a
to
57d42b2
Compare
tmeschter
approved these changes
May 15, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the change, but it is so substantive that there is a limit to how useful a review can be. :-) I did have a few minor comments.
...sualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/LanguageServiceHost.cs
Show resolved
Hide resolved
...tem.Managed/ProjectSystem/Tree/Dependencies/Subscriptions/ConfiguredDependencyFilterBlock.cs
Outdated
Show resolved
Hide resolved
melytc
approved these changes
May 15, 2023
src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/IProjectTreeExtensions.cs
Show resolved
Hide resolved
MiYanni
reviewed
May 16, 2023
...soft.VisualStudio.ProjectSystem.Managed/ProjectSystem/ProjectConfigurationSliceExtensions.cs
Outdated
Show resolved
Hide resolved
...isualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependenciesTreeBuilder.cs
Show resolved
Hide resolved
...isualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependenciesTreeBuilder.cs
Show resolved
Hide resolved
...isualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependenciesTreeBuilder.cs
Show resolved
Hide resolved
...sualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependenciesTreeProvider.cs
Show resolved
Hide resolved
...sualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependenciesTreeProvider.cs
Show resolved
Hide resolved
...ft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/DependencyTreeFlags.cs
Show resolved
Hide resolved
...tudio.ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/Snapshot/DependenciesSnapshot.cs
Show resolved
Hide resolved
....ProjectSystem.Managed/ProjectSystem/Tree/Dependencies/Snapshot/DependenciesSnapshotSlice.cs
Show resolved
Hide resolved
drewnoakes
removed
the
Feature-Language-Service
Populating the Roslyn workspace with references, source files, analyzers, etc
label
Nov 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Feature-Dependency-Node
"Dependencies" node in Solution Explorer that display project, binary & package references
Feature-Multi-Targeting
Targeting and building for multiple frameworks.
Feature-NuGet
NuGet integration including pushing it properties, project and package references, and Pack support.
Performance-Scenario-Solution-Explorer
This issue affects solution explorer performance.
Performance-Scenario-Solution-Open
This issue affects solution open performance.
Tenet-Performance
This issue affects the "Performance" tenet.
Tenet-User Friendly
This issue affects the "User Friendly" tenet; UI usability, accessibility or high-DPI related.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8940 Dependencies tree refactor
Fixes #3642 Dependency node does not handle configuration switches
Fixes #6183 Use Dataflow and snapshots for dependencies node data processing
Fixes #2618 Dependencies node fails to handle custom target frameworks
Fixes #2525 Remove all concepts of "target framework" and that configurations are driven from them
Fixes #2795 Remove concept of "short name" in TargetFramework
Fixes #5044 Don't show dependency warning icons while waiting for design-time build
Fixes #5519 DependenciesSnapshotProvider is marked with [ProjectAutoLoad] with a dynamic capability
Fixes #5045 Provide feedback that dependencies node is populating
Fixes #8998 When changing a package version, update rather than replace the tree node
Fixes #2524 Dependency node doesn't handle Multi-targeting with a multi portable frameworks
Fixes #2617 TargetFramework fails to differentiate by NuGet profile name
Fixes #6203 Multiple equivalent target framework short names cause exception
This rewrite brings the dependencies tree in line with the current best practices for modelling project data. Specifically:
AggregateCrossTargetProjectContext
and related types).The dependencies tree now populates much sooner, and no longer shows yellow triangles during project load for projects that are already restored, which greatly improves perceived performance. Fewer tree updates also improve actual performance.
A simpler representation of dependencies is now made. Instead of
IDependencyModel
which had many properties, we now haveIDependency
with only a few. If a dependency exposes a browse object, it may also implementIDependencyWithBrowseObject
which adds a few extra properties. If we expose additional capabilities in future, we can add more interfaces to support that, which providers may opt in to.The rewrite introduces the distinction between "configured" and "unconfigured" dependencies. For example, all dependencies coming from MSBuild are configured, while for a TypeScript/JavaScript project NPM packages are unconfigured (they have no concept of MSBuild configurations). Unconfigured dependencies are displayed at the top level, while configured dependencies are displayed beneath a node reflecting their configuration (when multi-targeting).
We also introduce an abstraction for MSBuild properties that come from project/joint rule sources. There is no common representation for dependencies in MSBuild, so a "factory" abstraction is introduced to take care of mapping between items and their metadata to a common representation.
A new
DependencyGroupType
type was added to model information common to all dependencies of a given type.Other miscellaneous changes in this rewrite:
IDependency
, removing the need for the additionalIDependencyViewModel
object and the copying associated with that.LegacyDependencySubscriber
.KnownProjectImageMonikers
which cachesProjectImageMoniker
instances, avoiding the locking and work done withinToProjectSystemType()
on each use of an image moniker at run time.Microsoft Reviewers: Open in CodeFlow