Improve error message for uninferrable types #38812 #39361

cengiz-io · 2017-01-28T14:41:35Z

Hello,

I tried to improve the error message for uninferrable types. The error code is E0282.

error[E0282]: type annotations needed
 --> /home/cengizIO/issue38812.rs:2:11
  |
2 |   let x = vec![];
  |       -   ^^^^^^ cannot infer type for `T`
  |       |
  |       consider giving `x` a type
  |
  = note: this error originates in a macro outside of the current crate

and

error[E0282]: type annotations needed
 --> /home/cengizIO/issue38812.rs:2:15
  |
2 |   let (x,) = (vec![],);
  |       ----    ^^^^^^ cannot infer type for `T`
  |       |
  |       consider giving a type to pattern
  |
  = note: this error originates in a macro outside of the current crate

What's changed?

Rust compiler now tries to find uninferred locals with type _ and adds them into the error message.

Wording

I'm probably wrong on wording that I used. Please feel free to suggest better alternatives.

Appreciation

Thanks @nikomatsakis for mentoring 🍺

Any comments/feedback is more than welcome!

Thank you

rust-highfive · 2017-01-28T14:41:42Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2017-01-28T22:22:21Z

r? @nikomatsakis

nikomatsakis · 2017-01-28T23:14:26Z

cc @dherman @jonathandturner @estebank -- any tips on wording much appreciated. One thing we weren't sure about was whether we could find a good way to integrate the "partial type" information that we have. For example, we we know that the variable x has type Vec<_>, we just don't know what the _ is.

nikomatsakis · 2017-01-28T23:15:06Z

src/librustc/traits/error_reporting.rs

+                                       "unable to fully infer type(s)");
+
+        err.note("type annotations or generic parameter binding required");
+        err.span_label(cause.span, &format!("cannot infer type"));


I think we should keep the old message here:

err.span_label(span, &format!("cannot infer type for `{}`", name));

In particular, the name is useful.

nikomatsakis · 2017-01-28T23:15:19Z

src/librustc/traits/error_reporting.rs

+        let mut err = struct_span_err!(self.tcx.sess,
+                                       cause.span,
+                                       E0282,
+                                       "unable to fully infer type(s)");


but I do prefer this message being short and snappy :)

It'd be nice if we could check how many types couldn't be fully inferred and provide the appropriate text.

Some possible things we could say:

"unable to fully infer types" -- I don't think the (s) is needed. There are always lots of types at play, a plural is always appropriate. =)

"type annotations needed"

"multiple possible types detected; annotations needed to clarify" -- or something like that?

I'd like to get across that rustc is refusing to guess what your program means, basically, by picking a random type on your behalf.

I think "type annotations needed" is the best of the three. IMHO it's better to tell me what to fix so that I can more quickly take the action to fix the error.

nikomatsakis · 2017-01-29T00:54:34Z

src/test/ui/codemap_tests/repair_span_std_macros.stderr

-   |             ^^^^^^ cannot infer type for `T`
+   |         -   ^^^^^^ cannot infer type
+   |         |
+   |         annotating the type for the variable `x` would help


can you add a similar test, but for the case where the thing being assigned is a more complex pattern?

Something like this:

let (x,) = (vec![],);

nikomatsakis · 2017-01-29T10:14:27Z

Hmm also this "note: type annotations or generic parameter binding required" feels a bit superfluous. I wonder where that comes from.

estebank · 2017-01-29T21:35:35Z

I feel the output would be slightly more helpful with something along the lines of:

error[E0282]: unable to fully infer type
  --> $DIR/missing-type-parameter.rs:14:5
   |
10 |     let x = Vec::new();
   |         -   ^^^^^^^^^^ cannot infer type for `T`
   |         |   |
   |         |   consider using `Vec::new<T>()` here, where `T` would be the appropriate type
   |         consider using `x: Vec<T>` here, where `T` would be the appropriate type
   = note: type annotations or generic parameter binding required

error[E0282]: unable to fully infer types
  --> $DIR/missing-type-parameter.rs:14:5
   |
10 |     let x = foo();
   |         -   ^^^^^ cannot infer type for `X` and `Y`
   |         |   |
   |         |   consider using `foo<(X, Y)>()` here, where `X` and `Y` would be the appropriate types
   |         consider using `x: (X, Y)` here, where `X` and `Y` would be the appropriate types
   = note: type annotations or generic parameter binding required

Thoughts?

sophiajt · 2017-01-30T20:30:38Z

Cool idea! I definitely like this approach.

