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

Type inference reports misleading diagonstics #69455

Open
contrun opened this issue Feb 25, 2020 · 11 comments
Open

Type inference reports misleading diagonstics #69455

contrun opened this issue Feb 25, 2020 · 11 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@contrun
Copy link
Contributor

contrun commented Feb 25, 2020

I tried this code:

fn main() {
    let xs: Vec<u64> = vec![1, 2, 3];
    println!("{}", 23u64 + xs.iter().sum());
}

I expected to see this happen: I expect rust to tell me I should be more precise about 23u64 + xs.iter().sum().

fn main() {
    let xs: Vec<u64> = vec![1, 2, 3];
    
    // This works
    let temp: u64 = xs.iter().sum();
    println!("{}", 23u64 + temp);
    
    // This reports error correctly.
    let test = 23u64 + xs.iter().sum();
    println!("{}", test)
}

Instead, this happened:

error[E0284]: type annotations needed for `std::vec::Vec<u64>`
 --> src/main.rs:3:26
  |
2 |     let xs: Vec<u64> = vec![1, 2, 3];
  |         -- consider giving `xs` the explicit type `std::vec::Vec<u64>`, where the type parameter `u64` is specified
3 |     println!("{}", 23u64 + xs.iter().sum());
  |                          ^ cannot infer type for type `u64`
  |
  = note: cannot resolve `<u64 as std::ops::Add<_>>::Output == _`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0284`.
error: could not compile `playground`.

A method call variant of this problem is

pub trait Test<Rhs = Self> {
    type Output;

    fn test(self, rhs: Rhs) -> Self::Output;
}

impl Test<u32> for u64 {
    type Output = u64;

    fn test(self, other: u32) -> u64 { self + (other as u64) }
}

impl Test<u64> for u64 {
    type Output = u64;

    fn test(self, other: u64) -> u64 {
        (self + other) as u64
    }
}


fn main() {
    let xs: Vec<u64> = vec![1, 2, 3];
    println!("{}", 23u64.test(xs.iter().sum()));
}

The only difference is that self.test is a method call, while + is a binary operator.

rustc --version --verbose

rustc 1.43.0-nightly (158127853 2020-03-10)
binary: rustc
commit-hash: 15812785344d913d779d9738fe3cca8de56f71d5
commit-date: 2020-03-10
host: x86_64-unknown-linux-gnu
release: 1.43.0-nightly
LLVM version: 9.0

Related issue: #69214

@contrun contrun added the C-bug Category: This is a bug. label Feb 25, 2020
@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 25, 2020
@SkiFire13
Copy link
Contributor

SkiFire13 commented Mar 21, 2020

It gets even more misleading if you use xs before that addition. For example:

fn main() {
    let xs: Vec<u64> = vec![1, 2, 3];
    let ys: Vec<u64> = xs.iter().map(|i: &u64| i*2).collect();
    let _: u64 = 23u64 + xs.iter().sum();
}

This gives the following error:

error[E0284]: type annotations needed for `&u64`
 --> src/main.rs:3:39
  |
3 |     let ys: Vec<u64> = xs.iter().map(|i: &u64| i*2).collect();
  |                                       ^ consider giving this closure parameter the explicit type `&u64`, where the type parameter `u64` is specified
  |
  = note: cannot resolve `<u64 as std::ops::Add<_>>::Output == u64`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0284`.
error: could not compile `playground`.

This doesn't even report the line where the actual error is which is even more misleading. Imagine if there were several other lines between line 3 and 4.

Edit: Added explicit type to the map closure parameter to show it's still the same error.

@contrun
Copy link
Contributor Author

contrun commented Mar 21, 2020

@SkiFire13 Thank you for your additional information.
The root cause of this problem is that rust in unable to resolve u64 as Add<_> where _ is the rhs of +. Rust tries to call need_type_info_err which only accepts the error type u64 and the error span as parameters. Rust then tries to find all possible location of the error type in the body. When it finds a closure of type u64, it will report an error. When it finds a generics with generic parameter u64, it will report an error. These are all false positives cases.
I tried to solve this problem in PR #69456. Unfortunately the cases are too complicated. The function need_type_error_info takes care of not only error in this case. My PR will resolve the problem mentioned here, but it will bring in some other problems. Please refer to my PR for the detailed failed test cases. I basically have given up on this PR. I think the best way we can fix this is to pass the specific error type to need_type_info_err. This may require quite some refactor.

