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

Methods provided via Deref<Target> are not being found #13238

Closed
woody77 opened this issue Sep 16, 2022 · 21 comments
Closed

Methods provided via Deref<Target> are not being found #13238

woody77 opened this issue Sep 16, 2022 · 21 comments
Labels
A-ty type system / type inference / traits / method resolution

Comments

@woody77
Copy link
Contributor

woody77 commented Sep 16, 2022

rust-analyzer version: (eg. output of "rust-analyzer: Show RA Version" command, accessible in VSCode via Ctrl/⌘+Shift+P)

rustc version:
rust-analyzer version: 0.4.1202-standalone
rustc 1.65.0-nightly (350cca3b6 2022-08-30)

relevant settings: VSCode remote, with a very large project (1000s of crates), no extra env settings for CHALK

Methods from OtherType that are defined for a type (ThisType) via implementing Deref<Target=OtherType> are not being found as valid symbols, and flagged as unresolvedReference.

Using the following struct:

struct Wrapper(pub String);

impl std::ops::Deref for Wrapper {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        self.0.as_str()
    }
}

The following do not resolve .chars():
Screen Shot 2022-09-15 at 5 02 03 PM

But the following do resolve chars() correctly:
Screen Shot 2022-09-15 at 5 02 11 PM

Full source (and proof that these work) at:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d5c1d59fa47c1228df2be80042ec41a1

@woody77
Copy link
Contributor Author

woody77 commented Sep 16, 2022

This used to work for us, and now we've been seeing this for the last few weeks at least.

@woody77
Copy link
Contributor Author

woody77 commented Sep 16, 2022

And to use all types from the same file:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1778880d91ecbeae9ee16cfd7c77e613

impl Wrapper {
    fn print_self(&self) {
        println!("{}", self.0);
    }
}

struct OtherWrapper(pub Wrapper);

impl std::ops::Deref for OtherWrapper {
    type Target=Wrapper;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

Using the methods from the Deref<Target=Wrapper> impl for OtherWrapper aren't resolved:

Screen Shot 2022-09-15 at 5 17 55 PM

@Veykril
Copy link
Member

Veykril commented Sep 16, 2022

So this only happens in this big project of yours?

@woody77
Copy link
Contributor Author

woody77 commented Sep 16, 2022

It's where I see it. It's both:

  • rust-project.json
  • 1000s of crates

We've had some issues in the past where we needed to do things like increase chalk's overflow limits to stop things from breaking. I don't have a good sense of how to go about seeing if that's an issue (turning on any logging at all becomes a massive flood due to the number of crates involved).

If you give some guidance on what tracing to turn on, I can see if this is something like chalk giving up on the resolution of the types.

@Veykril Veykril added the A-ty type system / type inference / traits / method resolution label Sep 17, 2022
@tbodt
Copy link

tbodt commented Sep 23, 2022

Bisected to 2c2520f

@woody77
Copy link
Contributor Author

woody77 commented Sep 23, 2022

Which is the sysroot support for project_json workspaces PR:
#12858

@tbodt
Copy link

tbodt commented Oct 21, 2022

On further investigation, this has nothing to do with Deref. The problem can be seen with just this:

trait Foo {
    type Bar;
    fn foo(&self) -> Bar;
}

impl Foo for () {
    type Bar = i32;
    fn foo(&self) -> Bar { 0 }
}

fn foo() {
    let _a = ().foo();
}

rust-analyzer can't infer the type of _a. Changing Bar to be a trait type parameter instead of an associated type fixes it. So the bug is something to do with associated types.

edit: this was a red herring, the actual problem was me not realizing I needed to write Self::Bar and not just Bar

@kangalio
Copy link

kangalio commented Oct 23, 2022

👍 This issue comes up quite often for me due to many useful methods being implemented on the Deref type.