It seems like we'll need to just pick the right place to annotate. Sometimes we'll have the variable decl to annotate, but other times we'll have to expression. As @estebank points out there's a bit of decision on which to choose, but I think we could come up with a heuristic that can just pick one of the choices.

As for the details:

I'm not a fan of the note. Do we need it?
I might make the secondary label "try explicitly giving x a type"
@nikomatsakis I do like the idea of telling us to fill in the blank, but I'm not sure of the best wording for that case.

nikomatsakis · 2017-01-30T21:20:25Z

@jonathandturner @estebank

It seems like we'll need to just pick the right place to annotate. Sometimes we'll have the variable decl to annotate, but other times we'll have to expression. As @estebank points out there's a bit of decision on which to choose, but I think we could come up with a heuristic that can just pick one of the choices.

So, my vision here was that we would have a series of places that we would suggest for where an annotation might be helpful. One option is a local variable; another is passing an explicit type parameter to a method or type etc. I don't think we should offer both -- that just seems overwhelming to me. But in any case I figured that eventually this code will grow with logic to gather the full set of possibilities and then pick the "best" thing to suggest. But for now, a local variable feels like a good starting point. Perhaps this is my personal bias: I try to avoid placing annotations anywhere but local variables. In general, I feel like if a type annotation is needed, the code is probably complex enough that I ought to introduce a variable anyway.

nikomatsakis · 2017-01-30T21:21:06Z

@jonathandturner

I'm not a fan of the note. Do we need it?

Which note are you referring to specifically? This one, "type annotations or generic parameter binding required"? If so, I agree we should remove it.

sophiajt · 2017-01-30T21:22:28Z

@nikomatsakis - yup, that's the one.

arielb1 · 2017-02-01T14:21:13Z

Maybe it would be better to complain about unresolved variables before doing the select_all_or_error call (which catches unresolved variables in most cases when resolving VAR: Sized)? It's not like you are going to resolve anything after the final selection. Then we can just span the variable directly.

nikomatsakis · 2017-02-01T17:19:54Z

@arielb1 either way, I think we will want to search the tree to find the place where we should put the suggestion; in general, it's not the case that the obligation span is likely to be what we want, I think, but also I think in general there will be many places one could put an annotation that would influence the type, and we'll want to grow good heuristics for picking the best/easiest one.

cengiz-io · 2017-02-02T21:37:15Z

src/test/ui/codemap_tests/issue-38812-2.rs

+// except according to those terms.
+
+fn main() {
+    let (x,) = (vec![],);


@nikomatsakis

cengiz-io · 2017-02-02T21:40:54Z

Hello @nikomatsakis

I removed the note, took another shot at messages and added a new test case.

@estebank @arielb1 and @jonathandturner Thanks for the feedback! 👍

arielb1 · 2017-02-02T22:46:10Z

@nikomatsakis

Which tree? Unresolved _: Sized obligations aren't a particularly good place to find missing type annotations. A typeck body pass that
a) finds missing type annotations
b) reports the correct error for them

Would be nice. Of course, also suppressing ambiguity errors for type variables that have already been reported (or just making all ambiguity errors ICEs after we're sure we can handle that) would be nice.

nikomatsakis · 2017-02-03T20:47:44Z

@arielb1 OK, I see your point. I agree that it'd be nice to do a walk of the tree and hunt down unresolved inference variables, but it seems to me that the code in question would be roughly the same either way. In other words, we'd still need some logic to walk the tree. I guess we could pull that logic into a bit of code in typeck, which I guess might be nicer, but it also seems reasonably logical to have it in the error-reporting module.

Getting the error from an obligation doesn't seem like the worst place to do it -- the only reason for us to ever fail to be able to resolve an obligation to either error or success is because of an uninferred type variable (it's not really specific to the Sized trait), right? However, I guess there are various corner cases where we don't encounter the error until writeback (e.g., let _x = []).

Still, it seems kind of repetitive that we have this writeback phase which diligently visits every type, but we also have to add another one to scour the tree and find them. An alternative is to keep the new code roughly where it is -- that is, have a function in the error-reporting logic for reporting an unconstrained type varable -- and then modify the writeback code to call this helper so we can give a better error in those cases (the current message is not as good).

cengiz-io · 2017-02-05T00:24:02Z

@arielb1 and @nikomatsakis : I don't have anything valuable to add to the discussion. That's why I'm spectating 🤔

If you think there will be no significant performance penalties or false positives, I say we get this merged and then start iterating over the better approach, if there'll be any.

You can also state any edge cases that you predict. Will be happy to test them

nikomatsakis · 2017-02-06T17:11:31Z

My feeling is that we should land this as is -- since it provides useful infrastructure for reporting unconstrained types -- and then add calls into writeback for the additional scenarios. @arielb1, are you ok with that?

