rust-analyzer icon indicating copy to clipboard operation
rust-analyzer copied to clipboard

False positive `non-primitive cast` error in recent nightly

Open frxstrem opened this issue 1 year ago โ€ข 31 comments

When working for a while in a large codebase, I get a lot of errors like this one from tracing::info! macro invocations:

non-primitive cast: &Arguments<_> as &dyn Value

This seems to come from an expression &(format_args!(...)) as &dyn Value in the macro expansion, which should work (and works according to rustc). std::fmt::Arguments<'_> implements tracing_core::field::Value, however rust-analyzer seems to in some cases still produce an error.

I've so far been unable to create a minimal reproducible example, I've only so far seen this in a fairly large (170k LOC) commercial codebase that I'm working with. Restarting rust-analyzer does make the error go away for a while, in my case for about 1.5 minutes after other errors are visible.

let _ = &format_args!("...") as &dyn std::io::Write; // <-- immediately has error
let _ = &format_args!("...") as &dyn tracing::Value; // <-- has an error after a while

The errors started showing up after updating the rust-analyzer extension earlier today, not sure what the version prior to the update was. The errors seem to not be present on the latest stable release (v0.3.2096). It seems like it may be related to the recently merged #17984.


rust-analyzer version: rust-analyzer version: 0.4.2098-standalone

rustc version: rustc 1.80.1 (3f5fd8dd4 2024-08-06)

editor or extension: VSCode, extension version v0.4.2098 (pre-release)

relevant settings: none that I can find

frxstrem avatar Sep 04 '24 12:09 frxstrem

CC #11847. We might have to mark it as experimental :disappointed:.

lnicola avatar Sep 04 '24 12:09 lnicola

I've only so far seen this in a fairly large (170k LOC) commercial codebase that I'm working with.

Is your codebase private?

ShoyuVanilla avatar Sep 05 '24 00:09 ShoyuVanilla

CC #11847. We might have to mark it as experimental ๐Ÿ˜ž.

This seems to be caused from coerce_unsize as you said. I'll try with the way rustc handles this ๐Ÿค”

ShoyuVanilla avatar Sep 05 '24 02:09 ShoyuVanilla

This seems to be caused from coerce_unsize as you said. I'll try with the way rustc handles this ๐Ÿค”

Our CoerceUnsized solving sadly isn't quite correct and probably can't be fixed before the new trait solver is integrated :/

flodiebold avatar Sep 05 '24 06:09 flodiebold

Our CoerceUnsized solving sadly isn't quite correct and probably can't be fixed before the new trait solver is integrated :/

It might be turned out otherwise, but I guess it could be. The rustc's coerce_unsized solving is ten years old(https://github.com/rust-lang/rust/pull/24619/files#diff-12aa26d34290be9b45ffddf48028dd5af1991fbc8a069414b3ecc250cf48dd0aR295) and I think that this is not so much dependent on sophisticated trait solving infra; it mostly does things by itself I think. I guess it might work well in r-a here if we adopt that "registering unsolved obligation as InferOk's obligations" strategy.

ShoyuVanilla avatar Sep 05 '24 06:09 ShoyuVanilla

The rustc's coerce_unsized solving is ten years old(https://github.com/rust-lang/rust/pull/24619/files#diff-12aa26d34290be9b45ffddf48028dd5af1991fbc8a069414b3ecc250cf48dd0aR295) and I think that this is not so much dependent on sophisticated trait solving infra; it mostly does things by itself I think. I guess it might work well in r-a here if we adopt that "registering unsolved obligation as InferOk's obligations" strategy.

It uses trait selection (the selcx.select call), which is part of the trait solving infrastructure and something that Chalk doesn't provide access to (but the new trait solver will). Of course it would be possible to replicate that in RA manually, but I'm not sure that's worth it.

flodiebold avatar Sep 05 '24 07:09 flodiebold

The rustc's coerce_unsized solving is ten years old(https://github.com/rust-lang/rust/pull/24619/files#diff-12aa26d34290be9b45ffddf48028dd5af1991fbc8a069414b3ecc250cf48dd0aR295) and I think that this is not so much dependent on sophisticated trait solving infra; it mostly does things by itself I think. I guess it might work well in r-a here if we adopt that "registering unsolved obligation as InferOk's obligations" strategy.

It uses trait selection (the selcx.select call), which is part of the trait solving infrastructure and something that Chalk doesn't provide access to (but the new trait solver will). Of course it would be possible to replicate that in RA manually, but I'm not sure that's worth it.

