Skip to content
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.

Sign up for GitHub

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

Skip resolved child dependencies in the new resolver if an identical one has already been resolved #6258

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Skip resolved child dependencies in the new resolver if an identical one has already been resolved #6258

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63c54d0
WIP
jeffkl Feb 6, 2025
498c6a4
Step
jeffkl Feb 7, 2025
05e0562
Step
jeffkl Feb 10, 2025
04b914e
Feedback
jeffkl Feb 11, 2025
1643a0e
Feedback
jeffkl Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,15 +1301,7 @@ .. chosenResolvedItem.Suppressions
continue;
}

HashSet<LibraryDependency>? runtimeDependencies = default;

// Evaluate the runtime dependencies if any
if (EvaluateRuntimeDependencies(ref childDependency, runtimeGraph, pair.RuntimeIdentifier, ref runtimeDependencies))
{
// EvaluateRuntimeDependencies() returns true if the version of the dependency was changed, which also changes the LibraryRangeIndex so that must be updated in the chosen item's array of library range indices.
chosenResolvedItem.SetRangeIndexForDependencyAt(i, _indexingTable.Index(childDependency.LibraryRange));
}

LibraryRangeIndex childLibraryRangeIndex = chosenResolvedItem.GetRangeIndexForDependencyAt(i);
bool isPackage = childDependency.LibraryRange.TypeConstraintAllows(LibraryDependencyTarget.Package);
bool isRootPackageReference = (currentDependencyGraphItem.LibraryDependencyIndex == LibraryDependencyIndex.Project) && isPackage;

Expand All @@ -1324,15 +1316,39 @@ .. chosenResolvedItem.Suppressions
continue;
}

// See if a dependency with the same version and suppressions has already been chosen
jeffkl marked this conversation as resolved.
Show resolved Hide resolved
if (resolvedDependencyGraphItems.TryGetValue(childLibraryDependencyIndex, out ResolvedDependencyGraphItem? childResolvedDependencyGraphItem)
&& childResolvedDependencyGraphItem.LibraryRangeIndex == childLibraryRangeIndex
&& childResolvedDependencyGraphItem.Suppressions.Count == 1
&& childResolvedDependencyGraphItem.Suppressions[0].Count == 0)
jeffkl marked this conversation as resolved.
Show resolved Hide resolved
{
if (!childResolvedDependencyGraphItem.IsRootPackageReference)
{
childResolvedDependencyGraphItem.Parents ??= new HashSet<LibraryRangeIndex>();
jeffkl marked this conversation as resolved.
Show resolved Hide resolved

// Keep track of the parents of this item
childResolvedDependencyGraphItem.Parents?.Add(currentDependencyGraphItem.LibraryRangeIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it's possible for Parents to be null here since we instantiate it on line 1326 if it is null. So, we should remove the operator to make that clear.

Suggested change
childResolvedDependencyGraphItem.Parents?.Add(currentDependencyGraphItem.LibraryRangeIndex);
childResolvedDependencyGraphItem.Parents.Add(currentDependencyGraphItem.LibraryRangeIndex);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've cleaned up the Parents stuff, let me know if you like it better now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ?. operator is still present when dereferencing the Parents which implies that Parents can be null and looking at the changes to the dependency graph item, it looks like there is a case where it can be null. In that case, we'll skip adding the currentDependencyGraphItem.LibraryRangeIndex to the Parents of the childResolvedDependencyGraphItem. This implicit relationship between whether the HashSet is instantiated and whether parents are kept track of worries me for code maintenance and clarity. Is it intentional that we would have a non-root package reference that has null parents that we don't want to keep track of even if the currentDependencyGraphItem has a LibraryRangeIndex? Could this relationship be represented more clearly through tests or could we add a comment? Alternatively, would it make sense to have an empty hash set for the Parents to avoid special casing when it's null or would that break things?

}

continue;
jeffkl marked this conversation as resolved.
Show resolved Hide resolved
}

HashSet<LibraryDependency>? runtimeDependencies = default;

// Evaluate the runtime dependencies if any
if (EvaluateRuntimeDependencies(ref childDependency, runtimeGraph, pair.RuntimeIdentifier, ref runtimeDependencies))
{
// EvaluateRuntimeDependencies() returns true if the version of the dependency was changed, which also changes the LibraryRangeIndex so that must be updated in the chosen item's array of library range indices.
chosenResolvedItem.SetRangeIndexForDependencyAt(i, _indexingTable.Index(childDependency.LibraryRange));
}

VersionRange? pinnedVersionRange = null;

// Determine if the package is transitively pinned
bool isCentrallyPinnedTransitiveDependency = isCentralPackageTransitivePinningEnabled
&& isPackage
&& pinnedPackageVersions?.TryGetValue(childLibraryDependencyIndex, out pinnedVersionRange) == true;

LibraryRangeIndex childLibraryRangeIndex = chosenResolvedItem.GetRangeIndexForDependencyAt(i);

if (isCentrallyPinnedTransitiveDependency && !isRootPackageReference)
{
// If central transitive pinning is enabled the LibraryDependency must be recreated as not to mutate the in-memory copy
Expand Down
Loading