E0053 needs tighter spans

[sophiajt]

From: src/test/compile-fail/E0053.rs

Some bonus tasks on this one:

• Add the original trait requirement, so that you can see both the error and what the original trait expects
• If possible, tighten the spans

Error E0053 currently is updated to the new error format, but the spans are very large. Ideally, instead of this:

error[E0053]: method `foo` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:19:5
   |
19 |     fn foo(x: i16) { } //~ ERROR E0053
   |     ^^^^^^^^^^^^^^^^^^ expected u16, found i16

error[E0053]: method `bar` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:20:5
   |
20 |     fn bar(&mut self) { } //~ ERROR E0053
   |     ^^^^^^^^^^^^^^^^^^^^^ values differ in mutability
   |
   = note: expected type `fn(&Bar)`
   = note:    found type `fn(&mut Bar)`

We could have smaller spans (with updated labels):

error[E0053]: method `foo` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:19:5
   |
19 |     fn foo(x: i16) { } //~ ERROR E0053
   |               ^^^ expected u16

error[E0053]: method `bar` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:20:5
   |
20 |     fn bar(&mut self) { } //~ ERROR E0053
   |            ^^^^ values differ in mutability
   |
   = note: expected type `fn(&Bar)`
   = note:    found type `fn(&mut Bar)`

It would also be great to show what the other definition was:

error[E0053]: method `foo` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:19:5
   |
12 |     fn foo(x: u16);
   |               --- original trait requirement
...
19 |     fn foo(x: i16) { } //~ ERROR E0053
   |               ^^^ expected u16

error[E0053]: method `bar` has an incompatible type for trait
  --> src/test/compile-fail/E0053.rs:20:5
   |
13 |     fn bar(&self);
   |            - original trait requirement
...
20 |     fn bar(&mut self) { } //~ ERROR E0053
   |            ^^^^ values differ in mutability
   |
   = note: expected type `fn(&Bar)`
   = note:    found type `fn(&mut Bar)`

[KiChjang]

Okay, this isn't quite easy at all. I originally thought of simply grabbing the hir::FnDecl and do a search on its inputs field, to find the argument that corresponds to the type that resulted in an error. This doesn't work for 2 reasons: 1) there is not a mapping between a ty::TyS and an hir::Ty, i.e. given an hir::Ty, there is not a way to find the ty::TyS that corresponds to it. 2) the ty::TyS struct does not contain any sort of name and/or ID that helps in searching the argument list of a hir::FnDecl.

My initial solution to this problem was to add a span field to every ty::TyS, but that isn't ideal, since it would be growing the TypeArena by quite a large amount during interning. Instead, we should make a pointer that points to the corresponding node or hir::Ty, but I'm not sure if that's possible.

[sophiajt]

Yeah, I'm also not a fan of adding the spans to ty. If we can't add the span to an ast node, we may just have to not shrink this particular span.

[KiChjang]

Note to self: look into FnCtxt.ast_ty_to_ty_cache and Inherited.locals.

[KiChjang]

Progress: now able to shorten span for mismatched types, still needs to shorten spans for differing mutability. I ended up having to use ccx.ast_ty_to_ty_cache to look up the NodeId that corresponds to the correct ty::Ty.

[sophiajt]

Niiiice. Looking forward to this fix.

[KiChjang]

Here's what I've managed to do:

which looks pretty feature-complete, aside from the span including the identifier for the first argument in the second error.

Originally I thought of refactoring Mutability into Spanned<Mutability>, but that causes 2 problems, first is that Mutability does not include the lifetime & of the pattern/type, and secondly, only the AST would have the span information, and typeck is at a later pass that operates on the HIR, and we don't want to pass on span information from the AST to the HIR during lowering (according to the IRC discussions with @eddyb) because HIR contains desugared syntax and should have the least amount of syntactical information as possible.

The implementation that I currently have has bugs though, watch what happens when I have two arguments - one self which is of type Bar and the other, which is also of type Bar:

This is because currently my implementation searches for the argument vector for the first type that matches the offending type as reported in typeck. Since in the impl fn bar, both arguments are of type &mut Bar, and so the code can't differentiate which one of them is the actual offending type, and would pick the first one.

I'm not sure if what I have is OK for submission right now, or whether I should just remove the shortened span for the latter case, and replace it with the whole line span.

[KiChjang]

Oh, nevermind. I think the answer is pretty obvious. E0050 already checks for whether the trait fn and the impl fn have the same number of arguments, and is done before checking for E0053, so the number of arguments for the trait fn and the impl fn are guaranteed to be the same. This means if I simply obtain the iterator for both argument vectors and zip them, comparing only their mutability until I find one that does not match, I would have found the index for the two arguments.

[KiChjang]

Looks like we're done here:

PR up in a bit.