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

Import trait not suggested #16989

Closed
tamasfe opened this issue Apr 1, 2024 · 18 comments · Fixed by #17270
Closed

Import trait not suggested #16989

tamasfe opened this issue Apr 1, 2024 · 18 comments · Fixed by #17270
Labels
C-bug Category: bug

Comments

@tamasfe
Copy link
Contributor

tamasfe commented Apr 1, 2024

rust-analyzer version: rust-analyzer version: 0.4.1905-standalone
rustc version: rustc 1.76.0 (07dca489a 2024-02-04)

editor or extension: vscode

issue:

We have an example trait somewhere:

trait NotInScope {
  fn not_in_scope(&self) {}
}

impl<T> NotInScope for T {}

When calling a trait function on a variable (foo.not_in_scope()) and the trait is not in scope, import of the trait is not offered anymore. rustc correctly fails with the following trait is implemented but not in scope; perhaps add a use for it: ....

@tamasfe tamasfe added the C-bug Category: bug label Apr 1, 2024
@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 1, 2024

The bad commit after a bisect turned out to be c246a93.
I could not produce an MCVE unfortunately, as it only happens in a large-ish private repo with a lot of generated code.

I can only guess what is required to repro:

  • generated code that is include!()-d
  • workspace with path dependencies
  • ???

Hopefully now that it is known which commit causes the issue, someone more knowledgeable than me can figure why it causes the issue.

@Veykril
Copy link
Member

Veykril commented Apr 2, 2024

Do you have any information on the implementations of said trait (and the crates they live in), the crate of the trait itself, the receiver type in your example (and the crate of your the receiver type if it is an ADT) and the dependency relationships between the releveant crates? That's "all" t hat is needed in terms of info for this

@davidbarsky looks like your approach does cause some problems 😕

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 2, 2024

I just checked it out, the structure looks something like this:

/
├── completion-here/
│   └── Cargo.toml
├── common/
│   ├── Cargo.toml
│   └── ...
├── proto/
│   ├── Cargo.toml
│   ├── build.rs
│   └── ...
├── proto-derive/
│   └── Cargo.toml
└── Cargo.toml

