Clearer error diagnostic on type mismatch within a range #67214
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
kevgrasso
changed the title
|
r? @estebank |
| /// Computing common supertype in the pattern guard for the arms of a match expression | ||
| MatchExpressionArmPattern { span: Span, ty: Ty<'tcx> }, | ||
| /// Computing common supertype in a pattern guard | ||
| PatternGuard(Box<PatternGuardCause<'tcx>>), |
There was a problem hiding this comment.
PatternGuard feels misleading as the usage in demand_eqtype_pat_diag is about patterns. (There's also no such thing as a pattern guard. Only arms have guards.)
| use rustc::traits::PredicateObligations; | ||
| use infer::InferOk; | ||
|
|
||
| let at_cause_eq = | | ||
| expect_ty: Ty<'tcx>, actual_type: Ty<'tcx>, cause_span: Span, | ||
| other_expr: Option<(Span, Ty<'tcx>)> | ||
| | { | ||
| if !actual_type.references_error() { | ||
| let other_expr = other_expr.filter(|(_, ty)| !ty.references_error()); | ||
| let cause = self.pat_guard_cause( | ||
| other_expr, cause_span, expected, discrim_span | ||
| ); | ||
|
|
||
| let result = self.at(&cause, self.param_env).eq(expect_ty, actual_type); | ||
| result.map_err(|terr| (cause, terr)) | ||
| } else { | ||
| Ok(InferOk {obligations: PredicateObligations::new(), value: ()}) | ||
| } | ||
| }; | ||
|
|
||
| // test if sides both sides of range have the compatible types | ||
| let lhs_result = at_cause_eq(expected, lhs_ty, begin.span, Some((end.span, rhs_ty))); | ||
| let rhs_result = at_cause_eq(expected, rhs_ty, end.span, Some((begin.span, lhs_ty))); | ||
|
|
||
| let joined_result = match (lhs_result, rhs_result) { | ||
| // full success, move forwards | ||
| ( | ||
| Ok(InferOk { obligations: lhs_obligation, value: () }), | ||
| Ok(InferOk { obligations: rhs_obligation, value: () }) | ||
| ) => { | ||
| self.register_predicates(lhs_obligation); | ||
| self.register_predicates(rhs_obligation); | ||
|
|
||
| // Now that we know the types can be unified we find the unified type and use | ||
| // it to type the entire expression. | ||
| Ok(self.resolve_vars_if_possible(&lhs_ty)) | ||
| }, | ||
| // only lhs is wrong | ||
| (Err((cause, terr)), Ok(_)) => | ||
| Err(self.report_mismatched_types(&cause, expected, lhs_ty, terr)), | ||
| // only rhs is wrong | ||
| (Ok(_), Err((cause, terr))) => | ||
| Err(self.report_mismatched_types(&cause, expected, rhs_ty, terr)), | ||
| // both sides are wrong | ||
| (Err(_), Err((rhs_cause, terr))) => { | ||
| if let Ok(_) = at_cause_eq(lhs_ty, rhs_ty, Span::default(), None) { | ||
| let cause = self.pat_guard_cause(None, span, expected, discrim_span); | ||
| Err(self.report_mismatched_types(&cause, expected, rhs_ty, terr)) | ||
| } else { | ||
| Err(self.report_mismatched_types(&rhs_cause, expected, rhs_ty, terr)) | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Subtyping doesn't matter here, as the value is some kind of scalar. | ||
| self.demand_eqtype_pat(span, expected, lhs_ty, discrim_span); | ||
| self.demand_eqtype_pat(span, expected, rhs_ty, discrim_span); | ||
| Some(common_type) | ||
| match joined_result { | ||
| Ok(common_type) => Some(common_type), | ||
| Err(mut err) => { | ||
| err.emit(); | ||
| None | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not happy about the amount of special casing, rawness, and the fragility of this type checking logic.
Looking right now to see if there's a less complicated way to get near the same results.
| } | ||
| } | ||
| } | ||
| if let Some((span, ty)) = other_range_expr { | ||
| err.span_label(span, format!("other endpoint has type `{}`", ty)); |
There was a problem hiding this comment.
I feel like "this endpoint has type will lend itself to less confusion in the output.
| @@ -355,8 +355,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| expected: Ty<'tcx>, | |||
| discrim_span: Option<Span>, | |||
| ) -> Option<Ty<'tcx>> { | |||
There was a problem hiding this comment.
I feel that if you change the return type to Result<Ty<'tcx>, DiagnosticBuilder<'tcx>> it will let you clean up a few lines below. Just make sure that in the single check_pat_range caller you emit the error.
| @@ -371,17 +378,71 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| self.emit_err_pat_range( | |||
| span, begin.span, end.span, lhs_fail, rhs_fail, lhs_ty, rhs_ty | |||
| ); | |||
There was a problem hiding this comment.
You'll have to change emit_err_pat_range to return the error instead of emitting it.
All of this then becomes
return self.emit_err_pat_range(
span, begin.span, end.span, lhs_fail, rhs_fail, lhs_ty, rhs_ty
span, begin.span, end.span, lhs_fail, rhs_fail, lhs_ty, rhs_ty
);
which lets you have the else clause implicit, removing one indentation level.
|
☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts. |
…, r=estebank typeck: note other end-point when checking range pats Fixes rust-lang#57389, alternative to rust-lang#67214 that should be less invasive to type checking logic. r? @estebank
…, r=estebank typeck: note other end-point when checking range pats Fixes rust-lang#57389, alternative to rust-lang#67214 that should be less invasive to type checking logic. r? @estebank
…, r=estebank typeck: note other end-point when checking range pats Fixes rust-lang#57389, alternative to rust-lang#67214 that should be less invasive to type checking logic. r? @estebank
|
@kevgrasso can you rebase this? thanks :) |
Dylan-DPC-zz
added
S-waiting-on-author
JohnCSimon
added
the
S-inactive
|
Pinging again from triage - |
closes #57389.
I tried implementing the
expected because of thisnote as shown in the example for the original issue, but I decided it would be easier and more useful to just annotate the type for the other endpoint of the range, no matter what.