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

Yet another glob import issue

Open RivenSkaye opened this issue 10 months ago • 15 comments

Previously opened as rust-lang/rust#122682 but moved here as requested. Some things have changed since, but they're not better. Old behavior in strikethrough because it might provide some pointers on what changed behavior is in play here? Seems to only be a problem with the VSCode extension now at least. I'm not getting this error reproduced in helix.

RA breaks on certain struct definitions from external crates, due to #[repr(transparent)] across the crate boundary. As a "fix" it makes you ~~go back and forth between the outer type and the transparently wrapped one~~ wrap it in the transparent type, but specifically in VSCode that errors claiming to expect the inner type. In the example provided near the bottom, these are HRESULT and i32.

This doesn't feel ergonomic, and more importantly the type resolution provided is broken to the point it's actively confusing. Where I'd expect a clearer resolution or no actions available, the quick fix advises users to ~~change wrapper -> underlying type and vice versa~~ use the transparent wrapper, and then it errors with no fix available. This creates a very confusing situation. Subsequently verifying the type signature through the VSCode plugin doesn't help, as it lists the underlying type rather than the transparent wrapper.

Image Image For this one, it does suggest making it a HRESULT, but then we go back to the previous screenshot. Image

The only solution that appeases the RA extension without compilation issues is HRESULT::Into<i32>.

rust-analyzer version: rust-analyzer 1.87.0-nightly (5bc62314 2025-02-16) with VSCode extension. Both pre-release (currently version 0.4.2320), and release (currently version 0.3.2319) channels.

rustc version: rustc 1.87.0-nightly (5bc623145 2025-02-16)

editor or extension: VSCode, both bundled RA and the one gotten through rustup

relevant settings: Tried with both the RA extension's bundled RA as well as the toolchain's.

  • CARGO_HOME=/home/martin/.rust/.cargo
  • RUSTUP_HOME=/home/martin/.rust

For helix the config is untouched Relevant VSCode settings are added to the bottom because code blocks in spoilers break. There is no global RA config. Only some personal rustmft for line lengths and other hills not to die on.

repository link (if public, optional): -

code snippet to reproduce:

Only one dependency needed. Don't know of any other crates with similar transparent structs by heart, and the bug doesn't reproduce within a crate or workspace.

[package]
    edition = "2024"
    name    = "ra-repr-mre"
    version = "0.1.0"

[dependencies]
    windows = {version = "0.60", features = ["Win32_Foundation"]}
use windows::core::{Error as WErr, HRESULT};

fn some_res(succeed: i32) -> Result<(), WErr> {
    match succeed {
        1 => Err(WErr::new(HRESULT(succeed), "This doesn't work")), // Error: expected an i32
        2 => Err(WErr::new(succeed, "Neither does this")),          // Error: expected a HRESULT. This also blocks compilation, and that seems like correct behavior to me
        // 3 and 255 removed, they added nothing of value.
        0 => Ok(()),
        _ => Err(WErr::new(HRESULT(succeed).into(), "This is what you need for it to work")),
    }
}

fn main() {
    some_res(1).expect("If you get here, compilation succeeded")
}

VSC settings, I've tweaked several of these to no avail.

"[rust]": {
        "editor.defaultFormatter": "rust-lang.rust-analyzer",
        "editor.formatOnSave": true,
        "editor.formatOnSaveMode": "file",
        "editor.inlayHints.enabled": "on"
    },
    "rust-analyzer.cargo.autoreload": true,
    "rust-analyzer.checkOnSave": true,
    "rust-analyzer.diagnostics.enable": true,
    "rust-analyzer.highlightRelated.breakPoints.enable": true,
    "rust-analyzer.highlightRelated.exitPoints.enable": true,
    "rust-analyzer.highlightRelated.references.enable": true,
    "rust-analyzer.highlightRelated.yieldPoints.enable": true,
    "rust-analyzer.hover.actions.enable": true,
    "rust-analyzer.hover.documentation.enable": true,
    "rust-analyzer.hover.links.enable": true,
    "rust-analyzer.imports.granularity.enforce": true,
    "rust-analyzer.imports.granularity.group": "crate",
    "rust-analyzer.imports.group.enable": true,
    "rust-analyzer.imports.merge.glob": false,
    "rust-analyzer.joinLines.joinAssignments": true,
    "rust-analyzer.restartServerOnConfigChange": true,
    "rust-analyzer.server.path": "rust-analyzer.exe",
    "rust-analyzer.signatureInfo.detail": "full",
    "rust-analyzer.signatureInfo.documentation.enable": true,
    "rust-analyzer.typing.autoClosingAngleBrackets.enable": true,
    "rust-analyzer.workspace.symbol.search.kind": "all_symbols",
    "rust-analyzer.workspace.symbol.search.scope": "workspace_and_dependencies",
    "rust-analyzer.semanticHighlighting.punctuation.specialization.enable": true,

RivenSkaye avatar Feb 25 '25 10:02 RivenSkaye

If HRESULT is defined as shown in the screenshot, i.e. struct HRESULT(i32), then RA's type error is correct. #[repr(transparent)] doesn't mean that an i32 would be implicitly converted to it. So apparently rust-analyzer and rustc disagree about the definition of HRESULT there; maybe the windows crate does some weird conditional thing there? I haven't looked yet.

flodiebold avatar Feb 25 '25 11:02 flodiebold

