Show pending obligations as unsatisfied constraints in report_similar_impl_candidates
#134348
+26
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
From the CI failure, it looks like the patch introduces a lot of redundant hints with constraints that are basically equivalent to the original constraint. I can try to work on hiding pending obligations that match the original. But I'm not quite sure how to do that within |
compiler-errors
requested changes
Dec 15, 2024
There was a problem hiding this comment.
I'm not totally certain what the motivation here is because there's no test and the issue you linked has a pretty large minimization, so I'm not actually sure what the root cause is and whether this is actually the best way to show that information.
Can you try to turn this into a more minimal example to try to explain better what information the compiler is missing in its diagnostics outputs?
Otherwise I'm not totally certain we can give good feedback on the approach in this PR, especially because making a totally new way of reporting unsatisfied predicates on a pretty common codepath (for unsatisfied impls) seems a bit excessive on first glance.
| @@ -17,6 +17,8 @@ use crate::traits::Obligation; | |||
| pub enum ScrubbedTraitError<'tcx> { | |||
| /// A real error. This goal definitely does not hold. | |||
| TrueError, | |||
| // A select error with pending obligations | |||
| Select(PredicateObligations<'tcx>), | |||
There was a problem hiding this comment.
I understand that it's not really documented, but this is antithetical to the design of ScrubbedTraitError. That type should be very lightweight.
There was a problem hiding this comment.
Totally understand your concern. I tried this mainly as a quick hack. I did this because through my ad hoc debugging, I uncovered that select_where_possible contains the pending constraints that I needed to display within the error message. But since the returned details are erased via ScrubbedTraitError, I tried to pass on the pending obligations so that I can show it inside the error message.
I'm not sure what is the best way to workaround this, but at least with this quick hack, I'm able to use this fork of the Rust compiler to significantly simplify debugging for my projects. So regardless of which approach is taken, I probably do need to extract the pending obligations in one way or another and then surface it inside my error messages.
Btw, I have also considered alternative approaches, such as writing a compiler plugin to extract the pending obligations for my specific use cases. However, as far as I looked, I couldn't find ways to implement compiler plugins that help recover or better report compile errors. So I do hope that I can find some ways to fix this upstream in Rust, so that I don't need to instruct my users to use my custom fork of Rust compiler to use my project.
|
@compiler-errors thanks for the quick feedback! Following is my response:
I tried to further simplify the example code, but the original code I given is already the minimal example. This is because the issue only arise when there are at least two levels of indirections via two or more blanket implementations. When there is only one blanket implementation, the compiler actually gives correct diagnostic. Click to show simplified code with correct error messageCompared to the original, we removed the // The following code demonstrates an incomplete error message returned from
// the Rust compiler, when there are unsatisfied constraints present in
// code that makes use of _context-generic programming_, as described at
// https://patterns.contextgeneric.dev/.
//
// More details about similar error is described here:
// https://patterns.contextgeneric.dev/debugging-techniques.html
// The trait to link a consumer trait with a provide trait, as explained by:
// https://patterns.contextgeneric.dev/consumer-provider-link.html#blanket-consumer-trait-implementation
pub trait HasComponents {
type Components;
}
// Our example consumer trait, with the context type being the implicit `Self` type
pub trait CanFormatToString {
fn format_to_string(&self) -> String;
}
// Our example provider trait, with the context type being the explicit `Context` type
pub trait StringFormatter<Context> {
fn format_to_string(context: &Context) -> String;
}
// A blanket implementation that links the consumer `CanFormatToString` with
// the provider `StringFormatter`, using `HasComponents`
impl<Context> CanFormatToString for Context
where
Context: HasComponents,
Context::Components: StringFormatter<Context>,
{
fn format_to_string(&self) -> String {
Context::Components::format_to_string(self)
}
}
// An example provider for `StringFormatter`, which has a generic
// implementation that requires `Context: debug`
pub struct FormatWithDebug;
impl<Context> StringFormatter<Context> for FormatWithDebug
where
Context: core::fmt::Debug,
{
fn format_to_string(context: &Context) -> String {
format!("{:?}", context)
}
}
// An example concrete context.
// Note: we pretend to forgot to derive `Debug` to cause error to be raised.
// FIXME: Uncomment the line below to fix the error.
// #[derive(Debug)]
pub struct Person {
pub first_name: String,
pub last_name: String,
}
// The components tied to the `Person` context
pub struct PersonComponents;
// Implement the consumer traits for `Person` using
// the aggregated provider `PersonComponents`
impl HasComponents for Person {
type Components = FormatWithDebug;
}
// Checks that `Person` implements `CanFormatToString`
pub trait CanUsePerson: CanFormatToString {}
// This should raise an error, since we didn't implement `Debug` for `Person`
impl CanUsePerson for Person {}With this, the error message does show that I hope that at least the diff is small enough that you can see how the error message degraded with few lines of changes. So I guess the issue does not arise if only one level of blanket implementation is involved. However, note that for my use case, the Rust code may contain arbitrary deeply nested use of blanket implementations.
Understandable. This is my first attempt in fixing the problem, and I'm sure there are better ways to do it. I just want to use this as a starting point for discussion, to help you better understand what I'm trying to solve. If you can give me hint on where else I should look to better handle my specific use case without affecting general use cases, I'm happy to make another attempt to work on this. |
This is definitely not a minimal example. A minimal example would have no extra details -- names could be simplified, unnecessary fields removed from structs, associated types can be removed if they're not necessary to demonstrate the issue. Frankly, I think it's going to be pretty difficult to give the suggestion you want it to, because in the eyes of the compiler, both |
I see, thanks for the clarification! So I suppose it is more about minimizing the syntax, not the core semantics. I have made another attempt to minimize the example based on your suggestion. Following is the code that produces helpful error message: (playground) trait HasTarget {
type Target;
}
trait SelfToStr {
fn self_to_str(&self) -> String;
}
trait CtxToStr<Ctx> {
fn ctx_to_str(ctx: &Ctx) -> String;
}
impl<Ctx> SelfToStr for Ctx
where
Ctx: HasTarget,
Ctx::Target: CtxToStr<Ctx>,
{
fn self_to_str(&self) -> String {
Ctx::Target::ctx_to_str(self)
}
}
struct CtxToDebugStr;
impl<Ctx: core::fmt::Debug> CtxToStr<Ctx> for CtxToDebugStr {
fn ctx_to_str(ctx: &Ctx) -> String {
format!("{ctx:?}")
}
}
// Uncomment to fix
// #[derive(Debug)]
struct Ctx;
impl HasTarget for Ctx {
type Target = CtxToDebugStr;
}
trait UseCtx: SelfToStr {}
impl UseCtx for Ctx {}The error message for the example code above is: However, adding just an extra blanket implementation is sufficient to break the error message: (playground) impl<Ctx, T> CtxToStr<Ctx> for T
where
T: HasTarget,
T::Target: CtxToStr<Ctx>,
{
fn ctx_to_str(ctx: &Ctx) -> String {
T::Target::ctx_to_str(ctx)
}
}Now the error message becomes:
I think that based on Rust's coherence rules, it should be clear that only one of the two potential implementations for Looking at the pending obigations, we can also see that the trait solver correctly ignores the blanket implementation, and does not list For my fix in this PR, I am also inside the branch We can also see that from the current error report, the line When I have the free time, I plan to make another debugging dive into the compiler and see which other branch was taken when the blanket implementation was added. I'm guessing that perhaps if we can ignore the blanket implementation, then Rust would potentially print back out the helpful error message, both for the simplified example and for more complex cases. |
|
☔ The latest upstream changes (presumably #139354) made this pull request unmergeable. Please resolve the merge conflicts. |
|
It sounds like this is waiting on the outcome of that compiler debugging dive so I'm going to mark this as waiting on author. Since this currently modifies trait system internals, I'd rather let Michael review and approve when he's comfortable with the changes 🙂 |
wesleywiser
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #134346
Summary
This PR attempts to fix the issue in #134346, by additional
helphints for each unsatisfied indirect constraint inside a blanket implementation.Details
To provide the extra information, a new variant
Select(PredicateObligations<'tcx>)is added toScrubbedTraitError<'tcx>to extract thepending_obligationsreturned fromselect_where_possible. Then insidereport_similar_impl_candidates, we iterate through every pending obligation in the errors, and print them out as ahelphint.Potential Issues
This is my first contribution to the Rust compiler project. I'm not sure of the best way to present the error messages, or handle them anywhere else in the codebase. If there are better ways to implement the improvement, I'm happy to modify the PR according to the suggestions.
An unresolved issue here is that the fix is only implemented for the old Rust trait solver. Compared to
OldSolverError, I don't see thepending_obligationsfield to be present insideNextSolverError. So I'm unsure of the best way to keep this forward compatible with the upcoming trait solver.I'm also not sure if the fix here would produce too noisy errors outside of my specific use case. When I test this fix with more complex projects that I have, the error messages may contain many unresolved constraints. However when scanning through all items, I determined that none of the listed unresolved constraints should be considered uninformative noise. Hence, I do think that it is essential to have all unresolved constraints listed in the error message.