Refactoring: Extract variable/function, Inline variable

[andycraig]

Robust refactoring is on the roadmap as a long term/difficult task. Is it time to introduce extract variable and extract function as refactoring options? I think the new XML parsing approach could help here, for example in identifying the appropriate location to insert the variable definition when extracting a variable.

VSCode refactoring overview
VSCode-specific, but gives some indication of how the refactoring Code Actions are expected to be used.

TypeScript language server extract symbol code
They do a lot of checks - I don’t think this level of thoroughness is needed initially.

[randy3k]

It is handled by the codeAction provider. We could also use this code from RStudio.

[andycraig]

@randy3k That looks very relevant. The codetools package also has some functions that could be useful (findGlobals, findLocals etc.), if it's okay to add that as a dependency.

I'm happy to work on this, although also very happy to leave it to someone else if anyone is keen.

[randy3k]

I don't think I would have the time and energy to look into it. I'd pass it for this time.

[andycraig]

@randy3k No worries :) I’ll have a go at it.

[andycraig]

I have been thinking about how to handle scopes when extracting variables. I had a look at how RStudio does it but I would like to use a slightly different approach to ensure that

1. The scopes of the original statements do not change, and
2. The extracted variable is placed in the same scope as the line it was extracted from.

I will proceed with the proposed versions below unless anyone has objections.

Example 1: Control structures without {}

Original code:

x <- 1
if(FALSE)
  print(paste0(x + 1))

Select 'x + 1', extract as 'y'.

RStudio result:

x <- 1
if(FALSE)
  y <- x + 1
  print(paste0(y))

Proposed result:

x <- 1
if(FALSE) {
  y <- x + 1
  print(paste0(y))
}

Example 2: One-line {}

Original code:

if(FALSE) {print(1)}

Select '1', extract as 'y'.

RStudio result:

y <- 1
if(FALSE) {print(y)}

Proposed result:

if(FALSE) {y <- 1
  print(y)}

[randy3k]

I agree ☝️

[andycraig]

Identifying scopes of control structures in R is a little bit more complicated than I had assumed it would be. It seems that the R syntax tree does not consider the body of an if (for example) to be a child of the if. In this example, the body (id 8) and the if (id 1) both have parent 11:

> expr <- parse(text = "if(TRUE) 1", keep.source = TRUE)
> getParseData(expr)
   line1 col1 line2 col2 id parent     token terminal text
11     1    1     1   10 11      0      expr    FALSE     
1      1    1     1    2  1     11        IF     TRUE   if
2      1    3     1    3  2     11       '('     TRUE    (
3      1    4     1    7  3      4 NUM_CONST     TRUE TRUE
4      1    4     1    7  4     11      expr    FALSE     
5      1    8     1    8  5     11       ')'     TRUE    )
7      1   10     1   10  7      8 NUM_CONST     TRUE    1
8      1   10     1   10  8     11      expr    FALSE 

So, checking whether the selected text for 'extract variable' is in an if etc. will probably involve using xml2::xml_siblings rather than just xml2::xml_parents.

A few test cases to include:

# Case 1 (select '1', in body of if on same line)
if(TRUE) 1

# Case 2 (select '1', in body of if on different line)
if(TRUE)
    1

# Case 3 (select '1', in body of if on same line, within another expression)
x <- if(TRUE) 1

# Case 4 (select '1', within expression in body of if on same line)
x <- if(TRUE) 1 + 2

[andycraig]

I'd still like to get this working at some point but it's taking a long time to get the top of my priority list. If there's anyone out there who wants this feature more urgently in the meantime they should feel free to take over.

[krlmlr]

We already have "Rename variable", is this provided explicitly by this extension, or is this some implicit magic?

[renkun-ken]

@krlmlr #339 implements the rename provider.

[krlmlr]

Thanks! A very simplistic "Extract variable" would solve ~95% of my use cases, even if context is not taken into account. (It's mostly about replacing "Ctrl + X" + type + move + type + "Ctrl + V" by a shortcut.) If the extracted code is selected after the operation, it's trivial to move it elsewhere.

This is a bit above me right now, though.

[albert-ying]

Is there any update on this? It would be much easier to do functional programming if there is an easy way to extract code trunk as a function.

[andycraig]

Hi @albert-ying, no update. I haven't been using R a lot recently so I am unlikely to get to this any time soon.