rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.21k stars 1.6k forks source link

A misdiagnosed case of type mismatch #11847

Open xffxff opened 2 years ago

xffxff commented 2 years ago

rust-analyzer version: bc08b8eff 2022-03-28 stable

rustc version: 1.59.0 (9d1b2106e 2022-02-23)

image I tried the example in https://smallcultfollowing.com/babysteps//blog/2022/03/29/dyn-can-we-make-dyn-sized/ and found that r-a gives wrong diagnostics.

The source code:

use std::fmt::Debug;

fn print_me_later(x: &dyn Debug) -> impl FnOnce() + '_ {
    move || println!("{x:?}")
}

fn main() {
    let f = print_me_later(&22);
    f()
}
flodiebold commented 2 years ago

Interestingly, it works with &22i32, so it seems to be some problem with the handling of integer type variables.

bitgaoshu commented 2 years ago

the literal, like 22, is actually an InferenceVar , except assign a type. In inferring , the InferenceVar with kind Integer can not coercion to Debug, but can coerce to Clone, Sized etc. When infer ended, some inferenceVar will fallback to default. So, hover on 22 is i32. Maybe what we need is to check coerce again after the fallback to eliminate mismatch

flodiebold commented 2 years ago

Hmm, I don't think that's how rustc handles this. I think rustc doesn't even try to prove that the variable implements Debug while trying to decide whether to coerce, and just records that as an obligation for later. We only coerce if we can prove either uniquely or with definite guidance that the coercion is possible: https://github.com/rust-lang/rust-analyzer/blob/1f709d54463972b189a3120be4073c507f2fbc00/crates/hir-ty/src/infer/coerce.rs#L613-L618 I think if we accepted any ambiguous result like the FIXME says this might work, though I'm not sure whether it would cause other problems (this requires at least testing on some codebases that it doesn't cause regressions). rustc does this in a more sophisticated way (it has its own trait solving loop specially for coercion), and I'm not sure whether this has any functional differences or is just for better diagnostics.

bitgaoshu commented 2 years ago

I agree. so I found sth like this. https://github.com/rust-lang/rust-analyzer/blob/1f709d54463972b189a3120be4073c507f2fbc00/crates/hir-ty/src/infer.rs#L431-L440

in line 431, fallback some inferenceVar to i32 so I think in line 437 can check coerce again

flodiebold commented 2 years ago

I don't think that's a good approach to fix this. The type mismatch is just one symptom of this bug; we need to correctly record the coercion.

bitgaoshu commented 2 years ago

yes.

  1. we have no enough info to decide what the literal type is. the hover i32 is a fallback, a result from L432. In L432, it just covert a InferenceVar, kind = Integer to I32, and no check overflow. so like , Issue #12155. Maybe it's on purpose.
  2. If we change Debug to Clone, of course not runnable, the mismatch info disappeared. Clone is builtin of InferenceVar, kind = Integer. but Debug is not. 截屏2022-05-05 12 03 09

So I think. there are two ways to correctly record the coercion. one is use default i32, the other is InferenceVar, kind = Integer

flodiebold commented 2 years ago

This problem isn't really related to fallback, it can also happen if we later do find out the concrete type for the literal:

fn main() {
    let i = 22;
    let f = print_me_later(&i);
    let u: u64 = i;
    f()
}

The thing that matters is that it's a variable at the point where we need to coerce, and we can't go back later and change the decision.

bitgaoshu commented 2 years ago

The mismatch's expected is decided. and the actual is not finally decided => When the variable's type is finally decided, the mismatch is finally decided.

flodiebold commented 2 years ago

As I mentioned before, it's not just about the type mismatch, we need to decide the coercion correctly. Here's a more complicated example where this matters for type inference down the line:

trait Foo<T> { fn get(&self) -> T; }

impl Foo<&'static str> for i32 {
    fn get(&self) -> &'static str {
        "ok"
    }
}

impl Foo<u32> for u32 {
    fn get(&self) -> u32 { *self }
}

fn test() {
    let x = 2;
    let d: &dyn Foo<_> = &x;
    let _: i32 = x;

    eprintln!("{}", d.get().len());
}
bitgaoshu commented 2 years ago

oh. Just for a diagnostic. For this issue, no matter what the literal type is , the diagnostic should not show. So, I think, just check if some literal type matches.

bitgaoshu commented 2 years ago

I also test // FIXME actually we maybe should also accept unknown guidance here. for now, it accept all test

WaffleLapkin commented 10 months ago

This is marked as https://github.com/rust-lang/rust-analyzer/labels/S-actionable, but is it really? Does anyone know how to fix r-a's type inference?

flodiebold commented 10 months ago

Yes, this is probably blocked on the new trait solver.