function_argument_linter() could look for missing() handling and mention it in the lint message

[MichaelChirico]

I think the simplest fix for existing violations of function_argument_linter() is to set a default of NULL for functions violating the design principle.

This doesn't always align the functions to the intent of the design guide, but it is safer for functions which may cause breaking changes when changing the argument order, and from what I've seen it's usually a good choice.

One case I can think of where it's not safe to blithely make this switch is when such arguments are handled with missing(). Consider:

foo <- function(x, y = 2, z) {
  if (missing(z)) z <- 3
  x * y * z
}

And existing usage foo(1, 2), which will break if changing to z = NULL by default -- there was an implicit default value of z set inside the function. We can of course accommodate this by changing missing(z) to is.null(z) or even making the default z = 3 explicit, but it is more complicated, at least.

OTOH, it should be straightforward for the linter logic to look, when identifying parameter zed as violating function_argument_linter(), for usages of missing(zed) in the applicable function body, and if found, mention this caveat in the linter message.

PS I'm also curious if anyone can think of other edge cases where setting this default can break existing code.

[IndrajeetPatil]

Note to ourselves:

Once this is implemented, also update the docs for function_argument_linter() to mention this caveat and to add an example of a function that uses missing().

[MichaelChirico]

I'm realizing my example here is flawed -- foo(1, 2) will continue to give 6 after changing the default:

foo <- function(x, y = 2, z = NULL) {
  if (missing(z)) z <- 3
  x * y * z
}
foo(1, 2)

Maybe I'm mis-remembering, but I recall breaking some test suites by neglecting to update a missing() usage when changing to NULL by default in some cases, but the example here doesn't capture that.

Or maybe it's just that mixing z = NULL and missing(z) like here is an odd design choice & it's usually preferable to switch missing() to is.null() in the process of refactoring -- so we include the note in the lint message not necessarily as a guard for back-compatibility, but to facilitate better refactoring.