Better handling of nested multi-line function definitions, e.g. R6

[MichaelChirico]

Closes #1136.

Written by Gemini.

One ambiguous case was the existing test case:

styler/tests/testthat/fun_dec/line_break_fun_dec-in.R

Lines 38 to 42 in 290d993

	a <- function(
	x,
	y) {
	x - 1
	}

Here, it's a bit tough to decide whether to style this as hanging or "open". I nudged the rule to select "hanging" here because (1) x and y are in the columns they would be for "hanging" and (2) the ) comes on the same line as y. See new test cases for more similar situations.

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 635ccb1 is merged into main:

• ✔️cache_applying: 25.8ms -> 25.8ms [-1.49%, +1.51%]
• ✔️cache_recording: 350ms -> 349ms [-1.11%, +0.57%]
• ✔️without_cache: 870ms -> 865ms [-1.49%, +0.33%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8cd54b1 is merged into main:

• ✔️cache_applying: 24.5ms -> 24.3ms [-2.2%, +0.58%]
• ✔️cache_recording: 323ms -> 324ms [-0.15%, +0.59%]
• ✔️without_cache: 786ms -> 787ms [-0.19%, +0.46%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[lorenzwalthert]

Thanks @MichaelChirico. I feel like what's confusing now is the notion of standard / single / double indent vs hanging / open indent. In the style guide, there is only ever talked about single and handing indent now it seems. Can you please re-name the rule you modified as well as the helper function and test comment to that effect?

[lorenzwalthert]

Also, if you rebase on main, you should get the pre-commit hooks passing.

[MichaelChirico]

OK, I had a pass at clarifying that, especially in the Rd write-up. pre-commit still stubbornly refuses to pass :)

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0272802 is merged into main:

• ✔️cache_applying: 26.2ms -> 26.1ms [-2.17%, +1.17%]
• ✔️cache_recording: 363ms -> 358ms [-3.93%, +1.17%]
• ✔️without_cache: 881ms -> 882ms [-0.67%, +0.88%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0ec1076 is merged into main:

• ✔️cache_applying: 25.5ms -> 25.5ms [-1.44%, +1.39%]
• ✔️cache_recording: 345ms -> 346ms [-0.4%, +1.11%]
• ✔️without_cache: 842ms -> 842ms [-0.56%, +0.46%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[lorenzwalthert]

Can you have a look at my changes @MichaelChirico ? I don't think we actually needed SYMBOL_SUB.

[MichaelChirico]

that's right, I wasn't reading it carefully enough myself. At a glance, my brain morphed it to be EQ_FORMALS but that doesn't make sense.

FWIW, we miss cases like function("a"=1) unless styler has already normalized those away by the time we get to this step. I wouldn't stress about it barring a user report.

[lorenzwalthert]

Thanks. I guess that won't be a problem.

styler:::parse_text('function("a"=1) NULL')
Error:
✖ Styling failed
Caused by error in `parse()`:
! <text>:1:10: unexpected string constant
1: function("a"
             ^

Show Traceback

[MichaelChirico]

Nice! I assumed it would work since it does in calls:

foo = function("a" = 1) NULL
# Error: unexpected string constant in "foo = function("a""
foo = function(a = 1) NULL
foo("a" = 2)
# NULL

Yet another corner case / inconsistency of the parser to mentally earmark 📝