Skip to content

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

Merged
merged 1 commit into from
Jun 4, 2015
Merged
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
15 changes: 5 additions & 10 deletions src/compiletest/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use std::fmt;
use std::fs::{self, File};
use std::io::BufReader;
use std::io::prelude::*;
use std::iter::repeat;
use std::net::TcpStream;
use std::path::{Path, PathBuf};
use std::process::{Command, Output, ExitStatus};
Expand Down Expand Up @@ -928,12 +927,12 @@ fn check_forbid_output(props: &TestProps,
}
}

fn check_expected_errors(expected_errors: Vec<errors::ExpectedError> ,
fn check_expected_errors(expected_errors: Vec<errors::ExpectedError>,
testfile: &Path,
proc_res: &ProcRes) {

// true if we found the error in question
let mut found_flags: Vec<_> = repeat(false).take(expected_errors.len()).collect();
let mut found_flags = vec![false; expected_errors.len()];

if proc_res.status.success() {
fatal("process did not return an error status");
Expand All @@ -954,14 +953,10 @@ fn check_expected_errors(expected_errors: Vec<errors::ExpectedError> ,
}
}

// A multi-line error will have followup lines which will always
// start with one of these strings.
// A multi-line error will have followup lines which start with a space
// or open paren.
fn continuation( line: &str) -> bool {
line.starts_with(" expected") ||
line.starts_with(" found") ||
// 1234
// Should have 4 spaces: see issue 18946
line.starts_with("(")
line.starts_with(" ") || line.starts_with("(")
Copy link
Contributor Author

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: and end type: (each with a preceding space).

}

// Scan and extract our error/warning messages,
Expand Down
89 changes: 56 additions & 33 deletions src/librustc_typeck/check/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 require_same_types by this stage. Is there a version that'll write a concrete type for the inferred int type, or fail? Or do you think I should just add this error back in?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get an ICE because of the span_bug call I added in place of this.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll poke around in infer and see if I can get it to work 😄

);
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;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

require_same_types outputs a message, so just returning prevents duplication.

Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis, this works... there's no need to call it again with rhs_ty is there?

Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelsproul nope, that looks good to me. (calling it rhs_ty would yield the same result)


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);
Expand Down
44 changes: 41 additions & 3 deletions src/librustc_typeck/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,47 @@ match d {
```
"##,

E0029: r##"
In a match expression, only numbers and characters can be matched against a
range. This is because the compiler checks that the range is non-empty at
compile-time, and is unable to evaluate arbitrary comparison functions. If you
want to capture values of an orderable type between two end-points, you can use
a guard.

```
// The ordering relation for strings can't be evaluated at compile time,
// so this doesn't work:
match string {
"hello" ... "world" => ...
_ => ...
}

// This is a more general version, using a guard:
match string {
s if s >= "hello" && s <= "world" => ...
_ => ...
}
```
"##,

E0030: r##"
When matching against a range, the compiler verifies that the range is
non-empty. Range patterns include both end-points, so this is equivalent to
requiring the start of the range to be less than or equal to the end of the
range.

For example:

```
match 5u32 {
// This range is ok, albeit pointless.
1 ... 1 => ...
// This range is empty, and the compiler can tell.
1000 ... 5 => ...
}
```
"##,

E0033: r##"
This error indicates that a pointer to a trait type cannot be implicitly
dereferenced by a pattern. Every trait defines a type, but because the
Expand Down Expand Up @@ -1107,9 +1148,6 @@ For more information see the [opt-in builtin traits RFC](https://github.com/rust
}

register_diagnostics! {
E0029,
E0030,
E0031,
E0034, // multiple applicable methods in scope
E0035, // does not take type parameters
E0036, // incorrect number of type parameters given for this method
Expand Down
16 changes: 0 additions & 16 deletions src/test/compile-fail/match-ill-type1.rs

This file was deleted.

28 changes: 19 additions & 9 deletions src/test/compile-fail/match-range-fail.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,32 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//error-pattern: lower range bound
//error-pattern: only char and numeric types
//error-pattern: mismatched types

fn main() {
match 5 {
6 ... 1 => { }
_ => { }
6 ... 1 => { }
_ => { }
};
//~^^^ ERROR lower range bound must be less than or equal to upper

match "wow" {
"bar" ... "foo" => { }
};
//~^^ ERROR only char and numeric types are allowed in range
//~| start type: &'static str
//~| end type: &'static str

match "wow" {
"bar" ... "foo" => { }
10 ... "what" => ()
};
//~^^ ERROR only char and numeric types are allowed in range
//~| start type: _
//~| end type: &'static str

match 5 {
'c' ... 100 => { }
_ => { }
'c' ... 100 => { }
_ => { }
};
//~^^^ ERROR mismatched types in range
//~| expected char
//~| found integral variable
}
26 changes: 26 additions & 0 deletions src/test/run-pass/match-range-infer.rs
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.

Copy link
Member

Choose a reason for hiding this comment

The 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")
}
}