Request: Refactor option to extract a non-capture closure into a function

[KSXGitHub]

Given the following code:

numbers // Vec<i32>
    .into_iter()
    .filter(|x| *x % 2 == 0)
    .collect::<Vec<_>>()

In VS Code, I want to select the predicate closure (|x| x % 2 == 0), click the yellow light bulb, and choose "Extract closure into function", and the code should turn into this:

numbers // Vec<i32>
    .into_iter()
    .filter(predicate)
    .collect::<Vec<_>>()

fn predicate(x: &i32) -> bool {
    *x % 2 == 0
}

Note: The name predicate was deduced from the signature of the filter method. However, I wouldn't complain if rust-analyzer instead asks me for the name of the function.

Note: Currently, an option called "Extract into function" exists, but it only moves a whole expression to a new function. In regard to closure, the new function it creates is a function that returns closure, not the closure itself.

[DJMcNab]

Obviously this is challenging in the general case, primarily due to capturing. However, doing this if the closure is non-capturing should work fine.

[KSXGitHub]

@DJMcNab For capturing case, can it just add parameters to the created function?

[Veykril]

Then you wouldn't get rid of the closure though as it would have to wrap and call the created function again at which point you can just use the extract function assist. Also note that we do not have capturing analysis yet.

[KSXGitHub]

@Veykril I see.

[flodiebold]

I think for now, I'd just ignore captures, and when we have capture analysis, we should disable the assist if the closure has captures. (Extracting a function with parameters for the captures can be done with the existing "Extract function" assist.)

[Skgland]

My current workaround is to only select the closure body (*x % 2 == 0 for the example) when using extract to function so

numbers // Vec<i32>
    .into_iter()
    .filter(|x| *x % 2 == 0)
    .collect::<Vec<_>>()

gets turned into

numbers // Vec<i32>
    .into_iter()
    .filter(|x| predicate(x))
    .collect::<Vec<_>>()
    
fn predicate(x: &i32) {
    *x % 2 == 0
}

and then to replace the unnecessary |x| function(x) closure with function.

[Skgland]

Note: The name predicate was deduced from the signature of the filter method. However, I wouldn't complain if rust-analyzer instead asks me for the name of the function.

That appears to be issue #17579 and #17587 implemented rename on extract but for variables and the PR author appears interested in adding it also for function extract #17579 (comment).

[Skgland]

Regarding Capture analysis wouldn't it work to check that the set of variables in the arguments of the function created by extract function on the closure body is a sub-set of the variables bound by the closure arguments?
extract function would already determines the captures of the snippet that's being extracted to generate the function arguments and when that set of captures only includes bindings from the closure arguments then the closure should be non-capturing.

[joshka]

Having looked at the extract variable code when implementing #17587, I think the logic for this should probably be just added to the extract function assist.

• When your cursor is in a closure, the extract function assist should navigate up the tree to find the closure as being the part that is extracted / replaced
• If the extracted function only has parameters that match the closure parameters, replace the entire closure with function_name, otherwise, replace only the body of the closure
• leave the cursor on the function name and trigger editor.action.rename via the same method as extract_variable does (extract function needs an update for this part already)

I don't see any particular need to worry about analyzing whether this is capturing or not, the part that is necessary is just knowing whether the params of the closure match exactly those in the function (probably this needs to make sure they are in the same order and match the mutability / types). It's possible that I'm missing something here however - correct me if I'm wrong.

[Skgland]

If I understand that correctly that would fall short when the closure destructures arguments with patterns or when it ignores arguments e.g. with a wildcard pattern.

I will add an example once I am at a PC and not on Mobile

[Skgland]

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(|(a, b)| a > b)
    .collect::<Vec<_>>()

should turn into

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(predicate)
    .collect::<Vec<_>>()

fn predicate((a, b): (i32, i32)) -> bool {
  a > b
}

but extract function would result in a signature mismatch as it would take in the tuple elements seperatly.

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(|(a, b)| predicate(a, b))
    .collect::<Vec<_>>()

fn predicate(a: i32, b: i32) -> bool {
  a > b
}

Similarly

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(|(a, _)| a > 5)
    .collect::<Vec<_>>()

should turn into

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(predicate)
    .collect::<Vec<_>>()

fn predicate((a, _): (i32, i32)) -> bool {
  a > 5
}

but extract function would result in a signature mismatch as it would only take a not (a, _)

pairs // Vec<(i32, i32)>
    .into_iter()
    .filter(|(a, _)| predicate(a))
    .collect::<Vec<_>>()

fn predicate(a: i32) -> bool {
  a > 5
}

The important part is that if the variables bound in the argument patterns matches, we want to re-use the closures argument declaration.

Closue binds CB = {a, b}
|(a, b)| {...}

Extract function on the body binds FB = {a,b}
fn (a: _, b: _) {...}


When FB ⊆ CB generate Function re-use argument patterns from closure

fn ((a,b):_) {...}

[Skgland]

I think having a separate convert closure to function would be better than to integrate this into extract function.

• it would keeps extract function simple
• no ambiguity between extracting something in a closure vs converting the closure

[joshka]

The same thing could be said for non closures though, which suggests that this is a matter of determining the binding and not that its closure specific.

let (a, b) = foo;
a + b

fn((a, b): (u32, u33) { a + b }

[Skgland]

The same thing could be said for non closures though, which suggests that this is a matter of determining the binding and not that its closure specific.

let (a, b) = foo;
a + b

fn((a, b): (u32, u33)) { a + b }

I don't think I would expect extract to function to ever have that result.
If only the bottom row is selected the captures are {a, b} as foo completely is outside the snippet,
if both rows are selected the captures are { foo }.

Resulting in

let (a, b) = foo;
fun_name(a, b)

fn fun_name(a: i32, b: i32) -> i32 {
    a + b
}

and

let (a, b) = foo;
fun_name(foo)
    
    
fn fun_name(foo: (i32, i32)) {
    let (a, b) = foo;
    a + b;
}

respectively. Which already works like this today.

[ChayimFriedman2]

Oh I didn't know about this issue but it is fixed by my PR #17940 (which also handles capturing closures, but not when the closure is passed into a function).