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

feature : assist delegate impl #14948

Merged
merged 4 commits into from Jun 22, 2023
Merged

feature : assist delegate impl #14948

merged 4 commits into from Jun 22, 2023

Conversation

alibektas
Copy link
Member

This PR ( fixes #14386 ) introduces a new IDE assist that generates a trait impl for a struct that delegates a field. This is a draft because the current ide_db::path_transform::PathTransform produces some unwanted results when it deals with extern crates, an example of which I attach as a GIF.

GIFs :

  1. A general case
    14386-functional

  2. A case where ide_db::path_transform::PathTransform fails to correctly resolve a property ( take Allocator as an example ) to its full path, thus causing an error to occur. ( Not to even mention that resolving this causes another error use of unstable library feature 'allocator_api' to occur
    14386-erroneous

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2023
@alibektas alibektas changed the title feature : assist generate delegate trait impl feature : assist delegate impl Jun 2, 2023
@alibektas
Copy link
Member Author

#14957 has a valid point that we may need to fix as well.

@Veykril
Copy link
Member

Veykril commented Jun 12, 2023

We can add a comment to that issue once this PR is merged. There are probably more assists that should look into that.

crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/generate_delegate_trait.rs Outdated Show resolved Hide resolved
Comment on lines +219 to +205
// FIXME : We can omit already implemented impl_traits
// But we don't know what the &[hir::Type] argument should look like.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this I think we need to make use of TraitRefs instead of plain Traits. TraitRefs basically consist of a Trait and it's arguments, given we expose TraitRefs in hir now we might as well want to change Type::impls_trait to take a TraitRef instead, or make a new method that accepts one. But let's leave that for a follow up

Remove scope_for_def calls as the definition have been removed entirely.
As a result of this change the problem with false path resolutions has been solved.
@alibektas
Copy link
Member Author

alibektas commented Jun 17, 2023

After the latest version the problem for which I left a GIF above has been solved. There are still a few other problems though. One problem was briefly mentioned above in GIF(2) , that in certain cases we generate trait implementation for unstable library features. Such traits should best be skipped entirely if it's possible. I am open to suggestions regarding this. The other problems are as far as I can see rather tiny. Next commit should address these issues.

@alibektas alibektas marked this pull request as ready for review June 20, 2023 09:26
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this mostly looks good but there is one thing that we should fix still, which can be seen in your second gif (im not talking about the unstable thing being proposed). We forget to account for the substituted generics. Example:

struct S { f: Foo<i32>$0 }

struct Foo<U>(U);

trait T<U> {
    type Ty;
    fn f(self) -> Self::Ty;
}
impl<U> T<U> for Foo<U> {
    type Ty = U;
    fn f(self) -> Self::Ty {
        self.0
    }
}

Current assist would generate the following which is invalid

impl<U> T<U> for S {
    type Ty = <Foo<U> as T<U>>::Ty;
    fn f(self) -> Self::Ty {
        <Foo<U> as T<U>>::f(self.f) // type error, `<Foo<U> as T<U>>::f` expects a value of type `Foo<U>`, but here we give it a `Foo<i32>`
    }
}

It should instead generate this:

impl T<i32> for S {
    type Ty = <Foo<i32> as T<i32>>::Ty;
    fn f(self) -> Self::Ty {
        <Foo<i32> as T<i32>>::f(self.f)
    }
}

@Veykril
Copy link
Member

Veykril commented Jun 22, 2023

Actually, fixing that seems pretty tricky so lets leave that out of this PR for a follow up.
@bors r+

@bors
Copy link
Collaborator

bors commented Jun 22, 2023

📌 Commit 4ed1197 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 22, 2023

⌛ Testing commit 4ed1197 with merge f0e00ed...

@bors
Copy link
Collaborator

bors commented Jun 22, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f0e00ed to master...

@bors bors merged commit f0e00ed into rust-lang:master Jun 22, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate delegate impl
4 participants