It seems to have both: https://github.com/microsoft/windows-rs/blob/c9ae41f507f41bf45d922b11b036f17435c419cc/crates/libs/result/src/bindings.rs#L51 and https://github.com/microsoft/windows-rs/blob/c9ae41f507f41bf45d922b11b036f17435c419cc/crates/libs/result/src/hresult.rs#L8 So probably some name resolution or conditional compilation problem?

flodiebold avatar Feb 25 '25 11:02 flodiebold

The windows crate explicitly exports the newtype version: https://github.com/microsoft/windows-rs/blob/c9ae41f507f41bf45d922b11b036f17435c419cc/crates/libs/result/src/lib.rs#L32 and the Error::new definition uses HRESULT via use super::*: https://github.com/microsoft/windows-rs/blob/c9ae41f507f41bf45d922b11b036f17435c419cc/crates/libs/result/src/error.rs#L91 So I'm still confused why it would come up with the i32 type alias instead of the newtype.

It even has an impl for HRESULT, which would not be allowed for the type alias: https://github.com/microsoft/windows-rs/blob/master/crates/libs/result/src/error.rs#L159

flodiebold avatar Feb 25 '25 11:02 flodiebold

Ah wait, I was reading the error messages wrong and RA is the one defining HRESULT as the type alias, not the other way around. Yeah then this seems to be a problem with our handling of glob imports.

flodiebold avatar Feb 25 '25 11:02 flodiebold

Oh, my bad on not including the info. The relevant HRESULT is defined as:

/// An error code value returned by most COM functions.
#[repr(transparent)]
#[derive(Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[must_use]
#[allow(non_camel_case_types)]
pub struct HRESULT(pub i32);

impl<T> From<Result<T>> for HRESULT {
    fn from(result: Result<T>) -> Self {
        if let Err(error) = result {
            return error.into();
        }
        HRESULT(0)
    }
}

// skipped impl HRESULT for brevity, see https://github.com/microsoft/windows-rs/blob/master/crates/libs/result/src/hresult.rs for impl

Where the From<Result<_,_>> implementations seem to only be available for the nearly identical (and non-public) structs NTSTATUS and WIN32_ERROR, as well as From<HRESULT> being there. I'll try and see if I can set up a more minimal MRE, possibly something that leverages local paths (but not workspaces) for deps

RivenSkaye avatar Feb 25 '25 13:02 RivenSkaye

well the relevant code seems to be basically this:

mod bindings {
    pub type HRESULT = i32;
}
use bindings::*;
mod hresult {
    pub struct HRESULT(i32);
}
pub use hresult::*;
pub use hresult::HRESULT;
mod error {
    use super::*;
    pub fn new_error(result: HRESULT) {}
}
fn test() {
    error::new_error(HRESULT(0));
}

but this doesn't seem to be enough to reproduce the problem in my RA, so maybe I'm missing something else. (I can't try to reproduce the original problem since I'm not on Windows.)

flodiebold avatar Feb 25 '25 13:02 flodiebold

Minimal reproduction:

mod bindings {
    pub type HRESULT = i32;
}
use bindings::*;
mod error {
    use super::*;
    pub fn nonzero_hresult(hr: HRESULT) {
        _ = hr.0;
    }
}
mod hresult {
    pub struct HRESULT(pub i32);
}
pub use hresult::HRESULT;

This is because the pub use hresult::HRESULT comes too late. If we move it above mod error, it works.

ChayimFriedman2 avatar Feb 26 '25 07:02 ChayimFriedman2

One day we will handle glob imports completely correct

Veykril avatar Feb 26 '25 08:02 Veykril

That day is far in the future, I fear 😂

If you are making a new language, choose between glob imports and cyclic modules. Having both leads to so many bugs.

ChayimFriedman2 avatar Feb 26 '25 08:02 ChayimFriedman2

rustc and cargo check don't have problems with this. Is there any specific reason that their approach can't be copied here? And is there any reason the exact same RA binary isn't producing this warning on the CLI or through helix?

RivenSkaye avatar Feb 26 '25 08:02 RivenSkaye

rustc had a lot more time and effort spent fixing its name resolution problems (and copying what it does is indeed what I intend to do to fix this issue).

I don't know about Helix, but how are you running rust-analyzer CLI? (Edit: Maybe because the mismatched types error is experimental?)

ChayimFriedman2 avatar Feb 26 '25 08:02 ChayimFriedman2

how are you running rust-analyzer CLI?

The troubleshooting guide's recommended rust-analyzer analysis-stats .

Edit: Maybe because ...

Could be, but I deleted the MRE code (and have long since changed the code that even led me to this issue) so I'm not in any position to check right now

RivenSkaye avatar Feb 26 '25 09:02 RivenSkaye

Please fix this issue.

AndreySmirnov81 avatar May 15 '25 16:05 AndreySmirnov81

@AndreySmirnov81 This issue is not a small take. In order for the fix to be workable and maintainable, a big undertaking is required (for more details, see #t-compiler/rust-analyzer > Nameres is cursed). I'm slowly drifting through it, but I have other things too, and this issue is not in the top of the list. If it is so important to you, you can fix it yourself. I can give mentoring instructions, but they'd be vague because the fix is basically "rewrite our whole early name resolution code".

ChayimFriedman2 avatar May 15 '25 17:05 ChayimFriedman2

That sounds very fun to do, but I doubt my employer would pay me for those hours.

Take your sweet time, it's a bit of an edge case to hit anyway

RivenSkaye avatar May 15 '25 18:05 RivenSkaye