Extract single expression part of line

[AckslD]

Consider the following python snippet:

def f(a, b):
    test = a + b
    print(test)
    if True:
        y = test
        z = y + b
    return z + y + (test)

If I select the whole first line of the body of the function and call :Refactor extract, I get:

def g(a, b):
    test = a + b
    return test



def f(a, b):
    test = g(a, b)

    print(test)
    if True:
        y = test
        z = y + b
    return z + y + (test)

However if I just select a + b, ie this where | indicates the visual selection:

def f(a, b):
    test = |a + b|
    print(test)
    if True:
        y = test
        z = y + b
    return z + y + (test)

I instead get:

def g(a, b):
    a + b



def f(a, b):
    test = a + b
    print(test)
    if True:
        y = test
        z = y + b
    return z + y + (test)

which is not useful. I would expect this to give:

def g(a, b):
    return a + b



def f(a, b):
    test = g(a, b)
    print(test)
    if True:
        y = test
        z = y + b
    return z + y + (test)

[TheLeoP]

Currently, the return values of a extracted function come from here:

refactoring.nvim/lua/refactoring/refactor/106.lua

Lines 31 to 84 in 93d69cc

	local function get_return_vals(refactor)
	local region_vars = utils.region_intersect(
	refactor.ts:get_local_declarations(refactor.scope),
	refactor.region
	)
	region_vars = vim.tbl_map(
	---@param node TSNode
	---@return TSNode[]
	function(node)
	return refactor.ts:get_local_var_names(node)[1]
	end,
	region_vars
	)
	region_vars = vim.tbl_filter(
	---@param node TSNode
	---@return TSNode[]
	function(node)
	return node
	end,
	region_vars
	)
	local refs = refactor.ts:get_references(refactor.scope)
	refs = utils.after_region(refs, refactor.region)
	refs = vim.tbl_map(
	---@param node TSNode
	---@return TSNode[]
	function(node)
	return utils.node_to_parent_if_needed(refactor, node)
	end,
	refs
	)
	region_vars = vim.tbl_map(
	---@param node TSNode
	---@return TSNode[]
	function(node)
	return utils.node_to_parent_if_needed(refactor, node)
	end,
	region_vars
	)
	local bufnr = refactor.buffers[1]
	local region_var_map = utils.nodes_to_text_set(bufnr, region_vars)
	local ref_map = utils.nodes_to_text_set(bufnr, refs)
	local return_vals =
	vim.tbl_keys(utils.table_key_intersect(region_var_map, ref_map))
	table.sort(return_vals)
	return return_vals
	end

The code search for variables declared in the selected region and for references to variables after the selected region. Then, it does an intersection between both sets. Your example does not work because your selected region a + b does not contain any variables to begin with.

A PR adding this feature would be welcomed.

[AckslD]

Thanks for explaining @TheLeoP! I now understand why it does what it does.

I wonder if it would make sense/could be possible to have a special case for when the visual selection is actually an expression and therefore would make sense to have as a return value? Not sure how easy it is though to know what's an expression in contrast to eg a statement from treesitter.

[TheLeoP]

In treesitter everything works with captures and you can check them using :InspectTree on Neovim nightly.

In the particular case of pyhton, it looks like the following capture would detect the right side of any expression statement (like the one in this example):

(expression_statement
  (assignment
    right: (_) @expression))

And probably something similar can be done with every language with a treesitter parsers. Of course, implementing this feature would be more complicated than simply finding this captures. You would need to check if the node that describes the selected region is an expression and treat it as an special case inside get_returns_vals. If someone is willing to tackle adding this feature in a PR, I'll help with any questions that arise in the process

[AckslD]

I think it would be nice if one could do this for all expressions, so even if it's not only the right-hand side of an assignment, maybe for example in

foo = x + |y + z|

where | denotes the visual selection. In principle one could express all these different cases for python manually with complicated captures but that doesn't feel like the right way. Especially since there then needs to be a library of captures for each language. Maybe that's the only way but would be nicer if there was some more straightforward way to decide if a node is a valid expression.

The alternative could be to just say that whenever the visual selection is charwise, we just make the visual selection the return value of the new function. This might give something invalid depending on what the user selects but maybe that's okay?

Btw, I'm happy to write the implementation once we've agreed on what is should do :)

[AckslD]

Maybe just doing this for charwise selection would make sense? It would then at least be consistent with extract variable.

Or maybe alternatively when there are no variable expressions in the selection?

[TheLeoP]

I think that it would make sense for charwise visual selection to behave like that only if there are no returned values. Right now, if you visually select charwise your example like:

|foo = x + y + z|

and foo is used after that line, the function would correctly return a value and simply changing the behaviour of charwise selection may (would?) broke that.

Also, since you mentioned arbitrary visual selection of expressions, you may come across some problems similar to the one explained in this comment

[AckslD]

I think that it would make sense for charwise visual selection to behave like that only if there are no returned values.

Yeah I think that would make sense 👍 Should that maybe even be independent of the selection mode? Ie if the selected node (however it was selected) has no assignment/returned values, we simply place it as the return value of the new function?

Also, since you mentioned arbitrary visual selection of expressions, you may come across some problems similar to #404 (comment)

Yep I noticed this indeed, but makes sense due to the tree structure of binary operations.

I also noticed another bug with indentation when extracting a variable in Python, will open another issue for that.

Edit:
Opened #448 for the above mentioned bug.

[TheLeoP]

Should that maybe even be independent of the selection mode?

I think that makes sense. If some problem with this approach becomes evident during implementation, we could always change this.