Skip to content

Fix extract function invalid self param - #20864

Merged
ChayimFriedman2 merged 4 commits into
rust-lang:masterfrom
A4-Tacks:extract-method-in-trait
Mar 31, 2026
Merged

ChayimFriedman2 merged 4 commits into
rust-lang:masterfrom
A4-Tacks:extract-method-in-trait

Conversation

@A4-Tacks

@A4-Tacks A4-Tacks commented Oct 19, 2025 •

Copy link
Copy Markdown
Member

Example

trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        $0self.f()+self.f()$0
    }
}

Before this PR

trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        fun_name(self)
    }
}

fn $0fun_name(&self) -> i32 {
    self.f()+self.f()
}

After this PR

trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        fun_name(self)
    }
}

fn $0fun_name(this: &impl Foo) -> i32 {
    this.f()+this.f()
}

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2025
@flodiebold

Copy link
Copy Markdown
Member

I'm not sure adding the method to the trait is usually going to be what the user wants?

@A4-Tacks

A4-Tacks commented Oct 19, 2025 •

Copy link
Copy Markdown
Member Author

I'm not sure adding the method to the trait is usually going to be what the user wants?

But it may be appropriate to do so instead of creating a generic function with all trait bounds and trait generic parameters

@Veykril

Veykril commented Oct 26, 2025

Copy link
Copy Markdown
Member

I think in this case, when the selection contains self locals, we might want to offer both extract to function and extract to trait method? I do agree we can't assume that the user wants a trait function in this case

@A4-Tacks
A4-Tacks marked this pull request as draft December 1, 2025 08:25
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2025
@A4-Tacks
A4-Tacks force-pushed the extract-method-in-trait branch from 6fb908c to 2fa44fd Compare March 2, 2026 02:56
@A4-Tacks A4-Tacks changed the title Fix extract method in trait outside the trait Fix extract function invalid self param Mar 2, 2026
Example
---
```rust
trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        $0self.f()+self.f()$0
    }
}
```

**Before this PR**

```rust
trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        fun_name(self)
    }
}

fn $0fun_name(&self) -> i32 {
    self.f()+self.f()
}
```

**After this PR**

```rust
trait Foo {
    fn f(&self) -> i32;

    fn foo(&self) -> i32 {
        fun_name(self)
    }
}

fn $0fun_name(this: &impl Foo) -> i32 {
    this.f()+this.f()
}
```
@A4-Tacks
A4-Tacks force-pushed the extract-method-in-trait branch from 2fa44fd to 1f95d93 Compare March 2, 2026 02:58
@A4-Tacks
A4-Tacks marked this pull request as ready for review March 2, 2026 03:04
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2026
@A4-Tacks

A4-Tacks commented Mar 2, 2026

Copy link
Copy Markdown
Member Author

Okay, I have implemented the conversion of a simple case from self to this

};

// FIXME: make trait arguments
let trait_name = ast::Trait::cast(insert_after.clone())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That seems brittle. There is no inherent correspondence between the insert_after node and the trait. Rather, pass the trait directly (or its name).

let name = fun.name.clone();
let mut call_expr = if fun.self_param.is_some() {
let args = fun.params.iter().map(|param| param.to_arg(ctx, fun.mods.edition));
let mut call_expr = if fun.make_this_param().is_some() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make_this_param() actually creates the param, that's redundant. Instead have has_this_param() or something.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A single function facilitates the maintenance of unified judgment logic, and I return a closure to solve redundant make

@ChayimFriedman2
ChayimFriedman2 added this pull request to the merge queue Mar 31, 2026
Merged via the queue into rust-lang:master with commit 5eb6f50 Mar 31, 2026
17 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2026
@A4-Tacks
A4-Tacks deleted the extract-method-in-trait branch March 31, 2026 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants