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: resolve Self type references in delegate method assist #15705

Merged
merged 3 commits into from Dec 8, 2023

Conversation

rmehri01
Copy link
Contributor

@rmehri01 rmehri01 commented Oct 3, 2023

This PR makes the delegate method assist resolve any Self type references in the parameters or return type. It also works across macros such as the uint_impl! macro used for saturating_mul in the issue example.

Closes #14485

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2023
@Veykril
Copy link
Member

Veykril commented Oct 4, 2023

Looking at the assist, I'm surprised we aren't using the PathTransform yet to fix up paths. We definitely should do that here instead (and then add support for substituting Self appropriately). Not sure how simple that is though.

@rmehri01
Copy link
Contributor Author

rmehri01 commented Oct 4, 2023

Yeah that sounds like a good idea, I could give it a try! PathTransform expects a SemanticsScope for the target scope though, so is there a way to get that with a macro in the way? Like in this example, since ctx.sema.scope won't work:

macro_rules! test_method {
    () => {
        fn test(self) -> Self {
            self
        }
    };
}

struct Bar;

impl Bar {
    test_method!();
}

struct Foo {
    bar: $0Bar,
}

@Veykril
Copy link
Member

Veykril commented Oct 5, 2023

Why wouldn't it work there? We can take a scope from a macro expansion's node just fine

@rmehri01 rmehri01 force-pushed the 14485_fix_delegate_self_references branch from c6487b5 to 5b2cd56 Compare October 5, 2023 18:48
@rmehri01
Copy link
Contributor Author

rmehri01 commented Oct 5, 2023

Ah I was running into an issue where it said the lookup failed in this instance of Semantics but I added sema.parse_or_expand, which seems to update the cache and gets it to work!

I added the change to PathTransform to change Self now but I'm not sure if its the right way to do it, seems to at least pass the tests I had before.

@@ -106,7 +107,10 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<'
|edit| {
// Create the function
let method_source = match method.source(ctx.db()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let method_source = match method.source(ctx.db()) {
let method_source = match ctx.sema.source(method) {

We can do this instead of the extra parse_or_expand, this will do the caching for us then

Comment on lines 190 to 194
PathTransform::generic_transformation(
&ctx.sema.scope(strukt.syntax()).unwrap(),
&ctx.sema.scope(method_source.syntax()).unwrap(),
)
.apply(f.syntax());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PathTransform::generic_transformation(
&ctx.sema.scope(strukt.syntax()).unwrap(),
&ctx.sema.scope(method_source.syntax()).unwrap(),
)
.apply(f.syntax());
if let Some((target, source)) =
ctx.sema.scope(strukt.syntax()).zip(ctx.sema.scope(method_source.syntax()))
{
PathTransform::generic_transformation(&target, &source).apply(f.syntax());
}

The unwraps shouldn't really be possible to hit here, but we are trying to not use unwraps in assist code if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, sorry about that!

}
}

ted::replace(path.syntax(), ast_ty.syntax());
Copy link
Member

Choose a reason for hiding this comment

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

We probably wanna prefer keeping Self if it does not need to be replaced. So a check if the source and target scopes have the same Self (and in that case, skip this transformation here entirely). We can give Ctx a new bool field for that and set it accordingly in build_ctx by giving SemanticsScope a function like fn has_same_self_type(&self, other: &SemanticsScope<'_>) -> bool which just checks if the top ImplDefScopes of each scope's resolver has the same def in them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah good catch, thanks :)

@Veykril
Copy link
Member

Veykril commented Dec 8, 2023

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

📌 Commit d8f0a9d has been approved by Veykril

It is now in the queue for this repository.

bors added a commit that referenced this pull request Dec 8, 2023
…=Veykril

fix: resolve Self type references in delegate method assist

This PR makes the delegate method assist resolve any `Self` type references in the parameters or return type. It also works across macros such as the `uint_impl!` macro used for `saturating_mul` in the issue example.

Closes #14485
@bors
Copy link
Collaborator

bors commented Dec 8, 2023

⌛ Testing commit d8f0a9d with merge fbe39bb...

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

💔 Test failed - checks-actions

@Veykril Veykril force-pushed the 14485_fix_delegate_self_references branch from d8f0a9d to 7e768cb Compare December 8, 2023 11:30
@Veykril
Copy link
Member

Veykril commented Dec 8, 2023

rebased
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

📌 Commit 7e768cb has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

⌛ Testing commit 7e768cb with merge 6bbb2ac...

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

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

@bors bors merged commit 6bbb2ac into rust-lang:master Dec 8, 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.

Delegate method assist doesn't fix up Self references
4 participants