Skip to content

Clearer error diagnostic on type mismatch within a range #67214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions src/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ use crate::hir::Node;
use crate::infer::{self, SuppressRegionErrors};
use crate::infer::opaque_types;
use crate::middle::region;
use crate::traits::{
IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
};
use crate::traits::{ObligationCause, ObligationCauseCode};
use crate::traits::{IfExpressionCause, PatternGuardCause, MatchExpressionArmCause};
use crate::ty::error::TypeError;
use crate::ty::{self, subst::{Subst, SubstsRef}, Region, Ty, TyCtxt, TypeFoldable};

Expand Down Expand Up @@ -607,22 +606,32 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
exp_found: Option<ty::error::ExpectedFound<Ty<'tcx>>>,
) {
match cause.code {
ObligationCauseCode::MatchExpressionArmPattern { span, ty } => {
if ty.is_suggestable() { // don't show type `_`
err.span_label(span, format!("this match expression has type `{}`", ty));
}
if let Some(ty::error::ExpectedFound { found, .. }) = exp_found {
if ty.is_box() && ty.boxed_ty() == found {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
err.span_suggestion(
span,
"consider dereferencing the boxed value",
format!("*{}", snippet),
Applicability::MachineApplicable,
);
ObligationCauseCode::PatternGuard(box PatternGuardCause {
match_expr_discrim, other_range_expr
}) => {
if let Some((span, ty)) = match_expr_discrim {
if ty.is_suggestable() { // don't show type `_`
err.span_label(
span,
format!("this match expression requires type `{}`", ty)
);
}
if let Some(ty::error::ExpectedFound { found, .. }) = exp_found {
if ty.is_box() && ty.boxed_ty() == found {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
err.span_suggestion(
span,
"consider dereferencing the boxed value",
format!("*{}", snippet),
Applicability::MachineApplicable,
);
}
}
}
}
if let Some((span, ty)) = other_range_expr {
err.span_label(span, format!("other endpoint has type `{}`", ty));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause {
source,
Expand Down Expand Up @@ -1338,6 +1347,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
_ => (false, None),
};
debug!("note_type_err: values={:?}", values);
let vals = match self.values_str(&values) {
Some((expected, found)) => Some((expected, found)),
None => {
Expand Down Expand Up @@ -1365,6 +1375,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
};

if let Some((expected, found)) = expected_found {
debug!(
"note_type_err: exp_found={:?}, expected={:?} found={:?}",
exp_found, expected, found
);
let expected_label = exp_found.map_or("type".into(), |ef| ef.expected.prefix_string());
let found_label = exp_found.map_or("type".into(), |ef| ef.found.prefix_string());
match (&terr, expected == found) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2273,7 +2273,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
match *cause_code {
ObligationCauseCode::ExprAssignable |
ObligationCauseCode::MatchExpressionArm { .. } |
ObligationCauseCode::MatchExpressionArmPattern { .. } |
ObligationCauseCode::PatternGuard { .. } |
ObligationCauseCode::IfExpression { .. } |
ObligationCauseCode::IfExpressionWithNoElse |
ObligationCauseCode::MainFunctionType |
Expand Down
10 changes: 8 additions & 2 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,8 @@ pub enum ObligationCauseCode<'tcx> {
/// Computing common supertype in the arms of a match expression
MatchExpressionArm(Box<MatchExpressionArmCause<'tcx>>),

/// 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>>),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Constants in patterns must have `Structural` type.
ConstPatternStructural,
Expand Down Expand Up @@ -299,6 +299,12 @@ pub struct MatchExpressionArmCause<'tcx> {
pub discrim_hir_id: hir::HirId,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct PatternGuardCause<'tcx> {
pub match_expr_discrim: Option<(Span, Ty<'tcx>)>,
pub other_range_expr: Option<(Span, Ty<'tcx>)>,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct IfExpressionCause {
pub then: Span,
Expand Down
9 changes: 6 additions & 3 deletions src/librustc/traits/structural_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,9 +532,12 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
})
})
}
super::MatchExpressionArmPattern { span, ty } => {
tcx.lift(&ty).map(|ty| super::MatchExpressionArmPattern { span, ty })
}
super::PatternGuard(box super::PatternGuardCause {
match_expr_discrim, other_range_expr,
}) => Some(super::PatternGuard(box super::PatternGuardCause {
match_expr_discrim: tcx.lift(&match_expr_discrim)?,
other_range_expr: tcx.lift(&other_range_expr)?,
})),
super::IfExpression(box super::IfExpressionCause { then, outer, semicolon }) => {
Some(super::IfExpression(box super::IfExpressionCause {
then,
Expand Down
11 changes: 2 additions & 9 deletions src/librustc_typeck/check/demand.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::check::FnCtxt;
use rustc::infer::InferOk;
use rustc::traits::{self, ObligationCause, ObligationCauseCode};
use rustc::traits::{self, ObligationCause};

use syntax::symbol::sym;
use syntax::util::parser::PREC_POSTFIX;
Expand Down Expand Up @@ -88,14 +88,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
actual: Ty<'tcx>,
match_expr_span: Option<Span>,
) -> Option<DiagnosticBuilder<'tcx>> {
let cause = if let Some(span) = match_expr_span {
self.cause(
cause_span,
ObligationCauseCode::MatchExpressionArmPattern { span, ty: expected },
)
} else {
self.misc(cause_span)
};
let cause = self.pat_guard_cause(None, cause_span, expected, match_expr_span);
self.demand_eqtype_with_origin(&cause, expected, actual)
}

Expand Down
21 changes: 20 additions & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ use rustc::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc::infer::unify_key::{ConstVariableOrigin, ConstVariableOriginKind};
use rustc::middle::region;
use rustc::mir::interpret::{ConstValue, GlobalId};
use rustc::traits::{self, ObligationCause, ObligationCauseCode, TraitEngine};
use rustc::traits::{self, ObligationCause, ObligationCauseCode, PatternGuardCause, TraitEngine};
use rustc::ty::{
self, AdtKind, CanonicalUserType, Ty, TyCtxt, Const, GenericParamDefKind,
ToPolyTraitRef, ToPredicate, RegionKind, UserType
Expand Down Expand Up @@ -2738,6 +2738,25 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.cause(span, ObligationCauseCode::MiscObligation)
}

pub fn pat_guard_cause(
&self,
other_range_expr: Option<(Span, Ty<'tcx>)>,
cause_span: Span,
expected: Ty<'tcx>,
match_expr_discrim: Option<Span>
) -> ObligationCause<'tcx> {
match (other_range_expr, match_expr_discrim) {
(Some(_), _) | (_, Some(_)) => self.cause(
cause_span, // error primary span
ObligationCauseCode::PatternGuard(box PatternGuardCause {
match_expr_discrim: match_expr_discrim.map(|span| (span, expected)),
other_range_expr
})
),
_ => self.misc(cause_span)
}
}

/// Resolves type and const variables in `ty` if possible. Unlike the infcx
/// version (resolve_vars_if_possible), this version will
/// also select obligations if it seems useful, in an effort
Expand Down
79 changes: 70 additions & 9 deletions src/librustc_typeck/check/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expected: Ty<'tcx>,
discrim_span: Option<Span>,
) -> Option<Ty<'tcx>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debug!("check_pat_range(begin={:?}, end={:?}, discrim_span={:?})",
begin, end, discrim_span
);

let lhs_ty = self.check_expr(begin);
let rhs_ty = self.check_expr(end);
debug!("check_pat_range: expected={:?}, expected.kind={:?}, lhs_ty={:?}, rhs_ty={:?}",
expected, expected.kind, lhs_ty, rhs_ty
);

// Check that both end-points are of numeric or char type.
let numeric_or_char = |ty: Ty<'_>| {
Expand All @@ -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
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return None;
}

// Now that we know the types can be unified we find the unified type and use
// it to type the entire expression.
let common_type = self.resolve_vars_if_possible(&lhs_ty);
None
} else {
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
}
}
}
Comment on lines +384 to +445
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

fn emit_err_pat_range(
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/block-result/issue-13624.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ error[E0308]: mismatched types
--> $DIR/issue-13624.rs:20:9
|
LL | match enum_struct_variant {
| ------------------- this match expression has type `()`
| ------------------- this match expression requires type `()`
LL | a::Enum::EnumStructVariant { x, y, z } => {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found enum `a::Enum`

Expand Down
8 changes: 5 additions & 3 deletions src/test/ui/error-codes/E0308-4.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
error[E0308]: mismatched types
--> $DIR/E0308-4.rs:4:9
--> $DIR/E0308-4.rs:4:15
|
LL | match x {
| - this match expression has type `u8`
| - this match expression requires type `u8`
LL | 0u8..=3i8 => (),
| ^^^^^^^^^ expected `u8`, found `i8`
| --- ^^^ expected `u8`, found `i8`
| |
| other endpoint has type `u8`

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ error[E0308]: mismatched types
--> $DIR/exclusive_range_pattern_syntax_collision.rs:5:13
|
LL | match [5..4, 99..105, 43..44] {
| ----------------------- this match expression has type `std::ops::Range<{integer}>`
| ----------------------- this match expression requires type `std::ops::Range<{integer}>`
LL | [_, 99.., _] => {},
| ^^^^ expected struct `std::ops::Range`, found integer
| ^^ expected struct `std::ops::Range`, found integer
|
= note: expected struct `std::ops::Range<{integer}>`
found type `{integer}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ error[E0308]: mismatched types
--> $DIR/exclusive_range_pattern_syntax_collision2.rs:5:13
|
LL | match [5..4, 99..105, 43..44] {
| ----------------------- this match expression has type `std::ops::Range<{integer}>`
| ----------------------- this match expression requires type `std::ops::Range<{integer}>`
LL | [_, 99..] => {},
| ^^^^ expected struct `std::ops::Range`, found integer
| ^^ expected struct `std::ops::Range`, found integer
|
= note: expected struct `std::ops::Range<{integer}>`
found type `{integer}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ LL | [..9, 99..100, _] => {},
| ^^^ help: try using the minimum value for the type: `MIN..9`

error[E0308]: mismatched types
--> $DIR/exclusive_range_pattern_syntax_collision3.rs:5:10
--> $DIR/exclusive_range_pattern_syntax_collision3.rs:5:12
|
LL | match [5..4, 99..105, 43..44] {
| ----------------------- this match expression has type `std::ops::Range<{integer}>`
| ----------------------- this match expression requires type `std::ops::Range<{integer}>`
LL | [..9, 99..100, _] => {},
| ^^^ expected struct `std::ops::Range`, found integer
| ^ expected struct `std::ops::Range`, found integer
|
= note: expected struct `std::ops::Range<{integer}>`
found type `{integer}`
Expand All @@ -19,7 +19,7 @@ error[E0308]: mismatched types
--> $DIR/exclusive_range_pattern_syntax_collision3.rs:5:15
|
LL | match [5..4, 99..105, 43..44] {
| ----------------------- this match expression has type `std::ops::Range<{integer}>`
| ----------------------- this match expression requires type `std::ops::Range<{integer}>`
LL | [..9, 99..100, _] => {},
| ^^^^^^^ expected struct `std::ops::Range`, found integer
|
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-11844.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0308]: mismatched types
--> $DIR/issue-11844.rs:6:9
|
LL | match a {
| - this match expression has type `std::option::Option<std::boxed::Box<{integer}>>`
| - this match expression requires type `std::option::Option<std::boxed::Box<{integer}>>`
LL | Ok(a) =>
| ^^^^^ expected enum `std::option::Option`, found enum `std::result::Result`
|
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-12552.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0308]: mismatched types
--> $DIR/issue-12552.rs:6:5
|
LL | match t {
| - this match expression has type `std::result::Result<_, {integer}>`
| - this match expression requires type `std::result::Result<_, {integer}>`
LL | Some(k) => match k {
| ^^^^^^^ expected enum `std::result::Result`, found enum `std::option::Option`
|
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-13466.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0308]: mismatched types
--> $DIR/issue-13466.rs:8:9
|
LL | let _x: usize = match Some(1) {
| ------- this match expression has type `std::option::Option<{integer}>`
| ------- this match expression requires type `std::option::Option<{integer}>`
LL | Ok(u) => u,
| ^^^^^ expected enum `std::option::Option`, found enum `std::result::Result`
|
Expand All @@ -13,7 +13,7 @@ error[E0308]: mismatched types
--> $DIR/issue-13466.rs:14:9
|
LL | let _x: usize = match Some(1) {
| ------- this match expression has type `std::option::Option<{integer}>`
| ------- this match expression requires type `std::option::Option<{integer}>`
...
LL | Err(e) => panic!(e)
| ^^^^^^ expected enum `std::option::Option`, found enum `std::result::Result`
Expand Down
Loading