Skip to content
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

Make need_type_info_err more conservative #73027

Merged
merged 2 commits into from
Jun 20, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 15 additions & 2 deletions src/librustc_infer/infer/error_reporting/need_type_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ impl<'a, 'tcx> Visitor<'tcx> for FindHirNodeVisitor<'a, 'tcx> {
if let (None, Some(ty)) =
(self.found_local_pattern, self.node_ty_contains_target(local.hir_id))
{
// There's a trade-off here - we can either check that our target span
doctorn marked this conversation as resolved.
Show resolved Hide resolved
// is contained in `local.span` or not. If we choose to check containment
// we can avoid some spurious suggestions (see #72690), but we lose
// the ability to report on things like:
//
// ```
// let x = vec![];
// ```
//
// because the target span will be in the macro expansion of `vec![]`.
// At present we choose not to check containment.
self.found_local_pattern = Some(&*local.pat);
self.found_node_ty = Some(ty);
}
Expand All @@ -99,8 +110,10 @@ impl<'a, 'tcx> Visitor<'tcx> for FindHirNodeVisitor<'a, 'tcx> {
if let (None, Some(ty)) =
(self.found_arg_pattern, self.node_ty_contains_target(param.hir_id))
{
self.found_arg_pattern = Some(&*param.pat);
self.found_node_ty = Some(ty);
if self.target_span.contains(param.pat.span) {
self.found_arg_pattern = Some(&*param.pat);
self.found_node_ty = Some(ty);
}
}
}
intravisit::walk_body(self, body);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0282]: type annotations needed
--> $DIR/expect-two-infer-vars-supply-ty-with-bound-region.rs:8:27
--> $DIR/expect-two-infer-vars-supply-ty-with-bound-region.rs:8:5
|
LL | with_closure(|x: u32, y| {});
| ^ consider giving this closure parameter a type
| ^^^^^^^^^^^^ cannot infer type for type parameter `B` declared on the function `with_closure`
Comment on lines 1 to +5
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a regression, right?

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 think this is the cost of making the suggestion more conservative. The point is at the moment we're struggling with things like this playground which urgently requires attention


error: aborting due to previous error

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-23046.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn let_<'var, VAR, F: for<'v> Fn(Expr<'v, VAR>) -> Expr<'v, VAR>>
}

fn main() {
let ex = |x| { //~ ERROR type annotations needed
let_(add(x,x), |y| {
let ex = |x| {
let_(add(x,x), |y| { //~ ERROR type annotations needed
let_(add(x, x), |x|x)})};
}
13 changes: 9 additions & 4 deletions src/test/ui/issues/issue-23046.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
error[E0282]: type annotations needed for `Expr<'_, VAR>`
--> $DIR/issue-23046.rs:17:15
error[E0282]: type annotations needed for the closure `fn(Expr<'_, _>) -> Expr<'_, _>`
--> $DIR/issue-23046.rs:18:9
|
LL | let ex = |x| {
| ^ consider giving this closure parameter the explicit type `Expr<'_, VAR>`, where the type parameter `VAR` is specified
LL | let_(add(x,x), |y| {
| ^^^^ cannot infer type for type parameter `VAR` declared on the function `let_`
|
help: give this closure an explicit return type without `_` placeholders
|
LL | let_(add(x, x), |x|-> Expr<'_, _> { x })})};
| ^^^^^^^^^^^^^^^^ ^
Comment on lines +1 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

...so it could be argued is this (but inference works with either suggestion, so this is acceptable).


error: aborting due to previous error

Expand Down
62 changes: 62 additions & 0 deletions src/test/ui/issues/issue-72690.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
fn no_err() {
|x: String| x;
let _ = String::from("x");
}

fn err() {
String::from("x".as_ref()); //~ ERROR type annotations needed
}

fn arg_pat_closure_err() {
|x| String::from("x".as_ref()); //~ ERROR type annotations needed
}

fn local_pat_closure_err() {
let _ = "x".as_ref(); //~ ERROR type annotations needed
}

fn err_first_arg_pat() {
String::from("x".as_ref()); //~ ERROR type annotations needed
|x: String| x;
}

fn err_second_arg_pat() {
|x: String| x;
String::from("x".as_ref()); //~ ERROR type annotations needed
}

fn err_mid_arg_pat() {
|x: String| x;
|x: String| x;
|x: String| x;
|x: String| x;
String::from("x".as_ref()); //~ ERROR type annotations needed
|x: String| x;
|x: String| x;
|x: String| x;
|x: String| x;
}

fn err_first_local_pat() {
String::from("x".as_ref()); //~ ERROR type annotations needed
let _ = String::from("x");
}

fn err_second_local_pat() {
let _ = String::from("x");
String::from("x".as_ref()); //~ ERROR type annotations needed
}

fn err_mid_local_pat() {
let _ = String::from("x");
let _ = String::from("x");
let _ = String::from("x");
let _ = String::from("x");
String::from("x".as_ref()); //~ ERROR type annotations needed
let _ = String::from("x");
let _ = String::from("x");
let _ = String::from("x");
let _ = String::from("x");
}

fn main() {}
88 changes: 88 additions & 0 deletions src/test/ui/issues/issue-72690.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
error[E0283]: type annotations needed
--> $DIR/issue-72690.rs:7:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`

error[E0282]: type annotations needed
--> $DIR/issue-72690.rs:11:6
|
LL | |x| String::from("x".as_ref());
| ^ consider giving this closure parameter a type

error[E0283]: type annotations needed
--> $DIR/issue-72690.rs:15:17
|
LL | let _ = "x".as_ref();
| ^^^^^^ cannot infer type for type `str`
|
= note: cannot satisfy `str: std::convert::AsRef<_>`

error[E0283]: type annotations needed
--> $DIR/issue-72690.rs:19:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`

error[E0283]: type annotations needed
--> $DIR/issue-72690.rs:25:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`

error[E0283]: type annotations needed
--> $DIR/issue-72690.rs:33:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`

error[E0283]: type annotations needed for `std::string::String`
--> $DIR/issue-72690.rs:41:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
LL | let _ = String::from("x");
| - consider giving this pattern a type
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`
Comment on lines +51 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to still give an incorrect suggestion.

Copy link
Contributor Author

@doctorn doctorn Jun 5, 2020

Choose a reason for hiding this comment

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

For these two incorrect suggestions the immediate solution is to require that local.span contains target_span, but doing so causes a huge number of other regressions - hence the trade-off discussed in the comment I added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(There's some discussion in this thread)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just looked at the thread. I would feel better about merging this PR as is if we had someone focusing on adding better tracking to ObligationCause. I already did a lot there for obligations in the type namespace but won't have time to look at it in value namespace. Would you be interested in digging into doing that?

Copy link
Contributor Author

@doctorn doctorn Jun 10, 2020

Choose a reason for hiding this comment

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

Yeah definitely! If you send me some details about what needs doing/where to start I can get on that right away


error[E0283]: type annotations needed for `std::string::String`
--> $DIR/issue-72690.rs:47:5
|
LL | let _ = String::from("x");
| - consider giving this pattern a type
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`
Comment on lines +62 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

...and this.


error[E0283]: type annotations needed for `std::string::String`
--> $DIR/issue-72690.rs:55:5
|
LL | let _ = String::from("x");
| - consider giving this pattern a type
...
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`

error: aborting due to 9 previous errors

Some errors have detailed explanations: E0282, E0283.
For more information about an error, try `rustc --explain E0282`.