Skip to content

Missing function argument linter #563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
renkun-ken opened this issue Nov 12, 2020 · 13 comments · Fixed by #565
Closed

Missing function argument linter #563

renkun-ken opened this issue Nov 12, 2020 · 13 comments · Fixed by #565

Comments

@renkun-ken
Copy link
Collaborator

renkun-ken commented Nov 12, 2020 •
edited
Loading

Although R allows missing argument in the following forms:

fun(x =)
fun(x=1,,2)
fun(1,)

such examples are almost always not intended in practice. For example, when editing the following list:

x <- list(
  a = 1,
  b = 2,
  c = 3
)

Removing c only will result in the following:

x <- list(
  a = 1,
  b = 2,
)

This is syntactically correct but will produce error at runtime.

A missing function argument linter will be useful to detect all these cases where (,, ,,, ,) or =) appear.

For matrix/array or data frame subsetting like mat[1,], arr[,1,2], df[, "col"], since using [ is not parsed as SYMBOL_FUNCTION_CALL, then the linter would by default not complain such usage.
@renkun-ken renkun-ken mentioned this issue Nov 12, 2020
Merged
3 tasks
@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Nov 12, 2020 via email

@renkun-ken
Copy link
Collaborator Author

Good catch. Looks like we could put some exceptions.

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Nov 12, 2020 via email

@AshesITR
Copy link
Collaborator

Another possible special case might be alist(x = )

@renkun-ken
Copy link
Collaborator Author

@MichaelChirico Yes, I could run it on the R source. I'm quite curious about the results.

@AshesITR Thanks

@renkun-ken
Copy link
Collaborator Author

I use the following code to run on the R source:

library(xml2)

rdir <- "~/Workspaces/github/r-source"
rfiles <- list.files(rdir, "\\.[rR]$", recursive = TRUE)

xpath <- "//expr[expr[SYMBOL_FUNCTION_CALL]]/*[
    self::OP-COMMA[preceding-sibling::*[1][self::OP-LEFT-PAREN or self::OP-COMMA]] or
    self::OP-COMMA[following-sibling::*[1][self::OP-RIGHT-PAREN]] or
    self::EQ_SUB[following-sibling::*[1][self::OP-RIGHT-PAREN or self::OP-COMMA]]
  ]"

res <- lapply(rfiles, function(file) {
  cat(file, "\n")
  tryCatch({
    text <- readLines(file.path(rdir, file))
    expr <- parse(text = text)
    xml_text <- xmlparsedata::xml_parse_data(expr)
    xml <- read_xml(xml_text)

    missing_args <- xml2::xml_find_all(xml, xpath)
    if (length(missing_args)) {
      func <- xml_find_all(missing_args,
        "preceding-sibling::expr/SYMBOL_FUNCTION_CALL")
      line <- unique(as.integer(xml2::xml_attr(missing_args, "line1")))
      list(file = file,
        func = xml_text(func),
        line = line,
        content = text[line]
      )
    }
  }, error = function(e) {
    message(e)
  })
})
res2 <- res[vapply(res, is.list, logical(1L))]
func_names <- unlist(lapply(res2, function(x) x$func))
sort(table(func_names), decreasing = TRUE)
func_names
              switch               matrix                alist             setNames 
                  99                   22                   12                    6 
              system       .MakeSignature             tempfile          .getGeneric 
                   3                    2                    2                    1 
                   c               check2             colMeans                 dput 
                   1                    1                    1                    1 
     globalVariables                    k                 list               Recall 
                   1                    1                    1                    1 
               scale suppressForeignCheck                wrapG 
                   1                    1                    1

Following are non-switch cases:

