fix: sup_trace to sub_trace #152317
fix: sup_trace to sub_trace #152317
Conversation
rustbot
added
S-waiting-on-review
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
This needs at minimum a test, but some explanation of why this change was made is also appropriate. |
Done. In The Even for non-PolySigs variants where |
|
Does this test fail without the changes in this PR? It doesn't seem like it would, so it'd be good to be sure. |
No, the test would not failed. Can not construct a failed test via ui.But i think this fix is still worth. personal view. |
|
Is there a change in diagnostics for the test (not necessarily just a compile fail to pass? While this change seems correct, a test that changes is important for 3 reasons:
Also, please update the OP with your reasoning so it's more obvious for others and posterity. |
|
No, the test output does not change — the cause parameter is only used for PolySigs values in values_str, and this test produces TraitRefs. The PolySigs path here is effectively unreachable because try_report_impl_not_conforming_to_trait intercepts that case earlier. So this is a pure correctness fix for a copy-paste error, not an observable behavior change. I've updated the OP with this analysis. |
|
If the added test doesn't change in any way, there is absolutely no reason to include it. I'm inclined to merge the change because it's "obvious", but the test needs to be removed. |
… report_sub_sup_conflict In `report_sub_sup_conflict`, when calling `values_str` for `sub_trace.values`, the code was incorrectly passing `sup_trace.cause` instead of `sub_trace.cause`. This is a copy-paste error from the preceding line which correctly uses `sup_trace.cause` for `sup_trace.values`. The `cause` parameter matters for `ValuePairs::PolySigs` comparisons, where `values_str` inspects `cause.code()` to extract `impl_item_def_id` and `trait_item_def_id` for richer function signature diagnostics. Using the wrong trace's cause could produce incorrect function def IDs, leading to misleading diagnostic output when the sub and sup traces originate from different obligations. Even for non-PolySigs variants where `cause` is currently unused by `values_str`, passing the semantically correct cause is the right thing to do for correctness and maintainability.
test has been deleted. |
|
Thanks. As a side note, I think the OP text you had is pretty unnecessarily verbose. I edited it to what it is now, which is exactly sufficient for why this change is being made. Like I said, it'd be nice to add a test for this, but given that there would be some very specific set of conditions for that test to trigger, and this is so small and obvious - I'm just going to r+ this. In the future, I would be a bit more careful. Adding tests which aren't impacted by the changes you make is basically a no-go. Overly verbose descriptions and justifications are hard to review, and largely unnecessary. This generally may fall under rust-lang/compiler-team#893 and other team members may be more inclined to outright close the PR. Hope this was a useful learning experience for you. @bors r+ |
rust-bors
bot
added
S-waiting-on-bors
fix: sup_trace to sub_trace This looks like a copy-past here from the line above.
…uwer Rollup of 7 pull requests Successful merges: - #151960 (rustc_parse: improve the error diagnostic for "missing let") - #152157 (Fix error spans for `asm!()` args that are macros) - #152317 (fix: sup_trace to sub_trace) - #150897 (rustc_parse_format: improve diagnostics for unsupported debug = syntax) - #151154 (Add `s390x-unknown-none-softfloat` with `RustcAbi::Softfloat`) - #152013 (Update to Xcode 26.2) - #152326 (Remove the compiler adhoc group)
Rollup merge of #152317 - cuiweixie:sup_trace, r=jackh726 fix: sup_trace to sub_trace This looks like a copy-past here from the line above.
4 participants
This looks like a copy-past here from the line above.