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

cengiz-io · 2017-04-11T22:02:39Z

Hello there!

What's this?

Previously, me and @nikomatsakis worked on error messages of uninferred locals. (#38812)

This aims to build up on that by moving certain type checks from writeback.

With this, E0101 and E0102 errors are getting obsoleted and no longer thrown.

They're replaced with customized versions of E0282s instead.

Sample Error Messages

`E0101` is getting converted into:

error[E0282]: type annotations needed
 --> test.rs:2:14
  |
2 |     let x = |_| {};
  |              ^ consider giving this closure parameter a type

error: aborting due to previous error

`E0102` is getting converted into:

error[E0282]: type annotations needed
 --> test.rs:2:9
  |
2 |     let x = [];
  |         ^
  |         |
  |         consider giving `x` a type
  |         cannot infer type for `[_; 0]`

error: aborting due to previous error

Annoyances

I think we need to change our way of type name resolving in relevant places, because that [_; 0] looks horrible IMHO.
I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this.

Tests

Tests of E0101 and E0102 are getting converted from compile-fail to ui tests.

Documentation

Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones.

Please do comment on messages, layouts and other details.

Appreciation

Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

rust-highfive · 2017-04-11T22:02:53Z

r? @pnkfelix

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

nikomatsakis · 2017-04-11T22:19:07Z

r? @nikomatsakis

nikomatsakis

Let me know what you think. If what I want is unclear, I can also try to do it locally, since I think it ought to be fairly simple and I'm not sure if I explained it well.

nikomatsakis · 2017-04-12T19:45:22Z

src/librustc/traits/error_reporting.rs

@@ -65,24 +65,41 @@ impl<'a, 'gcx, 'tcx> TraitErrorKey<'tcx> {
 struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
    infcx: &'a InferCtxt<'a, 'gcx, 'tcx>,
    target_ty: &'a Ty<'tcx>,
-    found_pattern: Option<&'a Pat>,
+    hir_map: &'a hir::map::Map<'gcx>,
+    found_pattern: Option<&'gcx Pat>,


maybe we should call this found_local_pattern

nikomatsakis · 2017-04-12T19:46:30Z

src/librustc/traits/error_reporting.rs

@@ -835,42 +854,51 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
        }
    }

