Skip switch() and other calls by default in unnecessary_nesting_linter()

[MichaelChirico]

Originally posted by @AshesITR in #2302 (comment)

Test case for switch():

test_that("unnecessary_nesting_linter skips one-expression switch statements", {
  expect_lint(
    trim_some("
      switch(x,
        a = {
          do_a()
        },
        b = {
          do_b()
        }
      )
    "),
    NULL,
    unnecessary_nesting_linter()
  )
})

Hint at implementation:

or self::expr/SYMBOL_FUNCTION_CALL[text() = 'switch']

See also #2326

[MichaelChirico]

It looks like we want the following calls to be allowed by default:

switch()

test_that()
with_parameters_test_that()

reactive()
observe()
observeEvent()
render*()

I'm not really seeing a common through-line for why these & not other calls should be excepted. Any thoughts? If so that would help naming the parameter. Otherwise we can just call it allow_calls.

[AshesITR]

Also

try()
tryCatch()
withCallingHandlers()

quote()
expression()
bquote()

I'd say those are functions that take an expression or expression block and compute on it in some way.

[MichaelChirico]

allow_code_callers?

switch() still feels like a bit of an outlier.

[AshesITR]

quote() doesn't really fit the "callers".
allow_functions = ...?

Also, do we want unoverridable defaults like in return_linter() or do we want to use a long list of default exceptions like with default-undesirable_functions?

[MichaelChirico]

Also, do we want unoverridable defaults like in return_linter() or do we want to use a long list of default exceptions like with default-undesirable_functions?

Is test_that() the only one where it's actually incorrect to omit { even in the one-expression case?

I would not allow override for cases where it's incorrect. The other cases are IMO stylistic and can be put as overrideable defaults.

[AshesITR]

I don't know of any other function that checks and complains. Functions that don't should always work without braces, but then again, test_that() works without braces in some way, too.

[MichaelChirico]

I'm not very familiar with {shiny}, what about these, are they akin to reactive()?

export(reactiveConsole)
export(reactiveFileReader)
export(reactivePoll)
export(reactiveTimer)
export(reactiveVal)
export(reactiveValues)
export(reactiveValuesToList)

Maybe we need allow_function_regex= too, for reactive.* and render.*?

[AshesITR]

I'm not very familiar with {shiny}, what about these, are they akin to reactive()?

export(reactiveConsole)
export(reactiveFileReader)
export(reactivePoll)
export(reactiveTimer)
export(reactiveVal)
export(reactiveValues)
export(reactiveValuesToList)

Maybe we need allow_function_regex= too, for reactive.* and render.*?

I don't know all of them, but no.
Common usages:

x <- reactiveVal(42)
y <- reactiveValues()

observe({
  y$a <- 1
  y$b <- 2
})

For shiny, the following usually get {} exprs:

a <- reactive({})
b <- observe({})
c <- observeEvent(expr_no_braces, {})
output$a <- renderXXX({})

[MichaelChirico]

Is that all of these render* functions then?

export(renderCachedPlot)
export(renderDataTable)
export(renderImage)
export(renderPlot)
export(renderPrint)
export(renderTable)
export(renderText)
export(renderUI)