Too many absolute_paths_linters false positives

[fangly]

Hi Jim,

This issue is related to issue #58.

I get quite a few false positives from the absolute path linter. The strings that generated the lints look like: "/aS: 3\n/bS: Inf\n/cS: -2.2\n/dS: x" and to me, the offending character is "\n" which is highly unlikely to be included in a file path. I envison that the linter should:

1. Consider the string as a whole and estimate whether it is a valid path or not (i.e. if it contains forbidden characters).
2. Determine if the path is absolute.

The problem then boils down to which characters should be forbidden in Linux and Windows paths. These webpages (http://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names, https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words) make it clear that it is list them and on Linux, almost all characters are allowed. But a pragmatic approach may be to blacklist:

• on Windows: * ? " < > |
• on both Linux and Windows: all standard ASCII control codes (https://en.wikipedia.org/wiki/C0_and_C1_control_codes#C0_.28ASCII_and_derivatives.29), i.e. \0 \a \n etc

Cheers,

Florent

[jimhester]

Yes too many false positives is why I removed the absolute paths linter from the defaults 97b23de.

[fangly]

I had a go at implementing what I suggested above:

windowsForbiddenChars <- c("*", "?", "\"", "<", ">", "|")
controlChars <- paste0("\\", intToUtf8(seq.int(0L, 31L), multiple=TRUE))

.isValidPath <- function(str) {
  # Path is invalid when matching a control codes (Win & Linux) or one of (Win only): * ? " < > |
  # This is a pragmatic approach and very weird but valid file paths may be reported as invalid.
  badChars <- controlChars
  if (re_matches(str, rex(or( group(letter, ":", some_of("/", "\\\\")),
                              group("\\\\", "\\\\"))))) {
    badChars <- c(windowsForbiddenChars, badChars)
  }
  !re_matches(str, rex(one_of(badChars)))
}

I then just added an if (.isValidPath(parsed$text)) { statement to R/absolute_paths_linter.R, just before for (regex in regexes) {.

All these additional test cases pass, and the false positives in my own package went away.

  linter <- absolute_paths_linter
  msg <- rex::escape("Do not use absolute paths.")

  expect_lint("'asdf'", NULL, linter)
  expect_lint("'asdf*'", NULL, linter)
  expect_lint("'asdf\n'", NULL, linter)

  expect_lint("'/asdf'", msg, linter)
  expect_lint("'/asdf*'", msg, linter)
  expect_lint("'/asdf\n'", NULL, linter)

  expect_lint("'C:/asdf'", msg, linter)
  expect_lint("'C:/asdf/*'", NULL, linter)
  expect_lint("'C:/asdf/\n'", NULL, linter)

Please consider including this code. We cannot do a perfect job of detecting all valid paths, but this should get rid of many false positives.

[tyner]

I'm curious to know why absolute paths seem to be frowned upon. R itself occasionally uses them, for example utils::sessionInfo hard-codes "/etc/os-release" and "/etc/system-release", base::Sys.timezone hard-codes "/etc/localtime", etc

[fangly]

I think it is because they are not portable, tyner. Though as you noted, there are unusual cases where you need absolute paths.

[fangly]

Fixed in #214. Use absolute_paths_linter(lax=TRUE).