Rustc branches on selcx.selext result as Ok(None), Err(..)(actually Err branches are two but they are doing error reports anyway) and Ok(Some(..)). Couldn't we do the similar things with db.trait_solve call result as Some(Solution(Ambig)), None and Some(Solution(Unique))?

ShoyuVanilla avatar Sep 05 '24 07:09 ShoyuVanilla

No, select isn't the same as full trait solving.

flodiebold avatar Sep 05 '24 07:09 flodiebold

No, select isn't the same as full trait solving.

Oh, I see ๐Ÿ˜ข

ShoyuVanilla avatar Sep 05 '24 07:09 ShoyuVanilla

So, we have another reason(one of so many) to quickly migrate into next-gen solver. I hope we could do so in near future. What would be the proper makeshift fix for this btw? ๐Ÿค” Some options I can think up are;

  1. Marking cast diagnostics as an experimental feature(so, not triggered by default)
  2. Marking the whole cast checks as an experimental feature
  3. Ignore cast errors when we are casting into a DST if they are not so obvious

ShoyuVanilla avatar Sep 05 '24 07:09 ShoyuVanilla

I've only so far seen this in a fairly large (170k LOC) commercial codebase that I'm working with.

Is your codebase private?

@ShoyuVanilla yes, it's private so unfortunately I haven't reproduced it with anything I can share here.

frxstrem avatar Sep 05 '24 08:09 frxstrem

What would be the proper makeshift fix for this btw?

My preference would be 3 if it's not too hard to implement, otherwise 1?

lnicola avatar Sep 05 '24 11:09 lnicola

I've only so far seen this in a fairly large (170k LOC) commercial codebase that I'm working with.

Is your codebase private?

@ShoyuVanilla yes, it's private so unfortunately I haven't reproduced it with anything I can share here.

No worries. Could you please tell me whether these false positive diagnostics still exist once the makeshift fix has been released? I'll mention you here when the nightly release done.

ShoyuVanilla avatar Sep 05 '24 13:09 ShoyuVanilla

What would be the proper makeshift fix for this btw?

My preference would be 3 if it's not too hard to implement, otherwise 1?

Mine, too ๐Ÿ˜„ Then I'll try with it

ShoyuVanilla avatar Sep 05 '24 13:09 ShoyuVanilla

The root cause of this might be a bit different from #11847 as we are calling table.fallback_if_possible() before cast checks at here;

https://github.com/rust-lang/rust-analyzer/blob/124c7482167ff6eea4f7663c0be87ea568ccd8c6/crates/hir-ty/src/infer.rs#L712-L725

The following test just passes in our current revision;

    #[test]
    fn cast_into_dst() {
        check_diagnostics(
            r#"
//- minicore: fmt, builtin_impls, coerce_unsized, dispatch_from_dyn
struct Box<T: ?Sized>(*const T);

impl<T, U> core::ops::CoerceUnsized<Box<U>> for Box<T>
where
    T: Unsize<U> + ?Sized,
    U: ?Sized,
{
}

impl<T: ?Sized> Box<T> {
    fn new(_: T) -> Self {
        loop {}
    }
}

fn test() {
    let _ = &42 as &dyn core::fmt::Debug;
    let _ = Box::new(7) as Box<dyn core::fmt::Debug>;
    let _ = Box::new([4]) as Box<[i32]>;
}
"#,
        )
    }

I'll look into this more ๐Ÿง

ShoyuVanilla avatar Sep 06 '24 01:09 ShoyuVanilla

Not sure since I coundn't reproduce this but this seems to be more complicated than first expectations. Here's some of my observations.

  1. The following is the definition of std::fmt::Arguments
#[lang = "format_arguments"]
#[stable(feature = "rust1", since = "1.0.0")]
#[derive(Copy, Clone)]
pub struct Arguments<'a> {
    // Format string pieces to print.
    pieces: &'a [&'static str],

    // Placeholder specs, or `None` if all specs are default (as in "{}{}").
    fmt: Option<&'a [rt::Placeholder]>,

    // Dynamic arguments for interpolation, to be interleaved with string
    // pieces. (Every argument is preceded by a string piece.)
    args: &'a [rt::Argument<'a>],
}

