Improve E0277 diagnostics for conditionally implemented traits#158494
Improve E0277 diagnostics for conditionally implemented traits#158494raushan728 wants to merge 2 commits into
Conversation
|
r? @mu001999 rustbot has assigned @mu001999. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
57f7526 to
893f9fc
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Could you try to add the label on unsatisfied bounds, as desired in the issue? I think we can achieve this with ObligationCtxt::new_with_diagnostics.
And for the logic here, perhaps you could check the implementation of impl_may_apply. Maybe the logic could even be shared between them.
@rustbot author
|
Reminder, once the PR becomes ready for a review, use |
893f9fc to
9f2d7b1
Compare
Thanks! I updated this to use @rustbot ready |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| self.tcx, | ||
| ObligationCause::dummy_with_span(span), | ||
| param_env, | ||
| clause.skip_norm_wip(), |
There was a problem hiding this comment.
| clause.skip_norm_wip(), | |
| clause.skip_normalization(), |
| let mut multi_span = MultiSpan::from_span(self.tcx.def_span(def_id)); | ||
| for error in failing_obligations { | ||
| multi_span | ||
| .push_span_label(error.obligation.cause.span, "unsatisfied trait bound"); |
There was a problem hiding this comment.
| .push_span_label(error.obligation.cause.span, "unsatisfied trait bound"); | |
| .push_span_label(error.root_obligation.cause.span, "unsatisfied trait bound"); |
There was a problem hiding this comment.
And I think we can print root_obligation.predicate here, because this span will only point to the bound self.
| LL | impl<It: Iterator> T1 for Option<It> {} | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ^^^^^^^^^--------^^^^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | unsatisfied trait bound | ||
| note: required for `Option<()>` to implement `T1` | ||
| --> $DIR/blame-trait-error.rs:21:20 | ||
| | | ||
| LL | impl<It: Iterator> T1 for Option<It> {} | ||
| | -------- ^^ ^^^^^^^^^^ | ||
| | | | ||
| | unsatisfied trait bound introduced here |
There was a problem hiding this comment.
These two seems to be duplicated, but I think it's okay because they are from different ways and this would be helpful like in https://github.com/raushan728/rust/blob/9f2d7b14cdbc0b4a1efa4dbaa867b57189b9bbc4/tests/ui/traits/error_reporting/conditionally-implemented-trait-158423.stderr
| { | ||
| return false; | ||
| } | ||
| let mut failing_obligations = Vec::new(); |
There was a problem hiding this comment.
Nit: I'd like to move the declaration of multi_span here and move failing_obligations into the arm of the bellow match.
Then we just need to modify the multi_span in the arm, and don't need to make failing_obligations mutable. We can write
let failing_obligations = if .. { self.probe(..) } else { vec![] };
for error in &failing_obligations { ... }
Fixes #158423.
The diagnostic previously reported that a trait was "implemented" even when the matching impl failed because its own where-clause requirements were not satisfied, which was misleading.
This change:
ObligationCtxt::new_with_diagnostics