-    fn need_type_info(&self, obligation: &PredicateObligation<'tcx>, ty: Ty<'tcx>) {
+    pub fn need_type_info(&self, body_id: hir::BodyId, span: Span, ty: Ty<'tcx>) {


Nit for future PR: feels like this function (and its associated visitor etc) would be ripe for promotion into a sub-module error_reporting/need_type_info.rs

Sounds good Will do as soon as this lands

nikomatsakis · 2017-04-12T19:46:57Z

src/librustc/traits/error_reporting.rs

            Some(NodeExpr(expr)) => local_visitor.visit_expr(expr),
            _ => ()
        }

+        if let Some(pattern) = local_visitor.found_arg_pattern {
+            err_span = pattern.span;
+            labels.clear();


should comment on why we are calling clear here; maybe give before/after example

nikomatsakis · 2017-04-12T19:50:27Z

src/librustc_typeck/check/writeback.rs

            self.tables.fru_field_types.insert(node_id, ftys);
        }
    }

    fn visit_type_nodes(&self) {
        for (&id, ty) in self.fcx.ast_ty_to_ty_cache.borrow().iter() {
-            let ty = self.resolve(ty, ResolvingTyNode(id));
+            let ty = self.resolve(ty, id.to_span(&self.fcx.tcx));


I have a vague fear that this will add to compilation times; these spans are only needed if an error occurs. I wonder if we could change resolve to take a closure, so that we only evaluate and get the span if an error occurs.

This was part of the reason that I suggested the Locatable trait; you could e.g. change resolve to take an &Locatable (i.e., a trait object), in which case the span field of Resolver would change I think to the type &'cx Locatable

bors · 2017-04-13T03:15:30Z

☔ The latest upstream changes (presumably #40570) made this pull request unmergeable. Please resolve the merge conflicts.

cengiz-io · 2017-04-15T22:56:57Z

Hello @nikomatsakis

It took me sometime to merge upstream changes and fix ui tests.

Only the performance tuning remains now. Will send it tomorrow ASAP.

Thanks!

arielb1 · 2017-04-17T20:29:30Z

src/librustc/diagnostics.rs

@@ -327,6 +327,54 @@ struct ListNode {
 This works because `Box` is a pointer, so its size is well-known.
 "##,

+E0101: r##"


You should comment this error out.(see E0211).

arielb1 · 2017-04-17T20:32:58Z

src/librustc/traits/error_reporting.rs

            } else {
-                err.span_label(pattern_span, &format!("consider giving a type to pattern"));
+                labels.push((pattern.span, format!("consider giving a type to pattern")));


English: consider giving the pattern a type

Hmm I agree this phrasing isn't the best. "consider giving a type to the pattern" or "consider giving the pattern a type" both seem fine to me.

arielb1 · 2017-04-17T20:36:50Z

src/librustc/traits/error_reporting.rs

-
-        err.span_label(cause.span, &format!("cannot infer type for `{}`", name));
+        let mut err_span = span;
+        let mut labels = vec![(span, format!("cannot infer type for `{}`", name))];


do we ever need this message? I think a local with inference types in it is always an error at this stage. I think that if we ever find a local/argument with inference variables in it we should have E0101 with only that local spanned.

Yes we do. That was decided long time ago in a different issue + PR.
#38812

Another thing that would be nice is to always use the type of the local,

E101: unable to infer the type of the local `x` let x = vec![]; // ^ `x` can only be resolved to `Vec<_>`

Yeah, I feel like it'd be nice to say what we do now of the type, but we never did settle on the precise format we wanted for it. (The discussion was in #39361)

arielb1 · 2017-04-17T20:40:17Z

src/librustc/traits/error_reporting.rs

@@ -66,37 +66,52 @@ impl<'a, 'gcx, 'tcx> TraitErrorKey<'tcx> {
 struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
    infcx: &'a InferCtxt<'a, 'gcx, 'tcx>,
    target_ty: &'a Ty<'tcx>,
-    found_pattern: Option<&'a Pat>,
+    hir_map: &'a hir::map::Map<'gcx>,
+    found_local_pattern: Option<&'gcx Pat>,


Why do you have separate options for found_local_pattern and found_arg_pattern? I'll have a single found_local enum.

They're not only holding target patterns, but also changing the flow of the result.

Can you share your suggestion with a concrete example please?

enum TypeVariableHolder<'gcx> { // later, TypeParam(...), Argument(&'gcx Pat), Local(&'gcx Pat), } struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { ..., found_variable_holder: Option<TypeVariableHolder<'gcx>> }

And find the maximum relevant TypeVariableHolder.

It doesn't seem like it makes much difference either way to me.

Actually it's mainly something I want to build on.

Makes sense. I would be fine landing as is, but also fine with adopting the single enum approach -- I imagine as we add more cases, we'll probably want a single enum, as you suggest. (Though I was wondering if it may be the case that we can't detect until the end whether certain patterns are met, and hence we wouldn't be able to decide the final state until the end.) Either way, easy to evolve as needed.

cengiz-io · 2017-04-17T23:21:03Z

Commits are squashed, and resolving logic has been optimized (relatively).

⏲

nikomatsakis · 2017-04-18T00:07:38Z

src/librustc_typeck/diagnostics.rs

@@ -4146,8 +4102,6 @@ register_diagnostics! {
 //  E0068,
 //  E0085,
 //  E0086,
-    E0103, // @GuillaumeGomez: I was unable to get this error, try your best!
-    E0104,


I don't think we usually remove these lints outright, but rather leave them commented out.. or at least that's what the surrounding lines seem to suggest. =)

cc @rust-lang/docs -- what is the policy when an error code is no longer issued? (And what about if a "long-form" error exists, as in the case of E0102 above?) Is this documented somewhere? (RFC 1567 doesn't seem to mention it.)

The general rule of thumb is "don't reuse error codes". It's okay to deprecate and just not output that code anymore, but reusing tends to mess with search results.

@cengizio

You should leave them commented out, like the previous ones.

@arielb1 commented them out.

Also instead of adding whole documentation as a comment, I've added // E0101 and // E0102 to register_diagnostics! call.

If you have both the commented documentation and a register_diagnostics! call for an error code, tidy complains. Even if they're only comments.

nikomatsakis · 2017-04-18T18:46:22Z

@bors r+

bors · 2017-04-18T18:46:23Z

📌 Commit 3092ac4 has been approved by nikomatsakis

@nikomatsakis

Move E0101 and E0102 logic into new E0282 mechanism rust-lang#40013 Hello there! Previously, me and @nikomatsakis worked on error messages of uninferred locals. (rust-lang#38812) This aims to build up on that by moving certain type checks from `writeback`. With this, `E0101` and `E0102` errors are getting obsoleted and no longer thrown. They're replaced with customized versions of `E0282`s instead. ```rust error[E0282]: type annotations needed --> test.rs:2:14 | 2 | let x = |_| {}; | ^ consider giving this closure parameter a type error: aborting due to previous error ``` ```rust error[E0282]: type annotations needed --> test.rs:2:9 | 2 | let x = []; | ^ | | | consider giving `x` a type | cannot infer type for `[_; 0]` error: aborting due to previous error ``` - I think we need to change our way of type name resolving in relevant places, because that `[_; 0]` looks horrible IMHO. - I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this. Tests of `E0101` and `E0102` are getting converted from `compile-fail` to `ui` tests. Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones. Please do comment on messages, layouts and other details. Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

@nikomatsakis

Move E0101 and E0102 logic into new E0282 mechanism rust-lang#40013 Hello there! Previously, me and @nikomatsakis worked on error messages of uninferred locals. (rust-lang#38812) This aims to build up on that by moving certain type checks from `writeback`. With this, `E0101` and `E0102` errors are getting obsoleted and no longer thrown. They're replaced with customized versions of `E0282`s instead. ```rust error[E0282]: type annotations needed --> test.rs:2:14 | 2 | let x = |_| {}; | ^ consider giving this closure parameter a type error: aborting due to previous error ``` ```rust error[E0282]: type annotations needed --> test.rs:2:9 | 2 | let x = []; | ^ | | | consider giving `x` a type | cannot infer type for `[_; 0]` error: aborting due to previous error ``` - I think we need to change our way of type name resolving in relevant places, because that `[_; 0]` looks horrible IMHO. - I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this. Tests of `E0101` and `E0102` are getting converted from `compile-fail` to `ui` tests. Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones. Please do comment on messages, layouts and other details. Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

@nikomatsakis

Move E0101 and E0102 logic into new E0282 mechanism rust-lang#40013 Hello there! Previously, me and @nikomatsakis worked on error messages of uninferred locals. (rust-lang#38812) This aims to build up on that by moving certain type checks from `writeback`. With this, `E0101` and `E0102` errors are getting obsoleted and no longer thrown. They're replaced with customized versions of `E0282`s instead. ```rust error[E0282]: type annotations needed --> test.rs:2:14 | 2 | let x = |_| {}; | ^ consider giving this closure parameter a type error: aborting due to previous error ``` ```rust error[E0282]: type annotations needed --> test.rs:2:9 | 2 | let x = []; | ^ | | | consider giving `x` a type | cannot infer type for `[_; 0]` error: aborting due to previous error ``` - I think we need to change our way of type name resolving in relevant places, because that `[_; 0]` looks horrible IMHO. - I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this. Tests of `E0101` and `E0102` are getting converted from `compile-fail` to `ui` tests. Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones. Please do comment on messages, layouts and other details. Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

@nikomatsakis

Move E0101 and E0102 logic into new E0282 mechanism rust-lang#40013 Hello there! Previously, me and @nikomatsakis worked on error messages of uninferred locals. (rust-lang#38812) This aims to build up on that by moving certain type checks from `writeback`. With this, `E0101` and `E0102` errors are getting obsoleted and no longer thrown. They're replaced with customized versions of `E0282`s instead. ```rust error[E0282]: type annotations needed --> test.rs:2:14 | 2 | let x = |_| {}; | ^ consider giving this closure parameter a type error: aborting due to previous error ``` ```rust error[E0282]: type annotations needed --> test.rs:2:9 | 2 | let x = []; | ^ | | | consider giving `x` a type | cannot infer type for `[_; 0]` error: aborting due to previous error ``` - I think we need to change our way of type name resolving in relevant places, because that `[_; 0]` looks horrible IMHO. - I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this. Tests of `E0101` and `E0102` are getting converted from `compile-fail` to `ui` tests. Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones. Please do comment on messages, layouts and other details. Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

bors · 2017-04-19T01:22:18Z

⌛ Testing commit 3092ac4 with merge e49664e...

frewsxcv · 2017-04-19T01:27:06Z

@bors retry

prioritizing rollup which includes this

bors · 2017-04-19T01:27:13Z

⌛ Testing commit 3092ac4 with merge 84be2df...

@nikomatsakis

Move E0101 and E0102 logic into new E0282 mechanism #40013 Hello there! ## What's this? Previously, me and @nikomatsakis worked on error messages of uninferred locals. (#38812) This aims to build up on that by moving certain type checks from `writeback`. With this, `E0101` and `E0102` errors are getting obsoleted and no longer thrown. They're replaced with customized versions of `E0282`s instead. ## Sample Error Messages #### `E0101` is getting converted into: ```rust error[E0282]: type annotations needed --> test.rs:2:14 | 2 | let x = |_| {}; | ^ consider giving this closure parameter a type error: aborting due to previous error ``` #### `E0102` is getting converted into: ```rust error[E0282]: type annotations needed --> test.rs:2:9 | 2 | let x = []; | ^ | | | consider giving `x` a type | cannot infer type for `[_; 0]` error: aborting due to previous error ``` ## Annoyances - I think we need to change our way of type name resolving in relevant places, because that `[_; 0]` looks horrible IMHO. - I'm not terribly happy with the note ordering of errors. So please do point to code that might help me accomplish this. ## Tests Tests of `E0101` and `E0102` are getting converted from `compile-fail` to `ui` tests. ## Documentation Please help me with documentation update. There are some confusing places that needed an update but I'm not sure if I did the right ones. Please do comment on messages, layouts and other details. ## Appreciation Huge thanks goes to @nikomatsakis for being a patient and humble mentor along this long journey. 🍻

bors · 2017-04-19T04:02:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 84be2df to master...

rust-highfive assigned pnkfelix Apr 11, 2017

cengiz-io changed the title ~~fixes #40013~~ Move E0101 and E0102 logic into new E0282 mechanism #40013 Apr 11, 2017

rust-highfive assigned nikomatsakis and unassigned pnkfelix Apr 11, 2017

nikomatsakis requested changes Apr 12, 2017

View reviewed changes

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

arielb1 reviewed Apr 17, 2017

View reviewed changes

nikomatsakis reviewed Apr 18, 2017

View reviewed changes

Cengiz Can added 3 commits April 18, 2017 21:43

fixes #40013

6383de1

comment out removed error codes

8b45a21

change note for patterns

3092ac4

nikomatsakis approved these changes Apr 18, 2017

View reviewed changes

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 5 pull requests #41379

Closed

Mark-Simulacrum mentioned this pull request Apr 18, 2017

Move traits/error_reporting to its own module #41284

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 7 pull requests #41381

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 6 pull requests #41382

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 8 pull requests #41383

Closed

frewsxcv mentioned this pull request Apr 19, 2017

Rollup of 8 pull requests #41385

Closed

bors merged commit 3092ac4 into rust-lang:master Apr 19, 2017

This was referenced May 8, 2017

Rust 1.18 regression, yggie/mach, type annotations needed #41806

Closed

Rust 1.18 regression, constrained-0.1.0, cannot infer type #41796

Closed

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

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

Conversation

cengiz-io commented Apr 11, 2017

What's this?

Sample Error Messages

E0101 is getting converted into:

E0102 is getting converted into:

Annoyances

Tests

Documentation

Appreciation

rust-highfive commented Apr 11, 2017

nikomatsakis commented Apr 11, 2017

nikomatsakis left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 13, 2017

cengiz-io commented Apr 15, 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

arielb1 Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Apr 17, 2017 • edited Loading

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

cengiz-io commented Apr 17, 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

nikomatsakis commented Apr 18, 2017

bors commented Apr 18, 2017

bors commented Apr 19, 2017

frewsxcv commented Apr 19, 2017

bors commented Apr 19, 2017

bors commented Apr 19, 2017

`E0101` is getting converted into:

`E0102` is getting converted into:

arielb1 Apr 17, 2017 •

edited

Loading

arielb1 Apr 17, 2017 •

edited

Loading

arielb1 Apr 17, 2017 •

edited

Loading

arielb1 Apr 17, 2017 •

edited

Loading