This struct doesn't have any generic parameter other than lifetime one 'a, so it's very unlikely that it can be casted to &dyn Value somewhere (as I try to reproduce it) and cannot be casted to in other places (in your codebase). I guess r-a might be failing to import some dependencies or other issues around this.

  1. Your example code might be another problem.
let _ = &format_args!("...") as &dyn std::io::Write; // <-- immediately has error
let _ = &format_args!("...") as &dyn tracing::Value; // <-- has an error after a while

The first one immediately has error as you said, and the second one took a while to show up an error -> this is the clue.

I think that the first one is from the rust-analyzer's native diagnostics - and this is not false positive because Arguments doesn't implement Write -, i.e. the very diagnostics that made with rust-analyzer's own logic, and the second one is from the flycheck error, that rust analyzer runs cargo check (or cargo clippy if your setting is so) in the background and passes its output to your text editor. The former is releatively faster because it re-uses unchaged part of the codebase that parsed by rust-analyzer and the later may take some time because it just runs cargo check or cargo clippy.

Of course, it might be the case that just second one took more time to evaluate but if the native-flycheck difference is the case, your example code is another issue that has something to do with flycheck.

To verify this, you may test with just typing the second line let _ = &format_args!("...") as &dyn tracing::Value; without saving if you are using default rust-analyzer options(because saving triggers flycheck) or setting rust-analyzer.checkOnSave as false to disable flycheck on save

ShoyuVanilla avatar Sep 06 '24 07:09 ShoyuVanilla

So I poked around in the code a bit more, and managed to actually get a reproducible example in a separate workspace:

use tonic::{body::BoxBody, client::GrpcService};

pub fn foo<S>()
where
    S: GrpcService<BoxBody>,
    S::ResponseBody: 'static, // <--- key line
{
    let _ = &format_args!("...") as &dyn tracing::Value;
}

(using tonic 0.12.2)

When the marked line is commented out, the code compiles fine, but when it's present rust-analyzer gives the error. In both cases cargo clippy and cargo check produce no errors.

Running with a slightly updated RA pre-release this time, v0.4.2103.

frxstrem avatar Sep 09 '24 14:09 frxstrem

Oh, it would be really helpful to fix this issue. Thanks! Until this is fixed it might be convenient to disable those false positives with rust-analyzer.diagnostics.disabled https://rust-analyzer.github.io/manual.html#diagnostics

ShoyuVanilla avatar Sep 09 '24 14:09 ShoyuVanilla

Miri also hits this incorrect diagnostics, in this line.

RalfJung avatar Sep 09 '24 16:09 RalfJung

Minimized with no dependency other than std (Works fine with other type alias Error definitions)

trait Foo {
    type Error;
    type Assoc;
}

trait Bar {
    type Error;
}

type Error = Box<dyn std::error::Error>;

trait Baz {
    type Assoc: Bar;
    type Error: Into<Error>;
}

impl<T, U> Baz for T
where
    T: Foo<Assoc = U>,
    T::Error: Into<Error>,
    U: Bar,
    <U as Bar>::Error: Into<Error>,
{
    type Assoc = U;
    type Error = T::Error;
}

struct S;
trait Trait {}
impl Trait for S {}

fn test<T>()
where
    T: Baz,
    T::Assoc: 'static
{
    let _ = &S as &dyn Trait;
          //^^^^^^^^^^^^^^^^ ERROR
}

ShoyuVanilla avatar Sep 09 '24 17:09 ShoyuVanilla

Running rust-analyzer analysis-stats here overflows stack, even the previous versions before #17984 . Interesting ๐Ÿค”

Database loaded:     682.30ms, 10mb (metadata 371.19ms, 140kb; build 45.06ms, 16kb)
  item trees: 1
Item Tree Collection: 995.80ยตs, 0b
  crates: 1, mods: 1, decls: 13, bodies: 3, adts: 2, consts: 0
Item Collection:     3.89s, 144mb
Body lowering:       484.42ms, 45mb     
1/3 33% processing: tracing-infer::test
thread 'main' has overflowed its stack

and CHALK_DEBUG="debug" rust-analyzer diagnostics . &> diag.log output is gigabytes long ๐Ÿคฃ

Maybe we are messing up with chalk for just simple coercion with complecated trait env.

A line from the log;

generalize_ty ty=Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Bar::Error<[?0 := Foo::Error<[?0 := !0_0]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>]>, universe_index=U0, variance=Invariant

