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 3 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 @@ -15,6 +15,7 @@
* New `sprintf_linter()` (#544, #578, @renkun-ken)
* Exclusions specified in the `.lintr` file are now relative to the location of that file
and support excluding entire directories (#158, #438, @AshesITR)
* New `backport_linter()` for detecting mismatched R version dependencies (#506, @MichaelChirico)

# lintr 2.0.1

Expand Down
83 changes: 83 additions & 0 deletions R/backport_linter.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
#' @describeIn linters that checks for usage of unavailable functions. Not reliable for testing r-devel dependencies.
#' @export
backport_linter <- function(r_version = "4.0.3") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the default auto-detect, e.g. getRversion()?
Or even better for Packages, get the R version from the Depends field in DESCRIPTION? (this might be worth a separate PR, also suppressing lints for stuff imported from {backports})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea, and a good direction for follow-up. Actually I think we can go beyond backports (there may be other backport packages) and try to check namespace imports and/or internally defined functions & skip those. New argument skip= defaulting to infer_skips where we apply some logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest using except as the argument name - it's already used in missing_argument_linter with the same purpose:

#' @param except a character vector of function names as exceptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the default r_version be the current R version?

If a package author wants to use this linter, then one must specify the r_version to be consistent with that appears in the package DESCRIPTION?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I used getRversion(), I guess we could make it guess_r_version() & check the description first before defaulting to getRversion()?

function(source_file) {
if (is.null(source_file$xml_parsed_content) || r_version >= "r-devel") return(list())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a lexicographic comparison to "r-devel"?
Both "r-release" and "r-oldrel" would trigger this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I should just leave this as "r" -- I don't think we want to support tracking devel dependencies, I just wanted something that would be bigger than all R version numbers (at first I used "999999" as the name, "r-devel" felt better but you raise a good point)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we consult names(backports) to find whether the selected version is supported?
The highest of those versions wouldn't cause lints anyway so we can tail() it to obtain a definitive list of R versions that can result in lints if I'm not mistaken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if so we'd have to include in the list all released versions (e.g. 3.5.3).

And sorry I'm just waking up, I'm not following your second point

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point.
Wouldn't we never lint if r_version is at least getRversion()?
i.e. if (r_version >= getRversion()) return(list()).
That way if r_version is older than the current environment, backport problems can be detected.
If it isn't there are no backports that can be used from the current R version which would be a problem in r_version, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's basically right, but I do think it's possible to run this on a machine that has a different environment than required. Say I work on two machines and the one I'm using today has an old version. We wouldn't want to skip then necessarily right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the old version, the used backports would either break the code completely (which you'd notice), or the backports were already safely ported to your old version. No?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's if we run tests, but lintr is static analysis so we could in principle run it on a different machine that doesn't execute code.

If that machine is running for a bigger library of R code (say a local CRAN), there may be different code requirements.

I think it's basically an edge case... but taking a step back, the goal of this is to get around/change the r_version >= "r" part? I'm happier with that now than I was with r_version >= "r-devel"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Let's just leave it as-is for now.
Maybe later become smart about what r_versions are found in backports and thus will potentially cause lints.


xml <- source_file$xml_parsed_content

#browser()
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved

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, which may over-simplify -- cbind makes sure apply 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 <- apply(needs_backport, 1L, any)
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved

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(
`r-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", "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"
)
)
28 changes: 28 additions & 0 deletions tests/testthat/test-backport_linter.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
test_that("backport_linter detects backwards-incompatibility", {
# this test may be too fragile?
expect_lint("numToBits(1)", NULL, backport_linter("r-devel"))

# 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())

expect_lint(
"numToBits(2)",
rex("numToBits (R r-devel) is not available for dependency R >= 4.0.0."),
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
backport_linter("4.0.0")
)
# symbols as well as calls
expect_lint(
"lapply(1:10, numToBits)",
rex("numToBits (R 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 r-devel) is not available for dependency R >= 3.0.0.")
),
backport_linter("3.0.0")
)
})