From fb7866f813b54c2eb07f06b24462a62efca4d0ce Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 21:29:16 +0000 Subject: [PATCH 01/11] vastly improve the system for excluding tests from fuzzing --- .dev/ast_fuzz_test.R | 99 ++++++++++--------- .dev/maybe_fuzz_content.R | 44 +++++++-- tests/testthat/test-any_duplicated_linter.R | 4 +- tests/testthat/test-assignment_linter.R | 6 +- tests/testthat/test-brace_linter.R | 4 +- tests/testthat/test-cache.R | 4 +- tests/testthat/test-coalesce_linter.R | 4 +- tests/testthat/test-commas_linter.R | 4 +- tests/testthat/test-exclusions.R | 8 +- tests/testthat/test-expect_lint.R | 4 +- .../test-function_left_parentheses_linter.R | 10 +- tests/testthat/test-function_return_linter.R | 4 +- tests/testthat/test-get_source_expressions.R | 2 +- tests/testthat/test-if_switch_linter.R | 12 ++- .../test-implicit_assignment_linter.R | 4 +- tests/testthat/test-indentation_linter.R | 8 +- tests/testthat/test-infix_spaces_linter.R | 4 +- tests/testthat/test-is_numeric_linter.R | 2 +- tests/testthat/test-keyword_quote_linter.R | 4 +- tests/testthat/test-knitr_formats.R | 2 +- tests/testthat/test-line_length_linter.R | 4 +- tests/testthat/test-lint.R | 4 +- tests/testthat/test-make_linter_from_regex.R | 2 +- tests/testthat/test-nested_pipe_linter.R | 8 +- tests/testthat/test-object_usage_linter.R | 2 +- tests/testthat/test-one_call_pipe_linter.R | 10 +- tests/testthat/test-paren_body_linter.R | 4 +- tests/testthat/test-pipe_consistency_linter.R | 4 +- .../testthat/test-pipe_continuation_linter.R | 4 +- tests/testthat/test-return_linter.R | 2 +- tests/testthat/test-semicolon_linter.R | 4 +- tests/testthat/test-spaces_inside_linter.R | 4 +- .../test-spaces_left_parentheses_linter.R | 4 +- .../test-trailing_blank_lines_linter.R | 4 +- .../test-trailing_whitespace_linter.R | 4 +- .../test-undesirable_operator_linter.R | 8 +- .../test-unnecessary_nesting_linter.R | 4 +- .../test-unnecessary_placeholder_linter.R | 4 +- tests/testthat/test-unreachable_code_linter.R | 12 +-- tests/testthat/test-unused_import_linter.R | 2 +- 40 files changed, 182 insertions(+), 145 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index 01b938136..c35cc0d98 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -53,7 +53,7 @@ if ( } contents[wrong_number_def_idx] <- - 'wrong_number_fmt <- "got %d lints instead of %d%s\\nFile contents:\\n%s"' + ' wrong_number_fmt <- "got %d lints instead of %d%s\\nFile contents:\\n%s"' contents[wrong_number_use_idx] <- gsub("\\)$", ", readChar(file, file.size(file)))", contents[wrong_number_use_idx]) writeLines(contents, expect_lint_file) @@ -66,61 +66,68 @@ withr::defer({ suppressMessages(pkgload::load_all()) +can_parse <- \(lines) !inherits(tryCatch(parse(text = lines), error = identity), "error") +get_str <- \(x) tail(unlist(strsplit(x, ": ", fixed = TRUE)), 1L) + + # beware lazy eval: originally tried adding a withr::defer() in each iteration, but # this effectively only runs the last 'defer' expression as the names are only # evaluated at run-time. So instead keep track of all edits in this object. -# this approach to implementing 'nofuzz' feels painfully manual, but I couldn't -# figure out how else to get 'testthat' to give us what we need -- the failures -# object in the reporter is frustratingly inconsistent in whether the trace -# exists, and even if it does, we'd have to text-mangle to get the corresponding -# file names out. Also, the trace 'srcref' happens under keep.source=FALSE, -# so we lose any associated comments anyway. even that would not solve the issue -# of getting top-level exclusions done for 'nofuzz start|end' ranges, except -# maybe if it enabled us to reuse lintr's own exclude() system. -# therefore we take this approach: pass over the test suite first and comment out -# any tests/units that have been marked 'nofuzz'. restore later. one consequence -# is there's no support for fuzzer-specific exclusion, e.g. we fully disable -# the unnecessary_placeholder_linter() tests because |> and _ placeholders differ. test_restorations <- list() for (test_file in list.files("tests/testthat", pattern = "^test-", full.names = TRUE)) { - xml <- read_xml(xmlparsedata::xml_parse_data(parse(test_file, keep.source = TRUE))) - # parent::* to catch top-level comments (exprlist). matches one-line nofuzz and start/end ranges. - nofuzz_lines <- xml_find_all(xml, "//COMMENT[contains(text(), 'nofuzz')]/parent::*") - if (length(nofuzz_lines) == 0L) next - - test_original <- test_lines <- readLines(test_file) - - for (nofuzz_line in nofuzz_lines) { - comments <- xml_find_all(nofuzz_line, "COMMENT[contains(text(), 'nofuzz')]") - comment_text <- xml_text(comments) - # handle start/end ranges first. - start_idx <- grep("nofuzz start", comment_text, fixed = TRUE) - end_idx <- grep("nofuzz end", comment_text, fixed = TRUE) - if (length(start_idx) != length(end_idx) || any(end_idx < start_idx)) { - stop(sprintf( - "Mismatched '# nofuzz start' (%s), '# nofuzz end' (%s) in %s", - toString(start_idx), toString(end_idx), test_file - )) + test_lines <- readLines(test_file) + one_expr_idx <- grep("# nofuzz:", test_lines) + range_start_idx <- grep("^\\s*# fuzzer disable:", test_lines) + if (length(one_expr_idx) == 0L && length(range_start_idx) == 0L) next + + test_original <- test_lines + pd <- getParseData(parse(test_file)) + + for (one_line in rev(one_expr_idx)) { + end_line <- one_line + while (end_line <= length(test_lines) && !can_parse(test_lines[one_line:end_line])) { + end_line <- end_line + 1L } - - comment_ranges <- Map(`:`, - as.integer(xml_attr(comments[start_idx], "line1")), - as.integer(xml_attr(comments[end_idx], "line1")) - ) - for (comment_range in comment_ranges) { - test_lines[comment_range] <- paste("#", test_lines[comment_range]) + if (end_line > length(test_lines)) { + stop("Unable to parse any expression starting from line ", one_line) } + comment_txt <- subset(pd, line1 == one_line & token == "COMMENT", select = "text", drop = TRUE) + deactivated <- get_str(comment_text) + test_lines <- c( + head(test_lines, one_line - 1L), + sprintf("deactivate_fuzzers('%s')", deactivated), + test_lines[one_line:end_line], + sprintf("activate_fuzzers('%s')", deactivated), + tail(test_lines, -end_line) + ) + } - if (length(start_idx) > 0L && !any(!start_idx & !end_idx)) next + if (length(one_expr_idx)) { + writeLines(test_lines, test_file) + pd <- getParseData(parse(test_file)) + range_start_idx <- grep("^\\s*# fuzzer disable:", test_lines) + } + + range_end_idx <- grep("^\\s*# fuzzer enable:", test_lines) - # NB: one-line tests line expect_lint(...) # nofuzz are not supported, - # since the comment will attach to the parent test_that() & thus comment - # out the whole unit. Easiest solution is just to spread out those few tests for now. - comment_range <- as.integer(xml_attr(nofuzz_line, "line1")):as.integer(xml_attr(nofuzz_line, "line2")) - test_lines[comment_range] <- paste("#", test_lines[comment_range]) + if (length(range_start_idx) != length(range_end_idx) || any(range_end_idx < range_start_idx)) { + stop(sprintf( + "Mismatched '# fuzzer disable' (%s), '# fuzzer enable' (%s) in %s", + toString(range_start_idx), toString(range_end_idx), test_file + )) } - writeLines(test_lines, test_file) + for (ii in seq_along(range_start_idx)) { + start_line <- test_lines[range_start_idx[ii]] + test_lines[range_start_idx[ii]] <- + gsub("#.*", sprintf("deactivate_fuzzers('%s')", get_str(start_line)), start_line) + end_line <- test_lines[range_end_idx[ii]] + test_lines[range_end_idx[ii]] <- + gsub("#.*", sprintf("activate_fuzzers('%s')", get_str(end_line)), end_line) + } + + if (length(range_start_idx)) writeLines(test_lines, test_file) + test_restorations <- c(test_restorations, list(list(file = test_file, lines = test_original))) } withr::defer(for (restoration in test_restorations) writeLines(restoration$lines, restoration$file)) @@ -160,7 +167,7 @@ if (length(invalid_failures) > 0L) { \(x) sprintf("%s:%s", x$file, x$test), character(1L) ) - cat("Some fuzzed tests failed unexpectedly!\n") + cat(sprintf("%d fuzzed tests failed unexpectedly!\n", length(invalid_failures))) print(invalid_failures) stop("Use # nofuzz [start|end] to mark false positives or fix any bugs.") } diff --git a/.dev/maybe_fuzz_content.R b/.dev/maybe_fuzz_content.R index 74bd2a127..b72aa3cda 100644 --- a/.dev/maybe_fuzz_content.R +++ b/.dev/maybe_fuzz_content.R @@ -9,13 +9,7 @@ maybe_fuzz_content <- function(file, lines) { file.copy(file, new_file, copy.mode = FALSE) } - apply_fuzzers(new_file, fuzzers = list( - function_lambda_fuzzer, - pipe_fuzzer, - dollar_at_fuzzer, - comment_injection_fuzzer, - assignment_fuzzer - )) + apply_fuzzers(new_file, fuzzers = .fuzzers$active) new_file } @@ -122,3 +116,39 @@ apply_fuzzers <- function(f, fuzzers) { invisible() } + +.fuzzers <- new.env() +.fuzzers$active <- list( + function_lambda = function_lambda_fuzzer, + pipe = pipe_fuzzer, + dollar_at = dollar_at_fuzzer, + comment_injection = comment_injection_fuzzer, + assignment = assignment_fuzzer +) +.fuzzers$inactive <- list() + +deactivate_fuzzers <- function(names_str) { + req <- unlist(strsplit(names_str, " ", fixed = TRUE)) + if (!all(req %in% names(.fuzzers$active))) { + stop(sprintf( + "Invalid attempt to deactivate fuzzers: '%s'\n Currently active fuzzers: %s\n Currently inactive fuzzers: %s", + names_str, toString(names(.fuzzers$active)), toString(names(.fuzzers$inactive)) + )) + } + .fuzzers$inactive[req] <- .fuzzers$active[req] + .fuzzers$active[req] <- NULL + invisible() +} + +activate_fuzzers <- function(names_str) { + req <- unlist(strsplit(names_str, " ", fixed = TRUE)) + if (!all(req %in% names(.fuzzers$inactive))) { + stop(sprintf( + "Invalid attempt to activate fuzzers: '%s'\n Currently active fuzzers: %s\n Currently inactive fuzzers: %s", + names_str, toString(names(.fuzzers$active)), toString(names(.fuzzers$inactive)) + )) + } + .fuzzers$active[req] <- .fuzzers$inactive[req] + .fuzzers$inactive[req] <- NULL + invisible() +} diff --git a/tests/testthat/test-any_duplicated_linter.R b/tests/testthat/test-any_duplicated_linter.R index 15038b0b5..ac538c3c2 100644 --- a/tests/testthat/test-any_duplicated_linter.R +++ b/tests/testthat/test-any_duplicated_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: dollar_at test_that("any_duplicated_linter skips allowed usages", { linter <- any_duplicated_linter() @@ -81,4 +81,4 @@ test_that("any_duplicated_linter catches expression with two types of lint", { linter ) }) -# nofuzz end +# fuzzer enable: dollar_at diff --git a/tests/testthat/test-assignment_linter.R b/tests/testthat/test-assignment_linter.R index 270c31502..ceab52ff3 100644 --- a/tests/testthat/test-assignment_linter.R +++ b/tests/testthat/test-assignment_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: assignment test_that("assignment_linter skips allowed usages", { linter <- assignment_linter() @@ -67,7 +67,7 @@ test_that("arguments handle <<- and ->/->> correctly", { ) }) -test_that("arguments handle trailing assignment operators correctly", { +test_that("arguments handle trailing assignment operators correctly", { # nofuzz: comment_injection linter_default <- assignment_linter() linter_no_trailing <- assignment_linter(allow_trailing = FALSE) expect_no_lint("x <- y", linter_no_trailing) @@ -391,4 +391,4 @@ test_that("implicit '<-' assignments inside calls are ignored where top-level '< expect_no_lint("for (i in foo(idx <- is.na(y))) which(idx)", linter) expect_no_lint("for (i in foo(bar(idx <- is.na(y)))) which(idx)", linter) }) -# nofuzz end +# fuzzer enable: assignment diff --git a/tests/testthat/test-brace_linter.R b/tests/testthat/test-brace_linter.R index 2257dc49f..ed9a83c28 100644 --- a/tests/testthat/test-brace_linter.R +++ b/tests/testthat/test-brace_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("brace_linter lints braces correctly", { open_curly_msg <- rex::rex( "Opening curly braces should never go on their own line" @@ -634,4 +634,4 @@ test_that("function shorthand is treated like 'full' function", { linter ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-cache.R b/tests/testthat/test-cache.R index 24e97651d..c84270b60 100644 --- a/tests/testthat/test-cache.R +++ b/tests/testthat/test-cache.R @@ -406,7 +406,7 @@ test_that("find_new_line returns the correct line if it is after the current lin # -test_that("lint with cache uses the provided relative cache directory", { # nofuzz +test_that("lint with cache uses the provided relative cache directory", { # nofuzz: assignment path <- withr::local_tempdir("my_cache_dir") linter <- assignment_linter() @@ -420,7 +420,7 @@ test_that("lint with cache uses the provided relative cache directory", { # nofu expect_true(dir.exists(path)) }) -test_that("it works outside of a package", { # nofuzz +test_that("it works outside of a package", { # nofuzz: assignment linter <- assignment_linter() local_mocked_bindings(find_package = function(...) NULL) diff --git a/tests/testthat/test-coalesce_linter.R b/tests/testthat/test-coalesce_linter.R index ced4a3e07..460f02ce4 100644 --- a/tests/testthat/test-coalesce_linter.R +++ b/tests/testthat/test-coalesce_linter.R @@ -47,7 +47,7 @@ test_that("coalesce_linter blocks simple disallowed usage", { ) }) -test_that("coalesce_linter blocks usage with implicit assignment", { # nofuzz +test_that("coalesce_linter blocks usage with implicit assignment", { # nofuzz: assignment linter <- coalesce_linter() lint_msg <- rex::rex("Use x %||% y instead of if (is.null(x))") lint_msg_not <- rex::rex("Use x %||% y instead of if (!is.null(x))") @@ -63,7 +63,7 @@ test_that("coalesce_linter blocks usage with implicit assignment", { # nofuzz expect_lint("if (!is.null(s <- foo(x))) { s } else { y }", lint_msg_not, linter) }) -test_that("lints vectorize", { # nofuzz +test_that("lints vectorize", { # nofuzz: assignment expect_lint( trim_some("{ if (is.null(x)) y else x diff --git a/tests/testthat/test-commas_linter.R b/tests/testthat/test-commas_linter.R index 8ef94955b..5bce6b295 100644 --- a/tests/testthat/test-commas_linter.R +++ b/tests/testthat/test-commas_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("returns the correct linting (with default parameters)", { linter <- commas_linter() msg_after <- rex::rex("Put a space after a comma.") @@ -114,4 +114,4 @@ test_that("returns the correct linting (with 'allow_trailing' set)", { linter ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index cca09b0f0..3ace3d4f2 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -56,7 +56,7 @@ test_that("it gives the expected error message when there is mismatch between mu ) }) -test_that("partial matching works for exclusions but warns if no linter found", { # nofuzz +test_that("partial matching works for exclusions but warns if no linter found", { # nofuzz: assignment lintr:::read_settings(NULL) expect_warning( @@ -110,7 +110,7 @@ test_that("#1413: lint_dir properly excludes files", { expect_length(lint_dir(tmp), 0L) }) -test_that("#1442: is_excluded_files works if no global exclusions are specified", { +test_that("#1442: is_excluded_files works if no global exclusions are specified", { # nofuzz: assignment withr::local_options(lintr.linter_file = "lintr_test_config") tmp <- withr::local_tempdir() @@ -142,7 +142,7 @@ test_that("#1442: is_excluded_files works if no global exclusions are specified" ) # 3 lints: assignment_linter(), quotes_linter() and line_length_linter() - expect_lint( # nofuzz + expect_lint( file = file.path(tmp, "bad.R"), checks = list( list(linter = "assignment_linter", line_number = 1L), @@ -153,7 +153,7 @@ test_that("#1442: is_excluded_files works if no global exclusions are specified" expect_length(lint_dir(tmp), 3L) }) -test_that("next-line exclusion works", { # nofuzz +test_that("next-line exclusion works", { # nofuzz: assignment withr::local_options( lintr.exclude = "# NL", lintr.exclude_next = "# NLN", diff --git a/tests/testthat/test-expect_lint.R b/tests/testthat/test-expect_lint.R index 28406d36d..1d5b3809e 100644 --- a/tests/testthat/test-expect_lint.R +++ b/tests/testthat/test-expect_lint.R @@ -2,7 +2,7 @@ # thus less than ideal to test expect_lint(), which can process multiple lints. If you want to test # for failure, always put the lint check or lint field that must fail first. -# nofuzz start +# fuzzer disable: assignment linter <- assignment_linter() lint_msg <- "Use one of <-, <<- for assignment, not =" @@ -85,4 +85,4 @@ test_that("execution without testthat gives the right errors", { expect_error(expect_no_lint(), lint_msg("expect_no_lint")) expect_error(expect_lint_free(), lint_msg("expect_lint_free")) }) -# nofuzz end +# fuzzer enable: assignment diff --git a/tests/testthat/test-function_left_parentheses_linter.R b/tests/testthat/test-function_left_parentheses_linter.R index d9343364d..91d76f171 100644 --- a/tests/testthat/test-function_left_parentheses_linter.R +++ b/tests/testthat/test-function_left_parentheses_linter.R @@ -7,7 +7,7 @@ test_that("function_left_parentheses_linter skips allowed usages", { expect_no_lint("base::print(blah)", linter) expect_no_lint('base::"print"(blah)', linter) expect_no_lint("base::print(blah, fun(1))", linter) - expect_no_lint("blah <- function(blah) { }", linter) # nofuzz + expect_no_lint("blah <- function(blah) { }", linter) # nofuzz: comment_injection expect_no_lint("(1 + 1)", linter) expect_no_lint("( (1 + 1) )", linter) expect_no_lint("if (blah) { }", linter) @@ -18,9 +18,9 @@ test_that("function_left_parentheses_linter skips allowed usages", { expect_no_lint("c(1, 2, 3)[(2 - 1)]", linter) expect_no_lint("list(1, 2, 3)[[(2 - 1)]]", linter) expect_no_lint("range(10)[(2 - 1):(10 - 1)]", linter) - expect_no_lint("function(){function(){}}()()", linter) # nofuzz - expect_no_lint("c(function(){})[1]()", linter) # nofuzz - expect_no_lint("function(x) (mean(x) + 3)", linter) # nofuzz + expect_no_lint("function(){function(){}}()()", linter) # nofuzz: comment_injection + expect_no_lint("c(function(){})[1]()", linter) # nofuzz: comment_injection + expect_no_lint("function(x) (mean(x) + 3)", linter) # nofuzz: comment_injection expect_no_lint('"blah (1)"', linter) }) @@ -197,7 +197,7 @@ test_that("newline in character string doesn't trigger false positive (#1963)", ) }) -test_that("shorthand functions are handled", { # nofuzz +test_that("shorthand functions are handled", { # nofuzz: comment_injection skip_if_not_r_version("4.1.0") linter <- function_left_parentheses_linter() fun_lint_msg <- rex::rex("Remove spaces before the left parenthesis in a function definition.") diff --git a/tests/testthat/test-function_return_linter.R b/tests/testthat/test-function_return_linter.R index ac37695d1..bc5421bf4 100644 --- a/tests/testthat/test-function_return_linter.R +++ b/tests/testthat/test-function_return_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: assignment test_that("function_return_linter skips allowed usages", { lines_simple <- trim_some(" foo <- function(x) { @@ -97,4 +97,4 @@ test_that("lints vectorize", { function_return_linter() ) }) -# nofuzz end +# fuzzer enable: assignment diff --git a/tests/testthat/test-get_source_expressions.R b/tests/testthat/test-get_source_expressions.R index 975a152a2..6a5a35367 100644 --- a/tests/testthat/test-get_source_expressions.R +++ b/tests/testthat/test-get_source_expressions.R @@ -108,7 +108,7 @@ test_that("Can read non UTF-8 file", { withr::local_options(list(lintr.linter_file = tempfile())) proj_dir <- test_path("dummy_projects", "project") withr::local_dir(proj_dir) - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: assignment file = "cp1252.R", linters = list() ) diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index 867473413..a2165cc3a 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -86,7 +86,8 @@ test_that("multiple lints have right metadata", { ) }) -test_that("max_branch_lines= and max_branch_expressions= arguments work", { # nofuzz +# fuzzer disable: comment_injection +test_that("max_branch_lines= and max_branch_expressions= arguments work", { max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) max_lines4_linter <- if_switch_linter(max_branch_lines = 4L) max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) @@ -225,7 +226,7 @@ test_that("max_branch_lines= and max_branch_expressions= arguments work", { # no expect_no_lint(five_expr_three_lines_lines, max_expr4_linter) }) -test_that("max_branch_lines= and max_branch_expressions= block over-complex switch() too", { # nofuzz +test_that("max_branch_lines= and max_branch_expressions= block over-complex switch() too", { max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) max_lines4_linter <- if_switch_linter(max_branch_lines = 4L) max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) @@ -388,7 +389,7 @@ test_that("max_branch_lines= and max_branch_expressions= block over-complex swit expect_lint(five_expr_three_lines_lines, lint_msg, max_expr4_linter) }) -test_that("max_branch_lines= and max_branch_expressions= interact correctly", { # nofuzz +test_that("max_branch_lines= and max_branch_expressions= interact correctly", { linter <- if_switch_linter(max_branch_lines = 5L, max_branch_expressions = 3L) lint_msg <- rex::rex("Prefer switch() statements over repeated if/else equality tests") @@ -438,7 +439,7 @@ test_that("max_branch_lines= and max_branch_expressions= interact correctly", { ) }) -test_that("max_branch_lines= and max_branch_expressions= work for a terminal 'else' branch", { # nofuzz +test_that("max_branch_lines= and max_branch_expressions= work for a terminal 'else' branch", { max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) lint_msg <- rex::rex("Prefer repeated if/else statements over overly-complicated switch() statements.") @@ -481,7 +482,7 @@ test_that("max_branch_lines= and max_branch_expressions= work for a terminal 'el expect_lint(default_long_lines, lint_msg, max_expr2_linter) }) -test_that("max_branch_lines= and max_branch_expressions= are guided by the most complex branch", { # nofuzz +test_that("max_branch_lines= and max_branch_expressions= are guided by the most complex branch", { max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) lint_msg <- rex::rex("Prefer repeated if/else statements over overly-complicated switch() statements.") @@ -520,3 +521,4 @@ test_that("max_branch_lines= and max_branch_expressions= are guided by the most expect_lint(switch_one_branch_lines, lint_msg, max_lines2_linter) expect_lint(switch_one_branch_lines, lint_msg, max_expr2_linter) }) +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-implicit_assignment_linter.R b/tests/testthat/test-implicit_assignment_linter.R index 7f9fbcf08..18cb171f9 100644 --- a/tests/testthat/test-implicit_assignment_linter.R +++ b/tests/testthat/test-implicit_assignment_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: assignment test_that("implicit_assignment_linter skips allowed usages", { linter <- implicit_assignment_linter() @@ -504,4 +504,4 @@ test_that("call-less '(' mentions avoiding implicit printing", { linter ) }) -# nofuzz end +# fuzzer enable: assignment diff --git a/tests/testthat/test-indentation_linter.R b/tests/testthat/test-indentation_linter.R index 19a78b520..51de471aa 100644 --- a/tests/testthat/test-indentation_linter.R +++ b/tests/testthat/test-indentation_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("indentation linter flags unindented expressions", { linter <- indentation_linter(indent = 2L) @@ -153,7 +153,6 @@ test_that("indentation linter flags improper closing curly braces", { ) }) -# nofuzz start test_that("function argument indentation works in tidyverse-style", { linter <- indentation_linter() expect_no_lint( @@ -263,7 +262,7 @@ test_that("function argument indentation works in tidyverse-style", { ) }) -test_that("function argument indentation works in always-hanging-style", { +test_that("function argument indentation works in always-hanging-style", { # nofuzz: function_lambda linter <- indentation_linter(hanging_indent_style = "always") expect_no_lint( trim_some(" @@ -357,7 +356,6 @@ test_that("function argument indentation works in always-hanging-style", { linter ) }) -# nofuzz end test_that("indentation with operators works", { linter <- indentation_linter() @@ -913,4 +911,4 @@ test_that("for loop gets correct linting", { linter ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-infix_spaces_linter.R b/tests/testthat/test-infix_spaces_linter.R index ba7182f83..7a898e402 100644 --- a/tests/testthat/test-infix_spaces_linter.R +++ b/tests/testthat/test-infix_spaces_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("returns the correct linting", { ops <- c( "+", @@ -236,4 +236,4 @@ test_that("lints vectorize", { infix_spaces_linter() ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index d0026e1f7..1887249d2 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -28,7 +28,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { expect_lint("is.integer(x) || is.numeric(x)", lint_msg, linter) # identical expressions match too - expect_lint( # nofuzz + expect_lint( # fuzzer disable: dollar_at "is.integer(DT$x) || is.numeric(DT$x)", lint_msg, linter diff --git a/tests/testthat/test-keyword_quote_linter.R b/tests/testthat/test-keyword_quote_linter.R index b562a9ac5..a814eee4b 100644 --- a/tests/testthat/test-keyword_quote_linter.R +++ b/tests/testthat/test-keyword_quote_linter.R @@ -109,7 +109,7 @@ test_that("keyword_quote_linter blocks quoted assignment targets", { expect_lint('1 -> "a b"', backtick_msg, linter) }) -test_that("keyword_quote_linter blocks quoted $, @ extractions", { # nofuzz +test_that("keyword_quote_linter blocks quoted $, @ extractions", { # nofuzz: dollar_at linter <- keyword_quote_linter() backtick_msg <- rex::rex("Use backticks to create non-syntactic names, not quotes.") dollar_msg <- rex::rex("Only quote targets of extraction with $ if necessary") @@ -131,7 +131,7 @@ test_that("keyword_quote_linter blocks quoted $, @ extractions", { # nofuzz expect_lint("x@`foo` = 1", at_msg, linter) }) -test_that("multiple lints are generated correctly", { # nofuzz +test_that("multiple lints are generated correctly", { # nofuzz: dollar_at linter <- keyword_quote_linter() expect_lint( diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R index 8a70c7c87..398f91cc7 100644 --- a/tests/testthat/test-knitr_formats.R +++ b/tests/testthat/test-knitr_formats.R @@ -120,7 +120,7 @@ test_that("it handles asciidoc", { ) }) -test_that("it does _not_ handle brew", { # nofuzz +test_that("it does _not_ handle brew", { # nofuzz: comment_injection expect_lint("'<% a %>'\n", checks = list( regexes[["quotes"]], diff --git a/tests/testthat/test-line_length_linter.R b/tests/testthat/test-line_length_linter.R index 483ecff5a..afbd51a93 100644 --- a/tests/testthat/test-line_length_linter.R +++ b/tests/testthat/test-line_length_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("line_length_linter skips allowed usages", { linter <- line_length_linter(80L) @@ -72,4 +72,4 @@ test_that("Multiple lints give custom messages", { line_length_linter(5L) ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 8ed680b3a..941ba3794 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -104,7 +104,7 @@ test_that("lint() results do not depend on the position of the .lintr", { ) }) -test_that("lint uses linter names", { # nofuzz +test_that("lint uses linter names", { # nofuzz: assignment expect_lint( "a = 2", list(linter = "bla"), @@ -146,7 +146,7 @@ test_that("lint() results from file or text should be consistent", { expect_identical(lint_from_file, lint_from_text) }) -test_that("exclusions work with custom linter names", { # nofuzz +test_that("exclusions work with custom linter names", { # nofuzz: assignment expect_no_lint( "a = 2 # nolint: bla.", linters = list(bla = assignment_linter()), diff --git a/tests/testthat/test-make_linter_from_regex.R b/tests/testthat/test-make_linter_from_regex.R index 8dc53db04..b83e0e25c 100644 --- a/tests/testthat/test-make_linter_from_regex.R +++ b/tests/testthat/test-make_linter_from_regex.R @@ -1,4 +1,4 @@ -test_that("make_linter_from_regex works", { # nofuzz +test_that("make_linter_from_regex works", { # nofuzz: assignment linter <- lintr:::make_linter_from_regex("-", "style", "Silly lint.")() expect_lint("a <- 2L", "Silly lint.", linter) expect_no_lint("a = '2-3'", linter) diff --git a/tests/testthat/test-nested_pipe_linter.R b/tests/testthat/test-nested_pipe_linter.R index f79dbd3e6..0af5ead5f 100644 --- a/tests/testthat/test-nested_pipe_linter.R +++ b/tests/testthat/test-nested_pipe_linter.R @@ -14,7 +14,7 @@ test_that("nested_pipe_linter skips allowed usages", { ) # pipes fitting on one line can be ignored - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection "bind_rows(a %>% select(b), c %>% select(b))", linter ) @@ -25,7 +25,7 @@ test_that("nested_pipe_linter skips allowed usages", { expect_no_lint("switch(x, a = x, x %>% foo())", linter) # inline switch inputs are not linted - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection trim_some(" switch( x %>% foo(), @@ -128,7 +128,7 @@ test_that("Native pipes are handled as well", { linter_inline <- nested_pipe_linter(allow_inline = FALSE) lint_msg <- rex::rex("Don't nest pipes inside other calls.") - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection "bind_rows(a |> select(b), c |> select(b))", linter ) @@ -150,7 +150,7 @@ test_that("Native pipes are handled as well", { ) }) -test_that("lints vectorize", { # nofuzz +test_that("lints vectorize", { # nofuzz: comment_injection lint_msg <- rex::rex("Don't nest pipes inside other calls.") lines <- trim_some("{ diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index 3bb9cbbb9..331bf7122 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -883,7 +883,7 @@ test_that("dplyr's .env-specified objects are marked as 'used'", { skip_if_not_installed("rlang") linter <- object_usage_linter() - expect_lint( # nofuzz + expect_lint( # nofuzz: dollar_at trim_some(" foo <- function(df) { source <- 1 diff --git a/tests/testthat/test-one_call_pipe_linter.R b/tests/testthat/test-one_call_pipe_linter.R index 9cf7a60e9..89f750724 100644 --- a/tests/testthat/test-one_call_pipe_linter.R +++ b/tests/testthat/test-one_call_pipe_linter.R @@ -12,7 +12,7 @@ test_that("one_call_pipe_linter skips allowed usages", { expect_no_lint("x %<>% as.character()", linter) }) -# nofuzz start +# fuzzer disable: pipe test_that("one_call_pipe_linter blocks simple disallowed usages", { linter <- one_call_pipe_linter() lint_msg <- rex::rex("Avoid pipe %>% for expressions with only a single call.") @@ -25,7 +25,7 @@ test_that("one_call_pipe_linter blocks simple disallowed usages", { # nested case expect_lint("x %>% inner_join(y %>% filter(is_treatment))", lint_msg, linter) }) -# nofuzz end +# fuzzer enable: pipe test_that("one_call_pipe_linter skips data.table chains", { linter <- one_call_pipe_linter() @@ -52,7 +52,7 @@ test_that("one_call_pipe_linter treats all pipes equally", { expect_no_lint('data %>% filter(type == "console") %$% obscured_id %>% unique()', linter) }) -test_that("multiple lints are generated correctly", { # nofuzz +test_that("multiple lints are generated correctly", { # nofuzz: pipe expect_lint( trim_some("{ a %>% b() @@ -74,7 +74,7 @@ test_that("Native pipes are handled as well", { linter <- one_call_pipe_linter() - expect_lint( # nofuzz + expect_lint( # nofuzz: pipe "x |> foo()", rex::rex("Avoid pipe |> for expressions with only a single call."), linter @@ -84,7 +84,7 @@ test_that("Native pipes are handled as well", { expect_no_lint("x |> foo() %>% bar()", linter) expect_no_lint("x %>% foo() |> bar()", linter) - expect_lint( # nofuzz + expect_lint( # nofuzz: pipe trim_some("{ a %>% b() c |> d() diff --git a/tests/testthat/test-paren_body_linter.R b/tests/testthat/test-paren_body_linter.R index d82c1738c..2bdd38b6d 100644 --- a/tests/testthat/test-paren_body_linter.R +++ b/tests/testthat/test-paren_body_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection testthat::test_that("paren_body_linter returns correct lints", { linter <- paren_body_linter() lint_msg <- rex::rex("Put a space between a right parenthesis and a body expression.") @@ -96,4 +96,4 @@ test_that("function shorthand is handled", { expect_lint("\\()test", lint_msg, linter) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-pipe_consistency_linter.R b/tests/testthat/test-pipe_consistency_linter.R index 57c6df83b..ee38f6696 100644 --- a/tests/testthat/test-pipe_consistency_linter.R +++ b/tests/testthat/test-pipe_consistency_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: pipe test_that("pipe_consistency skips allowed usage", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter() @@ -161,4 +161,4 @@ test_that("pipe_consistency_linter works with other magrittr pipes", { linter ) }) -# nofuzz end +# fuzzer enable: pipe diff --git a/tests/testthat/test-pipe_continuation_linter.R b/tests/testthat/test-pipe_continuation_linter.R index 470ae9c5d..adb4efd3b 100644 --- a/tests/testthat/test-pipe_continuation_linter.R +++ b/tests/testthat/test-pipe_continuation_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("pipe-continuation correctly handles stand-alone expressions", { linter <- pipe_continuation_linter() lint_msg <- rex::rex("Put a space before `%>%` and a new line after it,") @@ -202,4 +202,4 @@ local({ .cases = cases ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-return_linter.R b/tests/testthat/test-return_linter.R index 9cb9a23a6..4ae026b3d 100644 --- a/tests/testthat/test-return_linter.R +++ b/tests/testthat/test-return_linter.R @@ -704,7 +704,7 @@ test_that("except= and except_regex= combination works", { ) }) -test_that("return_linter skips brace-wrapped inline functions", { # nofuzz +test_that("return_linter skips brace-wrapped inline functions", { # nofuzz: comment_injection expect_no_lint("function(x) { sum(x) }", return_linter(return_style = "explicit")) }) diff --git a/tests/testthat/test-semicolon_linter.R b/tests/testthat/test-semicolon_linter.R index f11d7bbb8..03a02d5a4 100644 --- a/tests/testthat/test-semicolon_linter.R +++ b/tests/testthat/test-semicolon_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("semicolon_linter skips allowed usages", { linter <- semicolon_linter() @@ -152,4 +152,4 @@ test_that("Compound semicolons only", { fixed = TRUE ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-spaces_inside_linter.R b/tests/testthat/test-spaces_inside_linter.R index e2c93329e..02e7e05e4 100644 --- a/tests/testthat/test-spaces_inside_linter.R +++ b/tests/testthat/test-spaces_inside_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("spaces_inside_linter skips allowed usages", { linter <- spaces_inside_linter() @@ -244,4 +244,4 @@ test_that("spaces_inside_linter blocks disallowed usages with a pipe", { test_that("terminal missing keyword arguments are OK", { expect_no_lint("alist(missing_arg = )", spaces_inside_linter()) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-spaces_left_parentheses_linter.R b/tests/testthat/test-spaces_left_parentheses_linter.R index 6d7956daf..773f2fdb7 100644 --- a/tests/testthat/test-spaces_left_parentheses_linter.R +++ b/tests/testthat/test-spaces_left_parentheses_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("spaces_left_parentheses_linter skips allowed usages", { linter <- spaces_left_parentheses_linter() @@ -112,4 +112,4 @@ test_that("lints vectorize", { spaces_left_parentheses_linter() ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-trailing_blank_lines_linter.R b/tests/testthat/test-trailing_blank_lines_linter.R index f6060e0ec..ab9175272 100644 --- a/tests/testthat/test-trailing_blank_lines_linter.R +++ b/tests/testthat/test-trailing_blank_lines_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("trailing_blank_lines_linter doesn't block allowed usages", { linter <- trailing_blank_lines_linter() @@ -159,4 +159,4 @@ test_that("blank lines in knitr chunks produce lints", { linters = linter ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-trailing_whitespace_linter.R b/tests/testthat/test-trailing_whitespace_linter.R index 82e5e56bb..ab06650ca 100644 --- a/tests/testthat/test-trailing_whitespace_linter.R +++ b/tests/testthat/test-trailing_whitespace_linter.R @@ -1,4 +1,4 @@ -# nofuzz start +# fuzzer disable: comment_injection test_that("returns the correct linting", { linter <- trailing_whitespace_linter() lint_msg <- rex::rex("Remove trailing whitespace.") @@ -68,4 +68,4 @@ test_that("also handles trailing whitespace in string constants", { trailing_whitespace_linter(allow_in_strings = FALSE) ) }) -# nofuzz end +# fuzzer enable: comment_injection diff --git a/tests/testthat/test-undesirable_operator_linter.R b/tests/testthat/test-undesirable_operator_linter.R index 09ce92fb1..8085a6135 100644 --- a/tests/testthat/test-undesirable_operator_linter.R +++ b/tests/testthat/test-undesirable_operator_linter.R @@ -7,19 +7,19 @@ test_that("linter returns correct linting", { expect_no_lint("cat(\"10$\")", linter) expect_lint( "a <<- log(10)", - list(message = msg_assign, line_number = 1L, column_number = 3L), + list(msg_assign, line_number = 1L, column_number = 3L), linter ) - expect_lint( # nofuzz + expect_lint( # nofuzz: dollar_at "data$parsed == c(1, 2)", - list(message = msg_dollar, line_number = 1L, column_number = 5L), + list(msg_dollar, line_number = 1L, column_number = 5L), linter ) expect_no_lint("`%%`(10, 2)", linter) }) -test_that("undesirable_operator_linter handles '=' consistently", { # nofuzz +test_that("undesirable_operator_linter handles '=' consistently", { # nofuzz: assignment linter <- undesirable_operator_linter(op = c("=" = "As an alternative, use '<-'")) expect_lint("a = 2L", rex::rex("Avoid undesirable operator `=`."), linter) diff --git a/tests/testthat/test-unnecessary_nesting_linter.R b/tests/testthat/test-unnecessary_nesting_linter.R index 649a1c3ac..9286b65a0 100644 --- a/tests/testthat/test-unnecessary_nesting_linter.R +++ b/tests/testthat/test-unnecessary_nesting_linter.R @@ -311,7 +311,7 @@ test_that("unnecessary_nesting_linter passes for multi-line braced expressions", ) }) -test_that("unnecessary_nesting_linter skips if unbracing won't reduce nesting", { # nofuzz +test_that("unnecessary_nesting_linter skips if unbracing won't reduce nesting", { linter <- unnecessary_nesting_linter() @@ -817,7 +817,7 @@ patrick::with_parameters_test_that( ) ) -test_that("allow_functions= works", { # nofuzz '})' break-up by comment +test_that("allow_functions= works", { # nofuzz: comment_injection linter_default <- unnecessary_nesting_linter() linter_foo <- unnecessary_nesting_linter(allow_functions = "foo") expect_lint("foo(x, {y}, z)", "Reduce the nesting of this statement", linter_default) diff --git a/tests/testthat/test-unnecessary_placeholder_linter.R b/tests/testthat/test-unnecessary_placeholder_linter.R index d69cbf3ed..362f6deca 100644 --- a/tests/testthat/test-unnecessary_placeholder_linter.R +++ b/tests/testthat/test-unnecessary_placeholder_linter.R @@ -19,7 +19,7 @@ patrick::with_parameters_test_that( pipe = pipes ) -patrick::with_parameters_test_that( # nofuzz +patrick::with_parameters_test_that( # nofuzz: pipe "unnecessary_placeholder_linter blocks simple disallowed usages", { expect_lint( @@ -38,7 +38,7 @@ patrick::with_parameters_test_that( # nofuzz pipe = pipes ) -test_that("lints vectorize", { # nofuzz +test_that("lints vectorize", { # nofuzz: pipe lint_msg <- rex::rex("Don't use the placeholder (`.`) when it's not needed") expect_lint( diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index e251fd761..a90f0e881 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -55,7 +55,7 @@ test_that("unreachable_code_linter works in sub expressions", { linter ) - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection trim_some(" foo <- function(bar) { if (bar) { @@ -177,7 +177,7 @@ test_that("unreachable_code_linter works with next and break in sub expressions" linter ) - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection trim_some(" foo <- function(bar) { if (bar) { @@ -282,7 +282,7 @@ test_that("unreachable_code_linter passes on multi-line functions", { expect_no_lint(lines, unreachable_code_linter()) }) -test_that("unreachable_code_linter ignores comments on the same expression", { # nofuzz +test_that("unreachable_code_linter ignores comments on the same expression", { # nofuzz: comment_injection linter <- unreachable_code_linter() expect_no_lint( @@ -297,7 +297,7 @@ test_that("unreachable_code_linter ignores comments on the same expression", { # ) }) -test_that("unreachable_code_linter ignores comments on the same line", { # nofuzz +test_that("unreachable_code_linter ignores comments on the same line", { # nofuzz: comment_injection lines <- trim_some(" foo <- function(x) { return(y^2) # y^3 @@ -339,7 +339,7 @@ test_that("unreachable_code_linter finds unreachable comments", { ) }) -test_that("unreachable_code_linter finds expressions in the same line", { # nofuzz +test_that("unreachable_code_linter finds expressions in the same line", { # nofuzz: comment_injection msg <- rex::rex("Remove code and comments coming after return() or stop()") linter <- unreachable_code_linter() @@ -450,7 +450,7 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { lintr.exclude_end = "#\\s*TestNoLintEnd" )) - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: comment_injection trim_some(" foo <- function() { do_something diff --git a/tests/testthat/test-unused_import_linter.R b/tests/testthat/test-unused_import_linter.R index 4c2bac2ff..81e90ad4e 100644 --- a/tests/testthat/test-unused_import_linter.R +++ b/tests/testthat/test-unused_import_linter.R @@ -7,7 +7,7 @@ test_that("unused_import_linter lints as expected", { # SYMBOL usage is detected expect_no_lint("library(dplyr)\ndo.call(tibble, args = list(a = 1))", linter) # SPECIAL usage is detected - expect_no_lint( # nofuzz + expect_no_lint( # nofuzz: pipe trim_some(" library(magrittr) 1:3 %>% mean() From fd5dfbd5d97f64b4c8f9fe114ed63f9ce910af51 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 21:50:45 +0000 Subject: [PATCH 02/11] caught a legit omission --- .dev/ast_fuzz_test.R | 2 +- R/any_duplicated_linter.R | 69 +++++++++++---------- tests/testthat/test-any_duplicated_linter.R | 25 +++++--- 3 files changed, 55 insertions(+), 41 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index c35cc0d98..e919f848d 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -169,5 +169,5 @@ if (length(invalid_failures) > 0L) { ) cat(sprintf("%d fuzzed tests failed unexpectedly!\n", length(invalid_failures))) print(invalid_failures) - stop("Use # nofuzz [start|end] to mark false positives or fix any bugs.") + stop("Fix any bugs, or use '# nofuzz'/'# fuzzer [dis|en]able' to mark false positives.") } diff --git a/R/any_duplicated_linter.R b/R/any_duplicated_linter.R index bada723cb..1cf77ef5a 100644 --- a/R/any_duplicated_linter.R +++ b/R/any_duplicated_linter.R @@ -52,42 +52,40 @@ any_duplicated_linter <- function() { # the final parent::expr/expr gets us to the expr on the other side of EQ; # this lets us match on either side of EQ, where following-sibling # assumes we are before EQ, preceding-sibling assumes we are after EQ. - length_unique_xpath_parts <- glue(" - //{ c('EQ', 'NE', 'GT', 'LT') } - /parent::expr - /expr[ - expr[1][SYMBOL_FUNCTION_CALL[text() = 'length']] - and expr[expr[1][ - SYMBOL_FUNCTION_CALL[text() = 'unique'] - and ( - following-sibling::expr = - parent::expr - /parent::expr - /parent::expr - /expr - /expr[1][SYMBOL_FUNCTION_CALL[text()= 'length']] - /following-sibling::expr - or - following-sibling::expr[OP-DOLLAR or LBB]/expr[1] = - parent::expr - /parent::expr - /parent::expr - /expr - /expr[1][SYMBOL_FUNCTION_CALL[text()= 'nrow']] - /following-sibling::expr - ) - ]] - ] - ") - length_unique_xpath <- paste(length_unique_xpath_parts, collapse = " | ") + length_comparison_xpath <- " + parent::expr + /parent::expr[expr/SYMBOL_FUNCTION_CALL[text() = 'length']] + /parent::expr[EQ or NE or GT or LT] + " + length_unique_xpath <- " + expr/expr/expr[1][ + SYMBOL_FUNCTION_CALL[text() = 'unique'] + and ( + following-sibling::expr = + parent::expr + /parent::expr + /parent::expr + /expr + /expr[1][SYMBOL_FUNCTION_CALL[text() = 'length']] + /following-sibling::expr + or + following-sibling::expr[OP-DOLLAR or LBB]/expr[1] = + parent::expr + /parent::expr + /parent::expr + /expr + /expr[1][SYMBOL_FUNCTION_CALL[text() = 'nrow']] + /following-sibling::expr + ) + ]" - uses_nrow_xpath <- "./parent::expr/expr/expr[1]/SYMBOL_FUNCTION_CALL[text() = 'nrow']" + uses_nrow_xpath <- "./expr/expr[1]/SYMBOL_FUNCTION_CALL[text() = 'nrow']" Linter(linter_level = "expression", function(source_expression) { - xml <- source_expression$xml_parsed_content - xml_calls <- source_expression$xml_find_function_calls("any") + any_calls <- source_expression$xml_find_function_calls("any") + unique_calls <- source_expression$xml_find_function_calls("unique") - any_duplicated_expr <- xml_find_all(xml_calls, any_duplicated_xpath) + any_duplicated_expr <- xml_find_all(any_calls, any_duplicated_xpath) any_duplicated_lints <- xml_nodes_to_lints( any_duplicated_expr, source_expression = source_expression, @@ -95,7 +93,12 @@ any_duplicated_linter <- function() { type = "warning" ) - length_unique_expr <- xml_find_all(xml, length_unique_xpath) + in_length_comparison <- !is.na(xml_find_first(unique_calls, length_comparison_xpath)) + unique_calls <- strip_comments_from_subtree( + xml_parent(xml_parent(xml_parent(unique_calls[in_length_comparison]))) + ) + is_length_unique <- !is.na(xml_find_first(unique_calls, length_unique_xpath)) + length_unique_expr <- unique_calls[is_length_unique] lint_message <- ifelse( is.na(xml_find_first(length_unique_expr, uses_nrow_xpath)), "anyDuplicated(x) == 0L is better than length(unique(x)) == length(x).", diff --git a/tests/testthat/test-any_duplicated_linter.R b/tests/testthat/test-any_duplicated_linter.R index ac538c3c2..50c8c64dc 100644 --- a/tests/testthat/test-any_duplicated_linter.R +++ b/tests/testthat/test-any_duplicated_linter.R @@ -2,12 +2,12 @@ test_that("any_duplicated_linter skips allowed usages", { linter <- any_duplicated_linter() - expect_lint("x <- any(y)", NULL, linter) - expect_lint("y <- duplicated(z)", NULL, linter) + expect_no_lint("x <- any(y)", linter) + expect_no_lint("y <- duplicated(z)", linter) # extended usage of any is not covered - expect_lint("any(duplicated(y), b)", NULL, linter) - expect_lint("any(b, duplicated(y))", NULL, linter) + expect_no_lint("any(duplicated(y), b)", linter) + expect_no_lint("any(b, duplicated(y))", linter) }) test_that("any_duplicated_linter blocks simple disallowed usages", { @@ -29,12 +29,13 @@ test_that("any_duplicated_linter catches length(unique()) equivalencies too", { # non-matches ## different variable - expect_lint("length(unique(x)) == length(y)", NULL, linter) + expect_no_lint("length(unique(x)) == length(y)", linter) ## different table - expect_lint("length(unique(DF$x)) == nrow(DT)", NULL, linter) - expect_lint("length(unique(l1$DF$x)) == nrow(l2$DF)", NULL, linter) + expect_no_lint("length(unique(DF$x)) == nrow(DT)", linter) + expect_no_lint("length(unique(l1$DF$x)) == nrow(l2$DF)", linter) # lintable usage + #debug(linter) expect_lint("length(unique(x)) == length(x)", lint_msg_x, linter) # argument order doesn't matter expect_lint("length(x) == length(unique(x))", lint_msg_x, linter) @@ -44,6 +45,16 @@ test_that("any_duplicated_linter catches length(unique()) equivalencies too", { # match with nesting too expect_lint("nrow(l$DF) == length(unique(l$DF[['col']]))", lint_msg_df, linter) + # including under comment torture + expect_lint( + trim_some(" + nrow(l$ # comment + DF) == length(unique(l$DF[['col']])) + "), + lint_msg_df, + linter + ) + # !=, <, and > usages are all alternative ways of writing a test for dupes # technically, the direction of > / < matter, but writing # length(unique(x)) > length(x) doesn't seem like it would ever happen. From c20a891e9da6383ba78d597a44e9e3e6dd3cc125 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 22:47:32 +0000 Subject: [PATCH 03/11] another legit omission --- R/keyword_quote_linter.R | 2 +- tests/testthat/test-keyword_quote_linter.R | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/R/keyword_quote_linter.R b/R/keyword_quote_linter.R index 9426dcaa2..bc43456dc 100644 --- a/R/keyword_quote_linter.R +++ b/R/keyword_quote_linter.R @@ -136,7 +136,7 @@ keyword_quote_linter <- function() { ) extraction_expr <- extraction_expr[invalid_extraction_quoting] - extractor <- xml_find_chr(extraction_expr, "string(preceding-sibling::*[1])") + extractor <- xml_find_chr(extraction_expr, "string(preceding-sibling::*[not(self::COMMENT)][1])") gen_extractor <- ifelse(extractor == "$", "[[", "slot()") extraction_lints <- xml_nodes_to_lints( diff --git a/tests/testthat/test-keyword_quote_linter.R b/tests/testthat/test-keyword_quote_linter.R index a814eee4b..895b022f7 100644 --- a/tests/testthat/test-keyword_quote_linter.R +++ b/tests/testthat/test-keyword_quote_linter.R @@ -129,6 +129,16 @@ test_that("keyword_quote_linter blocks quoted $, @ extractions", { # nofuzz: dol expect_lint("x@'foo' = 1", at_msg, linter) expect_lint("x@`foo` <- 1", at_msg, linter) expect_lint("x@`foo` = 1", at_msg, linter) + + # comment torture + expect_lint( + trim_some(" + x@ # comment + `foo` <- 1 + "), + at_msg, + linter + ) }) test_that("multiple lints are generated correctly", { # nofuzz: dollar_at From 12d465b99203187c527d9c682b9f8618a9ca2212 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 22:47:47 +0000 Subject: [PATCH 04/11] more nofuzz --- tests/testthat/test-get_source_expressions.R | 2 +- tests/testthat/test-indentation_linter.R | 2 +- tests/testthat/test-is_numeric_linter.R | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-get_source_expressions.R b/tests/testthat/test-get_source_expressions.R index 6a5a35367..3e290d1f2 100644 --- a/tests/testthat/test-get_source_expressions.R +++ b/tests/testthat/test-get_source_expressions.R @@ -108,7 +108,7 @@ test_that("Can read non UTF-8 file", { withr::local_options(list(lintr.linter_file = tempfile())) proj_dir <- test_path("dummy_projects", "project") withr::local_dir(proj_dir) - expect_no_lint( # nofuzz: assignment + expect_no_lint( # nofuzz: assignment comment_injection file = "cp1252.R", linters = list() ) diff --git a/tests/testthat/test-indentation_linter.R b/tests/testthat/test-indentation_linter.R index 51de471aa..d5b5042df 100644 --- a/tests/testthat/test-indentation_linter.R +++ b/tests/testthat/test-indentation_linter.R @@ -153,7 +153,7 @@ test_that("indentation linter flags improper closing curly braces", { ) }) -test_that("function argument indentation works in tidyverse-style", { +test_that("function argument indentation works in tidyverse-style", { # nofuzz: function_lambda linter <- indentation_linter() expect_no_lint( trim_some(" diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index 1887249d2..55a82a3ab 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -28,7 +28,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { expect_lint("is.integer(x) || is.numeric(x)", lint_msg, linter) # identical expressions match too - expect_lint( # fuzzer disable: dollar_at + expect_lint( # nofuzz: dollar_at "is.integer(DT$x) || is.numeric(DT$x)", lint_msg, linter From 0d45ec67bc4e21e7f097268a831422a005d48ef6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 22:47:55 +0000 Subject: [PATCH 05/11] typo --- .dev/ast_fuzz_test.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index e919f848d..8fb0c6968 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -92,7 +92,7 @@ for (test_file in list.files("tests/testthat", pattern = "^test-", full.names = stop("Unable to parse any expression starting from line ", one_line) } comment_txt <- subset(pd, line1 == one_line & token == "COMMENT", select = "text", drop = TRUE) - deactivated <- get_str(comment_text) + deactivated <- get_str(comment_txt) test_lines <- c( head(test_lines, one_line - 1L), sprintf("deactivate_fuzzers('%s')", deactivated), @@ -171,3 +171,5 @@ if (length(invalid_failures) > 0L) { print(invalid_failures) stop("Fix any bugs, or use '# nofuzz'/'# fuzzer [dis|en]able' to mark false positives.") } + +browser() From 36f645e2caaab04379103f3cfdf3bc696f797655 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 23:40:58 +0000 Subject: [PATCH 06/11] more robustness --- .dev/ast_fuzz_test.R | 34 ++++++++++++-------- .dev/maybe_fuzz_content.R | 9 +++++- tests/testthat/test-cache.R | 2 -- tests/testthat/test-exclusions.R | 10 +++--- tests/testthat/test-get_source_expressions.R | 2 +- 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index 8fb0c6968..f33d54590 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -76,30 +76,36 @@ get_str <- \(x) tail(unlist(strsplit(x, ": ", fixed = TRUE)), 1L) test_restorations <- list() for (test_file in list.files("tests/testthat", pattern = "^test-", full.names = TRUE)) { test_lines <- readLines(test_file) - one_expr_idx <- grep("# nofuzz:", test_lines) + one_expr_idx <- grep("# nofuzz", test_lines) range_start_idx <- grep("^\\s*# fuzzer disable:", test_lines) if (length(one_expr_idx) == 0L && length(range_start_idx) == 0L) next test_original <- test_lines pd <- getParseData(parse(test_file)) - for (one_line in rev(one_expr_idx)) { - end_line <- one_line - while (end_line <= length(test_lines) && !can_parse(test_lines[one_line:end_line])) { + for (start_line in rev(one_expr_idx)) { + end_line <- start_line + while (end_line <= length(test_lines) && !can_parse(test_lines[start_line:end_line])) { end_line <- end_line + 1L } if (end_line > length(test_lines)) { - stop("Unable to parse any expression starting from line ", one_line) + stop("Unable to parse any expression starting from line ", start_line) + } + comment_txt <- subset(pd, line1 == start_line & token == "COMMENT", select = "text", drop = TRUE) + # blanket disable means the test cannot be run. this happens e.g. for tests of encoding + # that are too complicated to deal with in this GHA. + if (comment_txt == "# nofuzz") { + test_lines[start_line:end_line] <- "" + } else { + deactivated <- get_str(comment_txt) + test_lines <- c( + head(test_lines, start_line - 1L), + sprintf("deactivate_fuzzers('%s')", deactivated), + test_lines[start_line:end_line], + sprintf("activate_fuzzers('%s')", deactivated), + tail(test_lines, -end_line) + ) } - comment_txt <- subset(pd, line1 == one_line & token == "COMMENT", select = "text", drop = TRUE) - deactivated <- get_str(comment_txt) - test_lines <- c( - head(test_lines, one_line - 1L), - sprintf("deactivate_fuzzers('%s')", deactivated), - test_lines[one_line:end_line], - sprintf("activate_fuzzers('%s')", deactivated), - tail(test_lines, -end_line) - ) } if (length(one_expr_idx)) { diff --git a/.dev/maybe_fuzz_content.R b/.dev/maybe_fuzz_content.R index b72aa3cda..4033e85b8 100644 --- a/.dev/maybe_fuzz_content.R +++ b/.dev/maybe_fuzz_content.R @@ -1,4 +1,11 @@ maybe_fuzz_content <- function(file, lines) { + # some tests (esp. involving encoding) have to be skipped entirely + # since the logic here is not able to copy over relevant configs/settings + active_fuzzers <- .fuzzers$active + if (length(active_fuzzers) == 0L) { + return(file) + } + if (is.null(file)) { new_file <- tempfile() con <- file(new_file, encoding = "UTF-8") @@ -9,7 +16,7 @@ maybe_fuzz_content <- function(file, lines) { file.copy(file, new_file, copy.mode = FALSE) } - apply_fuzzers(new_file, fuzzers = .fuzzers$active) + apply_fuzzers(new_file, fuzzers = active_fuzzers) new_file } diff --git a/tests/testthat/test-cache.R b/tests/testthat/test-cache.R index c84270b60..9763088b2 100644 --- a/tests/testthat/test-cache.R +++ b/tests/testthat/test-cache.R @@ -404,8 +404,6 @@ test_that("find_new_line returns the correct line if it is after the current lin expect_identical(lintr:::find_new_line(3L, "foobar3", t1), 3L) }) -# - test_that("lint with cache uses the provided relative cache directory", { # nofuzz: assignment path <- withr::local_tempdir("my_cache_dir") linter <- assignment_linter() diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index 3ace3d4f2..464f204a4 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -112,7 +112,7 @@ test_that("#1413: lint_dir properly excludes files", { test_that("#1442: is_excluded_files works if no global exclusions are specified", { # nofuzz: assignment withr::local_options(lintr.linter_file = "lintr_test_config") - tmp <- withr::local_tempdir() + withr::local_dir(withr::local_tempdir()) writeLines( trim_some(" @@ -125,7 +125,7 @@ test_that("#1442: is_excluded_files works if no global exclusions are specified" ) ) "), - file.path(tmp, "lintr_test_config") + "lintr_test_config" ) writeLines( @@ -138,19 +138,19 @@ test_that("#1442: is_excluded_files works if no global exclusions are specified" # long comment # comment "), - file.path(tmp, "bad.R") + "bad.R" ) # 3 lints: assignment_linter(), quotes_linter() and line_length_linter() expect_lint( - file = file.path(tmp, "bad.R"), + file = "bad.R", checks = list( list(linter = "assignment_linter", line_number = 1L), list(linter = "quotes_linter", line_number = 1L), list(linter = "line_length_linter", line_number = 1L) ) ) - expect_length(lint_dir(tmp), 3L) + expect_length(lint_dir(), 3L) }) test_that("next-line exclusion works", { # nofuzz: assignment diff --git a/tests/testthat/test-get_source_expressions.R b/tests/testthat/test-get_source_expressions.R index 3e290d1f2..975a152a2 100644 --- a/tests/testthat/test-get_source_expressions.R +++ b/tests/testthat/test-get_source_expressions.R @@ -108,7 +108,7 @@ test_that("Can read non UTF-8 file", { withr::local_options(list(lintr.linter_file = tempfile())) proj_dir <- test_path("dummy_projects", "project") withr::local_dir(proj_dir) - expect_no_lint( # nofuzz: assignment comment_injection + expect_no_lint( # nofuzz file = "cp1252.R", linters = list() ) From 732d1ff58fd0a1ec4df63e6cfdcee48951cab394 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 23:58:46 +0000 Subject: [PATCH 07/11] more nofuzz progress --- .dev/ast_fuzz_test.R | 2 -- tests/testthat/test-assignment_linter.R | 4 +++- tests/testthat/test-exclusions.R | 2 +- tests/testthat/test-get_source_expressions.R | 4 ++-- tests/testthat/test-indentation_linter.R | 4 ++-- tests/testthat/test-infix_spaces_linter.R | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index f33d54590..51dd4f8f6 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -177,5 +177,3 @@ if (length(invalid_failures) > 0L) { print(invalid_failures) stop("Fix any bugs, or use '# nofuzz'/'# fuzzer [dis|en]able' to mark false positives.") } - -browser() diff --git a/tests/testthat/test-assignment_linter.R b/tests/testthat/test-assignment_linter.R index ceab52ff3..623b57b43 100644 --- a/tests/testthat/test-assignment_linter.R +++ b/tests/testthat/test-assignment_linter.R @@ -67,7 +67,8 @@ test_that("arguments handle <<- and ->/->> correctly", { ) }) -test_that("arguments handle trailing assignment operators correctly", { # nofuzz: comment_injection +# fuzzer disable: comment_injection +test_that("arguments handle trailing assignment operators correctly", { linter_default <- assignment_linter() linter_no_trailing <- assignment_linter(allow_trailing = FALSE) expect_no_lint("x <- y", linter_no_trailing) @@ -213,6 +214,7 @@ test_that("allow_trailing interacts correctly with comments in braced expression linter ) }) +# fuzzer enable: comment_injection test_that("%<>% throws a lint", { expect_lint("x %<>% sum()", "Avoid the assignment pipe %<>%", assignment_linter()) diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index 464f204a4..85b9b8324 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -110,7 +110,7 @@ test_that("#1413: lint_dir properly excludes files", { expect_length(lint_dir(tmp), 0L) }) -test_that("#1442: is_excluded_files works if no global exclusions are specified", { # nofuzz: assignment +test_that("#1442: is_excluded_files works if no global exclusions are specified", { # nofuzz withr::local_options(lintr.linter_file = "lintr_test_config") withr::local_dir(withr::local_tempdir()) diff --git a/tests/testthat/test-get_source_expressions.R b/tests/testthat/test-get_source_expressions.R index 975a152a2..da1c111fb 100644 --- a/tests/testthat/test-get_source_expressions.R +++ b/tests/testthat/test-get_source_expressions.R @@ -104,11 +104,11 @@ test_that("Multi-byte character truncated by parser is ignored", { }) }) -test_that("Can read non UTF-8 file", { +test_that("Can read non UTF-8 file", { # nofuzz withr::local_options(list(lintr.linter_file = tempfile())) proj_dir <- test_path("dummy_projects", "project") withr::local_dir(proj_dir) - expect_no_lint( # nofuzz + expect_no_lint( file = "cp1252.R", linters = list() ) diff --git a/tests/testthat/test-indentation_linter.R b/tests/testthat/test-indentation_linter.R index d5b5042df..2b5cb8ce9 100644 --- a/tests/testthat/test-indentation_linter.R +++ b/tests/testthat/test-indentation_linter.R @@ -228,7 +228,7 @@ test_that("function argument indentation works in tidyverse-style", { # nofuzz: ) # anchor is correctly found with assignments as well - expect_no_lint( + expect_no_lint( # nofuzz: assignment trim_some(" test <- function(a = 1L, b = 2L) { @@ -323,7 +323,7 @@ test_that("function argument indentation works in always-hanging-style", { # nof ) # anchor is correctly found with assignments as well - expect_no_lint( + expect_no_lint( # nofuzz: assignment trim_some(" test <- function(a = 1L, b = 2L) { diff --git a/tests/testthat/test-infix_spaces_linter.R b/tests/testthat/test-infix_spaces_linter.R index 7a898e402..b0bdde324 100644 --- a/tests/testthat/test-infix_spaces_linter.R +++ b/tests/testthat/test-infix_spaces_linter.R @@ -191,7 +191,7 @@ test_that("mixed unary & binary operators aren't mis-lint", { ) }) -test_that("parse tags are accepted by exclude_operators", { +test_that("parse tags are accepted by exclude_operators", { # nofuzz: assignment expect_no_lint("sum(x, na.rm=TRUE)", infix_spaces_linter(exclude_operators = "EQ_SUB")) expect_no_lint("function(x, na.rm=TRUE) { }", infix_spaces_linter(exclude_operators = "EQ_FORMALS")) expect_no_lint("x=1", infix_spaces_linter(exclude_operators = "EQ_ASSIGN")) From 1b3b41d0816c38fd996fa657abddb7818452599f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 21:23:37 -0700 Subject: [PATCH 08/11] more nofuzz, some tidying, comments --- .dev/ast_fuzz_test.R | 11 +++++++++-- .dev/maybe_fuzz_content.R | 19 ++++++------------- tests/testthat/test-infix_spaces_linter.R | 2 +- tests/testthat/test-lint.R | 2 +- .../test-unnecessary_nesting_linter.R | 3 +-- tests/testthat/test-unreachable_code_linter.R | 4 ++-- 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index 51dd4f8f6..2db49e2a7 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -69,14 +69,21 @@ suppressMessages(pkgload::load_all()) can_parse <- \(lines) !inherits(tryCatch(parse(text = lines), error = identity), "error") get_str <- \(x) tail(unlist(strsplit(x, ": ", fixed = TRUE)), 1L) - # beware lazy eval: originally tried adding a withr::defer() in each iteration, but # this effectively only runs the last 'defer' expression as the names are only # evaluated at run-time. So instead keep track of all edits in this object. +# these have to be enabled/disabled at runtime as it's not possible to disentagle which +# fuzzer caused the error ex-post (and it might be the interaction of >1 at issue). +# an earlier approach was like the current 'nofuzz' -- just comment out the troublesome +# tests from being run at all. But that led to a very quickly growing set of tests being +# skipped totally, which also hid some issues that are surfaced by the current approach. +# Another idea would be to just leave the enable/disable calls as code in the test suite, +# but I prefer the current approach of leaving them as comments: (1) it's more consistent +# with the 'nolint' exclusion system and (2) it doesn't distract the casual reader as much. test_restorations <- list() for (test_file in list.files("tests/testthat", pattern = "^test-", full.names = TRUE)) { test_lines <- readLines(test_file) - one_expr_idx <- grep("# nofuzz", test_lines) + one_expr_idx <- grep("# nofuzz", test_lines, fixed = TRUE) range_start_idx <- grep("^\\s*# fuzzer disable:", test_lines) if (length(one_expr_idx) == 0L && length(range_start_idx) == 0L) next diff --git a/.dev/maybe_fuzz_content.R b/.dev/maybe_fuzz_content.R index 4033e85b8..2cdc0678f 100644 --- a/.dev/maybe_fuzz_content.R +++ b/.dev/maybe_fuzz_content.R @@ -1,11 +1,4 @@ maybe_fuzz_content <- function(file, lines) { - # some tests (esp. involving encoding) have to be skipped entirely - # since the logic here is not able to copy over relevant configs/settings - active_fuzzers <- .fuzzers$active - if (length(active_fuzzers) == 0L) { - return(file) - } - if (is.null(file)) { new_file <- tempfile() con <- file(new_file, encoding = "UTF-8") @@ -16,7 +9,7 @@ maybe_fuzz_content <- function(file, lines) { file.copy(file, new_file, copy.mode = FALSE) } - apply_fuzzers(new_file, fuzzers = active_fuzzers) + apply_fuzzers(new_file, fuzzers = .fuzzers$active) new_file } @@ -107,7 +100,7 @@ apply_fuzzers <- function(f, fuzzers) { return(invisible()) } - unedited <- lines <- readLines(f) + unedited <- lines <- readLines(f, warn = FALSE) for (fuzzer in fuzzers) { updated_lines <- fuzzer(pd, lines) if (is.null(updated_lines)) next # skip some I/O if we can @@ -126,11 +119,11 @@ apply_fuzzers <- function(f, fuzzers) { .fuzzers <- new.env() .fuzzers$active <- list( - function_lambda = function_lambda_fuzzer, - pipe = pipe_fuzzer, - dollar_at = dollar_at_fuzzer, + assignment = assignment_fuzzer, comment_injection = comment_injection_fuzzer, - assignment = assignment_fuzzer + dollar_at = dollar_at_fuzzer, + function_lambda = function_lambda_fuzzer, + pipe = pipe_fuzzer ) .fuzzers$inactive <- list() diff --git a/tests/testthat/test-infix_spaces_linter.R b/tests/testthat/test-infix_spaces_linter.R index b0bdde324..8104095ca 100644 --- a/tests/testthat/test-infix_spaces_linter.R +++ b/tests/testthat/test-infix_spaces_linter.R @@ -213,7 +213,7 @@ test_that("parse tags are accepted by exclude_operators", { # nofuzz: assignment expect_lint(text, list(col_formals, col_sub), infix_spaces_linter(exclude_operators = "EQ_ASSIGN")) }) -test_that("lints vectorize", { +test_that("lints vectorize", { # nofuzz: assignment lint_msg <- rex::rex("Put spaces around all infix operators.") expect_lint( diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 941ba3794..bacbeec3b 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -146,7 +146,7 @@ test_that("lint() results from file or text should be consistent", { expect_identical(lint_from_file, lint_from_text) }) -test_that("exclusions work with custom linter names", { # nofuzz: assignment +test_that("exclusions work with custom linter names", { # nofuzz: assignment comment_injection expect_no_lint( "a = 2 # nolint: bla.", linters = list(bla = assignment_linter()), diff --git a/tests/testthat/test-unnecessary_nesting_linter.R b/tests/testthat/test-unnecessary_nesting_linter.R index 9286b65a0..fcc5d3720 100644 --- a/tests/testthat/test-unnecessary_nesting_linter.R +++ b/tests/testthat/test-unnecessary_nesting_linter.R @@ -311,8 +311,7 @@ test_that("unnecessary_nesting_linter passes for multi-line braced expressions", ) }) -test_that("unnecessary_nesting_linter skips if unbracing won't reduce nesting", { - +test_that("unnecessary_nesting_linter skips if unbracing won't reduce nesting", { # nofuzz: comment_injection linter <- unnecessary_nesting_linter() expect_no_lint( diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index a90f0e881..25447e3a1 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -442,7 +442,7 @@ test_that("unreachable_code_linter ignores code after foo$stop(), which might be ) }) -test_that("unreachable_code_linter ignores terminal nolint end comments", { +test_that("unreachable_code_linter ignores terminal nolint end comments", { # nofuzz: assignment comment_injection linter <- unreachable_code_linter() withr::local_options(list( @@ -450,7 +450,7 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { lintr.exclude_end = "#\\s*TestNoLintEnd" )) - expect_no_lint( # nofuzz: comment_injection + expect_no_lint( trim_some(" foo <- function() { do_something From 436da46097b7c607e0389c24622174870d50937e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 21:35:32 -0700 Subject: [PATCH 09/11] live one on the line! --- R/is_numeric_linter.R | 18 +++++++-------- tests/testthat/test-is_numeric_linter.R | 29 ++++++++++++++++++------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R index ed7f35309..db23c198f 100644 --- a/R/is_numeric_linter.R +++ b/R/is_numeric_linter.R @@ -42,15 +42,12 @@ is_numeric_linter <- function() { # testing things like is.numeric(x) || is.integer(x) or_xpath <- glue(" - //OR2 - /parent::expr[ - expr/{is_numeric_expr} - and expr/{is_integer_expr} - and - expr/{is_numeric_expr}/following-sibling::expr[1] - = expr/{is_integer_expr}/following-sibling::expr[1] - ] + //OR2/parent::expr[expr/{is_numeric_expr} and expr/{is_integer_expr}] ") + node_match_xpath <- glue("self::*[ + expr/{is_numeric_expr}/following-sibling::expr[1] + = expr/{is_integer_expr}/following-sibling::expr[1] + ]") # testing class(x) %in% c("numeric", "integer") class_xpath <- " @@ -69,9 +66,10 @@ is_numeric_linter <- function() { Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content - or_expr <- xml_find_all(xml, or_xpath) + or_expr <- strip_comments_from_subtree(xml_find_all(xml, or_xpath)) + expr_match <- !is.na(xml_find_first(or_expr, node_match_xpath)) or_lints <- xml_nodes_to_lints( - or_expr, + or_expr[expr_match], source_expression = source_expression, lint_message = paste( "Use `is.numeric(x)` instead of the equivalent `is.numeric(x) || is.integer(x)`.", diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index 55a82a3ab..454be3430 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -30,18 +30,31 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { # identical expressions match too expect_lint( # nofuzz: dollar_at "is.integer(DT$x) || is.numeric(DT$x)", - lint_msg, + lint_msg, linter ) # line breaks don't matter - lines <- trim_some(" - if ( - is.integer(x) - || is.numeric(x) - ) TRUE - ") - expect_lint(lines, lint_msg, linter) + expect_lint( + trim_some(" + if ( + is.integer(x) + || is.numeric(x) + ) TRUE + "), + lint_msg, + linter + ) + + # nor do comments + expect_lint( + trim_some(" + is.integer(DT$ #comment + x) || is.numeric(DT$x) + "), + lint_msg, + linter + ) # caught when nesting expect_lint("all(y > 5) && (is.integer(x) || is.numeric(x))", lint_msg, linter) From 87e85887fe62309a5b0a9f091e1e4fe4f5812212 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 21:48:45 -0700 Subject: [PATCH 10/11] more tidying & nofuzz stress testing --- .dev/ast_fuzz_test.R | 3 +-- NEWS.md | 3 +++ tests/testthat/test-exclusions.R | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.dev/ast_fuzz_test.R b/.dev/ast_fuzz_test.R index 2db49e2a7..e2b1aadf7 100644 --- a/.dev/ast_fuzz_test.R +++ b/.dev/ast_fuzz_test.R @@ -154,8 +154,7 @@ all_classes <- unlist(lapply( reporter$get_results(), \(test) lapply(test$results, \(x) class(x)[1L]) )) -cat("Summary of test statuses:\n") -print(table(all_classes)) +print(table(`Summary of test statuses:` = all_classes)) # ignore any test that failed for expected reasons, e.g. some known lint metadata changes # about line numbers or the contents of the line. this saves us having to pepper tons of diff --git a/NEWS.md b/NEWS.md index ab976b6cc..790b89214 100644 --- a/NEWS.md +++ b/NEWS.md @@ -33,6 +33,7 @@ * `boolean_arithmetic_linter()` finds many more cases like `sum(x | y) == 0` where the total of a known-logical vector is compared to 0 (#1580, @MichaelChirico). * New argument `include_s4_slots` for the `xml_find_function_calls()` entry in the `get_source_expressions()` to govern whether calls of the form `s4Obj@fun()` are included in the result (#2820, @MichaelChirico). * General handling of logic around where comments can appear in code has been improved (#2822, @MichaelChirico). In many cases, this is a tiny robustness fix for weird edge cases unlikely to be found in practice, but in others, this improves practical linter precision (reduced false positives and/or false negatives). The affected linters (with annotations for changes noteworthy enough to have gotten a dedicated bug) are: + + `any_duplicated_linter()` + `brace_linter()` + `coalesce_linter()` + `comparison_negation_linter()` #2826 @@ -42,6 +43,8 @@ + `if_switch_linter()` + `ifelse_censor_linter()` #2826 + `implicit_assignment_linter()` + + `is_numeric_linter()` + + `keyword_quote_linter()` + `length_test_linter()` + `literal_coercion_linter()` #2824 + `matrix_apply_linter()` #2825 diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index 85b9b8324..31d9784c5 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -56,7 +56,7 @@ test_that("it gives the expected error message when there is mismatch between mu ) }) -test_that("partial matching works for exclusions but warns if no linter found", { # nofuzz: assignment +test_that("partial matching works for exclusions but warns if no linter found", { # nofuzz: assignment comment_injection lintr:::read_settings(NULL) expect_warning( From 357f56f4ed4e07a95d6f3a01e55ea9ca44cce605 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 11 Mar 2025 22:11:01 -0700 Subject: [PATCH 11/11] missed nofuzz in new test --- tests/testthat/test-is_numeric_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index 454be3430..e22fde8ee 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -47,7 +47,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { ) # nor do comments - expect_lint( + expect_lint( # nofuzz: dollar_at trim_some(" is.integer(DT$ #comment x) || is.numeric(DT$x)