The plot is the following, and contains several twists:

  • Code is generated in proto/build.rs, the generated code lives in OUT_DIR and is included via tonic's include_proto.
  • The trait itself is generated by a derive macro that lives in proto-derive inside the generated code (the only way to hook into tonic's code generation)
  • There's yet another trait that is generated by a macro_rules! macro in common and acts as a wrapper such as trait Wrapper { type Foo: NotInScope; fn foo(&self) -> Self::Foo; }, this macro is also used via derive where the other trait was generated, so this wrapper trait is generated by a macro that is generated by a macro inside generated code

The trait is used via the wrapper trait that is bound to a generic type:

fn foo<T: Wrapper>(t: T) {
  t.foo().not_in_scope();
         ^^^^^^^^^^^^^
}

So there are several layers of macroception as well as generics, I'm not surprised no one else has run into this.

I'll try to get an MCVE sometime this week based on this setup.

@davidbarsky
Copy link
Contributor

looks like your approach does cause some problems 😕

ugh, good to know. given that basically nobody uses include! and friends at work, it's not surprising nobody reported issues. i wonder if this occurs due to some mismapped crate-of-origin business in macro expansion? then again, idk how hygiene works :/

@tamasfe: i'm sorry about the trouble!

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 7, 2024

I set up an MCVE here: https://github.com/tamasfe/ra-completion-repro/blob/master/crates/completion-here/src/lib.rs

It seems that adding the "wrapper" trait is what breaks completion:

pub trait Wrapper {
    type Inner: NotInScope;
    fn inner(&self) -> Self::Inner;
}

So far it doesn't seem to matter where this trait is defined, via macro, include! or any other means, completion does not seem to work even if it is defined in the same crate.

It also seems to me that any macros or include! do not seem to matter, it is enough to define Wrapper and NotInScope in separate crates.

@tamasfe
Copy link
Contributor Author

tamasfe commented May 8, 2024

@Veykril @davidbarsky sorry to bother, it seems my knowledge is rather limited here and I won't have enough time to dive into the issue anytime soon.

Would it be possible to (partially?) revert the changes in the linked commit until this is solved, as the codebase I'm working with relies heavily on the patterns outlied in this issue and we have to maintain a separate older RA version.

@davidbarsky
Copy link
Contributor

@Veykril @davidbarsky sorry to bother, it seems my knowledge is rather limited here and I won't have enough time to dive into the issue anytime soon.

Would it be possible to (partially?) revert the changes in the linked commit until this is solved, as the codebase I'm working with relies heavily on the patterns outlied in this issue and we have to maintain a separate older RA version.

I didn't see your update with the reproduction, sorry! I don't think I will revert it, but I'll work on reproducing it tomorrow.

@davidbarsky
Copy link
Contributor

davidbarsky commented May 8, 2024

If I don't make sufficient progress tomorrow, I was thinking of adding a config option to disable the filtering I introduced in that PR. Would that be okay with you?

@tamasfe
Copy link
Contributor Author

tamasfe commented May 8, 2024

If I don't make sufficient progress tomorrow, I can add a config option to disable the filtering I introduced in that PR: is that okay?

Yeah, thank you, much appreciated!

@davidbarsky
Copy link
Contributor

Commenting to link to #16555, since I couldn't find this issue be linked from the PR.

@davidbarsky
Copy link
Contributor

You may have alluded to this already, but the reproduction can be even more minimal. Writing the following in a single crate:

pub trait NotInScope {
    fn not_in_scope(&self);
}

pub trait Wrapper {
    type Inner: NotInScope;
    fn inner(&self) -> Self::Inner;
}

pub struct Whatever {
    pub id: i32,
}

impl NotInScope for Whatever {
    fn not_in_scope(&self) {
        todo!()
    }
}

...and in another crate, trying to do completions on:

// No completion here for generic type
fn no_completion<T: Wrapper>(whatever: T) {
    whatever.inner().$0
}

...results in no completions. Something about the associated type isn't being recorded in hir_ty::method_resolution::TraitImpls. Commenting out bit of code, however, results in completions returning.

@davidbarsky
Copy link
Contributor

My conclusions is that TraitImpls isn't aware of any associated types. I'll see what I can do to make it aware, but if there isn't anything I can do easily, I'll build something atop of #17252 with a configuration option.

@flodiebold
Copy link
Member

flodiebold commented May 20, 2024

Why would TraitImpls need to be aware of associated types? What would it mean for it to be aware of associated types?

@flodiebold
Copy link
Member

I think probably the fix rather needs to be to adapt the logic in import_assets.rs to not do this filtering for associated types (and placeholder types).

@davidbarsky
Copy link
Contributor

Why would TraitImpls need to be aware of associated types? What would it mean for it to be aware of associated types?

i don't honestly know: all i can see is that one of the traits (Wrapper) that I'd naively expect to be in scope isn't found. I have no idea why.

@flodiebold
Copy link
Member

Yeah, if I understand correctly that's because the filtering logic is incorrect for these kinds of receiver types -- it's based on the assumption that the type can only implement traits for which a possibly matching impl exists, but for an associated type it can also implement traits through implied bounds. The assumption also doesn't work for placeholder types, like in this case:

fn foo<T: SomeTrait>(t: T) { t.<|> }

but here it doesn't matter since any such trait will be in scope automatically already, so we don't need to care about it in autoimport.
TraitImpls just gives you actual impls. There is no impl for the associated type, so it's correct that it returns nothing.

@davidbarsky
Copy link
Contributor

thanks for the tip! this should fix it: #17270

@tamasfe
Copy link
Contributor Author

tamasfe commented May 21, 2024

Just checked the PR out, works great indeed, thank you!

@bors bors closed this as completed in 653b69e May 22, 2024
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
4 participants