vscode-R

vscode-R copied to clipboard

Describe the bug

Lines ending in commas seem to cause R: Run Selection/Line to send the wrong lines to the console.

To Reproduce

1. Create a file temp.R with the following content:

list(
    1,
    2
)

2. Place cursor on second or third line
3. Use command R: Run Selection/Line

Expected: All four lines are sent to console (if cursor was on second line), or only '2' is sent to console (if cursor was on third line)

Actual: Middle two lines are sent to console:

> 1,
Error: unexpected ',' in "1,"
>     2
[1] 2

If cursor is on third line, sending all four lines to the console would be fine too.

The problem is that the algorithm treats commas the same as operators like '+', so it views lines 2 and 3 as a complete expression. But a comma can only exist inside parentheses or brackets. The correct behaviour would be: If a comma is encountered, the algorithm keeps consuming lines until it finds surrounding parentheses/brackets.

This, or a similar issue, seems to be related to the radian console. For me, long hard-wrapped lines terminated by a comma worked fine when sent to the native R terminal or in radian after adding options(radian.auto_match = FALSE).

Commas doesn't seem to matter in this problem. In the following code with no comma, if I place the cursor on the second line and execute the Run Line command, only the second line will be sent to the terminal.

print(
  1
)

The line ending in ( may be causing the problem.

The following code will send all three lines to the terminal regardless of which line the cursor is on.

print(1+
1
)

On the other hand, if the cursor is on the second line in the following code, only 1 will be sent.

print(
1
+1)

A line without an operator at the end of the line may be the problem. If I place the cursor on the second line of the following code and execute it, 1 will be sent to the terminal.

print(foobar
1
)

print(1
1
)

print(1+1
1
)

print(%
1
)

If I put an operator at the end of the first line, all three lines will be sent to the terminal.

print(+
1
)

print(-
1
)

print(=
1
)

print(>
1
)

print(%%
1
)

The general concept was to send the 'minimum bit of runnable code' to the console. That's why 1 is sent to the console in this example with the cursor on the second line:

print(
  1
)

The main exception to that concept is that it also looks for operators at the end of the previous line, as you've found. I think I chose that behaviour mainly to handle chains of lines ending in %>%, and possibly for compatibility with RStudio.

So those cases are intended behaviour. The comma examples aren't intended behaviour, because the comma code isn't runnable without the surrounding parentheses/brackets and so what gets sent to the console produces an error.

The intended behaviour could be changed, of course - I'm just noting what the intended behaviour was when this was implemented.

Thank you for the explanation. I'm convinced that this is the intended behavior except for commas.

It is true that at that time it was hard to imagine a situation without a comma in parentheses except with the magritter's pipe operator %>%, but now it is more likely to occur since the native pipe operator |> was introduced in R 4.1 this year. For example, the following code will be interpreted as quote(sort(mtcars$mpg, decreasing = TRUE)) by R.

mtcars$mpg |>
  sort(
    decreasing = TRUE
  ) |>
  quote()

However, if I place the cursor on the third line and execute it, only the third line will be sent to the terminal.

So it may be worthwhile to consider changing the intended behavior.

Something to consider if changing the existing behaviour is how to handle nesting. Consider the following case:

a(
    b(
        1
    )
)

If the cursor is on line 3, should it send b(1) or a(b(1))? How about if the cursor is on line 2?

I don't use R much these days and haven't made any changes to the vscode-R code base in some time, so it's unlikely I'll make a PR relating to this any time soon. But anyone else is of course welcome to take it on if they'd like. If changing intended behaviour, I recommend getting agreement from one or two other active contributors before starting on the code modifications.

I think the best way to see the current intended behaviour for selecting code to be sent to the terminal is the unit tests in this file: https://github.com/REditorSupport/vscode-R/blob/master/src/test/suite/extension.test.ts

I don't quite understand why this is the expected behavior. Everything inside a parenthesis, are there because their working together makes it work... in addition to the parenthesis itself. It's a similar issue to whether or not line execution should involve pipe. It doesn't make sense to execute without pipe, and it doesn't make sense to execute an individual line inside a parenthesis without the function that calls it.

In the example of:

a(
    b(
        1
    )
)

Yes, all 5 lines should be executed.

Hi @brianmsm, thank you for the feedback. It does seem like a few people have found the existing behaviour surprising and undesirable. It could be changed but it would need a volunteer to make the code changes.

If anyone would like to volunteer, I'd recommend getting agreement from one or two active contributors before starting on the code modifications.

I have a question, what part of the code is handling this behavior?

@brianmsm Here's the function that handles it: https://github.com/REditorSupport/vscode-R/blob/da579cc17a9485f7a11bf5e75bf6b9c347eca116/src/selection.ts#L161

Changing the behaviour could perhaps be achieved by adding ( to this line:

https://github.com/REditorSupport/vscode-R/blob/da579cc17a9485f7a11bf5e75bf6b9c347eca116/src/lineCache.ts#L73

Then the algorithm would check for a ( at the end of the previous line in the same way as it currently checks for %>% etc.

Any update on this? This is the only thing I am missing from RStudio.

Would anyone like to volunteer to try making the one-line change I suggested above? I think that would probably fix the comma issue and also change the behaviour so that parenthesised expressions split over several lines are sent to the console in full. If that works, a few unit tests would also need to be updated and then a PR could be made.

If no volunteers I will probably make the change myself at some point but hard to say when.

Thank you for making this happen :)