Merge branch 'main' into sarif-extdata

NEWS.md

@@ -5,6 +5,7 @@
 * `infix_spaces_linter()` distinguishes `<-`, `:=`, `<<-` and `->`, `->>`, i.e. `infix_spaces_linter(exclude_operators = "->")` will no longer exclude `->>` (#2115, @MichaelChirico). This change is breaking for users relying on manually-supplied `exclude_operators` containing `"<-"` to also exclude `:=` and `<<-`. The fix is to manually supply `":="` and `"<<-"` as well. We don't expect this change to affect many users, the fix is simple, and the new behavior is much more transparent, so we are including this breakage in a minor release.
 * Removed `find_line()` and `find_column()` entries from `get_source_expressions()` expression-level objects. These have been marked deprecated since version 3.0.0. No users were found on GitHub.
 * There is experimental support for writing config in plain R scripts (as opposed to DCF files; #1210, @MichaelChirico). The script is run in a new environment and variables matching settings (`?default_settings`) are copied over. In particular, this removes the need to write R code in a DCF-friendly way, and allows normal R syntax highlighting in the saved file. We may eventually deprecate the DCF approach in favor of this one; user feedback is welcome on strong preferences for either approach, or for a different approach like YAML. Generally you should be able to convert your existing `.lintr` file to an equivalent R config by replacing the `:` key-value separators with assignments (`<-`). By default, such a config is searched for in a file named '.lintr.R'. This is a mildly breaking change if you happened to be keeping a file '.lintr.R' around since that file is given precedence over '.lintr'.
+ + We also validate config files up-front make it clearer when invalid configs are present (#2195, @MichaelChirico). There is a warning for "invalid" settings, i.e., settings not part of `?default_settings`. We think this is more likely to affect users declaring settings in R, since any variable defined in the config that's not a setting must be removed to make it clearer which variables are settings vs. ancillary.
 
 ## Bug fixes
 

R/backport_linter.R

@@ -4,7 +4,7 @@
 #'
 #' @param r_version Minimum R version to test for compatibility
 #' @param except Character vector of functions to be excluded from linting.
-#' Use this to list explicitly defined backports, e.g. those imported from the {backports} package or manually
+#' Use this to list explicitly defined backports, e.g. those imported from the `{backports}` package or manually
 #' defined in your package.
 #'
 #' @examples

R/exclude.R

@@ -10,18 +10,25 @@
 #' "@details",
 #' "Exclusions can be specified in three different ways.",
 #' "",
-#' "1. single line in the source file. default: `# nolint`, possibly followed by a listing of linters to exclude.",
+#' "1. Single line in the source file. default: `# nolint`, possibly followed by a listing of linters to exclude.",
 #' " If the listing is missing, all linters are excluded on that line. The default listing format is",
 #' paste(
 #' " `#",
 #' "nolint: linter_name, linter2_name.`. There may not be anything between the colon and the line exclusion tag"
 #' ),
 #' " and the listing must be terminated with a full stop (`.`) for the linter list to be respected.",
-#' "2. line range in the source file. default: `# nolint start`, `# nolint end`. `# nolint start` accepts linter",
+#' "2. Line range in the source file. default: `# nolint start`, `# nolint end`. `# nolint start` accepts linter",
 #' " lists in the same form as `# nolint`.",
-#' "3. exclusions parameter, a named list of files with named lists of linters and lines to exclude them on, a named",
-#' " list of the files and lines to exclude, or just the filenames if you want to exclude the entire file, or the",
-#' " directory names if you want to exclude all files in a directory."
+#' "3. Exclusions parameter, a list with named and/or unnamed entries. ",
+#' " Outer elements have the following characteristics:",
+#' " 1. Unnamed elements specify filenames or directories.",
+#' " 2. Named elements are a vector or list of line numbers, with `Inf` indicating 'all lines'.",
+#' " The name gives a path relative to the config.",
+#' " 1. Unnamed elements denote exclusion of all linters in the given path or directory.",
+#' " 2. Named elements, where the name specifies a linter, denote exclusion for that linter.",
+#' " For convenience, a vector can be used in place of a list whenever it would not introduce ambiguity, e.g.",
+#' " a character vector of files to exclude or a vector of lines to exclude.",
+#' NULL
 #' )
 exclude <- function(lints, exclusions = settings$exclusions, linter_names = NULL, ...) {
  if (length(lints) <= 0L) {

R/scalar_in_linter.R

@@ -1,7 +1,7 @@
 #' Block usage like x %in% "a"
 #'
 #' `vector %in% set` is appropriate for matching a vector to a set, but if
-#' that set has size 1, `==` is more appropriate. `%chin%` from `data.table`
+#' that set has size 1, `==` is more appropriate. `%chin%` from `{data.table}`
 #' is matched as well.
 #'
 #' `scalar %in% vector` is OK, because the alternative (`any(vector == scalar)`)

R/seq_linter.R

@@ -4,7 +4,7 @@
 #' `1:NROW(...)` and `1:NCOL(...)` expressions in base-R, or their usage in
 #' conjunction with `seq()` (e.g., `seq(length(...))`, `seq(nrow(...))`, etc.).
 #'
-#' Additionally, it checks for `1:n()` (from dplyr) and `1:.N` (from data.table).
+#' Additionally, it checks for `1:n()` (from `{dplyr}`) and `1:.N` (from `{data.table}`).
 #'
 #' These often cause bugs when the right-hand side is zero.
 #' It is safer to use [base::seq_len()] or [base::seq_along()] instead.

R/settings.R

@@ -32,6 +32,7 @@ read_settings <- function(filename) {
  }
 
  config <- read_config_file(config_file)
+ validate_config_file(config, config_file, default_settings)
 
  for (setting in names(default_settings)) {
  value <- get_setting(setting, config, default_settings)
@@ -77,6 +78,97 @@ read_config_file <- function(config_file) {
  config
 }
 
+validate_config_file <- function(config, config_file, defaults) {
+ matched <- names(config) %in% names(defaults)
+ if (!all(matched)) {
+ warning("Found unused settings in config '", config_file, "': ", toString(names(config)[!matched]))
+ }
+
+ validate_regex(config,
+ c("exclude", "exclude_next", "exclude_start", "exclude_end", "exclude_linter", "exclude_linter_sep")
+ )
+ validate_character_string(config, c("encoding", "cache_directory", "comment_token"))
+ validate_true_false(config, c("comment_bot", "error_on_lint"))
+ validate_linters(config$linters)
+ validate_exclusions(config$exclusions)
+}
+
+is_character_string <- function(x) is.character(x) && length(x) == 1L && !is.na(x)
+is_valid_regex <- function(str) !inherits(tryCatch(grepl(str, ""), condition = identity), "condition")
+is_single_regex <- function(x) is_character_string(x) && is_valid_regex(x)
+is_true_false <- function(x) is.logical(x) && length(x) == 1L && !is.na(x)
+
+validate_keys <- function(config, keys, test, what) {
+ for (key in keys) {
+ val <- config[[key]]
+ if (is.null(val)) {
+ next
+ }
+ if (!test(val)) {
+ stop("Setting '", key, "' should be ", what, ", not '", toString(val), "'.")
+ }
+ }
+}
+
+validate_regex <- function(config, keys) {
+ validate_keys(config, keys, is_single_regex, "a single regular expression")
+}
+
+validate_character_string <- function(config, keys) {
+ validate_keys(config, keys, is_character_string, "a character string")
+}
+
+validate_true_false <- function(config, keys) {
+ validate_keys(config, keys, is_true_false, "TRUE or FALSE")
+}
+
+validate_linters <- function(linters) {
+ if (is.null(linters)) {
+ return(invisible())
+ }
+
+ is_linters <- vapply(linters, is_linter, logical(1L))
+ if (!all(is_linters)) {
+ stop(
+ "Setting 'linters' should be a list of linters, but found non-linters at elements ",
+ toString(which(!is_linters)), "."
+ )
+ }
+}
+
+validate_exclusions <- function(exclusions) {
+ if (is.null(exclusions)) {
+ return(invisible())
+ }
+
+ exclusion_names <- names2(exclusions)
+ has_names <- exclusion_names != ""
+ unnamed_is_string <-
+ vapply(exclusions[!has_names], function(x) is.character(x) && length(x) == 1L && !is.na(x), logical(1L))
+ if (!all(unnamed_is_string)) {
+ stop(
+ "Unnamed entries of setting 'exclusions' should be strings naming files or directories, check entries: ",
+ toString(which(!has_names)[!unnamed_is_string]), "."
+ )
+ }
+ for (ii in which(has_names)) validate_named_exclusion(exclusions, ii)
+}
+
+validate_named_exclusion <- function(exclusions, idx) {
+ entry <- exclusions[[idx]]
+ if (is.list(entry)) {
+ valid_entry <- vapply(entry, function(x) is.numeric(x) && !anyNA(x), logical(1L))
+ } else {
+ valid_entry <- is.numeric(entry) && !anyNA(entry)
+ }
+ if (!all(valid_entry)) {
+ stop(
+ "Named entries of setting 'exclusions' should designate line numbers for exclusion, ",
+ "check exclusion: ", idx, "."
+ )
+ }
+}
+
 lintr_option <- function(setting, default = NULL) getOption(paste0("lintr.", setting), default)
 
 get_setting <- function(setting, config, defaults) {

R/xp_utils.R

@@ -112,7 +112,7 @@ xp_find_location <- function(xml, xpath) {
 
 #' Strip XPath 2.0-style comments from an XPath
 #'
-#' xml2 uses XPath 1.0, which has no support for comments. But comments are
+#' `{xml2}` uses XPath 1.0, which has no support for comments. But comments are
 #' useful in a codebase with as many XPaths as we maintain, so we fudge our
 #' way to XPath 2.0-ish support by writing this simple function to remove comments.
 #'

R/zzz.R

@@ -250,7 +250,7 @@ default_undesirable_operators <- all_undesirable_operators[names(all_undesirable
 #' - `exclude`: pattern used to exclude a line of code
 #' - `exclude_start`, `exclude_end`: patterns used to mark start and end of the code block to exclude
 #' - `exclude_linter`, `exclude_linter_sep`: patterns used to exclude linters
-#' - `exclusions`:a list of files to exclude
+#' - `exclusions`: a list of exclusions, see [exclude()] for a complete description of valid values.
 #' - `cache_directory`: location of cache directory
 #' - `comment_token`: a GitHub token character
 #' - `comment_bot`: decides if lintr comment bot on GitHub can comment on commits

tests/testthat/test-lint_package.R

@@ -217,9 +217,13 @@ test_that("package using .lintr.R config lints correctly", {
 
  # config produces unused variables
  withr::local_options(lintr.linter_file = "lintr_test_config_extraneous")
- expect_length(lint_package(r_config_pkg), 2L)
+ expect_warning(
+ expect_length(lint_package(r_config_pkg), 2L),
+ "Found unused settings in config",
+ fixed = TRUE
+ )
 
- # DCF is preferred if multiple matched configs
+ # R is preferred if multiple matched configs
  withr::local_options(lintr.linter_file = "lintr_test_config_conflict")
  lints <- as.data.frame(lint_package(r_config_pkg))
  expect_identical(unique(basename(lints$filename)), "testthat.R")