Skip to content

Created an assist for inlining a function's body into its caller #7131

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

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan commented Jan 2, 2021

This introduces an inline_function assist which will convert code like this:

fn add(a: u32, b: u32) -> u32 { a + b }
fn main() {
    let x = add<|>(1, 2);
}

Into something like this:

fn add(a: u32, b: u32) -> u32 { a + b }
fn main() {
    let x = { 
        let a = 1; 
        let b = 2; 
        a + b 
    };
}

Fixes #6863.

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jan 2, 2021

The tests currently fail because the replaced code isn't formatted correctly. Is it possible to ask rust-analyzer to automatically format the replacement code, or should I just update the doc-test to have each bound parameter on its own line?

  fn add(a: u32, b: u32) -> u32 { a + b }
  fn main() {
-     let x = { let a = 1; let b = 2; a + b };
+     let x = { 
+ let a = 1;
+ let b = 2;
+ a + b
+ };
  }

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jan 2, 2021

Some further improvements:

  • It might make sense to remove the extra set of curly braces and add the parameters bindings (let a = ... and let b = ...) to the line above
    • I'm not sure how we'd prevent accidentally shadowing variables in the callee's scope
  • if the function body only uses a parameter once, or the parameter is Copy and has no side-effects, we could do a simple find/replace to swap each parameter with its value instead of introducing new variables
    • This breaks down when your function parameters are patterns (e.g. fn foo(Foo { a, b }: Foo)) instead of simple identifiers
    • This would trigger borrowing errors if you were accepting &mut T parameters
    • How would this behave when the parameter gets shadowed? (e.g. fn foo(data: &[u8]) { let data = &data[1..]; })
  • Should this be extended to method calls?

@Michael-F-Bryan
Copy link
Contributor Author

The trivial example from the original issue is now expanded as desired. Is someone able to review the PR and let me know their thoughts?

In particular, I'd like to know whether I should implement the extensions/tweaks from my previous comment. @Nadrieril, do you think it'd be worth trying to "inline" the block we replace the original function call with?

At the moment it'll generate something like this:

fn add(a: u32, b: u32) -> u32 { a + b }

fn main() {
    let x = {
        let a = 1;
        let b = 2;
        a + b
  };
}

Which can be simplified down to let x = 1 + 2 by copy/pasting the arguments into the corresponding variables. However, that's not possible in the general case.

@Nadrieril
Copy link
Member

Yay, thanks for implementing this!

* It might make sense to remove the extra set of curly braces and add the parameters bindings (`let a = ...` and `let b = ...`) to the line above

I would find that useful but not a deal-breaker if you don't want to risk shadowing.

* if the function body only uses a parameter once, or the parameter is `Copy` and has no side-effects, we could do a simple find/replace to swap each parameter with its value instead of introducing new variables

That's easy enough to do by hand with the "inline variable" code action, so I'd rather do it myself if I want.

* Should this be extended to method calls?

Definitely! Also trait methods if possible.

@Michael-F-Bryan
Copy link
Contributor Author

@Nadrieril, can you give me an example of what inlining a method call or trait method call would look like? And what happens if the call being inlined is part of a method chain (e.g. the bar in self.foo().bar().baz())?

@Nadrieril
Copy link
Member

I dunno, the only difference with functions is that self is special. I expect something like:

struct Bar(Baz);
impl Bar {
    fn bar(&self, n: usize) -> &Baz {
        self.0.whatever(n)
    }
}
fn main() {
    let x = Foo;
    x.foo().bar<|>(0).baz()
}

===>

// Same `Bar`...
fn main() {
    let x = Foo;
    {
        let _self = &x.foo();
        let n = 0;
        (_self.0.whatever(n)).baz()
    }
}

I don't know how to name the dummy self variable. Could be this, could be r#self...

@matklad
Copy link
Member

matklad commented Jan 3, 2021

Is it possible to ask rust-analyzer to automatically format the replacement code,

not really, see #4182. There's Indent to do some formatting manually.

For tweaks, I suggest starting reasonably simple -- the full impl would be pretty complex. If you want to do "only one usage" trick, see the "remove unused parameter" assist for the search infra.

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Overall, lgtm!

@matklad
Copy link
Member

matklad commented Jan 3, 2021

huh, not sure why ci fails...

@matklad
Copy link
Member

matklad commented Jan 3, 2021

bors try

bors bot added a commit that referenced this pull request Jan 3, 2021
@bors
Copy link
Contributor

bors bot commented Jan 3, 2021

try

Build failed:

@Michael-F-Bryan
Copy link
Contributor Author

Thanks for the review, @matklad!

Would you like me to squash everything into a single commit before merging?

I'm not sure why the Rust Cross job is failing. It seems to think that calling <hir::code_model::Function as hir::has_source::HasSource>::source() returns an Option<InFile<Fn>> when actually the trait definition guarantees we'll return InFile<Fn>... Is there something in the powerpc-unknown-linux-gnu which adds methods or changes trait definitions?

@lnicola
Copy link
Member

lnicola commented Jan 4, 2021

Would you like me to squash everything into a single commit before merging?

We don't ask contributors to squash, but we don't squash on merge either. In this case, your changes are pretty well-contained, so it would make sense to have a single commit. You might also want to do that because...

I'm not sure why the Rust Cross job is failing.

I'm not sure either, but see #7115. You should probably rebase over the current master because of that (source() now returns an Option<_>).

@Michael-F-Bryan
Copy link
Contributor Author

@lnicola thanks, I've rebased and CI is happy now.

statements.push(make::let_stmt(pattern, Some(value)).into());
}

statements.extend(body.statements());
Copy link
Member

Choose a reason for hiding this comment

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

this is ok for the initial impl, but it'll erase formatting, the prepend_stmt solution would better preserve original code.

@matklad
Copy link
Member

matklad commented Jan 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 5, 2021

@bors bors bot merged commit 5c10f2f into rust-lang:master Jan 5, 2021
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.

Feature request: inline a function
4 participants