lines <- unlist(lapply(res2, function(x) {
  sel <- x$func != "switch"
  sprintf("%s:%d:%s: %s",
    x$file, x$line[sel], x$func[sel],
    trimws(x$content[sel])
  )
}))
cat(lines, sep = "\n")
doc/manual/R-exts.R:75:matrix: grad <- matrix(, length(ans), length(theta),
src/library/base/R/duplicated.R:144:alist: args <- rep(alist(a=), ndim)
src/library/methods/R/MethodsList.R:294:Recall: evalArgs = evalArgs, useInherited = nextUseInherited, fdef = fdef,
src/library/methods/R/MethodsListClass.R:187:.MakeSignature: .MakeSignature(.Object, , list(...))
src/library/methods/R/MethodsListClass.R:189:.MakeSignature: .MakeSignature(.Object, , list(functionDef, ...))
src/library/methods/R/RMethodUtils.R:465:.getGeneric: .getGeneric(f,      , package)
src/library/stats/R/cancor.R:31:colMeans: xcenter <- colMeans(x,)
src/library/stats/R/glm.R:72:matrix: X <- if (!is.empty.model(mt)) model.matrix(mt, mf, contrasts) else matrix(,NROW(Y), 0L)
src/library/stats/R/glm.R:706:matrix: coef.table <- matrix(, 0L, 4L)
src/library/stats/R/glm.R:709:matrix: covmat.unscaled <- covmat <- matrix(, 0L, 0L)
src/library/stats/R/selfStart.R:52:alist: args <- setNames(rep(alist(a = ), length(argNams)), argNams)
src/library/stats/R/ts.R:181:matrix: x <- matrix(, n, ns)
src/library/stats/tests/arimaML.R:158:setNames: ss <- setNames(,2:20)
src/library/stats/tests/glm.R:13:setNames: links <- lapply(setNames(,linkNames), make.link)
src/library/stats/tests/glm.R:19:setNames: identical(setNames(,linkNames), vapply(links, `[[`, "", "name"))
src/library/stats/tests/ks-test.R:29:setNames: (alts <- setNames(, eval(formals(stats:::wilcox.test.default)$alternative)))
src/library/stats/tests/ts-tests.R:94:matrix: aics <- matrix(, 6, 6, dimnames=list(p=0:5, q=0:5))
src/library/tools/R/QC.R:1981:suppressForeignCheck: name %in% utils::suppressForeignCheck(, package))
src/library/tools/R/QC.R:4283:globalVariables: .glbs <- suppressMessages(utils::globalVariables(, package))
src/library/tools/R/QC.R:8769:alist: y[ind] <- rep.int(list(alist(irrelevant = )[[1L]]), length(ind))
src/library/tools/R/utils.R:1640:alist: formals(fx) <- alist(x=, y=)
src/library/tools/R/utils.R:2431:alist: "0" =, "no" =, "false" = FALSE,
src/library/utils/R/head.R:67:alist: args <- rep(alist(x, , drop = FALSE), c(1L, length(d), 1L))
src/library/utils/R/head.R:132:alist: args <- rep(alist(x, , drop = FALSE), c(1L, length(d), 1L))
src/library/utils/R/unix/create.post.R:92:system: status <- system(paste("mailx", cmdargs), , TRUE, TRUE)
src/library/utils/R/unix/create.post.R:94:system: status <- system(paste("Mail", cmdargs), , TRUE, TRUE)
src/library/utils/R/unix/create.post.R:96:system: status <- system(paste("/usr/ucb/mail", cmdargs), , TRUE, TRUE)
src/library/utils/R/unix/mac.install.R:56:tempfile: tmpDir <- tempfile(, lib)
src/library/utils/R/windows/install.packages.R:38:tempfile: tmpDir <- tempfile(, lib)
tests/eval-etc-2.R:29:matrix: l3 <- upper.tri(matrix(,3,3))
tests/eval-etc.R:75:k: X <- k(a=)
tests/eval-etc.R:181:alist: missL <- setNames(rep(list(alist(.=)$.), 3), c("",NA,"c"))
tests/eval-fns.R:27:alist: isMissObj <- function(obj) identical(obj, alist(a=)[[1]])
tests/reg-S4.R:250:wrapG: identical(wrapG(mm,,2), Gfun(mm, FALSE)))
tests/reg-tests-1a.R:879:list: try(scan("test.dat", what=list(,,,)))
tests/reg-tests-1a.R:1286:matrix: Y <- matrix(rnorm(20), , 2)
tests/reg-tests-1a.R:2177:matrix: z <- as.ts(matrix(rnorm(100), , 1))
tests/reg-tests-1a.R:2181:matrix: pacf(matrix(rnorm(100), , 1))
tests/reg-tests-1a.R:2601:matrix: a <- matrix(,0,5)
tests/reg-tests-1a.R:4819:alist: g <- alist(a=, b=4, c=)
tests/reg-tests-1b.R:1524:matrix: z <- matrix(rnorm(50),,2); z[6,] <- NA; z <- ts(z)
tests/reg-tests-1b.R:1611:matrix: x <- matrix(,0,0)
tests/reg-tests-1c.R:635:check2: stopifnot(identical(check2(one, , three), c(FALSE, TRUE, FALSE)))
tests/reg-tests-1c.R:991:matrix: mat.l <- list(m0  = matrix(, 0,2),
tests/reg-tests-1c.R:992:matrix: m0n = matrix(, 0,2, dimnames = list(NULL, paste0("c",1:2))),
tests/reg-tests-1c.R:1353:setNames: opts <- setNames(,c("bzip2", "xz", "gzip"))
tests/reg-tests-1c.R:1473:setNames: steps <- setNames(,
tests/reg-tests-1d.R:1011:alist: identical(al, as.pairlist(alist(name = , pos = -1L))))
tests/reg-tests-1d.R:1370:matrix: ec <- e0 <- matrix(, 0, 4) # a  0 x 4  matrix
tests/reg-tests-1d.R:1406:matrix: d0 <- as.data.frame(m0 <- matrix(,2,0))
tests/reg-tests-1d.R:2541:matrix: stopifnot(identical(NA_integer_, max.col(matrix(,1,0))))
tests/reg-tests-1d.R:2588:matrix: isSymmetric(matrix(,0,0, dimnames=list(NULL, NULL)))
tests/reg-tests-1d.R:2589:matrix: isSymmetric(matrix(,0,0))
tests/reg-tests-1d.R:3833:dput: x <- 1 - 2^-51 ; dput(x, , "all")
tests/reg-tests-2.R:219:scale: scale(tm, , FALSE)
tests/reg-tests-2.R:1409:matrix: x <- matrix(, 3, 0)
tests/reg-tests-2.R:2434:c: try(c(1,,2))
tests/reg-tests-2.R:2940:matrix: r <- matrix(,0,4, dimnames=list(Row=NULL, Col=paste0("c",1:4)))
tests/reg-tests-2.R:3023:alist: a <- alist(one = 1, two = )
tests/simple-true.R:168:alist: alist(x = , ... = , UseMethod("as.list")))

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Nov 13, 2020 via email