contrun added a commit to contrun/rust that referenced this issue Apr 8, 2020
This solves the method call part of issue
rust-lang#69455
I added a `target_span` field so as to pin down the exact location of
the error. We need a dedicated field `found_exact_method_call` to
prioritize situations like the test case `issue-69455.rs`. If we reuse
`found_method_call`, `found_local_pattern` will show up first. We can
not move `found_method_call` up, it is undesirable in various
situations.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 24, 2020
…or, r=estebank

fix misleading type annotation diagonstics

This solves the method call part of issue rust-lang#69455
@estebank
Copy link
Contributor

estebank commented Jan 8, 2023

Current output has a slight mistake in the span suggestion, I suspect due to the macro :-/

error[[E0282]](https://doc.rust-lang.org/nightly/error-index.html#E0282): type annotations needed
 --> src/main.rs:3:20
  |
3 |     println!("{}", 23u64 + xs.iter().sum());
  |                    ^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `T` declared on the associated function `new_display`
  |
  = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider specifying the generic argument
  |
3 |     println!("{}", 23u64 + xs.iter().sum()::<T>);
  |

The second example has the same problem:

error[E0282]: type annotations needed
  --> src/main.rs:24:20
   |
24 |     println!("{}", 23u64.test(xs.iter().sum()));
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `T` declared on the associated function `new_display`
   |
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider specifying the generic argument
   |
24 |     println!("{}", 23u64.test(xs.iter().sum())::<T>);
   |                                               +++++

error[E0283]: type annotations needed
  --> src/main.rs:24:41
   |
24 |     println!("{}", 23u64.test(xs.iter().sum()));
   |                          ----           ^^^ cannot infer type of the type parameter `S` declared on the associated function `sum`
   |                          |
   |                          required by a bound introduced by this call
   |
note: multiple `impl`s satisfying `u64: Test<_>` found
  --> src/main.rs:7:1
   |
7  | impl Test<u32> for u64 {
   | ^^^^^^^^^^^^^^^^^^^^^^
...
13 | impl Test<u64> for u64 {
   | ^^^^^^^^^^^^^^^^^^^^^^
help: consider specifying the generic argument
   |
24 |     println!("{}", 23u64.test(xs.iter().sum::<S>()));
   |                                            +++++

@m-ou-se
Copy link
Member

m-ou-se commented Jan 12, 2023

Current output has a slight mistake in the span suggestion, I suspect due to the macro :-/

I accidentally fixed that in a6d6589#diff-7f900f1a55b5da3794eca56bf34b4c0f473f32a8f78b4a04842b578f8e819e90

Edit: to clarify, this is not yet merged. It's now part of #106770, and might become part of #106745 depending on perf results.

@estebank
Copy link
Contributor

@m-ou-se drive by fixes ftw 🚗

@m-ou-se
Copy link
Member

m-ou-se commented Jan 12, 2023

Looks like my change that accidentally fixes this causes a performance regression, so I won't include it in my PR for now.

It does provide an interesting data point for this issue, though:

If format_args!("{}", 23u64 + xs.iter().sum()) expands to

Arguments::new_v1(, &[
    ArgumentV1::new_display(&(23u64 + xs.iter().sum()))
])

then the problematic suggestion appears. It attempts to suggest to add an explicit ::<T> to new_display. (Which is a call that is generated by the macro, so there's no good place to insert that ::<T>.)

But if it expands to

Arguments::new_v1(, &match (&(23u64 + xs.iter().sum()),) {
    args => [ArgumentV1::new_display(args.0)]
})

then it instead suggests adding an explicit ::<S> to sum. (Which is probably what we want.)

@estebank
Copy link
Contributor

@m-ou-se could that change on its own be landed, or would that also cause perf (or other) problems? It seems like it'd be safe to slightly change the desugaring to aid in this.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 12, 2023

It is specifically that one change that causes the performance regression. (Regardless of my other changes.) The extra match results in a noticible amount of extra work for rustc, apparently. (I have not investigated why.)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 13, 2023

Looks like both errors are generated in both cases, but one of them is supressed by this deduplicating mechanism. The only difference is which of the two errors is first.

A bit of a hacky solution could be to emit errors that do not have a span from an expansion before those that do, like this.

@estebank
Copy link
Contributor

estebank commented Jan 13, 2023

@m-ou-se would you mind publishing that change as a PR assigned to me? (don't include the async.stderr file, that's an ongoing issue I've noticed, faulty test someone else is fixing)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 13, 2023

@estebank #106820

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 14, 2023
…estebank

Deprioritize fulfillment errors that come from expansions.

Fixes (part of?) rust-lang#69455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants