Trait auto import not working for some cases (e.g. tokio AsyncWriteExt in async method)

[samlh]

(Edit: In this example) rustc returns a nice error if you forget to import a needed trait - it would be nice to have a suggestion in rust-analyser to add the use statement.

use tokio::io::AsyncWrite;

pub struct PlaylistWriter<F: AsyncWrite + Unpin + Send> {
    playlist_file: F,
}

impl<F: AsyncWrite + Unpin + Send> PlaylistWriter<F> {
    pub async fn start(mut playlist_file: F) -> eyre::Result<Self> {
        playlist_file.write_all(b"#EXTM3U\n").await?;

        Ok(PlaylistWriter { playlist_file })
    }
}

error[E0599]: no method named `write_all` found for type parameter `F` in the current scope
 --> src/temp.rs:9:23
  |
9 |         playlist_file.write_all(b"#EXTM3U\n").await?;
  |                       ^^^^^^^^^ method not found in `F`
  |
  = help: items from traits can only be used if the trait is in scope
  = note: the following trait is implemented but not in scope; perhaps add a `use` for it:
          `use tokio::io::util::async_write_ext::AsyncWriteExt;`

In this case, the ideal would be to suggest updating the use statement to:

use tokio::io::{AsyncWrite, AsyncWriteExt};

[SomeoneToIgnore]

Thank you for the report.

We have a functionality for importing traits, introduced in #3120
From a UI perspective, it works the same as for the struct auto import: you place a caret on the method and press Cmd/Ctrl + . to toggle the import hints.

Yet the trait auto imports may work worse than the regular auto imports, since for every trait candidate that has a method with the same name we attempt to do the trait solving with Chalk to ensure that the types used in the expression are correct.
To do the trait solving, the internal RA (rust-analyzer) type inference data is used and if RA does not know any type involved in the solving (or deduced it incorrectly), we're not able to prove that the trait is the right one and import it.

Unfortunately, current RA support for async/await is somewhat lacking due to #1791, #4018 and some other issues.
Also, I see that AsyncWriteExt is hidden behind a macro which might also affect the resolution in this particular case.

https://github.com/tokio-rs/tokio/blob/4748b2571fc02d5ebbfe59e457f0e8d8ef0eb5f3/tokio/src/macros/cfg.rs#L157

macro_rules! cfg_io_util {
    ($($item:item)*) => {
        $(
            #[cfg(feature = "io-util")]
            #[cfg_attr(docsrs, doc(cfg(feature = "io-util")))]
            $item
        )*
    }
}

We might lack the feature cfg support in macros too, but I'm not sure about this alas.

I have not debugged this case myself, but I'm 90% sure it's the trait solving that we fail at and we cannot improve the situtation that much before the async-related issues are dealt with.

So, I would have closed this issue and waited for the proper async/await support before creating another one, or at least changed the issue description to a more presice one to avoid confusion.

[samlh]

Sure, updated title

[samlh]

Hopefully a less complex example I've been running into:

[dependencies]
eyre = "0.5.0"

use std::process::{ExitStatus, Child};
// use eyre::WrapErr;

pub fn wait_for_exit(mut child: Child) -> eyre::Result<ExitStatus> {
    child.wait().wrap_err("process wait failed")
}

error[E0599]: no method named `wrap_err` found for enum `std::result::Result<std::process::ExitStatus, std::io::Error>` in the current scope
    --> src/temp.rs:6:18
     |
6    |     child.wait().wrap_err("process wait failed")
     |                  ^^^^^^^^ help: there is an associated function with a similar name: `map_err`
     | 
    ::: /home/samlh/.cargo/registry/src/github.com-1ecc6299db9ec823/eyre-0.5.0/src/lib.rs:1030:8
     |
1030 |     fn wrap_err<D>(self, msg: D) -> Result<T, Report<H>>
     |        --------
     |        |
     |        the method is available for `std::boxed::Box<std::result::Result<std::process::ExitStatus, std::io::Error>>` here
     |        the method is available for `std::sync::Arc<std::result::Result<std::process::ExitStatus, std::io::Error>>` here
     |        the method is available for `std::rc::Rc<std::result::Result<std::process::ExitStatus, std::io::Error>>` here
     |
     = help: items from traits can only be used if the trait is in scope
     = note: the following trait is implemented but not in scope; perhaps add a `use` for it:
             `use eyre::WrapErr;`

[lnicola]

Is the rustc suggestion marked as machine-applicable?

[bjorn3]

The suggestion is marked as MaybeIncorrect: https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/method/suggest.rs#L870

[SomeoneToIgnore]

Thanks for the reduced example, I've looked into it more and looks like I've found the culprit.

Recently, for the search in the dependencies, we've switched to a faster & more context-aware search index from a plain text search.
For searching in the local project's files, we keep using the previous text search approach.

To locate a trait to import for a function in question, we first look up the trait functions by their name, but the new index does not keep any function information: https://github.com/rust-analyzer/rust-analyzer/blob/08a58901e8deee22a541ed14f7e6b02e3c57132e/crates/ra_hir_def/src/import_map.rs#L37-L48
with ItemInNs defined as
https://github.com/rust-analyzer/rust-analyzer/blob/08a58901e8deee22a541ed14f7e6b02e3c57132e/crates/ra_hir_def/src/item_scope.rs#L180-L184

Since we still find trait imports in the local code and the auto_import.rs tests do not check importing from external crates, we overlooked the issue after the new index switch.

[SomeoneToIgnore]

I think at some point later it would be great to add a test to auto_import.rs that checks a separate crate imports, but the testing snippet infra might need some adjustments.

As for the trait imports, @jonas-schievink, should we store the function lookup data too in the index?

[jonas-schievink]

Hmm, yeah, we either need to store all trait methods in the ImportMap, or have some separate structure for looking up matching traits. I think adding that to ImportMap should work fine.

[jonas-schievink]

Oh, I wondered why this doesn't work anymore, didn't realize the ImportMap broke it. Will fix.