-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Tweak detection of multiple crate versions to be more encompassing #128849
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Can you add a test? |
This PR modifies cc @jieyouxu |
@jackh726 I updated the affected test (which incorporates the tweaks) and added a test case for the new logic. For reference, this is the new output:
|
As of the last commit, this is the output:
|
// FIXME: this is a giant hack for the benefit of this specific diagnostic. Because | ||
// we're so nested in method calls before the error gets emitted, bubbling a single bit | ||
// flag informing the top level caller to stop adding extra detail to the diagnostic, | ||
// would actually be harder to follow. So we do something naughty here: we consume the | ||
// diagnostic, emit it and leave in its place a "delayed bug" that will continue being | ||
// modified but won't actually be printed to end users. This *is not ideal*, but allows | ||
// us to reduce the verbosity of an error that is already quite verbose and increase its | ||
// specificity. Below we modify the main message as well, in a way that *could* break if | ||
// the implementation of Diagnostics change significantly, but that would be caught with | ||
// a make test failure when this diagnostic is tested. | ||
err.primary_message(format!( | ||
"{} because the trait comes from a different crate version", | ||
err.messages[0].0.as_str().unwrap(), | ||
)); | ||
let diag = err.clone(); | ||
err.downgrade_to_delayed_bug(); | ||
self.tcx.dcx().emit_diagnostic(diag); |
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.
I'm not married to this commit, and I'm happy to drop it if it'll make landing this PR easier, but the resulting output is better, imo.
This comment was marked as resolved.
This comment was marked as resolved.
Rebased. I will be unavailable for the coming weeks. If there are requested changes, I won't get to them until at the end of next month. |
☔ The latest upstream changes (presumably #130473) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Sorry for the review delay here.
r=me with rebase
@estebank any updates on this? this is already approved and just requires a rebase |
Previously, we only emitted the additional context if the type was in the same crate as the trait that appeared multiple times in the dependency tree. Now, we look at all traits looking for two with the same name in different crates with the same crate number, and we are more flexible looking for the types involved. This will work even if the type that implements the wrong trait version is from a different crate entirely. ``` error[E0277]: the trait bound `CustomErrorHandler: ErrorHandler` is not satisfied --> src/main.rs:5:17 | 5 | cnb_runtime(CustomErrorHandler {}); | ----------- ^^^^^^^^^^^^^^^^^^^^^ the trait `ErrorHandler` is not implemented for `CustomErrorHandler` | | | required by a bound introduced by this call | help: you have multiple different versions of crate `c` in your dependency graph --> src/main.rs:1:5 | 1 | use b::CustomErrorHandler; | ^ one version of crate `c` is used here, as a dependency of crate `b` 2 | use c::cnb_runtime; | ^ one version of crate `c` is used here, as a direct dependency of the current crate note: two types coming from two different versions of the same crate are different types even if they look the same --> /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.2/src/lib.rs:1:1 | 1 | pub trait ErrorHandler {} | ^^^^^^^^^^^^^^^^^^^^^^ this is the required trait | ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/b/src/lib.rs:1:1 | 1 | pub struct CustomErrorHandler {} | ----------------------------- this type doesn't implement the required trait | ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.1/src/lib.rs:1:1 | 1 | pub trait ErrorHandler {} | ---------------------- this is the found trait = help: you can use `cargo tree` to explore your dependency tree note: required by a bound in `cnb_runtime` --> /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.2/src/lib.rs:3:41 | 3 | pub fn cnb_runtime(_error_handler: impl ErrorHandler) {} | ^^^^^^^^^^^^ required by this bound in `cnb_runtime` ``` Fix rust-lang#89143.
``` error[E0277]: the trait bound `dep_2_reexport::Type: Trait` is not satisfied because the trait comes from a different crate version --> multiple-dep-versions.rs:7:18 | 7 | do_something(Type); | ^^^^ the trait `Trait` is not implemented for `dep_2_reexport::Type` | note: there are multiple different versions of crate `dependency` in the dependency graph --> /home/gh-estebank/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1 | 3 | pub struct Type(pub i32); | --------------- this type implements the required trait 4 | pub trait Trait { | ^^^^^^^^^^^^^^^ this is the required trait | ::: multiple-dep-versions.rs:1:1 | 1 | extern crate dep_2_reexport; | ---------------------------- one version of crate `dependency` is used here, as a dependency of crate `foo` 2 | extern crate dependency; | ------------------------ one version of crate `dependency` is used here, as a direct dependency of the current crate | ::: /home/gh-estebank/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:3:1 | 3 | pub struct Type; | --------------- this type doesn't implement the required trait 4 | pub trait Trait { | --------------- this is the found trait = note: two types coming from two different versions of the same crate are different types even if they look the same = help: you can use `cargo tree` to explore your dependency tree note: required by a bound in `do_something` --> /home/gh-estebank/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:12:24 | 12 | pub fn do_something<X: Trait>(_: X) {} | ^^^^^ required by this bound in `do_something` ```
``` error[E0277]: the trait bound `dep_2_reexport::Type: Trait` is not satisfied because the trait comes from a different crate version --> multiple-dep-versions.rs:7:18 | 7 | do_something(Type); | ^^^^ the trait `Trait` is not implemented for `dep_2_reexport::Type` | note: there are multiple different versions of crate `dependency` in the dependency graph --> /home/gh-estebank/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1 | 3 | pub struct Type(pub i32); | --------------- this type implements the required trait 4 | pub trait Trait { | ^^^^^^^^^^^^^^^ this is the required trait | ::: multiple-dep-versions.rs:1:1 | 1 | extern crate dep_2_reexport; | ---------------------------- one version of crate `dependency` is used here, as a dependency of crate `foo` 2 | extern crate dependency; | ------------------------ one version of crate `dependency` is used here, as a direct dependency of the current crate | ::: /home/gh-estebank/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:3:1 | 3 | pub struct Type; | --------------- this type doesn't implement the required trait 4 | pub trait Trait { | --------------- this is the found trait = note: two types coming from two different versions of the same crate are different types even if they look the same = help: you can use `cargo tree` to explore your dependency tree ``` The approach to accomplish this is a HACK, and we'd want a better way to do this. I believe that moving E0277 to be a structured diagnostic would help in that regard.
This comment has been minimized.
This comment has been minimized.
@Dylan-DPC thanks for the ping! @bors r=jackh726 |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5b20c45): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -8.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 781.906s -> 781.382s (-0.07%) |
Tweak detection of multiple crate versions to be more encompassing Previously, we only emitted the additional context if the type was in the same crate as the trait that appeared multiple times in the dependency tree. Now, we look at all traits looking for two with the same name in different crates with the same crate number, and we are more flexible looking for the types involved. This will work even if the type that implements the wrong trait version is from a different crate entirely. ``` error[E0277]: the trait bound `CustomErrorHandler: ErrorHandler` is not satisfied because the trait comes from a different crate version --> src/main.rs:5:17 | 5 | cnb_runtime(CustomErrorHandler {}); | ^^^^^^^^^^^^^^^^^^^^^ the trait `ErrorHandler` is not implemented for `CustomErrorHandler` | note: there are multiple different versions of crate `c` in the dependency graph --> /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.2/src/lib.rs:1:1 | 1 | pub trait ErrorHandler {} | ^^^^^^^^^^^^^^^^^^^^^^ this is the required trait | ::: src/main.rs:1:5 | 1 | use b::CustomErrorHandler; | - one version of crate `c` is used here, as a dependency of crate `b` 2 | use c::cnb_runtime; | - one version of crate `c` is used here, as a direct dependency of the current crate | ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/b/src/lib.rs:1:1 | 1 | pub struct CustomErrorHandler {} | ----------------------------- this type doesn't implement the required trait | ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.1/src/lib.rs:1:1 | 1 | pub trait ErrorHandler {} | ---------------------- this is the found trait = note: two types coming from two different versions of the same crate are different types even if they look the same = help: you can use `cargo tree` to explore your dependency tree ``` Fix rust-lang#89143.
Revert diagnostics hack to fix ICE 132920 This reverts 8a568d9 from rust-lang#128849 to fix the diagnostics ICE in rust-lang#132920. The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue. We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure). Sorry `@estebank` 🙏. I can close this if you'd prefer to fix it in a different way. Since it seems unexpected that rust-lang#128849 would impact the repro, here's how the error used to look before that PR. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` = help: the trait `Resource` is implemented for `Res` note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); ``` And how it looks now without the ICE. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` note: there are multiple different versions of crate `minibevy` in the dependency graph --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1 | 1 | pub trait Resource {} | ^^^^^^^^^^^^^^^^^^ this is the required trait | ::: src/main.rs:1:5 | 1 | use minibevy::Resource; | -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate 2 | use minirapier::Ray; | ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier` | ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1 | 1 | pub trait Resource {} | ------------------ this is the found trait = help: you can use `cargo tree` to explore your dependency tree note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); | ``` The improvements from rust-lang#128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely. Fixes rust-lang#132920.
Revert diagnostics hack to fix ICE 132920 This reverts 8a568d9 from rust-lang#128849 to fix the diagnostics ICE in rust-lang#132920. The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue. We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure). Sorry ``@estebank`` 🙏. I can close this if you'd prefer to fix it in a different way. Since it seems unexpected that rust-lang#128849 would impact the repro, here's how the error used to look before that PR. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` = help: the trait `Resource` is implemented for `Res` note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); ``` And how it looks now without the ICE. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` note: there are multiple different versions of crate `minibevy` in the dependency graph --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1 | 1 | pub trait Resource {} | ^^^^^^^^^^^^^^^^^^ this is the required trait | ::: src/main.rs:1:5 | 1 | use minibevy::Resource; | -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate 2 | use minirapier::Ray; | ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier` | ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1 | 1 | pub trait Resource {} | ------------------ this is the found trait = help: you can use `cargo tree` to explore your dependency tree note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); | ``` The improvements from rust-lang#128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely. Fixes rust-lang#132920.
Rollup merge of rust-lang#133304 - lqd:issue-132920, r=estebank Revert diagnostics hack to fix ICE 132920 This reverts 8a568d9 from rust-lang#128849 to fix the diagnostics ICE in rust-lang#132920. The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue. We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure). Sorry ``@estebank`` 🙏. I can close this if you'd prefer to fix it in a different way. Since it seems unexpected that rust-lang#128849 would impact the repro, here's how the error used to look before that PR. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` = help: the trait `Resource` is implemented for `Res` note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); ``` And how it looks now without the ICE. ```console warning: unused import: `minirapier::Ray` --> src/main.rs:2:5 | 2 | use minirapier::Ray; | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error[E0283]: type annotations needed --> src/main.rs:10:5 | 10 | insert_resource(Res.into()); | ^^^^^^^^^^^^^^^ ---------- type must be known at this point | | | cannot infer type of the type parameter `R` declared on the function `insert_resource` | = note: cannot satisfy `_: Resource` note: there are multiple different versions of crate `minibevy` in the dependency graph --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1 | 1 | pub trait Resource {} | ^^^^^^^^^^^^^^^^^^ this is the required trait | ::: src/main.rs:1:5 | 1 | use minibevy::Resource; | -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate 2 | use minirapier::Ray; | ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier` | ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1 | 1 | pub trait Resource {} | ------------------ this is the found trait = help: you can use `cargo tree` to explore your dependency tree note: required by a bound in `insert_resource` --> src/main.rs:4:23 | 4 | fn insert_resource<R: Resource>(_resource: R) {} | ^^^^^^^^ required by this bound in `insert_resource` help: consider specifying the generic argument | 10 | insert_resource::<R>(Res.into()); | +++++ help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds | 10 - insert_resource(Res.into()); 10 + insert_resource(Res); | ``` The improvements from rust-lang#128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely. Fixes rust-lang#132920.
Previously, we only emitted the additional context if the type was in the same crate as the trait that appeared multiple times in the dependency tree. Now, we look at all traits looking for two with the same name in different crates with the same crate number, and we are more flexible looking for the types involved. This will work even if the type that implements the wrong trait version is from a different crate entirely.
Fix #89143.