  • Vec: .iter(), .len(), .get(), .first()
  • String: .parse(), .split(), .trim().

Most notably, those unrecognized methods break autocomplete, Go To Definition and type inference.

@flodiebold
Copy link
Member

@kangalioo are you working in the same project as @woody77, or a similar large project using rust-project.json?

@woody77
Copy link
Contributor Author

woody77 commented Oct 23, 2022

While @tbodt did run down a red-herring, while talking with him about it Friday I confirmed that specifying a sysroot in rust-project.json causes RA to inject the sysroot crates:

.chain(mk_sysroot(sysroot.as_ref()))

This changed in: https://github.com/rust-lang/rust-analyzer/pull/12858/files#diff-53e4b6b2c1ff2465d8abd50db160753199426fd788ddcad925752e0838e28de2R233

Before, if rust-project.json specified a sysroot_src that would be used to load the sysroots. As of that change, specifying a sysroot causes the sysroot_src to be inferred, and then ProjectWorkspace::to_roots() will find a sysroot (actually a sysroot_src), and then inject the sysroot packages.

And now there are two sets of sysroot packages found in the deps of every crate, it seems?

How to best model this for RA in a cross-compiling, multi-architecture build like we (Fuchsia) have is unclear. RA today doesn't handle different configurations of crates, but we compile many for both host and target (often both different in OS and Arch: linux.x64 and mac.x64/aarch64 vs. fuchsia.arm64 or fuchsia.x64).

I can remove the code that adds the sysroot crates (from the class in GN that produces our rust-project.json file), although I'll have to think about how to locate the sysroot if they're not directly in the crate graph that it's generating.

@Veykril
Copy link
Member

Veykril commented Oct 24, 2022

Before, if rust-project.json specified a sysroot_src that would be used to load the sysroots. As of that change, specifying a sysroot causes the sysroot_src to be inferred, and then ProjectWorkspace::to_roots() will find a sysroot (actually a sysroot_src), and then inject the sysroot packages.

I am a bit confused, are you saying you are no longer specifying the sysroot_src as you did before? If you have the sysroot_src specified then behavior shouldn't really have changed for you here, no matter if you also specified the sysroot or not now from what I can tell?

@woody77
Copy link
Contributor Author

woody77 commented Oct 25, 2022

No, we've never specified sysroot_src. That was added after we added our rust-project.json emitter to GN, and were already adding the sysroot crates as part of rust-project.json (as was required at the time).

When we started specifying sysroot, it started to infer what the sysroot_src would have been, and RA started adding the sysroot crates to the list of roots, even though we've already included them.

This happens at:

(Some(sysroot), None) => {
// assume sysroot is structured like rustup's and guess `sysroot_src`
let sysroot_src =
sysroot.join("lib").join("rustlib").join("src").join("rust").join("library");
Some(Sysroot::load(sysroot, sysroot_src)?)

@Veykril
Copy link
Member

Veykril commented Oct 25, 2022

Hmm, so why are you now specifying sysroot when you are still managing the crates manually for it?

@woody77
Copy link
Contributor Author

woody77 commented Oct 26, 2022 via email

@Veykril
Copy link
Member

Veykril commented Oct 26, 2022

Right, unfortunately the way things are written atm we either expect a sysroot fully or we expect none. Changing things is a bit more involved and I'd need to do some digging to see if there is a feasible way to change stuff to not error out when certain parts are missing (by choice). Nevertheless it would be probably beneficial for you to look into supporting the sysroot via the correct fields instead of adding them manually if possible.

@woody77
Copy link
Contributor Author

woody77 commented Oct 26, 2022

I'm also (in my "spare time") looking at seeing what the impact is of not including the sysroot crates in our generated rust-project.json file. The only concern I have is the proc_macro crate, which we add to proc macros, and I haven't found if RA adds that for implicitly as well.

@Veykril
Copy link
Member

Veykril commented Oct 26, 2022

r-a should add the proc-macro crate to everything as a dependency (because every crate type, even non proc-macro ones have it as a dependency in rust)

@woody77
Copy link
Contributor Author

woody77 commented Oct 28, 2022

I've got a PR up for our rust-project.json generation, which removes the explicit addition of the sysroot crates, and lets rust-analyzer infer it from the sysroot field.

@woody77 woody77 closed this as completed Oct 28, 2022
@woody77
Copy link
Contributor Author

woody77 commented Oct 28, 2022

The CL has landed, and our next GN roll should pick it up and fix this for our developers. Thanks for all the debugging help!

@zhiqiangxu
Copy link

zhiqiangxu commented May 3, 2023

It seems this issue still exists occasionally at least for rust-analyzer version: 0.3.1498-standalone (3a27518fe 2023-04-30).

@flodiebold
Copy link
Member

@zhiqiangxu probably not the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution
Projects
None yet
Development

No branches or pull requests

6 participants