arielb1 · 2017-02-06T21:03:29Z

yep

@bors r+

bors · 2017-02-06T21:03:29Z

📌 Commit 3fa28cb has been approved by arielb1

@nikomatsakis

Improve error message for uninferrable types rust-lang#38812 Hello, I tried to improve the error message for uninferrable types. The error code is `E0282`. ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:11 | 2 | let x = vec![]; | - ^^^^^^ cannot infer type for `T` | | | consider giving `x` a type | = note: this error originates in a macro outside of the current crate ``` and ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:15 | 2 | let (x,) = (vec![],); | ---- ^^^^^^ cannot infer type for `T` | | | consider giving a type to pattern | = note: this error originates in a macro outside of the current crate ``` Rust compiler now tries to find uninferred `local`s with type `_` and adds them into the error message. I'm probably wrong on wording that I used. Please feel free to suggest better alternatives. Thanks @nikomatsakis for mentoring 🍺 Any comments/feedback is more than welcome! Thank you

@nikomatsakis

Improve error message for uninferrable types rust-lang#38812 Hello, I tried to improve the error message for uninferrable types. The error code is `E0282`. ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:11 | 2 | let x = vec![]; | - ^^^^^^ cannot infer type for `T` | | | consider giving `x` a type | = note: this error originates in a macro outside of the current crate ``` and ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:15 | 2 | let (x,) = (vec![],); | ---- ^^^^^^ cannot infer type for `T` | | | consider giving a type to pattern | = note: this error originates in a macro outside of the current crate ``` Rust compiler now tries to find uninferred `local`s with type `_` and adds them into the error message. I'm probably wrong on wording that I used. Please feel free to suggest better alternatives. Thanks @nikomatsakis for mentoring 🍺 Any comments/feedback is more than welcome! Thank you

Rollup of 15 pull requests - Successful merges: #38764, #39361, #39426, #39462, #39482, #39557, #39561, #39582, #39583, #39587, #39598, #39599, #39601, #39602, #39604 - Failed merges:

@nikomatsakis

Improve error message for uninferrable types rust-lang#38812 Hello, I tried to improve the error message for uninferrable types. The error code is `E0282`. ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:11 | 2 | let x = vec![]; | - ^^^^^^ cannot infer type for `T` | | | consider giving `x` a type | = note: this error originates in a macro outside of the current crate ``` and ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:15 | 2 | let (x,) = (vec![],); | ---- ^^^^^^ cannot infer type for `T` | | | consider giving a type to pattern | = note: this error originates in a macro outside of the current crate ``` Rust compiler now tries to find uninferred `local`s with type `_` and adds them into the error message. I'm probably wrong on wording that I used. Please feel free to suggest better alternatives. Thanks @nikomatsakis for mentoring 🍺 Any comments/feedback is more than welcome! Thank you

@nikomatsakis

Improve error message for uninferrable types rust-lang#38812 Hello, I tried to improve the error message for uninferrable types. The error code is `E0282`. ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:11 | 2 | let x = vec![]; | - ^^^^^^ cannot infer type for `T` | | | consider giving `x` a type | = note: this error originates in a macro outside of the current crate ``` and ```rust error[E0282]: type annotations needed --> /home/cengizIO/issue38812.rs:2:15 | 2 | let (x,) = (vec![],); | ---- ^^^^^^ cannot infer type for `T` | | | consider giving a type to pattern | = note: this error originates in a macro outside of the current crate ``` Rust compiler now tries to find uninferred `local`s with type `_` and adds them into the error message. I'm probably wrong on wording that I used. Please feel free to suggest better alternatives. Thanks @nikomatsakis for mentoring 🍺 Any comments/feedback is more than welcome! Thank you

Rollup of 13 pull requests - Successful merges: #38764, #39361, #39372, #39374, #39400, #39426, #39431, #39459, #39482, #39545, #39593, #39620, #39621 - Failed merges:

eddyb · 2017-03-07T16:52:47Z

src/librustc/traits/error_reporting.rs

+
+        err.span_label(cause.span, &format!("cannot infer type for `{}`", name));
+
+        let expr = self.tcx.hir.expect_expr(cause.body_id);


This shouldn't be expect_expr as cause.body_id can also be a fn declaration.

Thanks for highlighting this.

Here's the proposed PR: #40404

rust-highfive assigned arielb1 Jan 28, 2017

rust-highfive assigned nikomatsakis and unassigned arielb1 Jan 28, 2017

nikomatsakis reviewed Jan 28, 2017

View reviewed changes

nikomatsakis reviewed Jan 29, 2017