@AshesITR
Copy link
Collaborator

AshesITR commented Nov 13, 2020 •
edited
Loading

Most matrix() calls with missing arguments have at least one of nrow or ncol set to 0.
I could live with those firing a lint, since the call can be easily converted to matrix(nrow = 0, ncol = 0) or equivalent.

Same with setNames(, x) == setNames(nm = x)

@renkun-ken
Copy link
Collaborator Author

So currently, we could allow exceptions for switch and alist. Do we allow user to customize the exceptions using a function factory or we just hard code those?

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Nov 14, 2020 via email

@AshesITR
Copy link
Collaborator

AshesITR commented Nov 14, 2020 •
edited
Loading

I can imagine a few scenarios that make me lean towards configurability:

  1. Someone implements a method similar to the exclusion defaults, but custom. Imagine a function builder with syntax similar to alist.

  2. {lintr} is used to check code style for non-packages and the user decides to allow some missing argument cases in his individual styleguide, such as setNames(, x) seen in the R source

  3. Someone considers switch fall-through hard to read and explicitly wants to disallow it in his individual styleguide.

I feel this is somewhat similar to the undesirable functions linter and allowing custom exceptions is just a matter of moving the lineof code into the function signature. The default should definitely be "switch", "alist" IMO.

@renkun-ken
Copy link
Collaborator Author

I'm perfectly ok with both.

The problem I could see here is that it would break linter config if a linter initially implemented as non-configurable is then wrapped into a function factory to support some configuration. If this happens, then all .lintr config files will be required to change the something_linter to something_linter() to make lintr work properly.

@renkun-ken
Copy link
Collaborator Author

I guess allowing for customizable exceptions is a slightly better choice in this case. Not much cost any way.

@bersbersbers bersbersbers mentioned this issue May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Missing argument linter renkun-ken/lintr
3 participants
@AshesITR @renkun-ken @MichaelChirico