Add more detail to type inference error by estebank · Pull Request #61361 · rust-lang/rust

estebank · 2019-05-30T17:24:54Z

When encountering code where type inference fails, add more actionable
information:

fn main() {
    let foo = Vec::new();
}

error[E0282]: type annotations needed in `std::vec::Vec<T>`
  --> $DIR/vector-no-ann.rs:2:16
   |
LL |     let foo = Vec::new();
   |         ---   ^^^^^^^^ cannot infer type for `T` in `std::vec::Vec<T>`
   |         |
   |         consider giving `foo` a type

Fix #25633.

rust-highfive · 2019-05-30T17:24:59Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2019-05-30T17:28:16Z

cc @rust-lang/wg-diagnostics

We still need to modify type printing to optionally accept a TypeVariableTable in order to properly print std::vec::Vec<T>.

I think that's an important thing to focus on in the short/medium term, together with using the local ident for external paths (std::vec::Vec -> Vec).

src/test/ui/issues/issue-23046.stderr

src/librustc/infer/error_reporting/need_type_info.rs

src/librustc/ty/print/pretty.rs

Centril

I'm still not that sold on replacing _ but there seem to be bugs?

src/librustc/ty/print/pretty.rs

src/test/ui/issues/issue-20261.stderr

src/test/ui/issues/issue-17551.stderr

src/test/ui/issues/issue-7813.stderr

estebank · 2019-05-31T23:27:04Z

@varkor would you prefer the following output?

error[E0282]: type annotations needed for `std::vec::Vec<T>`
  --> $DIR/vector-no-ann.rs:2:16
   |
LL |     let foo = Vec::new();
   |         ---   ^^^^^^^^ cannot infer type for `T`
   |         |
   |         consider giving `foo` the type `std::vec::Vec<T>` with the type parameter `T` specified

We could also provide structured suggestions now that we have the type names, but I would like help to bikeshed the wording:

error[E0282]: type annotations needed for `T` in `std::vec::Vec<T>`
  --> $DIR/vector-no-ann.rs:2:16
   |
LL |     let foo = Vec::new();
   |         ---   ^^^^^^^^ cannot infer type
   |         |
   |         help: give `foo` a type with the type parameter `T` specified: `foo: std::vec::Vec<T>`

Also, I'm not sure we can verify with certainty that we're pointing at a let binding that doesn't have a type set, so I would prefer not to give a suggestion just yet.

For patterns I need to change the wording as it is subtly incorrect now.

When encountering code where type inference fails, add more actionable information: ``` fn main() { let foo = Vec::new(); } ``` ``` error[E0282]: type annotations needed for `std::vec::Vec<_>` --> $DIR/vector-no-ann.rs:2:16 | LL | let foo = Vec::new(); | --- ^^^^^^^^ cannot infer type for `T` | | | consider giving `foo` the type `std::vec::Vec<_>` with the type parameter `T` specified ``` We still need to modify type printing to optionally accept a `TypeVariableTable` in order to properly print `std::vec::Vec<T>`. CC rust-lang#25633.

varkor · 2019-06-01T19:55:10Z

src/test/ui/unboxed-closures/unboxed-closures-failed-recursive-fn-2.stderr

   |
 LL |     let mut closure0 = None;
-   |         ------------ consider giving `closure0` a type
+   |         ------------ consider giving `closure0` the type `std::option::Option<T>` with the type parameter `_` specified


I think it's a problem when the type parameters don't match up. (It'd be better for both to be _, as long as it's unambiguous.)

I believe what is happening here is that _ and T are actually not unified, they are separate types as far as typeck is concerned. I'm not sure what the best way to go about it here would be :-/

Cheated a bit and changed it to be:

error[E0282]: type annotations needed for `std::option::Option<T>` --> $DIR/unboxed-closures-failed-recursive-fn-2.rs:16:32 | LL | let mut closure0 = None; | ------------ consider giving `closure0` the explicit type `std::option::Option<T>`, with the type parameters specified ... LL | return c(); | ^^^ cannot infer type | = note: type must be known at this point

Yeah, that's probably better.

varkor · 2019-06-01T20:00:37Z

I think a tweak to the message makes it slightly clearer (i.e. not to have users thinking: "but foo has a type already...").

error[E0282]: type annotations needed for `std::vec::Vec<T>`
  --> $DIR/vector-no-ann.rs:2:16
   |
LL |     let foo = Vec::new();
   |         ---   ^^^^^^^^ cannot infer type for `T`
   |         |
   |         consider giving `foo` the explicit type `std::vec::Vec<T>`, where the type parameter `T` is specified

Also, I'm not sure we can verify with certainty that we're pointing at a let binding that doesn't have a type set, so I would prefer not to give a suggestion just yet.

Structured suggestions would be nice, but only if we don't get it wrong, yeah.

…name is `_`

varkor · 2019-06-03T00:04:16Z

Okay, I think this is good now! Thanks for being patient with all the back and forth about wording 😄

r=me if you're happy with the wording now too.

estebank · 2019-06-03T02:11:17Z

@bors r=varkor

Thanks for being patient with all the back and forth about wording

@varkor, we have high standards 😉

I'm just happy that the ty printing refactor made implementing this not only feasible, but rather straightforward, if hacky.

bors · 2019-06-03T02:11:19Z

📌 Commit e420f44 has been approved by varkor

