refactor(resolver): move yank policy to resolver layer

[weihanglo]

What does this PR try to resolve?

This PR moves move yank policy to resolver layer from source layer.

We mostly preserve the old Source::query behavior that filter out yanked by default.

The yanking policy logic doesn't go into VersionPreferences or sort_summaries yet because

• We want to minimize the refactor change. Changing the order of query and filter may change existing diagnostics
• [replace] resolution should not see yanked candidates either
• A future VersionPreferences predicate may still need to be invoked at this same point.

cc #17012, #17014 (comment)

How to test and review this PR?

Three tests are added to pin down more existing tests, along with #17082

Here is the list of all call site I audit:

Call site	Yanked versions handling
core/resolver/dep_cache.rs query	Filter
core/resolver/dep_cache.rs [replace] override	Filter
core/registry.rs patch collection	Filter
core/registry.rs query_overrides	Filter
core/registry.rs summary_for_patch unlocked-retry	Filter
core/registry.rs summary_for_patch name-only diagnostic	Filter
core/resolver/errors.rs alt_versions	Filter
core/resolver/errors.rs rejected_versions	Keep
core/resolver/errors.rs alt_names	Keep
ops/common_for_install_and_uninstall.rs select_dep_pkg (x2)	Filter
ops/cargo_update.rs upgrade_dependency	Filter
ops/cargo_update.rs report_latest (x4)	Filter
ops/cargo_add/mod.rs get_latest_dependency	Filter
ops/cargo_add/mod.rs select_package	Filter
ops/cargo_add/mod.rs populate_available_features	Filter
ops/registry/info/mod.rs query_summaries	Filter
ops/registry/publish.rs poll_one_package	Filter
ops/registry/publish.rs verify_unpublished	Filter
core/compiler/future_incompat.rs get_updates	Filter
crates/xtask-bump-check/src/xtask.rs check	Filter

[rustbot]

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage

[weihanglo]

This showcases where this refactor could be useful.

View changes since the review

[weihanglo]

This showcases where this refactor could be useful.

View changes since the review

[epage]

I'm hesitant about dropping this. Having it as an enum ensures explicit acknowledgement rather than hoping callers know to check this.

View changes since the review

[weihanglo]

We can keep this, though I wonder how we envision on IndexSummary evolution. Do we want to have variant like IndexSummary::TooNew for min-publish-age? It was done in #17012. Or we treat yanked a bit special but not others.

Actually having IndexSummary::TooNew may help as we can flag those at registry level, and apply the policy at resolver level. What do you think?

explicit acknowledgement rather than hoping callers know to check this.

Caller still need to manually check because we have IndexSummary::{as_summary, into_summary} providing a error-prone API bypassing it. And they are all over the places we call Summary::is_yanked in this PR.

[epage]

Is there anything we can do to help in this situation?

What is your read on how worth it this has been in the end?

[weihanglo]

I experimented a branch removing as_summary and into_summary. It was noisy. Some call sites only need something like s.as_summary().version() and maybe for that we should expose IndexSummary::version. Or maybe Invalid/Unsupported/Offline should be folded into one Unavailable and have their sub-enum.

Actually having IndexSummary::TooNew may help as we can flag those at registry level, and apply the policy at resolver level. What do you think?

This may look weird, as IndexSummary::parse currently doesn't have any registry specific logic. If we construct IndexSummary::TooNew there we need to thread registry config into. If we don't, we need to remap IndexSummary during query just like how IndexSummary::Offline is doing now.

On the other hand, the current change (removing IndexSummary:Yanked) is also weird because it pushes the yanked field down to Summary which is shared with git/path packages as well, and they don't have the concept of yanking.

I don't have a good answer to it though, maybe we can start with not removing IndexSummary::Yanked and see what we can do next?

[weihanglo]

Done the refactor. I plan to remove into_summary and as_summary in follow-up.