PrunePackageReference improvements - restore applies PrivateAssets=all and IncludeAssets=none to all prunable direct references

[nkolev92]

Bug

Fixes: NuGet/Home#14196

Description

Builds on top of #6540.

Design in NuGet/Home#14325.
This PR ensures that pruning privatizes direct packages .

This is the final of the pruning for directs work. Note that we have a P7 urgency and we're trying

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc. - Update the pruning documentation to clarify the behavior for direct references Home#14410
• Breaking change doc: [Breaking change]: PrunePackageReference, direct PackageReference that can be pruned are marked with PrivateAssets=all and IncludeAssets=none automatically. dotnet/docs#47292

[dotnet-policy-service]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET SDK Breaking Change Notification email list.

You can refer to the .NET SDK breaking change guidelines

[nkolev92]

@NuGet/nuget-client
Can I please get a review?

[jebriede]

We can reduce verbosity and make it more concise:

Suggested change

	static bool IsDependencyPruned(LibraryDependency dependency, IReadOnlyDictionary<string, PrunePackageReference> packagesToPrune)
	{
	if (packagesToPrune?.TryGetValue(dependency.Name, out PrunePackageReference packageToPrune) == true
	&& dependency.LibraryRange.VersionRange.Satisfies(packageToPrune.VersionRange.MaxVersion))
	{
	return true;
	}
	return false;
	}
	static bool IsDependencyPruned(LibraryDependency dependency, IReadOnlyDictionary<string, PrunePackageReference> packagesToPrune)
	{
	return packagesToPrune?.TryGetValue(dependency.Name, out var packageToPrune) == true
	&& dependency.LibraryRange.VersionRange.Satisfies(packageToPrune.VersionRange.MaxVersion);
	}

[nkolev92]

There was similar question in an previous PR, that implementation slows down debugging, since now you need to set a conditional breakpoint vs just setting a breakpoint.
Functionally they're the same, so no perf benefit.

From a readability perspective, I've always seen them as equivalent as well, especially in this case.

If you feel strongly I can change it, especially both you and Nigusu brought it up (although I'm guessing @zivkan will lean towards the current version, as that's something we've discussed previously)

[zivkan]

yes, I personally dislike return statements that do anything other than return a const or a variable, because I often want to inspect the value when debugging. The compiled code is the same, so I just see it as unhelpful code golf 😆

I'm fine with bool result = ... followed by return result; if we want to remove the if statement. But I just don't like combining the two into a single statement.

[jebriede]

It looks like this code is a duplicate of IncludeFlagUtils.IsDependencyPruned. This indicates to me that we could benefit from a data structure that handles the pruned dependency case and reduce the if conditionals. In lieu of that, we should deduplicate the code.

[nkolev92]

It is duplicated, but there's no type that would naturally own this, since TargetFrameworkInformation itself is a model and it has a dict of prune package refs.

The other consideration why I didn't add a type is that the feature is in development, so I didn't have enough scenarios to come up with an API.

Maybe I do now, but as of right now I've landed in the camp of I'd rather have 2 copies than a bad API.