So, my hypothesis is;

  1. We try to coerce &S into &dyn Trait while cast check
  2. Even though the coercion is straitforward, the chalk runs into an infinite recursion due to some recursiveness in trait environment of the function signature and fails due to the chalk recursion limit
  3. So, the coercion fails - this is false positiveness

I think that the miri one is another thing though ๐Ÿค”

ShoyuVanilla avatar Sep 09 '24 17:09 ShoyuVanilla

We are likely not spawning a bigger stack thread for analysis-stats (the server runs on a stack with a bigger thread usually) hence the overflow.

Veykril avatar Sep 10 '24 06:09 Veykril

Further testing shows indeterministic results ๐Ÿค”

fn main() {
    println!("Hello, world!");
}
pub trait From1<T>: Sized {
    fn from(_: T) -> Self;
}
pub trait Into1<T>: Sized {
    fn into(self) -> T;
}

impl<T, U> Into1<U> for T
where
    U: From1<T>,
{
    fn into(self) -> U {
        U::from(self)
    }
}

impl<T> From1<T> for T {
    fn from(t: T) -> T {
        t
    }
}

trait Foo {
    type ErrorType;
    type Assoc;
}

trait Bar {
    type ErrorType;
}

trait ErrorLike {}

struct BoxLike<T: ?Sized>(*const T);

impl<'a, E> From1<E> for BoxLike<dyn ErrorLike>
where
    E: Trait + 'a,
{
    fn from(_: E) -> Self {
        loop {}
    }
}

type BoxedError = BoxLike<dyn ErrorLike>;

trait Baz {
    type Assoc: Bar;
    type Error: Into1<BoxedError>;
}

impl<T, U> Baz for T
where
    T: Foo<Assoc = U>,
    T::ErrorType: Into1<BoxedError>,
    U: Bar,
    <U as Bar>::ErrorType: Into1<BoxedError>,
{
    type Assoc = U;
    type Error = T::ErrorType;
}
struct S;
trait Trait {}
impl Trait for S {}

fn test<T>()
where
    T: Baz,
    T::Assoc: 'static,
{
    let _ = &S as &dyn Trait;
}

running rust-analyzer diagnostics to the above sometimes fails to cast due to &dyn Trait is not sized here

https://github.com/rust-lang/rust-analyzer/blob/e35227d186acd47d8e5f78cbd792d57ddf47d74b/crates/hir-ty/src/infer/cast.rs#L114-L121

and sometimes with coercion failure, as before.

Obviously, the given types and coercions are nothing to do with T here

fn test<T>()
where
    T: Baz,
    T::Assoc: 'static,
{
    let _ = &S as &dyn Trait;
}

but commenting out T::Assoc: 'static bounds makes error gone as frxstrem said. Funny thing is that chaging T::Assoc: 'static bound to other non-lifetime bounds works like T::Assoc: Default, T::Assoc: Foo but adding lifetime with that as T::Assoc: Default + 'static doesn't works. Adding T::Assoc: Trait doesn't work either.

Maybe the chalk thinks that lifetime bounds and trait Trait bound should take into account when solving something for Trait and messes things up

ShoyuVanilla avatar Sep 10 '24 12:09 ShoyuVanilla

Just chiming in to let you know that I have hit this exact bug on stable (that has been released yesterday).

Downgrading VS Code extension to previous version helped.

wbenny avatar Sep 10 '24 16:09 wbenny

Just chiming in to let you know that I have hit this exact bug on stable (that has been released yesterday).

Downgrading VS Code extension to previous version helped.

I'm preparing a workaround fix for this. Sorry for the inconvenience ๐Ÿ˜ข

ShoyuVanilla avatar Sep 10 '24 16:09 ShoyuVanilla

I think that this false positives would be hidden in the nightly release with #18093 though it isn't really fixed in the right way(we need next-gen trait solver to fix this)

ShoyuVanilla avatar Sep 12 '24 03:09 ShoyuVanilla

Same issue

pythonberg1997 avatar Sep 14 '24 08:09 pythonberg1997

Same issue

Still getting this on nightly release?

ShoyuVanilla avatar Sep 14 '24 09:09 ShoyuVanilla

Got this on stable converting the println!() macros to tracing::info!() in burn-llama, had to downgrade to last version just to be able to use a logger.

kameko avatar Sep 15 '24 23:09 kameko

@kameko Are you still getting this on stable?

ChayimFriedman2 avatar Oct 01 '24 22:10 ChayimFriedman2