View reviewed changes

cengiz-io commented Feb 2, 2017

View reviewed changes

cengiz-io added 4 commits February 4, 2017 16:25

Improve error message for uninferrable types #38812

8579218

Remove extra newlines from expectation files

89ae2ca

Remove extra note and revert name in message

7aff6ad

Add a new ui test and update existing ones

3fa28cb

frewsxcv mentioned this pull request Feb 7, 2017

Rollup of 18 pull requests #39605

Closed

frewsxcv mentioned this pull request Feb 7, 2017

Rollup of 18 pull requests #39609

Closed

frewsxcv mentioned this pull request Feb 7, 2017

Rollup of 15 pull requests #39613

Closed

frewsxcv mentioned this pull request Feb 8, 2017

Rollup of 34 pull requests #39632

Closed

frewsxcv mentioned this pull request Feb 8, 2017

Rollup of 13 pull requests #39638

Merged

bors added a commit that referenced this pull request Feb 8, 2017

Auto merge of #39638 - frewsxcv:rollup, r=frewsxcv

10f6a5c

Rollup of 13 pull requests - Successful merges: #38764, #39361, #39372, #39374, #39400, #39426, #39431, #39459, #39482, #39545, #39593, #39620, #39621 - Failed merges:

bors merged commit 3fa28cb into rust-lang:master Feb 8, 2017

cengiz-io mentioned this pull request Feb 21, 2017

error message for uninferrable type #38812

Closed

eddyb reviewed Mar 7, 2017

View reviewed changes

eddyb mentioned this pull request Mar 9, 2017

Compiler unnecessary panicked due to a mistake in where clause #40294

Closed

nikomatsakis mentioned this pull request Apr 17, 2017

Move E0101 and E0102 logic into new E0282 mechanism #40013 #41236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message for uninferrable types #38812 #39361

Improve error message for uninferrable types #38812 #39361

cengiz-io commented Jan 28, 2017 •

edited

Loading

rust-highfive commented Jan 28, 2017

nikomatsakis commented Jan 28, 2017

nikomatsakis commented Jan 28, 2017

nikomatsakis Jan 28, 2017

cengiz-io Jan 31, 2017

nikomatsakis Jan 28, 2017

estebank Jan 29, 2017

nikomatsakis Jan 30, 2017

sophiajt Jan 31, 2017

nikomatsakis Jan 29, 2017

cengiz-io Jan 31, 2017

nikomatsakis commented Jan 29, 2017

estebank commented Jan 29, 2017

sophiajt commented Jan 30, 2017

nikomatsakis commented Jan 30, 2017

nikomatsakis commented Jan 30, 2017

sophiajt commented Jan 30, 2017

arielb1 commented Feb 1, 2017 •

edited

Loading

nikomatsakis commented Feb 1, 2017

cengiz-io Feb 2, 2017

cengiz-io commented Feb 2, 2017

arielb1 commented Feb 2, 2017

nikomatsakis commented Feb 3, 2017

cengiz-io commented Feb 5, 2017

nikomatsakis commented Feb 6, 2017

arielb1 commented Feb 6, 2017

bors commented Feb 6, 2017

eddyb Mar 7, 2017 •

edited

Loading

cengiz-io Mar 9, 2017


		err.span_label(cause.span, &format!("cannot infer type for `{}`", name));

		let expr = self.tcx.hir.expect_expr(cause.body_id);

Improve error message for uninferrable types #38812 #39361

Improve error message for uninferrable types #38812 #39361

Conversation

cengiz-io commented Jan 28, 2017 • edited Loading

What's changed?

Wording

Appreciation

rust-highfive commented Jan 28, 2017

nikomatsakis commented Jan 28, 2017

nikomatsakis commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jan 29, 2017

estebank commented Jan 29, 2017

sophiajt commented Jan 30, 2017

nikomatsakis commented Jan 30, 2017

nikomatsakis commented Jan 30, 2017

sophiajt commented Jan 30, 2017

arielb1 commented Feb 1, 2017 • edited Loading

nikomatsakis commented Feb 1, 2017

Choose a reason for hiding this comment

cengiz-io commented Feb 2, 2017

arielb1 commented Feb 2, 2017

nikomatsakis commented Feb 3, 2017

cengiz-io commented Feb 5, 2017

nikomatsakis commented Feb 6, 2017

arielb1 commented Feb 6, 2017

bors commented Feb 6, 2017

eddyb Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cengiz-io commented Jan 28, 2017 •

edited

Loading

arielb1 commented Feb 1, 2017 •

edited

Loading

eddyb Mar 7, 2017 •

edited

Loading