Improve diagnostics for ambiguous binops with as _ - #156591
Conversation
rustbot
added
S-waiting-on-author
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
|
r? @TaKO8Ki rustbot has assigned @TaKO8Ki. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
cc @mqudsi |
I not sure what you mean here, we are reusing existing |
|
github acting slow as usual -- the updates is yet to be processed |
Been checking the past 2 hours, but still seems to show nothing new... :( |
30e4f90 to
daf9b2e
Compare
It was a git cache issue 😪 |
daf9b2e to
c401e16
Compare
c401e16 to
af30177
Compare
|
@qaijuang Please reread my review comments (which you marked resolved) and try to implement my suggestions. |
|
Pinging @hkBst (I believe i've implemented your suggestions) |
This comment has been minimized.
This comment has been minimized.
I'm about to rebase |
ee63cc4 to
1977fc0
Compare
This comment has been minimized.
This comment has been minimized.
|
I'm not familiar with that area, @rustbot reroll |
There was a problem hiding this comment.
I've touched the cast type checking code before so here are two points I can comment on. Other than that, I unfortunately have to re-roll as well because I'm not otherwise familiar with type inference and especially not with the whole obligation topic that the meat of the diagnostic logic is about.
@rustbot reroll
|
|
||
| /// Prefer a pending operator ambiguity over a generic `as _` inference failure. | ||
| #[cold] | ||
| pub(crate) fn try_report_ambiguous_binop_for_infer_cast( |
There was a problem hiding this comment.
Since this is specific to casts, I would expect it to live in cast.rs, not in the general FnCtxt plumbing.
There was a problem hiding this comment.
Now moved to cast.rs
| if self.cast_ty.is_ty_var() { | ||
| self.cast_ty = | ||
| if let Some(guar) = fcx.try_report_ambiguous_binop_for_infer_cast(self.cast_span) { |
There was a problem hiding this comment.
If I understand correctly, both branches of the if let will end up reporting an error and setting self.cast_ty to an error type, and the rest of the check method won't do anything interesting (the cast-to-unsized branch shouldn't be taken, and then the "No sense in giving duplicate error messages" branch catches the error type).
If all of this is correct, then re-assigning self.cast_ty feels misleading to me. At first glance I thought this was a fix-up that alters the type so following logic can proceed a little further. I'd consider phrasing it like this:
self.cast_ty = fcx.resolve_vars_with_obligations();
debug!(...);
if self.cast_ty.is_ty_var() {
/* report ambiguity error or "type must be known" */
} else /* existing if-else ladder */
There was a problem hiding this comment.
It seems wrong to me, or at least unnecessary, that the code now leaves the self.cast_ty as a type variable instead of converting it to a type error. I don't fully understand what @hanna-kruppe was proposing, but I think we should keep the logic that sets self.cast_ty to an error type. The current version of this patch is quite bizarre as each branch of the if-else yields an ErrorGuaranteed but it's then swallowed by a semicolon.
camelid
added
S-waiting-on-author
28c113c to
da2b451
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| let cast_span = self.cast_span; | ||
|
|
||
| if let ObligationCauseCode::BinOp { rhs_span, .. } = obligation.cause.code() | ||
| && rhs_span.contains(cast_span) |
There was a problem hiding this comment.
seems we don't need to limit only rhs here, how about to also handle this testcase:
let _ = 42usize as _ == n;
| if (fcx.tcx.hir_span(*lhs_hir_id).contains(cast_span) | ||
| && lhs_ty.contains(self.cast_ty)) | ||
| || (rhs_span.contains(cast_span) && rhs_ty.contains(self.cast_ty)) | ||
| && matches!( |
There was a problem hiding this comment.
seems it's better to reject non-trait predicates before this if check, for example this code:
use std::ops::Add;
struct Rhs;
impl Add<Rhs> for u32 {
type Output = ();
fn add(self, _: Rhs) {}
}
impl Add<Rhs> for i32 {
type Output = ();
fn add(self, _: Rhs) {}
}
fn main() {
let _: () = 42usize as _ + Rhs;
}now we have two similar diagnostics:
error[E0284]: type annotations needed
--> /tmp/now.rs:18:28
|
18 | let _: () = 42usize as _ + Rhs;
| ^ cannot infer type
|
= note: cannot satisfy `<_ as Add<Rhs>>::Output == ()`
error[E0283]: type annotations needed
--> /tmp/now.rs:18:28
|
18 | let _: () = 42usize as _ + Rhs;
| ^ cannot infer type
|
note: multiple `impl`s satisfying `_: Add<Rhs>` found
--> /tmp/now.rs:5:1
|
5 | impl Add<Rhs> for u32 {
| ^^^^^^^^^^^^^^^^^^^^^
...
11 | impl Add<Rhs> for i32 {
| ^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 2 previous errors
b897f07 to
2527f0f
Compare
This comment has been minimized.
This comment has been minimized.
|
thanks! |
2527f0f to
9c80dd6
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. |
8 participants
View all comments
Fixes #156004
as _inside the RHS of an overloaded binary operation can leave the cast target unresolved while there is still a pending operator obligation that knows the more useful ambiguity.This PR keeps the normal resolved-cast path unchanged, such that when the cast target is still a type variable, cast checking now looks for a pending
BinOpobligation whose RHS contains the cast span and replays that obligation in a fresh diagnostic fulfillment context. If the replayed obligation is actually ambiguous, rustc reports the existing fulfillment ambiguity, including the competing impl candidates.Best reviewed commit-by-commit.