bors · 2019-06-03T02:45:42Z

⌛ Testing commit e420f44 with merge d59dcb2...

Add more detail to type inference error When encountering code where type inference fails, add more actionable information: ``` fn main() { let foo = Vec::new(); } ``` ``` error[E0282]: type annotations needed in `std::vec::Vec<T>` --> $DIR/vector-no-ann.rs:2:16 | LL | let foo = Vec::new(); | --- ^^^^^^^^ cannot infer type for `T` in `std::vec::Vec<T>` | | | consider giving `foo` a type ``` Fix #25633.

bors · 2019-06-03T05:40:35Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: varkor
Pushing d59dcb2 to master...

eddyb · 2019-06-10T08:38:46Z

src/librustc/ty/print/pretty.rs


    pub region_highlight_mode: RegionHighlightMode,
+
+    pub name_resolver: Option<Box<&'a dyn Fn(ty::sty::TyVid) -> Option<String>>>,


Please cc me on ty::print changes - but also, I have a few questions, such as "why is this a Box<&dyn Trait>?" - you can just remove the Box and everything would still work.

eddyb · 2019-06-10T08:39:57Z

src/librustc/ty/print/pretty.rs

-            ty::Infer(infer_ty) => p!(write("{}", infer_ty)),
+            ty::Infer(infer_ty) => {
+                if let ty::TyVar(ty_vid) = infer_ty {
+                    if let Some(name) = self.infer_ty_name(ty_vid) {


I'd probably override this in FmtPrinter's print_ty tbh, instead of adding infer_ty_name.

eddyb · 2019-06-10T08:40:54Z

src/librustc/ty/print/pretty.rs


 impl<F: fmt::Write> PrettyPrinter<'gcx, 'tcx> for FmtPrinter<'_, 'gcx, 'tcx, F> {
+    fn infer_ty_name(&self, id: ty::TyVid) -> Option<String> {
+        self.0.name_resolver.as_ref().and_then(|func| func(id))


There's a Deref impl, you don't need to write self.0.

rust-highfive assigned varkor May 30, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2019

estebank force-pushed the infer-type branch from 95cce7d to 90b5128 Compare May 30, 2019 17:26

This comment has been minimized.

Sign in to view

varkor reviewed May 30, 2019

View reviewed changes

src/test/ui/issues/issue-23046.stderr Outdated Show resolved Hide resolved

Centril suggested changes May 30, 2019

View reviewed changes

src/librustc/infer/error_reporting/need_type_info.rs Outdated Show resolved Hide resolved

varkor reviewed May 30, 2019

View reviewed changes

src/librustc/ty/print/pretty.rs Outdated Show resolved Hide resolved

Centril reviewed May 30, 2019

View reviewed changes

src/librustc/ty/print/pretty.rs Outdated Show resolved Hide resolved

src/test/ui/issues/issue-20261.stderr Outdated Show resolved Hide resolved

src/test/ui/issues/issue-17551.stderr Outdated Show resolved Hide resolved

src/test/ui/issues/issue-7813.stderr Outdated Show resolved Hide resolved

estebank force-pushed the infer-type branch from 82718cf to 9764380 Compare May 30, 2019 22:55

estebank added 4 commits May 31, 2019 18:33

Alternative wording for inference failure

65c2a7b

On type inference errors use the type argument name when possible

74fb3bb

``` error[E0282]: type annotations needed in `std::result::Result<i32, E>` --> file7.rs:3:13 | 3 | let b = Ok(4); | - ^^ cannot infer type for `E` in `std::result::Result<i32, E>` | | | consider giving `b` a type` ```

Tweak wording

ebbe725

estebank force-pushed the infer-type branch from 9764380 to ebbe725 Compare June 1, 2019 03:48

varkor reviewed Jun 1, 2019

View reviewed changes

estebank added 2 commits June 1, 2019 14:47

review comment: tweak wording

9616b33

Account for cases where we can find the type arg name, but the local …

e420f44

…name is `_`

varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2019

bors mentioned this pull request Jun 3, 2019

Update clippy #61467

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2019

bors merged commit e420f44 into rust-lang:master Jun 3, 2019

eddyb reviewed Jun 10, 2019

View reviewed changes

estebank mentioned this pull request Aug 12, 2019

When needing type annotation do not suggest using impl Trait or closure #63504

Closed


		pub region_highlight_mode: RegionHighlightMode,

		pub name_resolver: Option<Box<&'a dyn Fn(ty::sty::TyVid) -> Option<String>>>,

Uh oh!

Conversation

estebank commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 30, 2019

Uh oh!

estebank commented May 30, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estebank commented May 31, 2019

Uh oh!

varkor Jun 1, 2019

Choose a reason for hiding this comment

Uh oh!

estebank Jun 1, 2019

Choose a reason for hiding this comment

Uh oh!

estebank Jun 1, 2019

Choose a reason for hiding this comment

Uh oh!

varkor Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

varkor commented Jun 1, 2019

Uh oh!

varkor commented Jun 3, 2019

Uh oh!

estebank commented Jun 3, 2019

Uh oh!

bors commented Jun 3, 2019

Uh oh!

bors commented Jun 3, 2019

Uh oh!

bors commented Jun 3, 2019

Uh oh!

eddyb Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

estebank commented May 30, 2019 •

edited

Loading