-
Notifications
You must be signed in to change notification settings - Fork 14k
Fix false positive of "multiple different versions of crate X in the dependency graph" #149001
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
base: main
Are you sure you want to change the base?
Fix false positive of "multiple different versions of crate X in the dependency graph" #149001
Conversation
…raph Currently, If `expected_def_id` and `another_trait_def_id` have their crate imported as ExternCrateSource::Path the method get_extern_crate_renamed_symbol() will return None for both, resulting in a false positive. This fixes the issue by using a slitly different approach, we use a predicate instead and do the comparison of the item names only when both crates are imported as ExternCrateSource::Extern, otherwise false is returned.
|
r? @lcnr |
| Some(ExternCrate { src: ExternCrateSource::Extern(trait_def_id), .. }), | ||
| ) => { | ||
| let expected_sym = self.tcx.item_name(expected_def_id); | ||
| expected_sym == self.tcx.item_name(trait_def_id) && expected_sym != sym::std |
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.
why the check for != std?
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.
That's weird, it seems that, in some cases, there are tests in ui suite that report two crates being imported as std. In such a case, the predicate returns true. It's the test tests/ui/typeck/issue-50687-ice-on-borrow.rs, if I remember correctly. Note: I had the same kind of reports with the old version.
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.
that seems worth debugging because right now it feels arbitrary.
In such cases we should figure out why things work the way they do as
- it prevents confusing future readers by adding a comment#
- the "arbitrary" check may actually hide a more general underlying issue and without properly fixing it
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.
Well, no my bad, it's the tests tests/ui/suggestions/issue-105645.rs and tests/ui/suggestions/mut-borrow-needed-by-trait.rs both report : "there are multiple different versions of crate std in the dependency graph" :=D .
Yeah, I agree, I am going to investigate.
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.
can you add a test to our ui suite?
Sure |
cc #148892