-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve diagnostic messages for range patterns. #25743
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,41 +83,64 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, | |
demand::suptype(fcx, pat.span, expected, pat_ty); | ||
} | ||
ast::PatRange(ref begin, ref end) => { | ||
check_expr(fcx, &**begin); | ||
check_expr(fcx, &**end); | ||
|
||
let lhs_ty = fcx.expr_ty(&**begin); | ||
let rhs_ty = fcx.expr_ty(&**end); | ||
|
||
let lhs_eq_rhs = | ||
require_same_types( | ||
tcx, Some(fcx.infcx()), false, pat.span, lhs_ty, rhs_ty, | ||
|| "mismatched types in range".to_string()); | ||
|
||
let numeric_or_char = | ||
lhs_eq_rhs && (ty::type_is_numeric(lhs_ty) || ty::type_is_char(lhs_ty)); | ||
|
||
if numeric_or_char { | ||
match const_eval::compare_lit_exprs(tcx, &**begin, &**end, Some(lhs_ty), | ||
|id| {fcx.item_substs()[&id].substs | ||
.clone()}) { | ||
Some(Ordering::Less) | | ||
Some(Ordering::Equal) => {} | ||
Some(Ordering::Greater) => { | ||
span_err!(tcx.sess, begin.span, E0030, | ||
"lower range bound must be less than upper"); | ||
} | ||
None => { | ||
span_err!(tcx.sess, begin.span, E0031, | ||
"mismatched types in range"); | ||
} | ||
} | ||
} else { | ||
span_err!(tcx.sess, begin.span, E0029, | ||
"only char and numeric types are allowed in range"); | ||
check_expr(fcx, begin); | ||
check_expr(fcx, end); | ||
|
||
let lhs_ty = fcx.expr_ty(begin); | ||
let rhs_ty = fcx.expr_ty(end); | ||
|
||
// Check that both end-points are of numeric or char type. | ||
let numeric_or_char = |t| ty::type_is_numeric(t) || ty::type_is_char(t); | ||
let lhs_compat = numeric_or_char(lhs_ty); | ||
let rhs_compat = numeric_or_char(rhs_ty); | ||
|
||
if !lhs_compat || !rhs_compat { | ||
let span = if !lhs_compat && !rhs_compat { | ||
pat.span | ||
} else if !lhs_compat { | ||
begin.span | ||
} else { | ||
end.span | ||
}; | ||
|
||
// Note: spacing here is intentional, we want a space before "start" and "end". | ||
span_err!(tcx.sess, span, E0029, | ||
"only char and numeric types are allowed in range patterns\n \ | ||
start type: {}\n end type: {}", | ||
fcx.infcx().ty_to_string(lhs_ty), | ||
fcx.infcx().ty_to_string(rhs_ty) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nrc: I worked out that by removing this line I broke the match-ill-typed test I mentioned earlier. This error seems redundant as we've already run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you get an error when you run that test? If we do get an error, just a different one, then you can just change the test and its fine (assuming the error is not less precise). If you get no error at all, I would be more concerned. I guess if this is causing inference to infer the type of the lhs of the range due to the explicit type on the rhs of the range (in the test), then it is Ok. If we're just sort of ignoring the problem, then it's not Ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get an ICE because of the The full example is this: fn main() {
match 1 {
1 ... 2_usize => 1, //~ ERROR mismatched types in range
_ => 2,
};
} I think the best behaviour would be to infer the type of the start argument to match the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll poke around in |
||
); | ||
return; | ||
} | ||
|
||
// Check that the types of the end-points can be unified. | ||
let types_unify = require_same_types( | ||
tcx, Some(fcx.infcx()), false, pat.span, rhs_ty, lhs_ty, | ||
|| "mismatched types in range".to_string() | ||
); | ||
|
||
// It's ok to return without a message as `require_same_types` prints an error. | ||
if !types_unify { | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be an error message in this case, or is it allowed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment saying that please? |
||
} | ||
|
||
fcx.write_ty(pat.id, lhs_ty); | ||
// 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 = fcx.infcx().resolve_type_vars_if_possible(&lhs_ty); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nikomatsakis, this works... there's no need to call it again with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @michaelsproul nope, that looks good to me. (calling it |
||
|
||
fcx.write_ty(pat.id, common_type); | ||
|
||
// Finally we evaluate the constants and check that the range is non-empty. | ||
let get_substs = |id| fcx.item_substs()[&id].substs.clone(); | ||
match const_eval::compare_lit_exprs(tcx, begin, end, Some(&common_type), get_substs) { | ||
Some(Ordering::Less) | | ||
Some(Ordering::Equal) => {} | ||
Some(Ordering::Greater) => { | ||
span_err!(tcx.sess, begin.span, E0030, | ||
"lower range bound must be less than or equal to upper"); | ||
} | ||
None => tcx.sess.span_bug(begin.span, "literals of different types in range pat") | ||
} | ||
|
||
// subtyping doesn't matter here, as the value is some kind of scalar | ||
demand::eqtype(fcx, pat.span, expected, lhs_ty); | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment explaining what is being tested please? |
||
// Test that type inference for range patterns works correctly (is bi-directional). | ||
|
||
pub fn main() { | ||
match 1 { | ||
1 ... 3 => {} | ||
_ => panic!("should match range") | ||
} | ||
match 1 { | ||
1 ... 3u16 => {} | ||
_ => panic!("should match range with inferred start type") | ||
} | ||
match 1 { | ||
1u16 ... 3 => {} | ||
_ => panic!("should match range with inferred end type") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change was required to support
start type:
andend type:
(each with a preceding space).