Extract function assist #7535

cpud36 · 2021-02-03T10:59:06Z

This PR adds extract function/method assist. closes #5409.

Supported features

Assist should support extracting from expressions(1, 2 + 2, loop { }) and from a series of statements, e.g.:

foo();
$0bar();
baz();$0
quix();

Assist also supports extracting parameters, like:

fn foo() -> i32 {
  let n = 1;
  $0n + 1$0
}

// -
fn foo() -> i32 {
  let n = 1;
  fun_name(n)
}

fn fun_name(n: i32) -> i32 {
  n + 1
}

Extracting methods also generally works.

Assist allows referencing outer variables, both mutably and immutably, and handles handles access to variables local to extracted function:

fn foo() {
  let mut n = 1;
  let mut m = 2;
  let mut moved_v = Vec::new();
  let mut ref_mut_v = Vec::new();
  $0
  n += 1;
  let k = 1;
  moved_v.push(n);
  let r = &mut m;
  ref_mut_v.push(*r);
  let h = 3;
  $0
  n = ref_mut_v.len() + k;
  n -= h + m;
}

// -
fn foo() {
  let mut n = 1;
  let mut m = 2;
  let mut moved_v = Vec::new();
  let mut ref_mut_v = Vec::new();
 
  let (k, h) =  fun_name(&mut n, moved_v, &mut m, &mut ref_mut_v);

  n = ref_mut_v.len() + k;
  n -= h + m;
}

fn fun_name(n: &mut i32, mut moved_v: Vec<i32>, m: &mut i32, ref_mut_v: &mut Vec<i32>) -> (i32, i32) {
  *n += 1;
  let k = 1;
  moved_v.push(*n);
  let r = m;
  ref_mut_v.push(*r);
  let h = 3;
  (k, h)
}

So we handle both input and output paramters

Showcase

Working with non-`Copy` types

Consider the following example:

fn foo() {
  let v = Vec::new();
  $0
  let n = v.len();
  $0
  let is_empty = v.is_empty();
}

v must be a parameter to extracted function.
The question is, what type should it have.
It could be v: Vec<i32>, or v: &Vec<i32>.
The former is incorrect for Vec<i32>, but the later is silly for i32.

To resolve this we need to know if the type implements Copy trait.

I didn't find any api available from assists to query this.
hir_ty::method_resolution::implements seems relevant, but is isn't publicly re-exported from hir.

Star(`*`) token and pointer dereference

If I understand correctly, in order to create expression like *p, one should use ast::make::expr_prefix(T![*], ...), which
in turn calls token(T![*]).

token does not have star in tokens::SOURCE_FILE, so this panics.
I had to add * to SOURCE_FILE to make it work.

Correct me if this is not intended way to do this.

Lowering access `value -> mut ref -> shared ref`

Consider the following example:

fn foo() {
  let v = Vec::new();
  $0 let n = v.len(); $0
}

v is not used after extracted function body, so both v: &Vec<i32> and v: Vec<i32> would work.
Currently the later would be chosen.

We can however check the body of extracted function and conclude that v: &Vec<i32> is sufficient.
Using v: &Vec<i32>(that is a minimal required access level) might be a better default.
I am unsure.

Cleanup

The assist seems to be reasonably handling most of common cases.
If there are no concerns with code it produces(i.e. with test cases), I will start cleaning up

[edit]
added showcase

there are a few currently limitations: * no modifications of function body * does not handle mutability and references * no method support * may produce incorrect results

currently mut refernce will *not* be downgraded to shared if it is sufficient(see relevant test for example)

before child getter was used

when variable is defined inside extracted body export this variable to original scope via return value(s)

It currently allows only directly setting variable. No `&mut` references or methods.

Recognise &mut as variable modification. This allows extracting functions with `&mut var` with `var` being in outer scope

Veykril · 2021-02-04T01:29:58Z

Regarding whether a type implements copy you should be able to make use of https://github.com/rust-analyzer/rust-analyzer/blob/74a223faa33156be1b8b1a6880f9b63463027946/crates/hir/src/code_model.rs#L1662-L1662

matklad · 2021-02-04T09:05:39Z

I didn't find any api available from assists to query this.
hir_ty::method_resolution::implements seems relevant, but is isn't publicly re-exported from hir.

https://github.com/matklad/rust-analyzer/blob/1008aaae5821ce38975495c76d93b004888f2ed5/crates/hir/src/code_model.rs#L1662-L1670

Using v: &Vec(that is a minimal required access level) might be a better default. I am unsure.

I am not sure. I'd use whichever is simple. Long term, we need a spearate refactor to change function's parameter from T to &T

matklad

Wow, this is one excellent PR @cpud36! I am absolutely blown away by the attention to details here, 👍. One day, once I (or someone else) writes how_to_add_assist.md guide, this would the the showcase. Kudos!

To my mind, this is already in a state well above average for assists, so feel free to merge when you think this is ready.

One day, once the major infrastructure of ra is polished, we should go through all the existing assists and make sure they also meet a high standard of quality, set by this PR. Thanks!

matklad · 2021-02-04T08:45:50Z

crates/syntax/src/ast/make.rs

@@ -487,7 +487,7 @@ pub mod tokens {
    use crate::{ast, AstNode, Parse, SourceFile, SyntaxKind::*, SyntaxToken};

    pub(super) static SOURCE_FILE: Lazy<Parse<SourceFile>> =
-        Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true)\n;\n\n"));


matklad · 2021-02-04T08:46:06Z