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

Wrong lifetime variance in hover for recursive types #19455

Open
benediktsatalia opened this issue Mar 26, 2025 · 4 comments
Open

Wrong lifetime variance in hover for recursive types #19455

benediktsatalia opened this issue Mar 26, 2025 · 4 comments
Labels
C-bug Category: bug

Comments

@benediktsatalia
Copy link

benediktsatalia commented Mar 26, 2025

rust-analyzer version: rust-analyzer version: 0.3.2353-standalone (37acea8052 2025-03-23)

rustc version: rustc 1.84.1 (e71f9a9a9 2025-01-27)

editor or extension: VSCode with rust-analyzer version 0.3.2353

code snippet to reproduce:

use std::cell::RefCell;

/// This type is covariant:
/// ```
/// use rust_playground::lifetimes::Recursive;
/// fn is_covariant<'long, 'short>(value: Recursive<'long>)
/// where
///     'long: 'short,
/// {
///     let _: Recursive<'short> = value;
/// }
/// ```
/// and not contravariant:
/// ```compile_fail
/// use rust_playground::lifetimes::Recursive;
/// fn is_contravariant<'long, 'short>(value: Recursive<'short>)
/// where
///     'long: 'short,
/// {
///     let _: Recursive<'long> = value;
/// }
/// ```
pub struct Recursive<'a> {
    v: &'a i32,
    parent: Option<Box<Recursive<'a>>>,
}

/// This type is not covariant:
/// ```compile_fail
/// use rust_playground::lifetimes::Recursive1;
/// fn is_covariant<'long, 'short>(value: Recursive1<'long>)
/// where
///     'long: 'short,
/// {
///     let _: Recursive1<'short> = value;
/// }
/// ```
/// and not contravariant
/// ```compile_fail
/// use rust_playground::lifetimes::Recursive1;
/// fn is_contravariant<'long, 'short>(value: Recursive1<'short>)
/// where
///     'long: 'short,
/// {
///     let _: Recursive1<'long> = value;
/// }
/// ```
/// i.e. it is invariant
pub struct Recursive1<'a>(&'a i32, Option<Box<Recursive2<'a>>>);
pub struct Recursive2<'a>(RefCell<Recursive1<'a>>);

For all three defined structs if I hover over the 'a parameter in VSCode it shows me bivariant although Recursive is covariant and Recursive1 and Recursive2 are invariant over 'a.

This gets worse if for example the invariant structs are used together with other covariant types for example:

pub struct Mixed<'a>(&'a i32, Recursive1<'a>);

In this case when I hover over it, it shows covariant although in fact it is invariant.

@benediktsatalia benediktsatalia added the C-bug Category: bug label Mar 26, 2025
@Veykril
Copy link
Member

Veykril commented Mar 26, 2025

This is known and currently blocked on us bumping salsa to the version that has fixed point cycle support

#[test]
fn prove_fixedpoint() {
// FIXME: This is wrong, this should be `FixedPoint[T: covariant, U: covariant, V: covariant]`
// This is a limitation of current salsa where a cycle may only set a fallback value to the
// query result, but we need to solve a fixpoint here. The new salsa will have this
// fortunately.
check(
r#"
struct FixedPoint<T, U, V>(&'static FixedPoint<(), T, U>, V);
"#,
expect![[r#"
FixedPoint[T: bivariant, U: bivariant, V: bivariant]
"#]],
);
}

Anything recursive used currently falls back to bivariant

@benediktsatalia
Copy link
Author

Anything recursive used currently falls back to bivariant

Thanks for the quick response, wouldn't it be safer in some sense to fall back to invariant? I can see that bivariant signals there is something buggy going on, but if it then gets swallowed wrongly like in my example Mixed then all the information you get is not reliable. Even if you see Covariant it could be Invariant because somewhere deep inside there is some recursive type.

@Veykril
Copy link
Member

Veykril commented Mar 26, 2025

My thinking there was I believe that bivariant always implies an error state in Rust (except for generics in function parameters), but you are correct that this may cause some things to incorrectly remain covariant or contravariant when they are in fact invariant.

I imagine we properly fix this soon though, someone just needs to migrate us to the next salsa release (has to be cut still). The main annoyance there is that the general cycle handling changed and needs to adapted.

@davidbarsky
Copy link
Contributor

At Carl Meyer's suggestion, we should hold off on updating to Salsa's new fixpoint iteration until he fixes an exponential blowup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

3 participants