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

fix: rewrite code_action generate_delegate_trait #16112

Merged
merged 2 commits into from Jan 2, 2024

Conversation

roife
Copy link
Contributor

@roife roife commented Dec 13, 2023

I've made substantial enhancements to the "generate delegate trait" code action in rust-analyzer. Here's a summary of the changes:

Resolved the "Can’t find CONST_ARG@158..159 in AstIdMap" error

Fix #15804, fix #15968, fix #15108

The issue stemmed from an incorrect application of PathTransform in the original code. Previously, a new 'impl' was generated first and then transformed, causing PathTransform to fail in locating the correct AST node, resulting in an error. I rectified this by performing the transformation before generating the new 'impl' (using make::impl_trait), ensuring a step-by-step transformation of associated items.

Rectified generation of Self type

generate_delegate_trait is unable to properly handle trait with Self type.

Let's take the following code as an example:

trait Trait {
    fn f() -> Self;
}

struct B {}
impl Trait for B {
    fn f() -> B { B{} }
}

struct S {
    b: B,
}

Here, if we implement Trait for S, the type of f should be () -> Self, i.e. () -> S. However we cannot automatically generate a function that constructs S.

To ensure that the code action doesn't generate delegate traits for traits with Self types, I add a function named has_self_type to handle it.

Extended support for generics in structs and fields within this code action

The former version of generate_delegate_trait cannot handle structs with generics properly. Here's an example:

struct B<T> {
    a: T
}

trait Trait<T> {
    fn f(a: T);
}

impl<T1, T2> Trait<T1> for B<T2> {
    fn f(a: T1) -> T2 { self.a }
}

struct A {}
struct S {
    b$0 : B<A>,
}

The former version will generates improper code:

impl<T1, T2> Trait<T1, T2> for S {
    fn f(&self, a: T1) -> T1 {
        <B as Trait<T1, T2>>::f( &self.b , a)
    }
}

The rewritten version can handle generics properly:

impl<T1> Trait<T1> for S {
    fn f(&self, a: T1) -> T1 {
        <B<A> as Trait<T1>>::f(&self.b, a)
    }
}

See more examples in added unit tests.

I enabled support for generic structs in generate_delegate_trait through the following steps (using the code example provided):

  1. Initially, to prevent conflicts between the generic parameters in struct S and the ones in the impl of B, I renamed the generic parameters of S.
  2. Then, since B's parameters are instantiated within S, the original generic parameters of B needed removal within S (to avoid errors from redundant parameters). An important consideration here arises when Trait and B share parameters in B's impl. In such cases, these shared generic parameters cannot be removed.
  3. Next, I addressed the matching of types between B's type in S and its type in the impl. Given that some generic parameters in the impl are instantiated in B, I replaced these parameters with their instantiated results using PathTransform. For instance, in the example provided, matching B<A> and B<T2>, where T2 is instantiated as A, I replaced all occurrences of T2 in the impl with A (i.e. apply the instantiated generic arguments to the params).
  4. Finally, I performed transformations on each assoc item (also to prevent the initial issue) and handled redundant where clauses.

For a more detailed explanation, please refer to the code and comments. I welcome suggestions and any further questions!

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2023
@roife
Copy link
Contributor Author

roife commented Dec 13, 2023

Here's an simple example explaining the third step mentioned above. Say we have the following code:

struct B<T> {
    a: T
}

trait Trait<T> {
    fn f(&self) -> &T;
}

impl<T> Trait<T> for B<T> {
    fn f(&self) -> &T { &self.a }
}

struct S<T> {
    a: B<T>
}

To generate a delegate impl for a in S<T>, the proper implementation is as following:

// generated after rewritten
impl<T> Trait<T> for S<T> {
    fn f(&self) -> &T {
        <B<T> as Trait<T0>>::f(&self.a)
    }
}

However, if we do not match and replace instantiated generic arguments, we will get

impl<T, T0> Trait<T> for S<T0> {
    fn f(&self) -> &T {
        <B<T0> as Trait<T>>::f(&self.a)
    }
}

which is not correct.

Comment on lines +576 to +582
pub fn generic_arg_list(&self) -> Option<ast::GenericArgList> {
if let ast::Type::PathType(path_type) = self {
path_type.path()?.segment()?.generic_arg_list()
} else {
None
}
}
Copy link
Member

Choose a reason for hiding this comment

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

There maybe more than one generic arg list in a type, Qux<(Foo<A, B>, Bar<C, D>>) for example, also generic args might apear in other spots like [T], (T, U, V) etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! I hadn't considered this situation before.

These situations might make the analysis a bit more complex; I need to reorganize my current approach and think about how to elegantly address this issue. ❤️

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.

Small comment, but thats something that could be addressed as a follow up

@Veykril
Copy link
Member

Veykril commented Jan 2, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Jan 2, 2024

📌 Commit 5070534 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 2, 2024

⌛ Testing commit 5070534 with merge 34df296...

@bors
Copy link
Collaborator

bors commented Jan 2, 2024

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

@bors bors merged commit 34df296 into rust-lang:master Jan 2, 2024
10 checks passed
bors added a commit that referenced this pull request Jan 5, 2024
…t, r=Veykril

internal: clean and enhance readability for `generate_delegate_trait`

Continue from #16112

This PR primarily involves some cleanup and simple refactoring work, including:

- Adding numerous comments to layer the code and explain the behavior of each step.
- Renaming some variables to make them more sensible.
- Simplify certain operations using a more elegant approach.

The goal is to make this intricate implementation clearer and facilitate future maintenance.

In addition to this, the PR also removes redundant `path_transform` operations for `type_gen_args`.
Taking the example of `impl Trait<T1> for S<S1>`, where `S1` is considered. The struct `S` must be in the file where the user triggers code actions, so there's no need for the `path_transform`. Furthermore, before performing the transform, we've already renamed `S1`, ensuring it won't clash with existing generics parameters. Therefore, there's no need to transform it.
@roife roife deleted the rewrite-generate-delete-trait branch February 27, 2024 06:20
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
4 participants