diff --git a/.Rbuildignore b/.Rbuildignore index e609141bd..23f41e938 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -23,9 +23,11 @@ ^bench$ ^tests/testthat/dummy_packages/package/[.]Rbuildignore$ ^tests/testthat/dummy_packages/cp1252/[.]Rbuildignore$ +testthat-problems[.]rds$ ^_pkgdown\.yaml$ ^docs$ ^pkgdown$ ^vignettes/[^-]+.gif$ ^CRAN-SUBMISSION$ ^CODE_OF_CONDUCT\.md$ +^paper$ diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md new file mode 100644 index 000000000..10289cd35 --- /dev/null +++ b/.github/CONTRIBUTING.md @@ -0,0 +1,39 @@ +# Contributing to `{lintr}` + +This outlines how to propose a change to `{lintr}`. For a detailed discussion on contributing to this, r-lib, and other tidyverse packages, please see the [development contributing guide](https://rstd.io/tidy-contrib) and our [code review principles](https://code-review.tidyverse.org/). + +## Fixing typos + +You can fix typos, spelling mistakes, or grammatical errors in the documentation directly using the GitHub web interface, as long as the changes are made in the _source_ file. This generally means you'll need to edit [roxygen2 comments](https://roxygen2.r-lib.org/articles/roxygen2.html) in an `.R`, not a `.Rd` file. You can find the `.R` file that generates the `.Rd` by reading the comment in the first line. + +## Bigger changes + +If you want to make a bigger change, it's a good idea to first file an issue and make sure someone from the team agrees that it’s needed. If you’ve found a bug, please file an issue that illustrates the bug with a minimal [reprex](https://www.tidyverse.org/help/#reprex) (this will also help you write a unit test, if needed). See the tidyverse guide on [how to create a great issue](https://code-review.tidyverse.org/issues/) for more advice. + +### Adding a new linter + +If you wish to contribute a new linter, the [Creating new linters](https://lintr.r-lib.org/articles/creating_linters.html) article serves as a comprehensive guide. + +### Pull request process + +* Fork the package and clone onto your computer. If you haven't done this before, we recommend using `usethis::create_from_github("r-lib/lintr", fork = TRUE)`. + +* Install all development dependencies with `devtools::install_dev_deps()`, and then make sure the package passes R CMD check by running `devtools::check()`. If R CMD check doesn't pass cleanly, it's a good idea to ask for help before continuing. + +* Create a Git branch for your pull request (PR). We recommend using `usethis::pr_init("brief-description-of-change")`. At a minimum, please avoid submitting PRs from your fork's `main` branch` as this can make the review process more complicated. + +* Make your changes, commit them to Git, and create a PR using `usethis::pr_push()`. Follow the prompts in your browser to complete the process. Use a concise title for your PR that summarizes the change, and include `Fixes #issue-number` in the PR _description_. Doing so will automatically close the linked issue when the PR is merged. For complicated changes, add a textual overview of what your PR does in the description. Consider breaking up large PRs into a chain of more digestible+focused smaller PRs. + +* For user-facing changes, add a bullet appropriately in the top section of `NEWS.md` (i.e. below the first header). Follow the style described in . Most importantly, your audience for NEWS items is a package user, i.e., _not_ a package developer. + +### Code style + +* New code should follow the tidyverse [style guide](https://style.tidyverse.org). You can use the [styler](https://CRAN.R-project.org/package=styler) package to apply these styles. + +* We use [roxygen2](https://cran.r-project.org/package=roxygen2), with [Markdown syntax](https://cran.r-project.org/web/packages/roxygen2/vignettes/rd-formatting.html), for documentation. + +* We use [testthat](https://cran.r-project.org/package=testthat) for unit tests. Contributions with test cases included are easier to accept. + +## Code of Conduct + +Please note that the lintr project is released with a [Contributor Code of Conduct](CODE_OF_CONDUCT.md). By contributing to this project you agree to abide by its terms. diff --git a/.github/workflows/R-CMD-check-hard.yaml b/.github/workflows/R-CMD-check-hard.yaml index 54db1e064..977807d3c 100644 --- a/.github/workflows/R-CMD-check-hard.yaml +++ b/.github/workflows/R-CMD-check-hard.yaml @@ -48,6 +48,7 @@ jobs: - uses: r-lib/actions/setup-r-dependencies@v2 with: + install-quarto: false pak-version: devel dependencies: '"hard"' cache: false diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 110d148cb..f0848b43b 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -70,6 +70,7 @@ jobs: - uses: r-lib/actions/setup-r-dependencies@v2 with: + install-quarto: false extra-packages: | any::rcmdcheck needs: check diff --git a/.github/workflows/draft-pdf.yml b/.github/workflows/draft-pdf.yml new file mode 100644 index 000000000..d72255cdc --- /dev/null +++ b/.github/workflows/draft-pdf.yml @@ -0,0 +1,30 @@ +# TODO: delete this file once the paper is published +on: + push: + branches: [main] + pull_request: + branches: [main] + +jobs: + paper: + runs-on: ubuntu-latest + name: Paper Draft + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Build draft PDF + uses: openjournals/openjournals-draft-action@master + with: + journal: joss + # This should be the path to the paper within your repo. + paper-path: paper/paper.md + + - name: Upload + uses: actions/upload-artifact@v4 + with: + name: paper + # This is the output path where Pandoc will write the compiled + # PDF. Note, this should be the same directory as the input + # paper.md + path: paper/paper.pdf diff --git a/.github/workflows/pkgdown.yaml b/.github/workflows/pkgdown.yaml index dad368915..cc8ffb62f 100644 --- a/.github/workflows/pkgdown.yaml +++ b/.github/workflows/pkgdown.yaml @@ -39,7 +39,7 @@ jobs: - name: Deploy to GitHub pages 🚀 if: github.event_name != 'pull_request' - uses: JamesIves/github-pages-deploy-action@v4.6.1 + uses: JamesIves/github-pages-deploy-action@v4.7.1 with: clean: false branch: gh-pages diff --git a/.lintr b/.lintr index 0542b9eee..dd04ff1d4 100644 --- a/.lintr +++ b/.lintr @@ -17,7 +17,8 @@ linters: all_linters( options = NULL, message = "use cli::cli_inform()", warning = "use cli::cli_warn()", - stop = "use cli::cli_abort()" + stop = "use cli::cli_abort()", + normalizePath = "use normalize_path()" )), undesirable_operator_linter(modify_defaults( defaults = default_undesirable_operators, @@ -28,6 +29,8 @@ linters: all_linters( absolute_path_linter = NULL, library_call_linter = NULL, nonportable_path_linter = NULL, + # We now require R>=4.0.0 + strings_as_factors_linter = NULL, todo_comment_linter = NULL, # TODO(#2327): Enable this. unreachable_code_linter = NULL diff --git a/DESCRIPTION b/DESCRIPTION index 1cbd3983e..9f6dd32b5 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -21,7 +21,7 @@ License: MIT + file LICENSE URL: https://lintr.r-lib.org, https://github.com/r-lib/lintr BugReports: https://github.com/r-lib/lintr/issues Depends: - R (>= 3.6) + R (>= 4.0) Imports: backports (>= 1.1.7), cli (>= 3.4.0), @@ -56,7 +56,7 @@ Config/testthat/edition: 3 Config/testthat/parallel: true Encoding: UTF-8 Roxygen: list(markdown = TRUE) -RoxygenNote: 7.3.1 +RoxygenNote: 7.3.2 Collate: 'make_linter_from_xpath.R' 'xp_utils.R' diff --git a/NAMESPACE b/NAMESPACE index 767662c58..3da94d313 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,11 +1,5 @@ # Generated by roxygen2: do not edit by hand - -if (getRversion() >= "4.0.0") { - importFrom(tools, R_user_dir) -} else { - importFrom(backports, R_user_dir) -} S3method("[",lints) S3method(as.data.frame,lints) S3method(format,lint) @@ -182,7 +176,9 @@ importFrom(rex,re_matches) importFrom(rex,re_substitutes) importFrom(rex,regex) importFrom(rex,rex) +importFrom(stats,complete.cases) importFrom(stats,na.omit) +importFrom(tools,R_user_dir) importFrom(utils,capture.output) importFrom(utils,getParseData) importFrom(utils,getTxtProgressBar) diff --git a/NEWS.md b/NEWS.md index 39cc847a5..93c517465 100644 --- a/NEWS.md +++ b/NEWS.md @@ -17,6 +17,9 @@ * Drop support for posting GitHub comments from inside GitHub comment bot, Travis, Wercker, and Jenkins CI tools (spurred by #2148, @MichaelChirico). We rely on GitHub Actions for linting in CI, and don't see any active users relying on these alternatives. We welcome and encourage community contributions to get support for different CI system going again. * `cyclocomp_linter()` is no longer part of the default linters (#2555, @IndrajeetPatil) because the tidyverse style guide doesn't contain any guidelines on meeting certain complexity requirements. Note that users with `cyclocomp_linter()` in their configs may now need to install {cyclocomp} intentionally, in particular in CI/CD pipelines. * `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle) +* `lint()` and friends now normalize paths to forward slashes on Windows (@olivroy, #2613). +* `undesirable_function_linter()`, `undesirable_operator_linter()`, and `list_comparison_linter()` were removed from the tag `efficiency` (@IndrajeetPatil, #2655). If you use `linters_with_tags("efficiency")` to include these linters, you'll need to adjust your config to keep linting your code against them. We did not find any such users on GitHub. + ## Bug fixes @@ -26,6 +29,7 @@ * `.lintr` configs set by option `lintr.linter_file` or environment variable `R_LINTR_LINTER_FILE` can point to subdirectories (#2512, @MichaelChirico). * `indentation_linter()` returns `ranges[1L]==1L` when the offending line has 0 spaces (#2550, @MichaelChirico). * `literal_coercion_linter()` doesn't surface a warning about NAs during coercion for code like `as.integer("a")` (#2566, @MichaelChirico). +* `commented_code_linter()` can detect commented code that ends with a pipe (#2671, @jcken95) ## Changes to default linters @@ -57,6 +61,7 @@ * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico). * `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico). * `expect_no_lint()` was added as new function to cover the typical use case of expecting no lint message, akin to the recent {testthat} functions like `expect_no_warning()` (#2580, @F-Noelle). +* `lint()` and friends emit a message if no lints are found (#2643, @IndrajeetPatil). ### New linters @@ -82,10 +87,14 @@ ### Lint accuracy fixes: removing false positives * `object_name_linter()` and `object_length_linter()` ignore {rlang} name injection like `x |> mutate("{new_name}" := foo(col))` (#1926, @MichaelChirico). No checking is applied in such cases. {data.table} in-place assignments like `DT[, "sPoNGeBob" := "friend"]` are still eligible for lints. +* `object_usage_linter()` finds global variables assigned with `=` or `->`, which avoids some issues around "undefined global variables" in scripts (#2654, @MichaelChirico). ## Notes * All user-facing messages are now prepared using the `{cli}` package (#2418, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent. +* File locations in lints and error messages contain clickable hyperlinks to improve code navigation (#2645, #2588, @olivroy). +* {lintr} now depends on R version 4.0.0. It already does so implicitly due to recursive upstream dependencies requiring this version; we've simply made that dependency explicit and up-front (#2569, @MichaelChirico). +* Some code with parameters accepting regular expressions is less strict about whether there are capture groups (#2678, @MichaelChirico). In particular, this affects `unreachable_code_linter(allow_comment_regex=)` and `expect_lint(checks=)`. # lintr 3.1.2 diff --git a/R/absolute_path_linter.R b/R/absolute_path_linter.R index a27ebc1ea..38fffd40b 100644 --- a/R/absolute_path_linter.R +++ b/R/absolute_path_linter.R @@ -8,9 +8,7 @@ #' * contain at least two path elements, with one having at least two characters and #' * contain only alphanumeric chars (including UTF-8), spaces, and win32-allowed punctuation #' -#' @examplesIf getRversion() >= "4.0" -#' # Following examples use raw character constant syntax introduced in R 4.0. -#' +#' @examples #' # will produce lints #' lint( #' text = 'R"--[/blah/file.txt]--"', diff --git a/R/actions.R b/R/actions.R index 9a20a1986..606442b8f 100644 --- a/R/actions.R +++ b/R/actions.R @@ -2,6 +2,10 @@ in_github_actions <- function() { identical(Sys.getenv("GITHUB_ACTIONS"), "true") } +in_pkgdown <- function() { + identical(Sys.getenv("IN_PKGDOWN"), "true") +} + # Output logging commands for any lints found github_actions_log_lints <- function(lints, project_dir = "") { for (x in lints) { diff --git a/R/cache.R b/R/cache.R index e2a63bb4b..b177f4305 100644 --- a/R/cache.R +++ b/R/cache.R @@ -53,10 +53,10 @@ load_cache <- function(file, path = NULL) { invokeRestart("muffleWarning") }, error = function(e) { - cli_warn(c( - x = "Could not load cache file {.file {file}}:", - i = conditionMessage(e) - )) + cli_warn( + "Could not load cache file {.file {file}}:", + parent = e + ) } ) } # else nothing to do for source file that has no cache diff --git a/R/class_equals_linter.R b/R/class_equals_linter.R index 2dd24b83d..1f3483e9b 100644 --- a/R/class_equals_linter.R +++ b/R/class_equals_linter.R @@ -48,9 +48,10 @@ class_equals_linter <- function() { bad_expr <- xml_find_all(xml_calls, xpath) operator <- xml_find_chr(bad_expr, "string(*[2])") - lint_message <- sprintf( - "Use inherits(x, 'class-name'), is. or is(x, 'class') instead of comparing class(x) with %s.", - operator + lint_message <- paste0( + "Use inherits(x, 'class-name'), is. for S3 classes, ", + "or is(x, 'S4Class') for S4 classes, ", + "instead of comparing class(x) with ", operator, "." ) xml_nodes_to_lints( bad_expr, diff --git a/R/commented_code_linter.R b/R/commented_code_linter.R index fcc4af9ef..97f9c8d8e 100644 --- a/R/commented_code_linter.R +++ b/R/commented_code_linter.R @@ -83,9 +83,10 @@ commented_code_linter <- function() { all_comments <- xml_text(all_comment_nodes) code_candidates <- re_matches(all_comments, code_candidate_regex, global = FALSE, locations = TRUE) extracted_code <- code_candidates[, "code"] - # ignore trailing ',' when testing for parsability - extracted_code <- re_substitutes(extracted_code, rex(",", any_spaces, end), "") + # ignore trailing ',' or pipes ('|>', '%>%') when testing for parsability + extracted_code <- re_substitutes(extracted_code, rex(or(",", "|>", "%>%"), any_spaces, end), "") extracted_code <- re_substitutes(extracted_code, rex(start, any_spaces, ","), "") + is_parsable <- which(vapply(extracted_code, parsable, logical(1L))) lint_list <- xml_nodes_to_lints( diff --git a/R/exclude.R b/R/exclude.R index f3bbc5405..25a924cea 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -326,7 +326,7 @@ normalize_exclusions <- function(x, normalize_path = TRUE, paths[rel_path] <- file.path(root, paths[rel_path]) names(x) <- paths x <- x[file.exists(paths)] # remove exclusions for non-existing files - names(x) <- normalizePath(names(x)) # get full path for remaining files + names(x) <- normalize_path(names(x)) # get full path for remaining files } remove_line_duplicates( diff --git a/R/expect_lint.R b/R/expect_lint.R index 7bc1b65fd..78658ffc0 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -105,16 +105,10 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { ) # deparse ensures that NULL, list(), etc are handled gracefully ok <- if (field == "message") { - re_matches(value, check) + re_matches_logical(value, check) } else { isTRUE(all.equal(value, check)) } - if (!is.logical(ok)) { - cli_abort(c( - x = "Invalid regex result. Did you mistakenly have a capture group in the regex?", - i = "You can match parentheses with a character class, i.e. inside `[]`." - )) - } testthat::expect(ok, msg) }) }, diff --git a/R/extract.R b/R/extract.R index c9aa57b70..edd12a456 100644 --- a/R/extract.R +++ b/R/extract.R @@ -9,7 +9,7 @@ extract_r_source <- function(filename, lines, error = identity) { output <- rep.int(NA_character_, length(lines)) chunks <- tryCatch(get_chunk_positions(pattern = pattern, lines = lines), error = error) - if (inherits(chunks, "error") || inherits(chunks, "lint")) { + if (is_error(chunks) || is_lint(chunks)) { assign("e", chunks, envir = parent.frame()) # error, so return empty code return(output) diff --git a/R/get_source_expressions.R b/R/get_source_expressions.R index bc83712d5..3f1817023 100644 --- a/R/get_source_expressions.R +++ b/R/get_source_expressions.R @@ -86,7 +86,7 @@ get_source_expressions <- function(filename, lines = NULL) { source_expression$content <- get_content(source_expression$lines) parsed_content <- get_source_expression(source_expression, error = function(e) lint_parse_error(e, source_expression)) - if (inherits(e, "lint") && (is.na(e$line) || !nzchar(e$line) || e$message == "unexpected end of input")) { + if (is_lint(e) && (is.na(e$line) || !nzchar(e$line) || e$message == "unexpected end of input")) { # Don't create expression list if it's unreliable (invalid encoding or unhandled parse error) expressions <- list() } else { @@ -502,7 +502,7 @@ get_source_expression <- function(source_expression, error = identity) { error = error ) - if (inherits(parsed_content, c("error", "lint"))) { + if (is_error(parsed_content) || is_lint(parsed_content)) { assign("e", parsed_content, envir = parent.frame()) parse_error <- TRUE } @@ -513,7 +513,7 @@ get_source_expression <- function(source_expression, error = identity) { error = error ) - if (inherits(parsed_content, c("error", "lint"))) { + if (is_error(parsed_content) || is_lint(parsed_content)) { # Let parse errors take precedence over encoding problems if (!parse_error) assign("e", parsed_content, envir = parent.frame()) return() # parsed_content is unreliable if encoding is invalid @@ -636,8 +636,7 @@ fix_eq_assigns <- function(pc) { parent = integer(n_expr), token = character(n_expr), terminal = logical(n_expr), - text = character(n_expr), - stringsAsFactors = FALSE + text = character(n_expr) ) for (i in seq_len(n_expr)) { diff --git a/R/lint.R b/R/lint.R index 48ae96e81..1961acb05 100644 --- a/R/lint.R +++ b/R/lint.R @@ -52,7 +52,7 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = close(con) } - filename <- normalizePath(filename, mustWork = !inline_data) # to ensure a unique file in cache + filename <- normalize_path(filename, mustWork = !inline_data) # to ensure a unique file in cache source_expressions <- get_source_expressions(filename, lines) if (isTRUE(parse_settings)) { @@ -88,10 +88,10 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = lints[[length(lints) + 1L]] <- withCallingHandlers( get_lints(expr, linter, linters[[linter]], lint_cache, source_expressions$lines), error = function(cond) { - cli_abort(c( + cli_abort( "Linter {.fn linter} failed in {.file {filename}}:", - conditionMessage(cond) - )) + parent = cond + ) } ) } @@ -163,9 +163,10 @@ lint_dir <- function(path = ".", ..., pattern = pattern ) - # normalizePath ensures names(exclusions) and files have the same names for the same files. + # normalize_path ensures names(exclusions) and files have the same names for the same files. + # It also ensures all paths have forward slash # Otherwise on windows, files might incorrectly not be excluded in to_exclude - files <- normalizePath(dir( + files <- normalize_path(dir( path, pattern = pattern, recursive = TRUE, @@ -198,7 +199,7 @@ lint_dir <- function(path = ".", ..., lints <- reorder_lints(lints) if (relative_path) { - path <- normalizePath(path, mustWork = FALSE) + path <- normalize_path(path, mustWork = FALSE) lints[] <- lapply( lints, function(x) { @@ -249,7 +250,7 @@ lint_package <- function(path = ".", ..., if (is.null(pkg_path)) { cli_warn(c( - i = "Didn't find any R package searching upwards from {.file {normalizePath(path)}}" + i = "Didn't find any R package searching upwards from {.file {normalize_path(path)}}" )) return(NULL) } @@ -274,7 +275,7 @@ lint_package <- function(path = ".", ..., ) if (isTRUE(relative_path)) { - path <- normalizePath(pkg_path, mustWork = FALSE) + path <- normalize_path(pkg_path, mustWork = FALSE) lints[] <- lapply( lints, function(x) { @@ -410,16 +411,13 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec } max_col <- max(nchar(line) + 1L, 1L, na.rm = TRUE) if (!is_number(column_number) || column_number < 0L || column_number > max_col) { - cli_abort(c( - i = "{.arg column_number} must be an integer between {.val {0}} and {.code nchar(line) + 1} ({.val {max_col}})", - x = "Instead, it was {.val {column_number}}." - )) + cli_abort(" + {.arg column_number} must be an integer between {.val {0}} and {.val {max_col}} ({.code nchar(line) + 1}), + not {.obj_type_friendly {column_number}}. + ") } if (!is_number(line_number) || line_number < 1L) { - cli_abort(c( - i = "{.arg line_number} must be a positive integer.", - x = "Instead, it was {.val {line_number}}." - )) + cli_abort("{.arg line_number} must be a positive integer, not {.obj_type_friendly {line_number}}.") } check_ranges(ranges, max_col) @@ -449,25 +447,28 @@ is_valid_range <- function(range, max_col) { range[[2L]] <= max_col } -check_ranges <- function(ranges, max_col) { +check_ranges <- function(ranges, max_col, call = parent.frame()) { if (is.null(ranges)) { return() } if (!is.list(ranges)) { - cli_abort(c( - i = "{.arg ranges} must be {.code NULL} or a {.cls list}.", - x = "Instead, it was {.cls {class(ranges)}}." - )) + cli_abort( + "{.arg ranges} must be {.code NULL} or a list, not {.obj_type_friendly {ranges}}.", + call = call + ) } for (range in ranges) { if (!is_number(range, 2L)) { - cli_abort("{.arg ranges} must only contain {.cls integer} vectors of length 2 without {.code NA}s.") + cli_abort( + "{.arg ranges} must only contain integer vectors of length 2 without {.code NA}s.", + call = call + ) } else if (!is_valid_range(range, max_col)) { - cli_abort(c( - x = "Invalid range specified.", - i = "Argument {.arg ranges} must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 ({.val {max_col}})." - )) + cli_abort( + "{.arg ranges} must satisfy {.val {0}} <= range[1L] <= range[2L] <= {.val {max_col}} (nchar(line) + 1).", + call = call + ) } } } @@ -695,7 +696,7 @@ maybe_report_progress <- function(pb) { } maybe_append_error_lint <- function(lints, error, lint_cache, filename) { - if (inherits(error, "lint")) { + if (is_lint(error)) { error$linter <- "error" lints[[length(lints) + 1L]] <- error diff --git a/R/linter_tags.R b/R/linter_tags.R index c8cce3fec..451ddd565 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -82,11 +82,7 @@ available_linters <- function(packages = "lintr", tags = NULL, exclude_tags = "d } build_available_linters <- function(available, package, tags, exclude_tags) { - available_df <- data.frame( - linter = available[["linter"]], - package, - stringsAsFactors = FALSE - ) + available_df <- data.frame(linter = available[["linter"]], package) available_df$tags <- strsplit(available[["tags"]], split = " ", fixed = TRUE) if (!is.null(tags)) { matches_tags <- vapply(available_df$tags, function(linter_tags) any(linter_tags %in% tags), logical(1L)) @@ -108,7 +104,7 @@ build_available_linters <- function(available, package, tags, exclude_tags) { #' `data.frame` constructors don't handle zero-row list-columns properly, so supply `tags` afterwards. #' @noRd empty_linters <- function() { - empty_df <- data.frame(linter = character(), package = character(), stringsAsFactors = FALSE) + empty_df <- data.frame(linter = character(), package = character()) empty_df$tags <- list() empty_df } diff --git a/R/lintr-package.R b/R/lintr-package.R index 27f55bee3..d453bcab1 100644 --- a/R/lintr-package.R +++ b/R/lintr-package.R @@ -11,17 +11,12 @@ #' @importFrom cli cli_inform cli_abort cli_warn #' @importFrom glue glue glue_collapse #' @importFrom rex rex regex re_matches re_substitutes character_class -#' @importFrom stats na.omit +#' @importFrom stats complete.cases na.omit +#' @importFrom tools R_user_dir #' @importFrom utils capture.output getParseData getTxtProgressBar globalVariables head relist #' setTxtProgressBar tail txtProgressBar #' @importFrom xml2 as_list #' xml_attr xml_children xml_find_all xml_find_chr xml_find_lgl xml_find_num xml_find_first xml_name xml_text -#' @rawNamespace -#' if (getRversion() >= "4.0.0") { -#' importFrom(tools, R_user_dir) -#' } else { -#' importFrom(backports, R_user_dir) -#' } ## lintr namespace: end NULL diff --git a/R/methods.R b/R/methods.R index 4ff71c0a1..7a8fe3cc1 100644 --- a/R/methods.R +++ b/R/methods.R @@ -1,27 +1,16 @@ #' @export format.lint <- function(x, ..., width = getOption("lintr.format_width")) { - if (requireNamespace("cli", quietly = TRUE)) { - color <- switch(x$type, - warning = cli::col_magenta, - error = cli::col_red, - style = cli::col_blue, - cli::style_bold - ) - emph <- cli::style_bold - } else { - # nocov start - color <- identity - emph <- identity - # nocov end - } + color <- switch(x$type, + warning = cli::col_magenta, + error = cli::col_red, + style = cli::col_blue, + cli::style_bold + ) + emph <- cli::style_bold + line_ref <- build_line_ref(x) annotated_msg <- paste0( - emph( - x$filename, ":", - as.character(x$line_number), ":", - as.character(x$column_number), ": ", - sep = "" - ), + emph(line_ref, ": "), color(x$type, ": ", sep = ""), "[", x$linter, "] ", emph(x$message) @@ -40,6 +29,19 @@ format.lint <- function(x, ..., width = getOption("lintr.format_width")) { ) } +build_line_ref <- function(x) { + line_ref <- paste0( + x$filename, ":", + as.character(x$line_number), ":", + as.character(x$column_number) + ) + + if (!cli::ansi_has_hyperlink_support()) { + return(line_ref) + } + cli::format_inline("{.path {line_ref}}") +} + #' @export print.lint <- function(x, ...) { cat(format(x, ...)) @@ -92,7 +94,7 @@ print.lints <- function(x, ...) { inline_data <- x[[1L]][["filename"]] == "" if (!inline_data && use_rstudio_source_markers) { rstudio_source_markers(x) - } else if (in_github_actions()) { + } else if (in_github_actions() && !in_pkgdown()) { github_actions_log_lints(x, project_dir = github_annotation_project_dir) } else { lapply(x, print, ...) @@ -101,10 +103,14 @@ print.lints <- function(x, ...) { if (isTRUE(settings$error_on_lint)) { quit("no", 31L, FALSE) # nocov } - } else if (use_rstudio_source_markers) { - # Empty lints: clear RStudio source markers - rstudio_source_markers(x) + } else { + # Empty lints + cli_inform(c(i = "No lints found.")) + if (use_rstudio_source_markers) { + rstudio_source_markers(x) # clear RStudio source markers + } } + invisible(x) } @@ -163,8 +169,7 @@ as.data.frame.lints <- function(x, row.names = NULL, optional = FALSE, ...) { # type = vapply(x, `[[`, character(1L), "type"), message = vapply(x, `[[`, character(1L), "message"), line = vapply(x, `[[`, character(1L), "line"), - linter = vapply(x, `[[`, character(1L), "linter"), - stringsAsFactors = FALSE + linter = vapply(x, `[[`, character(1L), "linter") ) } @@ -198,7 +203,7 @@ summary.lints <- function(object, ...) { ) tbl <- table(filenames, types) filenames <- rownames(tbl) - res <- as.data.frame.matrix(tbl, stringsAsFactors = FALSE, row.names = NULL) + res <- as.data.frame.matrix(tbl, row.names = NULL) res$filenames <- filenames %||% character() nms <- colnames(res) res[order(res$filenames), c("filenames", nms[nms != "filenames"])] diff --git a/R/namespace.R b/R/namespace.R index a3fba3146..01ae56328 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -24,18 +24,18 @@ safe_get_exports <- function(ns) { # importFrom directives appear as list(ns, imported_funs) if (length(ns) > 1L) { - return(data.frame(pkg = ns[[1L]], fun = ns[[2L]], stringsAsFactors = FALSE)) + return(data.frame(pkg = ns[[1L]], fun = ns[[2L]])) } # relevant only if there are any exported objects fun <- getNamespaceExports(ns) if (length(fun) > 0L) { - data.frame(pkg = ns, fun = fun, stringsAsFactors = FALSE) + data.frame(pkg = ns, fun = fun) } } empty_namespace_data <- function() { - data.frame(pkg = character(), fun = character(), stringsAsFactors = FALSE) + data.frame(pkg = character(), fun = character()) } # filter namespace_imports() for S3 generics @@ -64,11 +64,7 @@ exported_s3_generics <- function(path = find_package(".")) { return(empty_namespace_data()) } - data.frame( - pkg = basename(path), - fun = unique(namespace_data$S3methods[, 1L]), - stringsAsFactors = FALSE - ) + data.frame(pkg = basename(path), fun = unique(namespace_data$S3methods[, 1L])) } is_s3_generic <- function(fun) { diff --git a/R/object_name_linter.R b/R/object_name_linter.R index 54cbb523d..ba9583039 100644 --- a/R/object_name_linter.R +++ b/R/object_name_linter.R @@ -146,13 +146,8 @@ object_name_linter <- function(styles = c("snake_case", "symbols"), regexes = ch } check_style <- function(nms, style, generics = character()) { - conforming <- re_matches(nms, style) + conforming <- re_matches_logical(nms, style) - # style has capture group(s) - if (is.data.frame(conforming)) { - # if any group is missing, all groups are missing, so just check the first column - conforming <- !is.na(conforming[[1L]]) - } # mark empty or NA names as conforming conforming <- is.na(nms) | !nzchar(nms) | conforming diff --git a/R/object_overwrite_linter.R b/R/object_overwrite_linter.R index 3c068503d..099129ed5 100644 --- a/R/object_overwrite_linter.R +++ b/R/object_overwrite_linter.R @@ -66,8 +66,7 @@ object_overwrite_linter <- function( ) pkg_exports <- data.frame( package = rep(packages, lengths(pkg_exports)), - name = unlist(pkg_exports), - stringsAsFactors = FALSE + name = unlist(pkg_exports) ) # Take the first among duplicate names, e.g. 'plot' resolves to base::plot, not graphics::plot diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 6b9466eff..1b7abce0b 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -148,8 +148,10 @@ get_assignment_symbols <- function(xml) { get_r_string(xml_find_all( xml, " - expr[LEFT_ASSIGN]/expr[1]/SYMBOL[1] | + expr[LEFT_ASSIGN or EQ_ASSIGN]/expr[1]/SYMBOL[1] | + expr[RIGHT_ASSIGN]/expr[2]/SYMBOL[1] | equal_assign/expr[1]/SYMBOL[1] | + expr_or_assign_or_help/expr[1]/SYMBOL[1] | expr[expr[1][SYMBOL_FUNCTION_CALL/text()='assign']]/expr[2]/* | expr[expr[1][SYMBOL_FUNCTION_CALL/text()='setMethod']]/expr[2]/* " diff --git a/R/path_utils.R b/R/path_utils.R index dd53d4316..f8ac2ac6f 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -133,6 +133,12 @@ split_path <- function(dirs, prefix) { dirs[nzchar(dirs)] } +#' Simple wrapper around normalizePath to ensure forward slash on Windows +#' https://github.com/r-lib/lintr/pull/2613 +#' @noRd +# nolint next: undesirable_function_linter, object_name_linter. +normalize_path <- function(path, mustWork = NA) normalizePath(path = path, winslash = "/", mustWork = mustWork) + #' @include utils.R path_linter_factory <- function(path_function, message, linter, name = linter_auto_name()) { force(name) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index 48d524c5b..d986dc184 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -19,6 +19,11 @@ #' linters = redundant_equals_linter() #' ) #' +#' lint( +#' text = "dt[is_tall == FALSE, y]", +#' linters = redundant_equals_linter() +#' ) +#' #' # okay #' lint( #' text = "if (any(x)) 1", @@ -30,6 +35,12 @@ #' linters = redundant_equals_linter() #' ) #' +#' # in `{data.table}` semantics, `dt[x]` is a join, `dt[(x)]` is a subset +#' lint( +#' text = "dt[(!is_tall), y]", +#' linters = redundant_equals_linter() +#' ) +#' #' @evalRd rd_tags("redundant_equals_linter") #' @seealso #' - [linters] for a complete list of linters available in lintr. diff --git a/R/return_linter.R b/R/return_linter.R index a0e1c245f..d0bc75d91 100644 --- a/R/return_linter.R +++ b/R/return_linter.R @@ -147,7 +147,8 @@ return_linter <- function( xml <- source_expression$xml_parsed_content if (defer_except) { assigned_functions <- xml_text(xml_find_all(xml, function_name_xpath)) - except <- union(except, assigned_functions[re_matches(assigned_functions, except_regex)]) + except <- + union(except, assigned_functions[re_matches_logical(assigned_functions, except_regex)]) except_xpath <- glue(except_xpath_fmt, except = except) body_xpath <- glue(body_xpath_fmt, except_xpath = except_xpath) } diff --git a/R/seq_linter.R b/R/seq_linter.R index decc02c66..d31b3dd08 100644 --- a/R/seq_linter.R +++ b/R/seq_linter.R @@ -7,7 +7,8 @@ #' 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. +#' Instead, it is safer to use [base::seq_len()] (to create a sequence of a specified *length*) or +#' [base::seq_along()] (to create a sequence *along* an object). #' #' @examples #' # will produce lints diff --git a/R/settings.R b/R/settings.R index 614d19767..475467335 100644 --- a/R/settings.R +++ b/R/settings.R @@ -12,7 +12,7 @@ #' This file is a DCF file, see [base::read.dcf()] for details. #' Here is an example of a `.lintr` file: #' -#' ```dcf +#' ``` #' linters: linters_with_defaults( #' any_duplicated_linter(), #' any_is_na_linter(), @@ -61,7 +61,8 @@ #' ``` #' #' @param filename Source file to be linted. -read_settings <- function(filename) { +#' @param call Passed to malformed to ensure linear trace. +read_settings <- function(filename, call = parent.frame()) { reset_settings() config_file <- find_config(filename) @@ -71,7 +72,7 @@ read_settings <- function(filename) { default_settings[["encoding"]] <- default_encoding } - config <- read_config_file(config_file) + config <- read_config_file(config_file, call = call) validate_config_file(config, config_file, default_settings) for (setting in names(default_settings)) { @@ -89,48 +90,53 @@ read_settings <- function(filename) { } } -read_config_file <- function(config_file) { +#' @param call Passed to malformed to ensure linear trace. +#' @noRd +read_config_file <- function(config_file, call = parent.frame()) { if (is.null(config_file)) { return(NULL) } + # clickable link for eventual error messages. + malformed_file <- link_config_file(config_file) # nolint: object_usage_linter. TODO(#2252). config <- new.env() if (endsWith(config_file, ".R")) { load_config <- function(file) sys.source(file, config, keep.source = FALSE, keep.parse.data = FALSE) malformed <- function(e) { - cli_abort(c( - "Malformed config file, ensure it is valid R syntax.", - conditionMessage(e) - )) + cli_abort( + "Malformed config file ({malformed_file}), ensure it is valid R syntax.", + parent = e, + call = call + ) } } else { load_config <- function(file) { dcf_values <- read.dcf(file, all = TRUE) for (setting in names(dcf_values)) { - parsed_setting <- tryCatch( + parsed_setting <- withCallingHandlers( str2lang(dcf_values[[setting]]), error = function(e) { - cli_abort(c( + cli_abort( "Malformed config setting {.field {setting}}:", - conditionMessage(e) - )) + parent = e + ) } ) setting_value <- withCallingHandlers( tryCatch( eval(parsed_setting), error = function(e) { - cli_abort(c( - "Error from config setting {.code {setting}} in {.code {format(conditionCall(e))}}:", - conditionMessage(e) - )) + cli_abort( + "Error from config setting {.code {setting}}.", + parent = e + ) } ), warning = function(w) { - cli_warn(c( - "Warning from config setting {.code {setting}} in {.code {format(conditionCall(w))}}:", - conditionMessage(w) - )) + cli_warn( + "Warning from config setting {.code {setting}}.", + parent = w + ) invokeRestart("muffleWarning") } ) @@ -138,10 +144,11 @@ read_config_file <- function(config_file) { } } malformed <- function(e) { - cli_abort(c( - x = "Malformed config file:", - i = conditionMessage(e) - )) + cli_abort( + "Malformed config file ({malformed_file}):", + parent = e, + call = call + ) } } withCallingHandlers( @@ -150,10 +157,10 @@ read_config_file <- function(config_file) { error = malformed ), warning = function(w) { - cli::cli_warn(c( - x = "Warning encountered while loading config:", - i = conditionMessage(w) - )) + cli::cli_warn( + "Warning encountered while loading config:", + parent = w + ) invokeRestart("muffleWarning") } ) @@ -164,7 +171,8 @@ validate_config_file <- function(config, config_file, defaults) { matched <- names(config) %in% names(defaults) if (!all(matched)) { unused_settings <- names(config)[!matched] # nolint: object_usage_linter. TODO(#2252). - cli_warn("Found unused settings in config {.str config_file}: {.field unused_settings}") + config_link <- link_config_file(config_file) # nolint: object_usage_linter. TODO(#2252). + cli_warn("Found unused settings in config file ({config_link}): {.field unused_settings}") } validate_regex(config, @@ -297,3 +305,10 @@ get_encoding_from_dcf <- function(file) { NULL } + +link_config_file <- function(path) { + cli::style_hyperlink( + cli::col_blue(basename(path)), + paste0("file://", path) + ) +} diff --git a/R/settings_utils.R b/R/settings_utils.R index 9489ea24e..02564efe5 100644 --- a/R/settings_utils.R +++ b/R/settings_utils.R @@ -8,7 +8,7 @@ has_rproj <- function(path) { } find_package <- function(path, allow_rproj = FALSE, max_depth = 2L) { - path <- normalizePath(path, mustWork = !allow_rproj) + path <- normalize_path(path, mustWork = !allow_rproj) if (allow_rproj) { found <- function(path) has_description(path) || has_rproj(path) } else { @@ -68,7 +68,7 @@ find_config <- function(filename) { dirname(filename) } - path <- normalizePath(path, mustWork = FALSE) + path <- normalize_path(path, mustWork = FALSE) # NB: This vector specifies a priority order for where to find the configs, # i.e. the first location where a config exists is chosen and configs which diff --git a/R/shared_constants.R b/R/shared_constants.R index 5f1371584..3dc20d615 100644 --- a/R/shared_constants.R +++ b/R/shared_constants.R @@ -115,7 +115,7 @@ get_token_replacement <- function(token_content, token_type) { # r_string gives the operator as you would write it in R code. # styler: off -infix_metadata <- data.frame(stringsAsFactors = FALSE, matrix(byrow = TRUE, ncol = 2L, c( +infix_metadata <- data.frame(matrix(byrow = TRUE, ncol = 2L, c( "OP-PLUS", "+", "OP-MINUS", "-", "OP-TILDE", "~", diff --git a/R/todo_comment_linter.R b/R/todo_comment_linter.R index 16e1de05f..5d6d94282 100644 --- a/R/todo_comment_linter.R +++ b/R/todo_comment_linter.R @@ -57,9 +57,9 @@ todo_comment_linter <- function(todo = c("todo", "fixme"), except_regex = NULL) comment_expr <- xml_find_all(xml, "//COMMENT") comment_text <- xml_text(comment_expr) - invalid_todo <- re_matches(comment_text, todo_comment_regex, ignore.case = TRUE) + invalid_todo <- re_matches_logical(comment_text, todo_comment_regex, ignore.case = TRUE) if (!is.null(valid_todo_regex)) { - invalid_todo <- invalid_todo & !re_matches(comment_text, valid_todo_regex) + invalid_todo <- invalid_todo & !re_matches_logical(comment_text, valid_todo_regex) } xml_nodes_to_lints( diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index 124b5a12f..f2e9f8d56 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -130,7 +130,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud drop_valid_comments <- function(expr, valid_comment_re) { is_valid_comment <- xml2::xml_name(expr) == "COMMENT" & - re_matches(xml_text(expr), valid_comment_re) + re_matches_logical(xml_text(expr), valid_comment_re) expr[!is_valid_comment] } diff --git a/R/use_lintr.R b/R/use_lintr.R index 16b33f279..73dc72e38 100644 --- a/R/use_lintr.R +++ b/R/use_lintr.R @@ -25,7 +25,7 @@ #' lintr::lint_dir() #' } use_lintr <- function(path = ".", type = c("tidyverse", "full")) { - config_file <- normalizePath(file.path(path, lintr_option("linter_file")), mustWork = FALSE) + config_file <- normalize_path(file.path(path, lintr_option("linter_file")), mustWork = FALSE) if (file.exists(config_file)) { cli_abort("Found an existing configuration file at {.file {config_file}}.") } diff --git a/R/utils.R b/R/utils.R index f6e709028..0820c9766 100644 --- a/R/utils.R +++ b/R/utils.R @@ -72,7 +72,7 @@ auto_names <- function(x) { if (is_linter(x)) { attr(x, "name", exact = TRUE) } else { - paste(deparse(x, 500L), collapse = " ") + deparse1(x) } } defaults <- vapply(x[empty], default_name, character(1L), USE.NAMES = FALSE) @@ -195,13 +195,23 @@ release_bullets <- function() { platform_independent_order <- function(x) order(tolower(x), method = "radix") platform_independent_sort <- function(x) x[platform_independent_order(x)] +#' re_matches with type-stable logical output +#' TODO(r-lib/rex#94): Use re_matches() option directly & deprecate this. +#' @noRd +re_matches_logical <- function(x, regex, ...) { + res <- re_matches(x, regex, ...) + if (is.data.frame(res)) { + res <- complete.cases(res) + } + res +} + #' Extract text from `STR_CONST` nodes #' #' Convert `STR_CONST` `text()` values into R strings. This is useful to account for arbitrary -#' character literals valid since R 4.0, e.g. `R"------[hello]------"`, which is parsed in -#' R as `"hello"`. It is quite cumbersome to write XPaths allowing for strings like this, -#' so whenever your linter logic requires testing a `STR_CONST` node's value, use this -#' function. +#' character literals, e.g. `R"------[hello]------"`, which is parsed in R as `"hello"`. +#' It is quite cumbersome to write XPaths allowing for strings like this, so whenever your +#' linter logic requires testing a `STR_CONST` node's value, use this function. #' NB: this is also properly vectorized on `s`, and accepts a variety of inputs. Empty inputs #' will become `NA` outputs, which helps ensure that `length(get_r_string(s)) == length(s)`. #' @@ -219,15 +229,14 @@ platform_independent_sort <- function(x) x[platform_independent_order(x)] #' get_r_string(expr_as_xml, "expr[3]") #' unlink(tmp) #' -#' # more importantly, extract strings under R>=4 raw strings -#' @examplesIf getRversion() >= "4.0.0" -#' tmp4.0 <- tempfile() -#' writeLines("c(R'(a\\b)', R'--[a\\\"\'\"\\b]--')", tmp4.0) -#' expr_as_xml4.0 <- get_source_expressions(tmp4.0)$expressions[[1L]]$xml_parsed_content -#' writeLines(as.character(expr_as_xml4.0)) -#' get_r_string(expr_as_xml4.0, "expr[2]") -#' get_r_string(expr_as_xml4.0, "expr[3]") -#' unlink(tmp4.0) +#' # more importantly, extract raw strings correctly +#' tmp_raw <- tempfile() +#' writeLines("c(R'(a\\b)', R'--[a\\\"\'\"\\b]--')", tmp_raw) +#' expr_as_xml_raw <- get_source_expressions(tmp_raw)$expressions[[1L]]$xml_parsed_content +#' writeLines(as.character(expr_as_xml_raw)) +#' get_r_string(expr_as_xml_raw, "expr[2]") +#' get_r_string(expr_as_xml_raw, "expr[3]") +#' unlink(tmp_raw) #' #' @export get_r_string <- function(s, xpath = NULL) { @@ -247,9 +256,12 @@ get_r_string <- function(s, xpath = NULL) { } is_linter <- function(x) inherits(x, "linter") +is_lint <- function(x) inherits(x, "lint") + +is_error <- function(x) inherits(x, "error") is_tainted <- function(lines) { - inherits(tryCatch(nchar(lines), error = identity), "error") + is_error(tryCatch(nchar(lines), error = identity)) } #' Check that the entries in ... are valid diff --git a/R/with.R b/R/with.R index f627f00ea..16d34fc7b 100644 --- a/R/with.R +++ b/R/with.R @@ -31,8 +31,11 @@ #' names(my_undesirable_functions) #' @export modify_defaults <- function(defaults, ...) { - if (missing(defaults) || !is.list(defaults) || !all(nzchar(names2(defaults)))) { - cli_abort("{.arg defaults} must be a named list.") + if (missing(defaults)) { + cli_abort("{.arg defaults} is a required argument, but is missing.") + } + if (!is.list(defaults) || !all(nzchar(names2(defaults)))) { + cli_abort("{.arg defaults} must be a named list, not {.obj_type_friendly {defaults}}.") } vals <- list(...) nms <- names2(vals) @@ -90,7 +93,7 @@ modify_defaults <- function(defaults, ...) { #' @export linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "deprecated") { if (!is.character(tags) && !is.null(tags)) { - cli_abort("{.arg tags} must be a {.cls character} vector, or {.code NULL}.") + cli_abort("{.arg tags} must be a character vector, or {.code NULL}, not {.obj_type_friendly {tags}}.") } tagged_linters <- list() @@ -102,8 +105,8 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep if (!all(available$linter %in% ns_exports)) { missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. TODO(#2252). cli_abort(c( - i = "Linters {.fn {missing_linters}} are advertised by {.fn available_linters}.", - x = "But these linters are not exported by package {.pkg {package}}." + x = "Can't find linters {.fn {missing_linters}}.", + i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } linter_factories <- mget(available$linter, envir = pkg_ns) @@ -178,9 +181,12 @@ linters_with_defaults <- function(..., defaults = default_linters) { dots <- list(...) if (missing(defaults) && "default" %in% names(dots)) { cli_warn(c( - "Did you mean {.arg defaults}?", - x = "{.arg default} is not an argument to {.fun linters_with_defaults}.", - i = "This warning will be removed when {.fun with_defaults} is fully deprecated." + x = " + {.arg default} is not an argument to {.help [{.fn linters_with_defaults}](lintr::linters_with_defaults)}. + ", + i = "Did you mean {.arg defaults}?", + # make message more subtle + cli::col_silver("This warning will be removed when {.fun with_defaults} is fully deprecated.") )) defaults <- dots$default nms <- names2(dots) @@ -207,10 +213,10 @@ call_linter_factory <- function(linter_factory, linter_name, package) { linter <- tryCatch( linter_factory(), error = function(e) { - cli_abort(c( + cli_abort( "Could not create linter with {.fun {package}::{linter_name}}.", - conditionMessage(e) - )) + parent = e + ) } ) # Otherwise, all linters would be called "linter_factory". diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index 9da6c450d..a893662bb 100644 --- a/R/xml_nodes_to_lints.R +++ b/R/xml_nodes_to_lints.R @@ -55,7 +55,7 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, } else if (!is_node(xml)) { cli_abort(c( x = "Expected an {.cls xml_nodeset}, a {.cls list} of xml_nodes, or an {.cls xml_node}.", - i = "Instead got an object of class(es): {.cls {class(xml)}}" + i = "Instead got {.obj_type_friendly {xml}}." )) } type <- match.arg(type, c("style", "warning", "error")) diff --git a/R/zzz.R b/R/zzz.R index 291539297..e281353db 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -168,9 +168,9 @@ default_undesirable_functions <- all_undesirable_functions[names(all_undesirable rd_auto_link <- function(x) { x <- unlist(x) - x <- gsub("([a-zA-Z0-9.]+)::([a-zA-Z0-9._]+)\\(\\)", "\\\\code{\\\\link[\\1:\\2]{\\1::\\2()}}", x) - x <- gsub("([^:a-zA-Z0-9._])([a-zA-Z0-9._]+)\\(\\)", "\\1\\\\code{\\\\link[=\\2]{\\2()}}", x) - x <- gsub("`([^`]+)`", "\\\\code{\\1}", x) + x <- gsub(R"{([a-zA-Z0-9.]+)::([a-zA-Z0-9._]+)\(\)}", R"(\\code{\\link[\1:\2]{\1::\2()}})", x) + x <- gsub(R"{([^:a-zA-Z0-9._])([a-zA-Z0-9._]+)\(\)}", R"(\1\\code{\\link[=\2]{\2()}})", x) + x <- gsub("`([^`]+)`", R"(\\code{\1})", x) x } @@ -181,7 +181,7 @@ rd_undesirable_functions <- function() { "The following functions are sometimes regarded as undesirable:", "\\itemize{", sprintf( - "\\item \\code{\\link[=%1$s]{%1$s()}} As an alternative, %2$s.", + R"(\item \code{\link[=%1$s]{%1$s()}} As an alternative, %2$s.)", names(default_undesirable_functions), alternatives ), "}" @@ -295,9 +295,8 @@ settings <- new.env(parent = emptyenv()) toset <- !(names(op_lintr) %in% names(op)) if (any(toset)) options(op_lintr[toset]) - # R>=4.0.0: deparse1 # R>=4.1.0: ...names - backports::import(pkgname, c("deparse1", "...names")) + backports::import(pkgname, "...names") utils::assignInMyNamespace("default_settings", list( linters = default_linters, diff --git a/_pkgdown.yaml b/_pkgdown.yaml index 8fd54393f..da3dbc60e 100644 --- a/_pkgdown.yaml +++ b/_pkgdown.yaml @@ -2,6 +2,7 @@ url: https://lintr.r-lib.org template: bootstrap: 5 + light-switch: true includes: in_header: | @@ -26,6 +27,10 @@ reference: contents: - ends_with("linter") + - title: Groups of linters + contents: + - ends_with("linters") + - title: Common default configurations contents: - all_undesirable_functions @@ -46,7 +51,6 @@ reference: - title: Meta-tooling contents: - - ends_with("linters") - Lint - checkstyle_output - sarif_output diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 95af98b61..1f070f45f 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -52,7 +52,7 @@ length_test_linter,common_mistakes efficiency lengths_linter,efficiency readability best_practices library_call_linter,style best_practices readability configurable line_length_linter,style readability default configurable -list_comparison_linter,best_practices common_mistakes efficiency +list_comparison_linter,best_practices common_mistakes literal_coercion_linter,best_practices consistency efficiency matrix_apply_linter,readability efficiency missing_argument_linter,correctness common_mistakes configurable @@ -108,8 +108,8 @@ terminal_close_linter,best_practices robustness todo_comment_linter,style configurable trailing_blank_lines_linter,style default trailing_whitespace_linter,style default configurable -undesirable_function_linter,style efficiency configurable robustness best_practices -undesirable_operator_linter,style efficiency configurable robustness best_practices +undesirable_function_linter,style configurable robustness best_practices +undesirable_operator_linter,style configurable robustness best_practices unnecessary_concatenation_linter,style readability efficiency configurable unnecessary_lambda_linter,best_practices efficiency readability configurable unnecessary_nested_if_linter,readability best_practices deprecated diff --git a/man/absolute_path_linter.Rd b/man/absolute_path_linter.Rd index f8b40a312..04fa1cf1a 100644 --- a/man/absolute_path_linter.Rd +++ b/man/absolute_path_linter.Rd @@ -18,9 +18,6 @@ If \code{TRUE}, only lint path strings, which Check that no absolute paths are used (e.g. "/var", "C:\\System", "~/docs"). } \examples{ -\dontshow{if (getRversion() >= "4.0") (if (getRversion() >= "3.4") withAutoprint else force)(\{ # examplesIf} -# Following examples use raw character constant syntax introduced in R 4.0. - # will produce lints lint( text = 'R"--[/blah/file.txt]--"', @@ -32,7 +29,7 @@ lint( text = 'R"(./blah)"', linters = absolute_path_linter() ) -\dontshow{\}) # examplesIf} + } \seealso{ \itemize{ diff --git a/man/efficiency_linters.Rd b/man/efficiency_linters.Rd index 37cb8b0d7..458375e27 100644 --- a/man/efficiency_linters.Rd +++ b/man/efficiency_linters.Rd @@ -22,7 +22,6 @@ The following linters are tagged with 'efficiency': \item{\code{\link{inner_combine_linter}}} \item{\code{\link{length_test_linter}}} \item{\code{\link{lengths_linter}}} -\item{\code{\link{list_comparison_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{matrix_apply_linter}}} \item{\code{\link{nested_ifelse_linter}}} @@ -38,8 +37,6 @@ The following linters are tagged with 'efficiency': \item{\code{\link{seq_linter}}} \item{\code{\link{sort_linter}}} \item{\code{\link{string_boundary_linter}}} -\item{\code{\link{undesirable_function_linter}}} -\item{\code{\link{undesirable_operator_linter}}} \item{\code{\link{unnecessary_concatenation_linter}}} \item{\code{\link{unnecessary_lambda_linter}}} \item{\code{\link{vector_logic_linter}}} diff --git a/man/get_r_string.Rd b/man/get_r_string.Rd index b1564c601..418d0f17c 100644 --- a/man/get_r_string.Rd +++ b/man/get_r_string.Rd @@ -15,10 +15,9 @@ and \code{xpath} is specified, it is extracted with \code{\link[xml2:xml_find_al } \description{ Convert \code{STR_CONST} \code{text()} values into R strings. This is useful to account for arbitrary -character literals valid since R 4.0, e.g. \code{R"------[hello]------"}, which is parsed in -R as \code{"hello"}. It is quite cumbersome to write XPaths allowing for strings like this, -so whenever your linter logic requires testing a \code{STR_CONST} node's value, use this -function. +character literals, e.g. \code{R"------[hello]------"}, which is parsed in R as \code{"hello"}. +It is quite cumbersome to write XPaths allowing for strings like this, so whenever your +linter logic requires testing a \code{STR_CONST} node's value, use this function. NB: this is also properly vectorized on \code{s}, and accepts a variety of inputs. Empty inputs will become \code{NA} outputs, which helps ensure that \code{length(get_r_string(s)) == length(s)}. } @@ -31,14 +30,13 @@ get_r_string(expr_as_xml, "expr[2]") get_r_string(expr_as_xml, "expr[3]") unlink(tmp) -# more importantly, extract strings under R>=4 raw strings -\dontshow{if (getRversion() >= "4.0.0") (if (getRversion() >= "3.4") withAutoprint else force)(\{ # examplesIf} -tmp4.0 <- tempfile() -writeLines("c(R'(a\\\\b)', R'--[a\\\\\"\'\"\\\\b]--')", tmp4.0) -expr_as_xml4.0 <- get_source_expressions(tmp4.0)$expressions[[1L]]$xml_parsed_content -writeLines(as.character(expr_as_xml4.0)) -get_r_string(expr_as_xml4.0, "expr[2]") -get_r_string(expr_as_xml4.0, "expr[3]") -unlink(tmp4.0) -\dontshow{\}) # examplesIf} +# more importantly, extract raw strings correctly +tmp_raw <- tempfile() +writeLines("c(R'(a\\\\b)', R'--[a\\\\\"\'\"\\\\b]--')", tmp_raw) +expr_as_xml_raw <- get_source_expressions(tmp_raw)$expressions[[1L]]$xml_parsed_content +writeLines(as.character(expr_as_xml_raw)) +get_r_string(expr_as_xml_raw, "expr[2]") +get_r_string(expr_as_xml_raw, "expr[3]") +unlink(tmp_raw) + } diff --git a/man/linters.Rd b/man/linters.Rd index 394bd6126..50f8baf7d 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -24,7 +24,7 @@ The following tags exist: \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=deprecated_linters]{deprecated} (6 linters)} -\item{\link[=efficiency_linters]{efficiency} (32 linters)} +\item{\link[=efficiency_linters]{efficiency} (29 linters)} \item{\link[=executing_linters]{executing} (6 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} @@ -88,7 +88,7 @@ The following linters exist: \item{\code{\link{lengths_linter}} (tags: best_practices, efficiency, readability)} \item{\code{\link{library_call_linter}} (tags: best_practices, configurable, readability, style)} \item{\code{\link{line_length_linter}} (tags: configurable, default, readability, style)} -\item{\code{\link{list_comparison_linter}} (tags: best_practices, common_mistakes, efficiency)} +\item{\code{\link{list_comparison_linter}} (tags: best_practices, common_mistakes)} \item{\code{\link{literal_coercion_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{matrix_apply_linter}} (tags: efficiency, readability)} \item{\code{\link{missing_argument_linter}} (tags: common_mistakes, configurable, correctness)} @@ -139,8 +139,8 @@ The following linters exist: \item{\code{\link{todo_comment_linter}} (tags: configurable, style)} \item{\code{\link{trailing_blank_lines_linter}} (tags: default, style)} \item{\code{\link{trailing_whitespace_linter}} (tags: configurable, default, style)} -\item{\code{\link{undesirable_function_linter}} (tags: best_practices, configurable, efficiency, robustness, style)} -\item{\code{\link{undesirable_operator_linter}} (tags: best_practices, configurable, efficiency, robustness, style)} +\item{\code{\link{undesirable_function_linter}} (tags: best_practices, configurable, robustness, style)} +\item{\code{\link{undesirable_operator_linter}} (tags: best_practices, configurable, robustness, style)} \item{\code{\link{unnecessary_concatenation_linter}} (tags: configurable, efficiency, readability, style)} \item{\code{\link{unnecessary_lambda_linter}} (tags: best_practices, configurable, efficiency, readability)} \item{\code{\link{unnecessary_nesting_linter}} (tags: best_practices, configurable, consistency, readability)} diff --git a/man/list_comparison_linter.Rd b/man/list_comparison_linter.Rd index 9a939a686..736e78256 100644 --- a/man/list_comparison_linter.Rd +++ b/man/list_comparison_linter.Rd @@ -29,5 +29,5 @@ lint( \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=common_mistakes_linters]{common_mistakes}, \link[=efficiency_linters]{efficiency} +\link[=best_practices_linters]{best_practices}, \link[=common_mistakes_linters]{common_mistakes} } diff --git a/man/read_settings.Rd b/man/read_settings.Rd index bc02121c1..84e53666e 100644 --- a/man/read_settings.Rd +++ b/man/read_settings.Rd @@ -4,10 +4,12 @@ \alias{read_settings} \title{Read lintr settings} \usage{ -read_settings(filename) +read_settings(filename, call = parent.frame()) } \arguments{ \item{filename}{Source file to be linted.} + +\item{call}{Passed to malformed to ensure linear trace.} } \description{ Lintr searches for settings for a given source file in the following order: @@ -25,7 +27,7 @@ or the environment variable \code{R_LINTR_LINTER_FILE} This file is a DCF file, see \code{\link[base:dcf]{base::read.dcf()}} for details. Here is an example of a \code{.lintr} file: -\if{html}{\out{
}}\preformatted{linters: linters_with_defaults( +\if{html}{\out{
}}\preformatted{linters: linters_with_defaults( any_duplicated_linter(), any_is_na_linter(), backport_linter("oldrel-4", except = c("R_user_dir", "str2lang")), diff --git a/man/redundant_equals_linter.Rd b/man/redundant_equals_linter.Rd index e901db9c8..3045cbc7b 100644 --- a/man/redundant_equals_linter.Rd +++ b/man/redundant_equals_linter.Rd @@ -26,6 +26,11 @@ lint( linters = redundant_equals_linter() ) +lint( + text = "dt[is_tall == FALSE, y]", + linters = redundant_equals_linter() +) + # okay lint( text = "if (any(x)) 1", @@ -37,6 +42,12 @@ lint( linters = redundant_equals_linter() ) +# in `{data.table}` semantics, `dt[x]` is a join, `dt[(x)]` is a subset +lint( + text = "dt[(!is_tall), y]", + linters = redundant_equals_linter() +) + } \seealso{ \itemize{ diff --git a/man/seq_linter.Rd b/man/seq_linter.Rd index 02c46af72..ece1e853c 100644 --- a/man/seq_linter.Rd +++ b/man/seq_linter.Rd @@ -15,7 +15,8 @@ conjunction with \code{seq()} (e.g., \code{seq(length(...))}, \code{seq(nrow(... Additionally, it checks for \code{1:n()} (from \code{{dplyr}}) and \code{1:.N} (from \code{{data.table}}). These often cause bugs when the right-hand side is zero. -It is safer to use \code{\link[base:seq]{base::seq_len()}} or \code{\link[base:seq]{base::seq_along()}} instead. +Instead, it is safer to use \code{\link[base:seq]{base::seq_len()}} (to create a sequence of a specified \emph{length}) or +\code{\link[base:seq]{base::seq_along()}} (to create a sequence \emph{along} an object). } \examples{ # will produce lints diff --git a/man/undesirable_function_linter.Rd b/man/undesirable_function_linter.Rd index 1a73f29ce..bfcbd947d 100644 --- a/man/undesirable_function_linter.Rd +++ b/man/undesirable_function_linter.Rd @@ -68,5 +68,5 @@ lint( \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=efficiency_linters]{efficiency}, \link[=robustness_linters]{robustness}, \link[=style_linters]{style} +\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=robustness_linters]{robustness}, \link[=style_linters]{style} } diff --git a/man/undesirable_operator_linter.Rd b/man/undesirable_operator_linter.Rd index 18bc5722b..2f1188116 100644 --- a/man/undesirable_operator_linter.Rd +++ b/man/undesirable_operator_linter.Rd @@ -52,5 +52,5 @@ lint( \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=efficiency_linters]{efficiency}, \link[=robustness_linters]{robustness}, \link[=style_linters]{style} +\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=robustness_linters]{robustness}, \link[=style_linters]{style} } diff --git a/paper/.gitignore b/paper/.gitignore new file mode 100644 index 000000000..075b2542a --- /dev/null +++ b/paper/.gitignore @@ -0,0 +1 @@ +/.quarto/ diff --git a/paper/apa.csl b/paper/apa.csl new file mode 100644 index 000000000..081857d9d --- /dev/null +++ b/paper/apa.csl @@ -0,0 +1,1916 @@ + + diff --git a/paper/paper.Rmd b/paper/paper.Rmd new file mode 100644 index 000000000..072cb1ef5 --- /dev/null +++ b/paper/paper.Rmd @@ -0,0 +1,255 @@ +--- +title: "Static Code Analysis for R" +date: "`r Sys.Date()`" +tags: ["R", "linter", "tidyverse"] +authors: + - name: Jim Hester + affiliation: 1 + orcid: 0000-0002-2739-7082 + - name: Florent Angly + affiliation: 6 + orcid: 0000-0002-8999-0738 + - name: Michael Chirico + affiliation: 2 + orcid: 0000-0003-0787-087X + - name: Russ Hyde + affiliation: 5 + orcid: ~ + - name: Ren Kun + affiliation: 7 + orcid: ~ + - name: Indrajeet Patil + orcid: 0000-0003-1995-6531 + affiliation: 4 + - name: Alexander Rosenstock + affiliation: 3 + orcid: ~ +affiliations: + - index: 1 + name: Netflix + - index: 2 + name: Google + - index: 3 + name: Mathematisches Institut der Heinrich-Heine-Universität Düsseldorf + - index: 4 + name: Preisenergie GmbH, Munich, Germany + - index: 5 + name: Jumping Rivers + - index: 6 + name: The University of Queensland + - index: 7 + name: Unknown +output: + md_document: + variant: "markdown" + preserve_yaml: true + standalone: true +bibliography: paper.bib +csl: apa.csl +link-citations: true +--- + +```{r setup, warning=FALSE, message=FALSE, echo=FALSE} +knitr::opts_chunk$set( + collapse = TRUE, + out.width = "100%", + comment = "#>" +) + +library(lintr) + +withr::local_options(list( + lintr.format_width = 60L, + cli.condition_unicode_bullets = FALSE +)) +``` + +# Statement of Need + +In computer programming, "linting" is the process of analyzing the source code to identify possible programming and stylistic problems [@enwiki:1260589258] and a linter is a tool used for linting. A linter analyzes code to identify potential errors, stylistic issues, or deviations from coding standards. It helps ensure consistency, readability, and best practices by flagging common mistakes, such as syntax errors, unused variables, or improper formatting. Linters are essential for improving code quality, preventing bugs, and maintaining a clean codebase, especially in collaborative development environments [@enwiki:1218663830]. `{lintr}` is an open-source package that provides linters for the R programming language, which is an interpreted, dynamically-typed programming language [@base2023], and is used by a wide range of researchers and data scientists. `{lintr}` can thus act as a valuable tool for R users to help improve the quality and reliability of their code. + +# Features + +By default, `{lintr}` enforces the tidyverse style guide [@Wickham2023,@Müller2024]. In this respect, it differs from other static code analysis tools in R (like `{codetools}` [@Tierney2024]), which are not opinionated and don't enforce any particular style of writing code, but, rather, check R code for possible problems (incidentally, `{lintr}` uses `{codetools}` as a backend for object usage linters). Additionally, `{lintr}` is concerned only with R code, so code-adjacent text like inline `{roxygen2}` comments [@Wickham2024roxy] will not be covered (cf. `{roxylint}` [@Kelkhoff2024]). + +As of this writing, `{lintr}` offers `r length(all_linters())` linters. + +```{r all_linters} +library(lintr) + +length(all_linters()) +``` + +Naturally, we can't discuss all of them here. To see the most up-to-date details about all the available linters, we encourage readers to visit . + +We will showcase one linter for each kind of common problem found in R code. + +- **Best practices** + +`{lintr}` offers linters that can detect problematic antipatterns and suggest alternatives that follow best practices. + +For example, expressions like `ifelse(x, TRUE, FALSE)` and `ifelse(x, FALSE, TRUE)` are redundant; just `x` or `!x` suffice in R code where logical vectors are a core data structure. The `redundant_ifelse_linter()` linter detects such discouraged usages. + +```{r redundant_ifelse_linter_with_lint} +lint( + text = "ifelse(x >= 2.5, TRUE, FALSE)", + linters = redundant_ifelse_linter() +) +``` + +```{r redundant_ifelse_linter_without_lint} +lint( + text = "x >= 2.5", + linters = redundant_ifelse_linter() +) +``` + +- **Efficiency** + +Sometimes users might not be aware of a more efficient way offered by R for carrying out a computation. `{lintr}` offers linters to improve code efficiency by avoiding common inefficient patterns. + +For example, the `any_is_na_linter()` linter detects usages of `any(is.na(x))` and suggests `anyNA(x)` as a more efficient alternative to detect presence of *any* missing values. + +```{r any_is_na_linter_with_lint} +lint( + text = "any(is.na(x), na.rm = TRUE)", + linters = any_is_na_linter() +) +``` + +`anyNA()` in R is more efficient than `any(is.na())` because it stops execution once a missing value is found, while `is.na()` evaluates the entire vector. + +```{r any_is_na_linter_without_lint} +lint( + text = "anyNA(x)", + linters = any_is_na_linter() +) +``` + +- **Readability** + +Coders spend significantly more time reading than writing code [@mcconnell2004code]. Thus, writing readable code makes the code more maintainable and reduces the possibility of introducing bugs stemming from a poor understanding of the code. + +`{lintr}` provides a number of linters that suggest more readable alternatives. For example, `comparison_negation_linter()` blocks usages like `!(x == y)` where a direct relational operator is appropriate. + +```{r comparison_negation_linter_with_lint} +lint( + text = "!x == 2", + linters = comparison_negation_linter() +) +``` + +Note also the complicated operator precedence. The more readable alternative here uses `!=`: + +```{r comparison_negation_linter_without_lint} +lint( + text = "x != 2", + linters = comparison_negation_linter() +) +``` + +- **Tidyverse style** + +`{lintr}` also provides linters to enforce the style used throughout the `{tidyverse}` [@Wickham2019] ecosystem of R packages. This style of coding has been outlined in the tidyverse style guide [@Wickham2023]. + +For example, the style guide recommends using snake_case for identifiers: + +```{r object_name_linter_with_lint} +lint( + text = "MyVar <- 1L", + linters = object_name_linter() +) +``` + +```{r object_name_linter_without_lint} +lint( + text = "my_var <- 1L", + linters = object_name_linter() +) +``` + +- **Common mistakes** + +One category of linters helps you detect some common mistakes statically and provide early feedback. + +For example, duplicate arguments in function calls can sometimes cause run-time errors: + +```{r duplicate_args_error_example, error=TRUE} +mean(x = 1:5, x = 2:3) +``` + +But `duplicate_argument_linter()` can check for this statically: + +```{r duplicate_argument_linter_with_lint} +lint( + text = "mean(x = 1:5, x = 2:3)", + linters = duplicate_argument_linter() +) +``` + +Even for cases where duplicate arguments are not an error, this linter explicitly discourages duplicate arguments. + +```{r duplicate_argument_linter_without_lint} +lint( + text = "list(x = TRUE, x = FALSE)", + linters = duplicate_argument_linter() +) +``` + +This is because objects with duplicated names objects can be hard to work with programmatically and should typically be avoided. + +```{r duplicate_arguments_example} +l <- list(x = TRUE, x = FALSE) +l["x"] +l[names(l) == "x"] +``` + +# Extensibility + +`{lintr}` is designed for extensibility by allowing users to easily create custom linting rules. +There are two main ways to customize it: + + - Use additional arguments in existing linters. For example, although tidyverse style guide prefers snake_case for identifiers, if a project's conventions require it, the relevant linter can be customized to support it: + + ```{r object_name_linter_with_custom_style} + lint( + text = "my.var <- 1L", + linters = object_name_linter(styles = "dotted.case") + ) + ``` + + - Create new linters (by leveraging functions like `lintr::make_linter_from_xpath()`) tailored to match project- or organization-specific coding standards. + +Indeed, `{goodpractice}` [@Padgham2024] bundles a set of custom linters that are not part of the default set of `{lintr}` linters, while `{box.linters}` [@Basa2024] extends `{lintr}` to support `{box}` modules [@Rudolph2024] and `{checklist}` includes linters as one of the strict checks for R packages [@Onkelinx2024]. `{flint}` [@Bacher2024] is a Rust-backed analogue inspired by `{lintr}` that also provides support for fixing lints. + +# Benefits of using `{lintr}` + +There are several benefits to using `{lintr}` to analyze and improve R code. One of the most obvious is that it can help users identify and fix problems in their code, which can save time and effort during the development process. By catching issues early on, `{lintr}` can help prevent bugs and other issues from creeping into code, which can save time and effort when it comes to debugging and testing. + +Another benefit of `{lintr}` is that it can help users write more readable and maintainable code. By enforcing a consistent style and highlighting potential issues, `{lintr}` can help users write code that is easier to understand and work with. This is especially important for larger projects or teams, where multiple contributors may be working on the same codebase and it is important to ensure that code is easy to follow and understand, particularly when frequently switching context among code primarily authored by different people. `{lintr}` is designed to be easy to use and integrate into existing workflows, and can be used as part of an automated build or continuous integration process. `{lintr}` also integrates with a number of popular IDEs and text editors, such as RStudio and Visual Studio Code, making it convenient for users to run `{lintr}` checks on their code as they work. + +It can also be a useful tool for teaching and learning R. By providing feedback on code style and potential issues, it can help users learn good coding practices and improve their skills over time. This can be especially useful for beginners, who may not yet be familiar with all of the best practices for writing R code. + +Finally, `{lintr}` has had a large and active user community since its birth in 2014 which has contributed to its rapid development, maintenance, and adoption. At the time of writing, `{lintr}` is in a mature and stable state and therefore provides a reliable API that is unlikely to feature fundamental breaking changes. + +# Conclusion + +`{lintr}` is a valuable tool for R users to help improve the quality and reliability of their code. Its static code analysis capabilities, combined with its flexibility and ease of use, make it relevant and valuable for a wide range of applications. + +# Licensing and Availability + +`{lintr}` is licensed under the MIT License, with all source code openly developed and stored on GitHub (), along with a corresponding issue tracker for bug reporting and feature enhancements. + +# Conflicts of interest + +The authors declare no conflict of interest. + +# Funding + +This work was not financially supported by any of the affiliated institutions of the authors. + +# Acknowledgments + +`{lintr}` would not be possible without the immense work of the [R-core team](https://www.r-project.org/contributors.html) who maintain the R language and we are deeply indebted to them. We are also grateful to all contributors to `{lintr}`. + +# References diff --git a/paper/paper.bib b/paper/paper.bib new file mode 100644 index 000000000..5b8e8db9e --- /dev/null +++ b/paper/paper.bib @@ -0,0 +1,125 @@ +@Article{Wickham2019, + title = {Welcome to the {tidyverse}}, + author = {Hadley Wickham and Mara Averick and Jennifer Bryan and Winston Chang and Lucy D'Agostino McGowan and Romain François and Garrett Grolemund and Alex Hayes and Lionel Henry and Jim Hester and Max Kuhn and Thomas Lin Pedersen and Evan Miller and Stephan Milton Bache and Kirill Müller and Jeroen Ooms and David Robinson and Dana Paige Seidel and Vitalie Spinu and Kohske Takahashi and Davis Vaughan and Claus Wilke and Kara Woo and Hiroaki Yutani}, + year = {2019}, + journal = {Journal of Open Source Software}, + volume = {4}, + number = {43}, + pages = {1686}, + doi = {10.21105/joss.01686}, + } + +@Manual{Wickham2023, + title = {The Tidyverse Style Guide}, + author = {Hadley Wickham}, + year = {2023}, + url = {https://style.tidyverse.org/index.html}, + } + +@Manual{base2023, + title = {{R}: A Language and Environment for Statistical Computing}, + author = {{R Core Team}}, + organization = {R Foundation for Statistical Computing}, + address = {Vienna, Austria}, + year = {2023}, + url = {https://www.R-project.org/}, + } + +@book{mcconnell2004code, + title={Code Complete}, + author={McConnell, Steve}, + year={2004}, + publisher={Pearson Education} + } + +@misc{enwiki:1218663830, + author = "{Wikipedia contributors}", + title = "Static program analysis --- {Wikipedia}{,} The Free Encyclopedia", + year = "2024", + url = "https://en.wikipedia.org/w/index.php?title=Static_program_analysis&oldid=1218663830", + note = "[Online; accessed 2-December-2024]" + } + +@misc{enwiki:1260589258, + author = "{Wikipedia contributors}", + title = "Lint (software) --- {Wikipedia}{,} The Free Encyclopedia", + year = "2024", + url = "https://en.wikipedia.org/w/index.php?title=Lint_(software)&oldid=1260589258", + note = "[Online; accessed 2-December-2024]" + } + +@Manual{Tierney2024, + title = {codetools: Code Analysis Tools for R}, + author = {Luke Tierney}, + year = {2024}, + note = {R package version 0.2-20}, + url = {https://CRAN.R-project.org/package=codetools}, + } + +@Manual{Bacher2024, + title = {flint: Find and Fix Lints in {R} Code}, + author = {Etienne Bacher}, + year = {2024}, + note = {R package version 0.0.2, +https://github.com/etiennebacher/flint}, + url = {https://flint.etiennebacher.com}, + } + +@Manual{Müller2024, + title = {styler: Non-Invasive Pretty Printing of R Code}, + author = {Kirill Müller and Lorenz Walthert and Indrajeet Patil}, + year = {2024}, + note = {R package version 1.10.3.9000, commit 6d2f0b34245b6bc712bf2fcabf240d9ca814f0ef}, + url = {https://github.com/r-lib/styler}, + } + +@Manual{Padgham2024, + title = {goodpractice: Advice on R Package Building}, + author = {Mark Padgham and Karina Marks and Daniel {de Bortoli} and Gabor Csardi and Hannah Frick and Owen Jones and Hannah Alexander}, + year = {2024}, + note = {R package version 1.0.5, +https://github.com/ropensci-review-tools/goodpractice}, + url = {https://docs.ropensci.org/goodpractice/}, + } + +@Manual{Rudolph2024, + title = {box: Write Reusable, Composable and Modular {R} Code}, + author = {Konrad Rudolph}, + year = {2024}, + note = {R package version 1.2.0}, + url = {https://CRAN.R-project.org/package=box}, + } + +@Manual{Basa2024, + title = {box.linters: Linters for 'box' Modules}, + author = {Ricardo Rodrigo Basa and Jakub Nowicki}, + year = {2024}, + note = {R package version 0.10.5}, + url = {https://CRAN.R-project.org/package=box.linters}, + } + +@Manual{Kelkhoff2024, + title = {roxylint: Lint 'roxygen2'-Generated Documentation}, + author = {Doug Kelkhoff}, + year = {2024}, + note = {R package version 0.1.0}, + url = {https://CRAN.R-project.org/package=roxylint}, + } + +@Manual{Wickham2024roxy, + title = {roxygen2: In-Line Documentation for {R}}, + author = {Hadley Wickham and Peter Danenberg and Gábor Csárdi and Manuel Eugster}, + year = {2024}, + note = {R package version 7.3.2}, + url = {https://CRAN.R-project.org/package=roxygen2}, + } + +@Manual{Onkelinx2024, + title = {checklist: A Thorough and Strict Set of Checks for {R} Packages and Source Code. Version 0.4.0}, + author = {Thierry Onkelinx}, + year = {2024}, + url = {https://inbo.github.io/checklist/}, + abstract = {An opinionated set of rules for R packages and R source code projects.}, + keywords = {quality control; documentation; publication}, + doi = {10.5281/zenodo.4028303}, + } diff --git a/paper/paper.md b/paper/paper.md new file mode 100644 index 000000000..c7a35d658 --- /dev/null +++ b/paper/paper.md @@ -0,0 +1,369 @@ +--- +title: "Static Code Analysis for R" +date: "2024-12-02" +tags: ["R", "linter", "tidyverse"] +authors: + - name: Jim Hester + affiliation: 1 + orcid: 0000-0002-2739-7082 + - name: Florent Angly + affiliation: 6 + orcid: 0000-0002-8999-0738 + - name: Michael Chirico + affiliation: 2 + orcid: 0000-0003-0787-087X + - name: Russ Hyde + affiliation: 5 + orcid: ~ + - name: Ren Kun + affiliation: 7 + orcid: ~ + - name: Indrajeet Patil + orcid: 0000-0003-1995-6531 + affiliation: 4 + - name: Alexander Rosenstock + affiliation: 3 + orcid: ~ +affiliations: + - index: 1 + name: Netflix + - index: 2 + name: Google + - index: 3 + name: Mathematisches Institut der Heinrich-Heine-Universität Düsseldorf + - index: 4 + name: Preisenergie GmbH, Munich, Germany + - index: 5 + name: Jumping Rivers + - index: 6 + name: The University of Queensland + - index: 7 + name: Unknown +output: + md_document: + variant: "markdown" + preserve_yaml: true + standalone: true +bibliography: paper.bib +csl: apa.csl +link-citations: true +--- + +# Statement of Need + +In computer programming, "linting" is the process of analyzing the +source code to identify possible programming and stylistic problems +[@enwiki:1260589258] and a linter is a tool used for linting. A linter +analyzes code to identify potential errors, stylistic issues, or +deviations from coding standards. It helps ensure consistency, +readability, and best practices by flagging common mistakes, such as +syntax errors, unused variables, or improper formatting. Linters are +essential for improving code quality, preventing bugs, and maintaining a +clean codebase, especially in collaborative development environments +[@enwiki:1218663830]. `{lintr}` is an open-source package that provides +linters for the R programming language, which is an interpreted, +dynamically-typed programming language [@base2023], and is used by a +wide range of researchers and data scientists. `{lintr}` can thus act as +a valuable tool for R users to help improve the quality and reliability +of their code. + +# Features + +By default, `{lintr}` enforces the tidyverse style guide +[@Wickham2023,@Müller2024]. In this respect, it differs from other +static code analysis tools in R (like `{codetools}` [@Tierney2024]), +which are not opinionated and don't enforce any particular style of +writing code, but, rather, check R code for possible problems +(incidentally, `{lintr}` uses `{codetools}` as a backend for object +usage linters). Additionally, `{lintr}` is concerned only with R code, +so code-adjacent text like inline `{roxygen2}` comments +[@Wickham2024roxy] will not be covered (cf. `{roxylint}` +[@Kelkhoff2024]). + +As of this writing, `{lintr}` offers 113 linters. + +``` r +library(lintr) + +length(all_linters()) +#> [1] 113 +``` + +Naturally, we can't discuss all of them here. To see the most up-to-date +details about all the available linters, we encourage readers to visit +. + +We will showcase one linter for each kind of common problem found in R +code. + +- **Best practices** + +`{lintr}` offers linters that can detect problematic antipatterns and +suggest alternatives that follow best practices. + +For example, expressions like `ifelse(x, TRUE, FALSE)` and +`ifelse(x, FALSE, TRUE)` are redundant; just `x` or `!x` suffice in R +code where logical vectors are a core data structure. The +`redundant_ifelse_linter()` linter detects such discouraged usages. + +``` r +lint( + text = "ifelse(x >= 2.5, TRUE, FALSE)", + linters = redundant_ifelse_linter() +) +#> :1:1: warning: [redundant_ifelse_linter] Just use the +#> logical condition (or its negation) directly instead of +#> calling ifelse(x, TRUE, FALSE) +#> ifelse(x >= 2.5, TRUE, FALSE) +#> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +``` + +``` r +lint( + text = "x >= 2.5", + linters = redundant_ifelse_linter() +) +``` + +- **Efficiency** + +Sometimes users might not be aware of a more efficient way offered by R +for carrying out a computation. `{lintr}` offers linters to improve code +efficiency by avoiding common inefficient patterns. + +For example, the `any_is_na_linter()` linter detects usages of +`any(is.na(x))` and suggests `anyNA(x)` as a more efficient alternative +to detect presence of *any* missing values. + +``` r +lint( + text = "any(is.na(x), na.rm = TRUE)", + linters = any_is_na_linter() +) +#> :1:1: warning: [any_is_na_linter] anyNA(x) is better +#> than any(is.na(x)). +#> any(is.na(x), na.rm = TRUE) +#> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ +``` + +`anyNA()` in R is more efficient than `any(is.na())` because it stops +execution once a missing value is found, while `is.na()` evaluates the +entire vector. + +``` r +lint( + text = "anyNA(x)", + linters = any_is_na_linter() +) +``` + +- **Readability** + +Coders spend significantly more time reading than writing code +[@mcconnell2004code]. Thus, writing readable code makes the code more +maintainable and reduces the possibility of introducing bugs stemming +from a poor understanding of the code. + +`{lintr}` provides a number of linters that suggest more readable +alternatives. For example, `comparison_negation_linter()` blocks usages +like `!(x == y)` where a direct relational operator is appropriate. + +``` r +lint( + text = "!x == 2", + linters = comparison_negation_linter() +) +#> :1:1: warning: [comparison_negation_linter] Use x != +#> y, not !(x == y). +#> !x == 2 +#> ^~~~~~~ +``` + +Note also the complicated operator precedence. The more readable +alternative here uses `!=`: + +``` r +lint( + text = "x != 2", + linters = comparison_negation_linter() +) +``` + +- **Tidyverse style** + +`{lintr}` also provides linters to enforce the style used throughout the +`{tidyverse}` [@Wickham2019] ecosystem of R packages. This style of +coding has been outlined in the tidyverse style guide [@Wickham2023]. + +For example, the style guide recommends using snake_case for +identifiers: + +``` r +lint( + text = "MyVar <- 1L", + linters = object_name_linter() +) +#> :1:1: style: [object_name_linter] Variable and +#> function name style should match snake_case or symbols. +#> MyVar <- 1L +#> ^~~~~ +``` + +``` r +lint( + text = "my_var <- 1L", + linters = object_name_linter() +) +``` + +- **Common mistakes** + +One category of linters helps you detect some common mistakes statically +and provide early feedback. + +For example, duplicate arguments in function calls can sometimes cause +run-time errors: + +``` r +mean(x = 1:5, x = 2:3) +#> Error in mean(x = 1:5, x = 2:3): formal argument "x" matched by multiple actual arguments +``` + +But `duplicate_argument_linter()` can check for this statically: + +``` r +lint( + text = "mean(x = 1:5, x = 2:3)", + linters = duplicate_argument_linter() +) +#> :1:15: warning: [duplicate_argument_linter] Avoid +#> duplicate arguments in function calls. +#> mean(x = 1:5, x = 2:3) +#> ^ +``` + +Even for cases where duplicate arguments are not an error, this linter +explicitly discourages duplicate arguments. + +``` r +lint( + text = "list(x = TRUE, x = FALSE)", + linters = duplicate_argument_linter() +) +#> :1:16: warning: [duplicate_argument_linter] Avoid +#> duplicate arguments in function calls. +#> list(x = TRUE, x = FALSE) +#> ^ +``` + +This is because objects with duplicated names objects can be hard to +work with programmatically and should typically be avoided. + +``` r +l <- list(x = TRUE, x = FALSE) +l["x"] +#> $x +#> [1] TRUE +l[names(l) == "x"] +#> $x +#> [1] TRUE +#> +#> $x +#> [1] FALSE +``` + +# Extensibility + +`{lintr}` is designed for extensibility by allowing users to easily +create custom linting rules. There are two main ways to customize it: + +- Use additional arguments in existing linters. For example, although + tidyverse style guide prefers snake_case for identifiers, if a + project's conventions require it, the relevant linter can be + customized to support it: + +``` r +lint( + text = "my.var <- 1L", + linters = object_name_linter(styles = "dotted.case") +) +``` + +- Create new linters (by leveraging functions like + `lintr::make_linter_from_xpath()`) tailored to match project- or + organization-specific coding standards. + +Indeed, `{goodpractice}` [@Padgham2024] bundles a set of custom linters +that are not part of the default set of `{lintr}` linters, while +`{box.linters}` [@Basa2024] extends `{lintr}` to support `{box}` modules +[@Rudolph2024] and `{checklist}` includes linters as one of the strict +checks for R packages [@Onkelinx2024]. `{flint}` [@Bacher2024] is a +Rust-backed analogue inspired by `{lintr}` that also provides support +for fixing lints. + +# Benefits of using `{lintr}` + +There are several benefits to using `{lintr}` to analyze and improve R +code. One of the most obvious is that it can help users identify and fix +problems in their code, which can save time and effort during the +development process. By catching issues early on, `{lintr}` can help +prevent bugs and other issues from creeping into code, which can save +time and effort when it comes to debugging and testing. + +Another benefit of `{lintr}` is that it can help users write more +readable and maintainable code. By enforcing a consistent style and +highlighting potential issues, `{lintr}` can help users write code that +is easier to understand and work with. This is especially important for +larger projects or teams, where multiple contributors may be working on +the same codebase and it is important to ensure that code is easy to +follow and understand, particularly when frequently switching context +among code primarily authored by different people. `{lintr}` is designed +to be easy to use and integrate into existing workflows, and can be used +as part of an automated build or continuous integration process. +`{lintr}` also integrates with a number of popular IDEs and text +editors, such as RStudio and Visual Studio Code, making it convenient +for users to run `{lintr}` checks on their code as they work. + +It can also be a useful tool for teaching and learning R. By providing +feedback on code style and potential issues, it can help users learn +good coding practices and improve their skills over time. This can be +especially useful for beginners, who may not yet be familiar with all of +the best practices for writing R code. + +Finally, `{lintr}` has had a large and active user community since its +birth in 2014 which has contributed to its rapid development, +maintenance, and adoption. At the time of writing, `{lintr}` is in a +mature and stable state and therefore provides a reliable API that is +unlikely to feature fundamental breaking changes. + +# Conclusion + +`{lintr}` is a valuable tool for R users to help improve the quality and +reliability of their code. Its static code analysis capabilities, +combined with its flexibility and ease of use, make it relevant and +valuable for a wide range of applications. + +# Licensing and Availability + +`{lintr}` is licensed under the MIT License, with all source code openly +developed and stored on GitHub (), along +with a corresponding issue tracker for bug reporting and feature +enhancements. + +# Conflicts of interest + +The authors declare no conflict of interest. + +# Funding + +This work was not financially supported by any of the affiliated +institutions of the authors. + +# Acknowledgments + +`{lintr}` would not be possible without the immense work of the [R-core +team](https://www.r-project.org/contributors.html) who maintain the R +language and we are deeply indebted to them. We are also grateful to all +contributors to `{lintr}`. + +# References {#references .unnumbered} diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index d21c33f22..be5481f7b 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -2,7 +2,7 @@ test_that("Lint() errors on invalid input", { dummy_line <- "abc" expect_error( Lint("dummy.R", line = dummy_line, column_number = NA_integer_), - "`column_number` must be an integer between 0 and `nchar(line) + 1` (4)", + "`column_number` must be an integer between 0 and 4 (`nchar(line) + 1`)", fixed = TRUE ) expect_error( @@ -12,22 +12,22 @@ test_that("Lint() errors on invalid input", { ) expect_error( Lint("dummy.R", ranges = c(1L, 3L)), - "`ranges` must be `NULL` or a ", + "`ranges` must be `NULL` or a list", fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(1L)), - "`ranges` must only contain vectors of length 2 without `NA`s.", + "`ranges` must only contain integer vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(c(1L, NA_integer_))), - "`ranges` must only contain vectors of length 2 without `NA`s.", + "`ranges` must only contain integer vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( Lint("dummy.R", line = dummy_line, ranges = list(c(1L, 2L), c(1L, 5L))), - "Argument `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).", + "`ranges` must satisfy 0 <= range[1L] <= range[2L] <= 4 (nchar(line) + 1).", fixed = TRUE ) }) diff --git a/tests/testthat/test-cache.R b/tests/testthat/test-cache.R index 62a0ed32c..a0024708b 100644 --- a/tests/testthat/test-cache.R +++ b/tests/testthat/test-cache.R @@ -437,7 +437,7 @@ test_that("cache = TRUE workflow works", { # Need a test structure with a safe to load .lintr withr::local_dir(file.path("dummy_packages", "package")) withr::local_options(lintr.linter_file = "lintr_test_config") - files <- normalizePath(list.files(recursive = TRUE, full.names = TRUE)) + files <- normalize_path(list.files(recursive = TRUE, full.names = TRUE)) # Manually clear cache (that function is exported) for (f in files) { diff --git a/tests/testthat/test-ci.R b/tests/testthat/test-ci.R index 3f4f98762..40401e934 100644 --- a/tests/testthat/test-ci.R +++ b/tests/testthat/test-ci.R @@ -1,5 +1,5 @@ test_that("GitHub Actions functionality works", { - withr::local_envvar(list(GITHUB_ACTIONS = "true")) + withr::local_envvar(list(GITHUB_ACTIONS = "true", IN_PKGDOWN = "false")) withr::local_options(lintr.rstudio_source_markers = FALSE) tmp <- withr::local_tempfile(lines = "x <- 1:nrow(y)") @@ -26,7 +26,7 @@ test_that("GitHub Actions functionality works in a subdirectory", { patrick::with_parameters_test_that( "GitHub Actions - error on lint works", { - withr::local_envvar(list(GITHUB_ACTIONS = "true", LINTR_ERROR_ON_LINT = env_var_value)) + withr::local_envvar(list(GITHUB_ACTIONS = "true", IN_PKGDOWN = "", LINTR_ERROR_ON_LINT = env_var_value)) withr::local_options(lintr.rstudio_source_markers = FALSE) tmp <- withr::local_tempfile(lines = "x <- 1:nrow(y)") @@ -51,3 +51,12 @@ patrick::with_parameters_test_that( }, env_var_value = list("", "F", NA, NULL) ) + +test_that("GitHub Actions log is skipped in pkgdown websites", { + withr::local_envvar(list(GITHUB_ACTIONS = "true", IN_PKGDOWN = "true")) + withr::local_options(lintr.rstudio_source_markers = FALSE) + tmp <- withr::local_tempfile(lines = "x <- 1:nrow(y)") + + l <- lint(tmp, linters = seq_linter()) + expect_output(print(l), "warning: [seq_linter]", fixed = TRUE) +}) diff --git a/tests/testthat/test-class_equals_linter.R b/tests/testthat/test-class_equals_linter.R index fb640e448..cc4495ec3 100644 --- a/tests/testthat/test-class_equals_linter.R +++ b/tests/testthat/test-class_equals_linter.R @@ -11,7 +11,7 @@ test_that("class_equals_linter skips allowed usages", { test_that("class_equals_linter blocks simple disallowed usages", { linter <- class_equals_linter() - lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')") + lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. for S3 classes, or is(x, 'S4Class') for S4 classes") expect_lint("if (class(x) == 'character') stop('no')", lint_msg, linter) expect_lint("is_regression <- class(x) == 'lm'", lint_msg, linter) @@ -20,7 +20,7 @@ test_that("class_equals_linter blocks simple disallowed usages", { test_that("class_equals_linter blocks usage of %in% for checking class", { linter <- class_equals_linter() - lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')") + lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. for S3 classes, or is(x, 'S4Class') for S4 classes") expect_lint("if ('character' %in% class(x)) stop('no')", lint_msg, linter) expect_lint("if (class(x) %in% 'character') stop('no')", lint_msg, linter) @@ -29,7 +29,7 @@ test_that("class_equals_linter blocks usage of %in% for checking class", { test_that("class_equals_linter blocks class(x) != 'klass'", { expect_lint( "if (class(x) != 'character') TRUE", - rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')"), + rex::rex("Use inherits(x, 'class-name'), is. for S3 classes, or is(x, 'S4Class') for S4 classes"), class_equals_linter() ) }) @@ -43,13 +43,13 @@ test_that("class_equals_linter skips usage for subsetting", { # but not further nesting expect_lint( "x[if (class(x) == 'foo') 1 else 2]", - rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')"), + rex::rex("Use inherits(x, 'class-name'), is. for S3 classes, or is(x, 'S4Class') for S4 classes"), linter ) }) test_that("lints vectorize", { - lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')") + lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. for S3 classes, or is(x, 'S4Class') for S4 classes") expect_lint( trim_some("{ diff --git a/tests/testthat/test-commented_code_linter.R b/tests/testthat/test-commented_code_linter.R index fadc98d5e..5154a5a94 100644 --- a/tests/testthat/test-commented_code_linter.R +++ b/tests/testthat/test-commented_code_linter.R @@ -103,3 +103,13 @@ test_that("commented_code_linter can detect operators in comments and lint corre commented_code_linter() ) }) + +test_that("commented_code_linter can detect commented code ending with pipes", { + linter <- commented_code_linter() + lint_msg <- rex::rex("Remove commented code.") + + expect_lint("# f() %>%", lint_msg, linter) + + skip_if_not_r_version("4.1.0") + expect_lint("# f() |>", lint_msg, linter) +}) diff --git a/tests/testthat/test-expect_lint.R b/tests/testthat/test-expect_lint.R index ea23dce34..f03d7e7a3 100644 --- a/tests/testthat/test-expect_lint.R +++ b/tests/testthat/test-expect_lint.R @@ -30,7 +30,7 @@ test_that("single check", { expect_success(expect_lint("a=1", list(message = lint_msg, line_number = 1L), linter)) expect_failure(expect_lint("a=1", list(2L, lint_msg), linter)) - expect_error(expect_lint("1:nrow(x)", "(group)", seq_linter()), "Invalid regex result", fixed = TRUE) + expect_success(expect_lint("1:nrow(x)", "(nrow)", seq_linter())) }) test_that("multiple checks", { diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R index 371848200..98a279a02 100644 --- a/tests/testthat/test-fixed_regex_linter.R +++ b/tests/testthat/test-fixed_regex_linter.R @@ -1,9 +1,3 @@ -# NB: escaping is confusing. We have to double-escape everything -- the first -# escape creates a string that will be parse()d, the second escape is normal -# escaping that would be done in R code. E.g. in "\\\\.", the R code would -# read like "\\.", but in order to create those two slashes, we need to write -# "\\\\." in the string here. - test_that("fixed_regex_linter skips allowed usages", { linter <- fixed_regex_linter() @@ -11,15 +5,15 @@ test_that("fixed_regex_linter skips allowed usages", { expect_lint("grep('x$', '', y)", NULL, linter) expect_lint("sub('[a-zA-Z]', '', y)", NULL, linter) expect_lint("grepl(fmt, y)", NULL, linter) - expect_lint("regexec('\\\\s', '', y)", NULL, linter) + expect_lint(R"{regexec('\\s', '', y)}", NULL, linter) expect_lint("grep('a(?=b)', x, perl = TRUE)", NULL, linter) expect_lint("grep('0+1', x, perl = TRUE)", NULL, linter) expect_lint("grep('1*2', x)", NULL, linter) expect_lint("grep('a|b', x)", NULL, linter) - expect_lint("grep('\\\\[|\\\\]', x)", NULL, linter) + expect_lint(R"{grep('\\[|\\]', x)}", NULL, linter) # if fixed=TRUE is already set, regex patterns don't matter - expect_lint("gsub('\\\\.', '', y, fixed = TRUE)", NULL, linter) + expect_lint(R"{gsub('\\.', '', y, fixed = TRUE)}", NULL, linter) # ignore.case=TRUE implies regex interpretation expect_lint("gsub('abcdefg', '', y, ignore.case = TRUE)", NULL, linter) @@ -37,10 +31,10 @@ test_that("fixed_regex_linter blocks simple disallowed usages", { linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("gsub('\\\\.', '', x)", lint_msg, linter) + expect_lint(R"{gsub('\\.', '', x)}", lint_msg, linter) expect_lint("grepl('abcdefg', x)", lint_msg, linter) expect_lint("gregexpr('a-z', y)", lint_msg, linter) - expect_lint("regexec('\\\\$', x)", lint_msg, linter) + expect_lint(R"{regexec('\\$', x)}", lint_msg, linter) expect_lint("grep('\n', x)", lint_msg, linter) # naming the argument doesn't matter (if it's still used positionally) @@ -51,13 +45,13 @@ patrick::with_parameters_test_that( "fixed_regex_linter is robust to unrecognized escapes error", { expect_lint( - sprintf("grep('\\\\%s', x)", char), + sprintf(R"{grep('\\%s', x)}", char), rex::rex("This regular expression is static"), fixed_regex_linter() ) expect_lint( - sprintf("strsplit('a%sb', '\\\\%s')", char, char), + sprintf(R"{strsplit('a%sb', '\\%s')}", char, char), rex::rex("This regular expression is static"), fixed_regex_linter() ) @@ -65,10 +59,10 @@ patrick::with_parameters_test_that( .cases = local({ char <- c( "^", "$", "{", "}", "(", ")", ".", "*", "+", "?", - "|", "[", "]", "\\\\", "<", ">", "=", ":", ";", "/", + "|", "[", "]", R"(\\)", "<", ">", "=", ":", ";", "/", "_", "-", "!", "@", "#", "%", "&", "~" ) - data.frame(char = char, .test_name = char, stringsAsFactors = FALSE) + data.frame(char = char, .test_name = char) }) ) @@ -87,7 +81,7 @@ test_that("fixed_regex_linter catches null calls to strsplit as well", { linter <- fixed_regex_linter() expect_lint("strsplit(x, '^x')", NULL, linter) - expect_lint("strsplit(x, '\\\\s')", NULL, linter) + expect_lint(R"{strsplit(x, '\\s')}", NULL, linter) expect_lint("strsplit(x, 'a(?=b)', perl = TRUE)", NULL, linter) expect_lint("strsplit(x, '0+1', perl = TRUE)", NULL, linter) expect_lint("strsplit(x, 'a|b')", NULL, linter) @@ -97,15 +91,15 @@ test_that("fixed_regex_linter catches null calls to strsplit as well", { expect_lint("tstrsplit(x, fmt)", NULL, linter) # if fixed=TRUE is already set, regex patterns don't matter - expect_lint("strsplit(x, '\\\\.', fixed = TRUE)", NULL, linter) - expect_lint("strsplit(x, '\\\\.', fixed = T)", NULL, linter) + expect_lint(R"{strsplit(x, '\\.', fixed = TRUE)}", NULL, linter) + expect_lint(R"{strsplit(x, '\\.', fixed = T)}", NULL, linter) }) test_that("fixed_regex_linter catches calls to strsplit as well", { linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("strsplit(x, '\\\\.')", lint_msg, linter) + expect_lint(R"{strsplit(x, '\\.')}", lint_msg, linter) expect_lint("strsplit(x, '[.]')", lint_msg, linter) expect_lint("tstrsplit(x, 'abcdefg')", lint_msg, linter) @@ -115,8 +109,8 @@ test_that("fixed_regex_linter is more exact about distinguishing \\s from \\:", linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("grep('\\\\s', '', x)", NULL, linter) - expect_lint("grep('\\\\:', '', x)", lint_msg, linter) + expect_lint(R"{grep('\\s', '', x)}", NULL, linter) + expect_lint(R"{grep('\\:', '', x)}", lint_msg, linter) }) ## tests for stringr functions @@ -126,12 +120,12 @@ test_that("fixed_regex_linter skips allowed stringr usages", { expect_lint("str_replace(y, '[a-zA-Z]', '')", NULL, linter) expect_lint("str_replace_all(y, '^x', '')", NULL, linter) expect_lint("str_detect(y, fmt)", NULL, linter) - expect_lint("str_extract(y, '\\\\s')", NULL, linter) - expect_lint("str_extract_all(y, '\\\\s')", NULL, linter) + expect_lint(R"{str_extract(y, '\\s')}", NULL, linter) + expect_lint(R"{str_extract_all(y, '\\s')}", NULL, linter) expect_lint("str_which(x, '1*2')", NULL, linter) # if fixed() is already set, regex patterns don't matter - expect_lint("str_replace(y, fixed('\\\\.'), '')", NULL, linter) + expect_lint(R"{str_replace(y, fixed('\\.'), '')}", NULL, linter) # namespace qualification doesn't matter expect_lint("stringr::str_replace(y, stringr::fixed('abcdefg'), '')", NULL, linter) @@ -141,16 +135,16 @@ test_that("fixed_regex_linter blocks simple disallowed usages of stringr functio linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("str_replace_all(x, '\\\\.', '')", lint_msg, linter) + expect_lint(R"{str_replace_all(x, '\\.', '')}", lint_msg, linter) expect_lint("str_detect(x, 'abcdefg')", lint_msg, linter) expect_lint("str_locate(y, 'a-z')", lint_msg, linter) - expect_lint("str_subset(x, '\\\\$')", lint_msg, linter) + expect_lint(R"{str_subset(x, '\\$')}", lint_msg, linter) expect_lint("str_which(x, '\n')", lint_msg, linter) # named, positional arguments are still caught expect_lint("str_locate(y, pattern = 'a-z')", lint_msg, linter) # nor do other named arguments throw things off - expect_lint("str_starts(x, '\\\\.', negate = TRUE)", lint_msg, linter) + expect_lint(R"{str_starts(x, '\\.', negate = TRUE)}", lint_msg, linter) }) test_that("fixed_regex_linter catches calls to str_split as well", { @@ -161,8 +155,8 @@ test_that("fixed_regex_linter catches calls to str_split as well", { expect_lint("str_split(x, fmt)", NULL, linter) # if fixed() is already set, regex patterns don't matter - expect_lint("str_split(x, fixed('\\\\.'))", NULL, linter) - expect_lint("str_split(x, '\\\\.')", lint_msg, linter) + expect_lint(R"{str_split(x, fixed('\\.'))}", NULL, linter) + expect_lint(R"{str_split(x, '\\.')}", lint_msg, linter) expect_lint("str_split(x, '[.]')", lint_msg, linter) }) @@ -187,24 +181,24 @@ test_that("one-character character classes with escaped characters are caught", linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("gsub('[\\n]', '', x)", lint_msg, linter) - expect_lint("gsub('[\\\"]', '', x)", lint_msg, linter) - expect_lint('gsub("\\\\<", "x", x, perl = TRUE)', lint_msg, linter) - - expect_lint("str_split(x, '[\\1]')", lint_msg, linter) - expect_lint("str_split(x, '[\\12]')", lint_msg, linter) - expect_lint("str_split(x, '[\\123]')", lint_msg, linter) - expect_lint("str_split(x, '[\\xa]')", lint_msg, linter) - expect_lint("str_split(x, '[\\x32]')", lint_msg, linter) - expect_lint("str_split(x, '[\\uF]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u01]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u012]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u0123]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U8]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U1d4d7]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u{1}]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U{F7D5}]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U{1D4D7}]')", lint_msg, linter) + expect_lint(R"{gsub('[\n]', '', x)}", lint_msg, linter) + expect_lint(R"{gsub('[\"]', '', x)}", lint_msg, linter) + expect_lint(R'{gsub("\\<", "x", x, perl = TRUE)}', lint_msg, linter) + + expect_lint(R"{str_split(x, '[\1]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\12]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\123]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\xa]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\x32]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\uF]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u01]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u012]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u0123]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U8]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U1d4d7]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u{1}]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U{F7D5}]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U{1D4D7}]')}", lint_msg, linter) }) test_that("bracketed unicode escapes are caught", { @@ -218,15 +212,15 @@ test_that("bracketed unicode escapes are caught", { test_that("escaped characters are handled correctly", { linter <- fixed_regex_linter() - expect_lint("gsub('\\n+', '', sql)", NULL, linter) + expect_lint(R"{gsub('\n+', '', sql)}", NULL, linter) expect_lint('gsub("\\n{2,}", "\n", D)', NULL, linter) - expect_lint('gsub("[\\r\\n]", "", x)', NULL, linter) - expect_lint('gsub("\\n $", "", y)', NULL, linter) - expect_lint('gsub("```\\n*```r*\\n*", "", x)', NULL, linter) + expect_lint(R'{gsub("[\r\n]", "", x)}', NULL, linter) + expect_lint(R'{gsub("\n $", "", y)}', NULL, linter) + expect_lint(R'{gsub("```\n*```r*\n*", "", x)}', NULL, linter) expect_lint('strsplit(x, "(;|\n)")', NULL, linter) - expect_lint('strsplit(x, "(;|\\n)")', NULL, linter) - expect_lint('grepl("[\\\\W]", x, perl = TRUE)', NULL, linter) - expect_lint('grepl("[\\\\W]", x)', NULL, linter) + expect_lint(R'{strsplit(x, "(;|\n)")}', NULL, linter) + expect_lint(R'{grepl("[\\W]", x, perl = TRUE)}', NULL, linter) + expect_lint(R'{grepl("[\\W]", x)}', NULL, linter) }) # make sure the logic is properly vectorized @@ -238,10 +232,10 @@ test_that("fixed replacements vectorize and recognize str_detect", { linter <- fixed_regex_linter() # properly vectorized expect_lint( - trim_some("{ + trim_some(R"({ grepl('abcdefg', x) - grepl('a[.]\\\\.b\\n', x) - }"), + grepl('a[.]\\.b\n', x) + })"), list( list(rex::rex('Use "abcdefg" with fixed = TRUE'), line_number = 2L), list(rex::rex('Use "a..b\\n" with fixed = TRUE'), line_number = 3L) @@ -289,37 +283,37 @@ robust_non_printable_unicode <- function() { # styler: off local({ .cases <- tibble::tribble( - ~.test_name, ~regex_expr, ~fixed_expr, - "[.]", "[.]", ".", - '[\\\"]', '[\\\"]', '\\"', - "[]]", "[]]", "]", - "\\\\.", "\\\\.", ".", - "\\\\:", "\\\\:", ":", - "\\\\<", "\\\\<", "<", - "\\\\$", "\\\\$", "$", - "[\\1]", "[\\1]", "\\001", - "\\1", "\\1", "\\001", - "[\\12]", "[\\12]", "\\n", - "[\\123]", "[\\123]", "S", - "a[*]b", "a[*]b", "a*b", - "abcdefg", "abcdefg", "abcdefg", - "abc\\U{A0DEF}ghi", "abc\\U{A0DEF}ghi", robust_non_printable_unicode(), - "a-z", "a-z", "a-z", - "[\\n]", "[\\n]", "\\n", - "\\n", "\n", "\\n", - "[\\u01]", "[\\u01]", "\\001", - "[\\u012]", "[\\u012]", "\\022", - "[\\u0123]", "[\\u0123]", "\u0123", - "[\\u{1}]", "[\\u{1}]", "\\001", - "[\\U1d4d7]", "[\\U1d4d7]", "\U1D4D7", - "[\\U{1D4D7}]", "[\\U{1D4D7}]", "\U1D4D7", - "[\\U8]", "[\\U8]", "\\b", - "\\u{A0}", "\\u{A0}", "\uA0", - "\\u{A0}\\U{0001d4d7}", "\\u{A0}\\U{0001d4d7}", "\uA0\U1D4D7", - "[\\uF]", "[\\uF]", "\\017", - "[\\U{F7D5}]", "[\\U{F7D5}]", "\UF7D5", - "[\\x32]", "[\\x32]", "2", - "[\\xa]", "[\\xa]", "\\n" + ~.test_name, ~regex_expr, ~fixed_expr, + "[.]", "[.]", ".", + '[\\\"]', R'([\"])', '\\"', + "[]]", "[]]", "]", + R"(\\.)", R"(\\.)", ".", + R"(\\:)", R"(\\:)", ":", + R"(\\<)", R"(\\<)", "<", + R"(\\$)", R"(\\$)", "$", + R"([\1])", R"([\1])", R"(\001)", + R"(\1)", R"(\1)", R"(\001)", + R"([\12])", R"([\12])", R"(\n)", + R"([\123])", R"([\123])", "S", + "a[*]b", "a[*]b", "a*b", + "abcdefg", "abcdefg", "abcdefg", + "abc\\U{A0DEF}ghi", "abc\\U{A0DEF}ghi", robust_non_printable_unicode(), + "a-z", "a-z", "a-z", + R"([\n])", R"([\n])", R"(\n)", + R"(\n)", "\n", R"(\n)", + R"([\u01])", R"([\u01])", R"(\001)", + R"([\u012])", R"([\u012])", R"(\022)", + R"([\u0123])", R"([\u0123])", "\u0123", + R"([\u{1}])", R"([\u{1}])", R"(\001)", + R"([\U1d4d7])", R"([\U1d4d7])", "\U1D4D7", + R"([\U{1D4D7}])", R"([\U{1D4D7}])", "\U1D4D7", + R"([\U8])", R"([\U8])", R"(\b)", + R"(\u{A0})", R"(\u{A0})", "\uA0", + R"(\u{A0}\U{0001d4d7})", R"(\u{A0}\U{0001d4d7})", "\uA0\U1D4D7", + R"([\uF])", R"([\uF])", R"(\017)", + R"([\U{F7D5}])", R"([\U{F7D5}])", "\UF7D5", + R"([\x32])", R"([\x32])", "2", + R"([\xa])", R"([\xa])", R"(\n)" ) if (.Platform$OS.type == "windows" && !hasName(R.Version(), "crt")) { skip_cases <- c( diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 6655f54dc..84ad15bc1 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -118,7 +118,7 @@ test_that("lint() results from file or text should be consistent", { lines <- c("x<-1", "x+1") file <- withr::local_tempfile(lines = lines) text <- paste(lines, collapse = "\n") - file <- normalizePath(file) + file <- normalize_path(file) lint_from_file <- lint(file, linters = linters) lint_from_lines <- lint(linters = linters, text = lines) diff --git a/tests/testthat/test-lint_dir.R b/tests/testthat/test-lint_dir.R index 24345ecd2..6da4b81bb 100644 --- a/tests/testthat/test-lint_dir.R +++ b/tests/testthat/test-lint_dir.R @@ -71,7 +71,7 @@ test_that("respects directory exclusions", { lints_norm <- lint_dir(the_dir, exclusions = "exclude-me", relative_path = FALSE) linted_files <- unique(names(lints_norm)) expect_length(linted_files, 1L) - expect_identical(linted_files, normalizePath(file.path(the_dir, "default_linter_testcode.R"))) + expect_identical(linted_files, normalize_path(file.path(the_dir, "default_linter_testcode.R"))) }) test_that("respect directory exclusions from settings", { diff --git a/tests/testthat/test-lint_package.R b/tests/testthat/test-lint_package.R index f0d2c0c48..b6c9b8b53 100644 --- a/tests/testthat/test-lint_package.R +++ b/tests/testthat/test-lint_package.R @@ -211,7 +211,7 @@ test_that("package using .lintr.R config lints correctly", { # config has bad R syntax expect_error( lint_package(test_path("dummy_packages", "RConfigInvalid")), - "Malformed config file, ensure it is valid R syntax", + "Malformed config file (lintr_test_config.R), ensure it is valid R syntax", fixed = TRUE ) @@ -219,7 +219,7 @@ test_that("package using .lintr.R config lints correctly", { withr::local_options(lintr.linter_file = "lintr_test_config_extraneous") expect_warning( expect_length(lint_package(r_config_pkg), 2L), - "Found unused settings in config", + "Found unused settings in config file", fixed = TRUE ) @@ -231,9 +231,6 @@ test_that("package using .lintr.R config lints correctly", { }) test_that("lintr need not be attached for .lintr.R configs to use lintr functions", { - # For some obscure reason, running in the subprocess on this specific version of R - # on Windows stopped working after PR #2446 with 'Package lintr not found'. - if (getRversion() == "3.6.3") skip_on_os("windows") exprs <- paste( 'options(lintr.linter_file = "lintr_test_config")', sprintf('lints <- lintr::lint_package("%s")', test_path("dummy_packages", "RConfig")), diff --git a/tests/testthat/test-linter_tags.R b/tests/testthat/test-linter_tags.R index 985cdb30d..92ee49f80 100644 --- a/tests/testthat/test-linter_tags.R +++ b/tests/testthat/test-linter_tags.R @@ -13,11 +13,7 @@ test_that("validate_linter_db works as expected", { ) expect_false(suppressWarnings(lintr:::validate_linter_db(df_empty, "mypkg"))) - df <- data.frame( - linter = "absolute_path_linter", - tags = "robustness", - stringsAsFactors = FALSE - ) + df <- data.frame(linter = "absolute_path_linter", tags = "robustness") expect_true(lintr:::validate_linter_db(df, "mypkg")) }) @@ -160,6 +156,7 @@ test_that("lintr help files are up to date", { ) # Counts of tags from available_linters() + # NB: as.data.frame.table returns stringsAsFactors=TRUE default in R>4 db_tag_table <- as.data.frame( table(tag = unlist(lintr_db$tags)), responseName = "n_linters", @@ -237,3 +234,7 @@ test_that("available_linters gives precedence to included tags", { available_linters(tags = "deprecated", exclude_tags = NULL) ) }) + +test_that("all linters have at least one tag", { + expect_true(all(lengths(available_linters()$tags) > 0L)) +}) diff --git a/tests/testthat/test-methods.R b/tests/testthat/test-methods.R index 9f454c1f4..b337169f1 100644 --- a/tests/testthat/test-methods.R +++ b/tests/testthat/test-methods.R @@ -56,14 +56,13 @@ test_that("as.data.frame.lints", { expect_s3_class(df, "data.frame") exp <- data.frame( - filename = rep("dummy.R", 2L), + filename = "dummy.R", line_number = c(1L, 2L), column_number = c(1L, 6L), type = c("style", "error"), message = c("", "Under no circumstances is the use of foobar allowed."), line = c("", "a <- 1"), - linter = c(NA_character_, NA_character_), # These are assigned in lint() now. - stringsAsFactors = FALSE + linter = NA_character_ # These are assigned in lint() now. ) expect_identical(df, exp) @@ -97,6 +96,13 @@ test_that("print.lint works", { expect_output(print(l), " 1:length(x)", fixed = TRUE) }) +test_that("print.lint works with empty lints", { + withr::local_options(list(lintr.rstudio_source_markers = FALSE)) + l <- lint(text = "1L") + + expect_message(print(l), "No lints found", fixed = TRUE) +}) + test_that("print.lint works for inline data, even in RStudio", { l <- lint("x = 1\n") diff --git a/tests/testthat/test-normalize_exclusions.R b/tests/testthat/test-normalize_exclusions.R index f2b505e8b..a3c779988 100644 --- a/tests/testthat/test-normalize_exclusions.R +++ b/tests/testthat/test-normalize_exclusions.R @@ -8,9 +8,9 @@ a <- withr::local_tempfile() b <- withr::local_tempfile() c <- withr::local_tempfile(tmpdir = ".") file.create(a, b, c) -a <- normalizePath(a) -b <- normalizePath(b) -c <- normalizePath(c) +a <- normalize_path(a) +b <- normalize_path(b) +c <- normalize_path(c) test_that("it merges two NULL or empty objects as an empty list", { expect_identical(lintr:::normalize_exclusions(c(NULL, NULL)), list()) @@ -132,7 +132,7 @@ test_that("it normalizes file paths, removing non-existing files", { t3[[c]] <- 5L:15L res <- list() res[[a]] <- list(1L:10L) - res[[normalizePath(c)]] <- list(5L:15L) + res[[c]] <- list(5L:15L) expect_identical(lintr:::normalize_exclusions(c(t1, t2, t3)), res) res <- list() diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index 299bd6e0e..355ea6328 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -748,6 +748,9 @@ test_that("symbols in formulas aren't treated as 'undefined global'", { }) test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { + linter <- object_usage_linter() + lint_msg <- "no visible binding for global variable 'column'" + expect_lint( trim_some(" foo <- function(x) { @@ -757,12 +760,8 @@ test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { ) } "), - list( - message = "no visible binding for global variable 'column'", - line_number = 4L, - column_number = 22L - ), - object_usage_linter() + list(lint_msg, line_number = 4L, column_number = 22L), + linter ) expect_lint( @@ -774,12 +773,8 @@ test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { ) } "), - list( - message = "no visible binding for global variable 'column'", - line_number = 4L, - column_number = 22L - ), - object_usage_linter() + list(lint_msg, line_number = 4L, column_number = 22L), + linter ) }) @@ -798,53 +793,34 @@ test_that("functional lambda definitions are also caught", { }) test_that("messages without location info are repaired", { + linter <- object_usage_linter() + global_function_msg <- rex::rex("no visible global function definition for", anything) + global_variable_msg <- rex::rex("no visible binding for global variable", anything) + local_variable_msg <- rex::rex("local variable", anything, "assigned but may not be used") + # regression test for #1986 expect_lint( - trim_some(" - foo <- function() no_fun() - "), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() no_fun()", + list(global_function_msg, line_number = 1L, column_number = 19L), + linter ) expect_lint( - trim_some(" - foo <- function(a = no_fun()) a - "), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 1L, - column_number = 21L - ), - object_usage_linter() + "foo <- function(a = no_fun()) a", + list(global_function_msg, line_number = 1L, column_number = 21L), + linter ) expect_lint( - trim_some(" - foo <- function() no_global - "), - list( - message = rex::rex("no visible binding for global variable", anything), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() no_global", + list(global_variable_msg, line_number = 1L, column_number = 19L), + linter ) expect_lint( - trim_some(" - foo <- function() unused_local <- 42L - "), - list( - message = rex::rex("local variable", anything, "assigned but may not be used"), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() unused_local <- 42L", + list(local_variable_msg, line_number = 1L, column_number = 19L), + linter ) # More complex case with two lints and missing location info @@ -854,17 +830,31 @@ test_that("messages without location info are repaired", { bar() "), list( - list( - message = rex::rex("local variable", anything, "assigned but may not be used"), - line_number = 1L, - column_number = 19L - ), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 2L, - column_number = 3L - ) + list(local_variable_msg, line_number = 1L, column_number = 19L), + list(global_function_msg, line_number = 2L, column_number = 3L) ), - object_usage_linter() + linter + ) +}) + +test_that("globals in scripts are found regardless of assignment operator", { + linter <- object_usage_linter() + + expect_lint( + trim_some(" + library(dplyr) + + global_const_eq = 5 + global_const_la <- 6 + 7 -> global_const_ra + + examplefunction <- function(df) { + df %>% + select(dist) %>% + mutate(power = global_const_eq + global_const_ra + global_const_la) + } + "), + NULL, + linter ) }) diff --git a/tests/testthat/test-return_linter.R b/tests/testthat/test-return_linter.R index b05b3abd3..58a5d1cf9 100644 --- a/tests/testthat/test-return_linter.R +++ b/tests/testthat/test-return_linter.R @@ -691,6 +691,20 @@ test_that("except_regex= argument works", { list(rex::rex("All functions must have an explicit return()."), line_number = 5L), linter ) + + # capture group doesn't cause issues, #2678 + expect_lint( + trim_some(" + TestFun <- function() { + non_return() + } + AssertFun <- function() { + non_return() + } + "), + NULL, + return_linter(return_style = "explicit", except_regex = "^(Test|Assert)") + ) }) test_that("except= and except_regex= combination works", { diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 2c48c1629..1653735e2 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -117,12 +117,12 @@ test_that("it has a smart default for encodings", { pkg_file <- test_path("dummy_packages", "cp1252", "R", "cp1252.R") expect_identical( - normalizePath(find_rproj_at(find_package(proj_file, allow_rproj = TRUE)), winslash = "/"), - normalizePath(test_path("dummy_projects", "project", "project.Rproj"), winslash = "/") + normalize_path(find_rproj_at(find_package(proj_file, allow_rproj = TRUE))), + normalize_path(test_path("dummy_projects", "project", "project.Rproj")) ) expect_identical( - normalizePath(find_package(pkg_file), winslash = "/"), - normalizePath(test_path("dummy_packages", "cp1252"), winslash = "/") + normalize_path(find_package(pkg_file)), + normalize_path(test_path("dummy_packages", "cp1252")) ) expect_identical(lintr:::find_default_encoding(proj_file), "ISO8859-1") diff --git a/tests/testthat/test-todo_comment_linter.R b/tests/testthat/test-todo_comment_linter.R index 103f9c8fc..b7793d1d8 100644 --- a/tests/testthat/test-todo_comment_linter.R +++ b/tests/testthat/test-todo_comment_linter.R @@ -57,4 +57,11 @@ test_that("except_regex= excludes valid TODO", { NULL, todo_comment_linter(except_regex = c("TODO\\(#[0-9]+\\):", "fixme\\(#[0-9]+\\):")) ) + + # ignore captured groups + expect_lint( + "# TODO(a)", + NULL, + todo_comment_linter(except_regex = "TODO\\((a|abc)\\)") + ) }) diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index 167909d50..b54d3b11e 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -687,6 +687,19 @@ test_that("allow_comment_regex= works", { NULL, linter_x1x2 ) + + # might contain capture groups, #2678 + expect_lint( + trim_some(" + function() { + stop('a') + # a + # ab + } + "), + NULL, + unreachable_code_linter(allow_comment_regex = "#\\s*(a|ab|abc)") + ) }) test_that("allow_comment_regex= obeys covr's custom exclusion when set", { diff --git a/tests/testthat/test-use_lintr.R b/tests/testthat/test-use_lintr.R index 68e089322..f56d3b304 100644 --- a/tests/testthat/test-use_lintr.R +++ b/tests/testthat/test-use_lintr.R @@ -6,8 +6,8 @@ test_that("use_lintr works as expected", { # check that newly created file is in the root directory expect_identical( - normalizePath(lintr_file, winslash = "/"), - file.path(normalizePath(tmp, winslash = "/"), ".lintr") + normalize_path(lintr_file), + file.path(normalize_path(tmp), ".lintr") ) # can't generate if a .lintr already exists @@ -28,8 +28,8 @@ test_that("use_lintr with type = full also works", { # check that newly created file is in the root directory expect_identical( - normalizePath(lintr_file, winslash = "/"), - file.path(normalizePath(tmp, winslash = "/"), ".lintr") + normalize_path(lintr_file), + file.path(normalize_path(tmp), ".lintr") ) lints <- lint_dir(tmp) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 5ab2560ad..9aa49d204 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -1,11 +1,18 @@ test_that("modify_defaults produces error with missing or incorrect defaults", { - lint_msg <- "`defaults` must be a named list." - expect_error(modify_defaults(), lint_msg, fixed = TRUE) - expect_error(modify_defaults("assignment_linter"), lint_msg, fixed = TRUE) + expect_error( + modify_defaults(), + "`defaults` is a required argument, but is missing", + fixed = TRUE + ) + expect_error( + modify_defaults("assignment_linter"), + "`defaults` must be a named list", + fixed = TRUE + ) }) test_that("linters_with_tags produces error with incorrect tags", { - expect_error(linters_with_tags(1L:4L), "`tags` must be a vector, or `NULL`.", fixed = TRUE) + expect_error(linters_with_tags(1L:4L), "`tags` must be a character vector, or `NULL`", fixed = TRUE) }) test_that("linters_with_defaults works as expected with unnamed args", { @@ -24,17 +31,12 @@ test_that("linters_with_defaults warns on unused NULLs", { test_that("linters_with_tags() verifies the output of available_linters()", { local_mocked_bindings( available_linters = function(...) { - data.frame( - linter = c("fake_linter", "very_fake_linter"), - package = "lintr", - tags = "", - stringsAsFactors = FALSE - ) + data.frame(linter = c("fake_linter", "very_fake_linter"), package = "lintr", tags = "") } ) expect_error( linters_with_tags(NULL), - "Linters `fake_linter()` and `very_fake_linter()` are advertised by `available_linters()`", + "Can't find linters `fake_linter()` and `very_fake_linter()`", fixed = TRUE ) }) diff --git a/vignettes/creating_linters.Rmd b/vignettes/creating_linters.Rmd index 7301ff680..b38dd7a5d 100644 --- a/vignettes/creating_linters.Rmd +++ b/vignettes/creating_linters.Rmd @@ -1,7 +1,5 @@ --- title: "Creating new linters" -author: "lintr maintainers" -date: "`r Sys.Date()`" output: rmarkdown::html_vignette vignette: > %\VignetteIndexEntry{Creating new linters} diff --git a/vignettes/lintr.Rmd b/vignettes/lintr.Rmd index e6c4a1da0..275b864e1 100644 --- a/vignettes/lintr.Rmd +++ b/vignettes/lintr.Rmd @@ -1,6 +1,5 @@ --- title: "Using lintr" -author: "Alexander Rosenstock" output: rmarkdown::html_vignette vignette: > %\VignetteIndexEntry{Using lintr} @@ -140,10 +139,7 @@ make_string <- function(x) { } } -defaults_table <- data.frame( - default = vapply(default_settings, make_string, character(1L)), - stringsAsFactors = FALSE -) +defaults_table <- data.frame(default = vapply(default_settings, make_string, character(1L))) # avoid conflict when loading lintr in echo=TRUE cell below rm(default_settings) @@ -196,8 +192,7 @@ make_setting_string <- function(linter_name) { defaults_table <- data.frame( row.names = names(default_linters), - settings = vapply(names(default_linters), make_setting_string, character(1L)), - stringsAsFactors = FALSE + settings = vapply(names(default_linters), make_setting_string, character(1L)) ) knitr::kable(defaults_table)