Skip to content
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.

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

first pass at implementing a backport_linter #622

Merged
merged 17 commits into from
Dec 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Collate:
'addins.R'
'assignment_linter.R'
'assignment_spaces_linter.R'
'backport_linter.R'
'cache.R'
'closed_curly_linter.R'
'commas_linter.R'
Expand Down
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* `spaces_inside_linter` ignores spaces preceding trailing comments (#636, @michaelchirico)
* `T_and_F_symbol_linter` is now part of the default linters (#517, #612, @AshesITR)
* `with_defaults()` no longer duplicates the `lintr_function` class when it is already present (#511, #612, @AshesITR)
* New `backport_linter()` for detecting mismatched R version dependencies (#506, @MichaelChirico)
* `paren_brace_linter` and `no_tab_linter` also use more reliable matching (e.g.,
excluding matches found in comments; #441 and #545, @russHyde)
* Fixed `spaces_left_parentheses_linter` sporadically causing warnings (#654, #674, @AshesITR)
Expand Down
86 changes: 86 additions & 0 deletions R/backport_linter.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#' @describeIn linters that checks for usage of unavailable functions. Not reliable for testing r-devel dependencies.
#' @export
backport_linter <- function(r_version = getRversion()) {
function(source_file) {
if (inherits(r_version, "numeric_version")) r_version <- format(r_version)
if (r_version < "3.0.0") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use numeric_version comparison here, else it will bite us when R 10.0.0 rolls out ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha that can be a problem for our future selves -- after 2050 at the current rate 😸

I prefer this way because the test below makes the same assumption by doing r_version < names(backports)

So really this assumption is backed into names(backports) as well. Maybe by R 10.0 R will allow objects to have objects as names like python

warning("It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.")
r_version <- "3.0.0"
}
if (is.null(source_file$xml_parsed_content)) return(list())

xml <- source_file$xml_parsed_content

names_xpath <- "//*[self::SYMBOL or self::SYMBOL_FUNCTION_CALL]"
all_names_nodes <- xml2::xml_find_all(xml, names_xpath)
all_names <- xml2::xml_text(all_names_nodes)

# guaranteed to include 1 by early return above; which.min fails if all TRUE (handled by nomatch)
needs_backport_names <- backports[1:(match(FALSE, r_version < names(backports), nomatch = length(backports)) - 1L)]

# not sapply/vapply, which may over-simplify to vector -- cbind makes sure we have a matrix so rowSums works
needs_backport <- do.call(cbind, lapply(needs_backport_names, function(nm) all_names %in% nm))
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
bad_idx <- rowSums(needs_backport) > 0L

lapply(which(bad_idx), function(ii) {
node <- all_names_nodes[[ii]]
line1 <- xml2::xml_attr(node, "line1")
col1 <- as.integer(xml2::xml_attr(node, "col1"))
if (xml2::xml_attr(node, "line2") == line1) {
col2 <- as.integer(xml2::xml_attr(node, "col2"))
} else {
col2 <- nchar(source_file$lines[line1])
}
Lint(
filename = source_file$filename,
line_number = as.integer(line1),
column_number = col1,
type = "warning",
message = sprintf(
"%s (R %s) is not available for dependency R >= %s.",
all_names[ii], names(needs_backport_names)[which(needs_backport[ii, ])], r_version
),
line = source_file$lines[[line1]],
ranges = list(c(col1, col2)),
linter = "backport_linter"
)
})
}
}

backports <- list(
`devel` = c("...names", "checkRdContents", "numToBits", "numToInts", "packBits"),
`4.0.0` = c(
".class2", ".S3method", "activeBindingFunction", "deparse1", "globalCallingHandlers",
"infoRDS", "list2DF", "marginSums", "proportions", "R_user_dir", "socketTimeout", "tryInvokeRestart"
),
`3.6.0` = c(
"asplit", "hcl.colors", "hcl.pals", "mem.maxNsize", "mem.maxVsize", "nullfile", "str2lang",
"str2expression", "update_PACKAGES"
),
`3.5.0` = c("...elt", "...length", "askYesNo", "getDefaultCluster", "isFALSE", "packageDate", "warnErrList"),
`3.4.0` = c(
"check_packages_in_dir_details", "CRAN_package_db", "debugcall", "hasName",
"isS3stdgeneric", "strcapture", "Sys.setFileTime", "undebugcall"
),
`3.3.0` = c(
".traceback", "chkDots", "curlGetHeaders", "endsWith", "grouping", "isS3method",
"makevars_site", "makevars_user", "Rcmd", "sigma", "startsWith", "strrep", "validEnc", "validUTF8"
),
`3.2.0` = c(
".getNamespaceInfo", "check_packages_in_dir_changes", "debuggingState",
"dir.exists", "dynGet", "extSoftVersion", "get0", "grSoftVersion", "hsearch_db",
"isNamespaceLoaded", "lengths", "libcurlVersion", "returnValue", "tclVersion", "toTitleCase", "trimws"
),
`3.1.3` = "pcre_config",
`3.1.2` = "icuGetCollate",
`3.1.1` = c(".nknots.smspl", "promptImport"),
`3.1.0` = c("agrepl", "anyNA", "changedFiles", "cospi", "fileSnapshot", "find_gs_cmd", "sinpi", "tanpi"),
`3.0.3` = "La_version",
`3.0.2` = c("assertCondition", "assertError", "assertWarning", "getVignetteInfo"),
`3.0.0` = c(
".onDetach", "bitwAnd", "bitwNot", "bitwOr", "bitwShiftL", "bitwShiftR", "bitwXor",
"check_packages_in_dir", "cite", "citeNatbib", "clearPushBack", "packageName",
"process.events", "provideDimnames", "quartz.save", "rep_len"
)
)
32 changes: 32 additions & 0 deletions tests/testthat/test-backport_linter.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
test_that("backport_linter detects backwards-incompatibility", {
# default should be current R version; all of these are included on our dependency
expect_lint(".getNamespaceInfo(dir.exists(lapply(x, toTitleCase)))", NULL, backport_linter())

# don't allow dependencies older than we've recorded
writeLines("x <- x + 1", tmp <- tempfile())
on.exit(unlink(tmp))

expect_warning(l <- lint(tmp, backport_linter("2.0.0")), "version older than 3.0.0", fixed = TRUE)
expect_identical(l, lint(tmp, backport_linter("3.0.0")))

expect_lint(
"numToBits(2)",
rex("numToBits (R devel) is not available for dependency R >= 4.0.0."),
backport_linter("4.0.0")
)
# symbols as well as calls
expect_lint(
"lapply(1:10, numToBits)",
rex("numToBits (R devel) is not available for dependency R >= 4.0.0."),
backport_linter("4.0.0")
)

expect_lint(
"trimws(...names())",
list(
rex("trimws (R 3.2.0) is not available for dependency R >= 3.0.0."),
rex("...names (R devel) is not available for dependency R >= 3.0.0.")
),
backport_linter("3.0.0")
)
})