Respect renamed dependencies when resolving configured paths - #17625
Respect renamed dependencies when resolving configured paths#17625fyrsta7 wants to merge 2 commits into
Conversation
|
Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews. In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews. |
| } | ||
| } | ||
| } | ||
| map |
There was a problem hiding this comment.
nit:
the init is getting a bit long which makes it harder to reason about...
Please split out into a method, since this way we can also add a good place for an brief explaing comment (that you arlready wrote in the PR description)
There was a problem hiding this comment.
Good point. I've moved the initialization into a collect_crates_by_name helper and added a comment explaining that prelude-visible --extern aliases are matched to loaded crates by source path and take precedence over colliding canonical names.
| renamed_dependency::forbidden_by_alias(); | ||
| renamed_dependency::forbidden_by_package_name(); |
There was a problem hiding this comment.
I think there is something I don't understand/know, but I would have expected these comments to be required...
Are they not??
| renamed_dependency::forbidden_by_alias(); | |
| renamed_dependency::forbidden_by_package_name(); | |
| renamed_dependency::forbidden_by_alias(); //~ disallowed_methods | |
| renamed_dependency::forbidden_by_package_name(); //~ disallowed_methods |
There was a problem hiding this comment.
Thanks for asking. This test uses the ui-cargo harness, which validates diagnostics through Cargo.stderr rather than compiletest's inline //~ annotations. I checked the existing ui-cargo tests and they follow the same snapshot-based pattern, so I kept both expected disallowed_methods diagnostics in Cargo.stderr.
| renamed_dependency = { package = "actual_dependency", path = "actual_dependency" } | ||
| other_dependency = { package = "renamed_dependency", path = "renamed_dependency" } |
There was a problem hiding this comment.
this is nasty and evil !!!
(it is a good test though)
Clippy's shared path resolver indexed external crates by their canonical metadata names. This meant that a path configured with the source-visible name of a renamed Cargo dependency could not resolve, even though that was the only name available in the crate's source code.
This keeps canonical-name lookup intact and also indexes prelude-visible
--externaliases by matching their canonical source paths to loaded crates. A source-visible alias takes precedence if it collides with another crate's canonical name, matching how paths resolve in source code. The shared fix applies todisallowed-methods,disallowed-types, and other configuration features that use the same path resolver.The UI-Cargo regression covers alias resolution, canonical-name compatibility, and a collision with another crate's canonical name.
Fixes #17585
changelog: [
disallowed_methods,disallowed_types]: resolve configured paths through renamed Cargo dependenciesTesting
TESTNAME=disallowed_paths_renamed_dependency cargo uitestcargo testcargo dev fmt --check