Clearer error diagnostic on type mismatch within a range by kevgrasso · Pull Request #67214 · rust-lang/rust

kevgrasso · 2019-12-11T03:01:37Z

closes #57389.
I tried implementing the expected because of this note 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.

rust-highfive · 2019-12-11T03:01:41Z

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2019-12-13T20:08:54Z

r? @estebank

Centril · 2019-12-13T20:10:52Z

src/librustc/traits/mod.rs

-    /// 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>>),


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.)

Centril · 2019-12-13T20:20:06Z

src/librustc_typeck/check/pat.rs

+            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
+                }
+            }
+        }


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.

estebank · 2019-12-13T21:47:50Z

src/librustc/infer/error_reporting/mod.rs

                    }
                }
+                if let Some((span, ty)) = other_range_expr {
+                    err.span_label(span, format!("other endpoint has type `{}`", ty));


I feel like "this endpoint has type will lend itself to less confusion in the output.

estebank · 2019-12-13T21:57:32Z

src/librustc_typeck/check/pat.rs

@@ -355,8 +355,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        expected: Ty<'tcx>,
        discrim_span: Option<Span>,
    ) -> Option<Ty<'tcx>> {


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.

estebank · 2019-12-13T21:59:28Z

src/librustc_typeck/check/pat.rs

@@ -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
            );


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.

bors · 2019-12-22T22:40:40Z

☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts.

@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

@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

@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

Dylan-DPC-zz · 2020-01-15T06:20:20Z

@kevgrasso can you rebase this? thanks :)

JohnCSimon · 2020-01-25T05:51:07Z

Pinging again from triage -
@kevgrasso - Unfortunately, I'm closing this out as inactive, when you have time to address the merge conflicts again please feel free to reopen this PR.
Note: please don't push to the PR while it is closed as that prevents it from being reopened.

kevgrasso added 2 commits December 10, 2019 20:37

clearer error diagnostic on type mismatch within a range

481bc0e

tests updated to match

3194585

rust-highfive assigned matthewjasper Dec 11, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2019

kevgrasso changed the title ~~E0308range~~ Clearer error diagnostic on type mismatch within a range Dec 12, 2019

rust-highfive assigned estebank and unassigned matthewjasper Dec 13, 2019

Centril reviewed Dec 13, 2019

View reviewed changes

estebank reviewed Dec 13, 2019

View reviewed changes

Centril mentioned this pull request Dec 13, 2019

typeck: note other end-point when checking range pats #67287

Merged

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2020

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 25, 2020

JohnCSimon closed this Jan 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clearer error diagnostic on type mismatch within a range#67214

Clearer error diagnostic on type mismatch within a range#67214
kevgrasso wants to merge 2 commits intorust-lang:masterfrom
kevgrasso:E0308range

kevgrasso commented Dec 11, 2019

Uh oh!

rust-highfive commented Dec 11, 2019

Uh oh!

Centril commented Dec 13, 2019

Uh oh!

Centril Dec 13, 2019

Uh oh!

Centril Dec 13, 2019

Uh oh!

estebank Dec 13, 2019

Uh oh!

estebank Dec 13, 2019

Uh oh!

estebank Dec 13, 2019

Uh oh!

bors commented Dec 22, 2019

Uh oh!

Dylan-DPC-zz commented Jan 15, 2020

Uh oh!

JohnCSimon commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

kevgrasso commented Dec 11, 2019

Uh oh!

rust-highfive commented Dec 11, 2019

Uh oh!

Centril commented Dec 13, 2019

Uh oh!

Centril Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

estebank Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

estebank Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

estebank Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 22, 2019

Uh oh!

Dylan-DPC-zz commented Jan 15, 2020

Uh oh!

JohnCSimon commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants