Skip to content

Commit

Permalink
set structure() as an undesirable function by default (#2228)
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaelChirico authored Oct 5, 2023
1 parent 62858ba commit b49ee9d
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 83 deletions.
2 changes: 1 addition & 1 deletion .lintr
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ linters: linters_with_defaults(
sort_linter(),
sprintf_linter(),
strings_as_factors_linter(),
undesirable_function_linter(c(Sys.setenv = NA_character_, mapply = NA_character_)),
undesirable_function_linter(c(Sys.setenv = NA_character_, mapply = NA_character_, structure = NA_character_)),
unnecessary_nested_if_linter(),
unnecessary_lambda_linter(),
unnecessary_concatenation_linter(allow_single_expression = FALSE),
Expand Down
2 changes: 1 addition & 1 deletion NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
* `unreachable_code_linter()` checks for code inside `if (FALSE)` and other conditional loops with deterministically false conditions (#1428, @ME0265).
* `implicit_assignment_linter()` gains an argument `allow_lazy` (default `FALSE`) that allows optionally skipping lazy assignments like `A && (B <- foo(A))` (#2016, @MichaelChirico).
* `unused_import_linter()` gains an argument `interpret_glue` (default `TRUE`) paralleling that in `object_usage_linter()` to toggle whether `glue::glue()` expressions should be inspected for exported object usage (#2042, @MichaelChirico).
* `default_undesirable_functions` is updated to also include `Sys.unsetenv()` (#2192, @IndrajeetPatil).
* `default_undesirable_functions` is updated to also include `Sys.unsetenv()` and `structure()` (#2192 and #2228, @IndrajeetPatil and @MichaelChirico).
* Linters with logic around the magrittr pipe `%>%` consistently apply it to the other pipes `%!>%`, `%T>%`, `%<>%` (and possibly `%$%`) where appropriate (#2008, @MichaelChirico).
+ `brace_linter()`
+ `pipe_call_linter()`
Expand Down
11 changes: 8 additions & 3 deletions R/zzz.R
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ all_undesirable_functions <- modify_defaults(
untrace = paste(
"remove this likely leftover from debugging.",
"It is only useful for interactive debugging with trace()"
)
),
structure =
"Use class<-, names<-, and attr<- to set attributes"
)

#' @rdname default_undesirable_functions
Expand All @@ -194,12 +196,14 @@ default_undesirable_functions <- all_undesirable_functions[names(all_undesirable
"setwd",
"sink",
"source",
"structure",
"Sys.setenv",
"Sys.setlocale",
"Sys.unsetenv",
"trace",
"undebug",
"untrace"
"untrace",
NULL
)]

#' @rdname default_undesirable_functions
Expand Down Expand Up @@ -232,7 +236,8 @@ all_undesirable_operators <- modify_defaults(
default_undesirable_operators <- all_undesirable_operators[names(all_undesirable_operators) %in% c(
":::",
"<<-",
"->>"
"->>",
NULL
)]

#' Default lintr settings
Expand Down
52 changes: 25 additions & 27 deletions tests/testthat/test-checkstyle_output.R
Original file line number Diff line number Diff line change
@@ -1,33 +1,31 @@
test_that("return lint report as checkstyle xml", {
lints <- structure(
list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "foo"
),
Lint(
filename = "test_file",
line_number = 2L,
column_number = 1L,
type = "style",
line = "another line",
message = "bar"
),
Lint(
filename = "test_file2",
line_number = 1L,
column_number = 1L,
type = "warning",
line = "yet another line",
message = "baz"
)
lints <- list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "foo"
),
class = "lints"
Lint(
filename = "test_file",
line_number = 2L,
column_number = 1L,
type = "style",
line = "another line",
message = "bar"
),
Lint(
filename = "test_file2",
line_number = 1L,
column_number = 1L,
type = "warning",
line = "yet another line",
message = "baz"
)
)
class(lints) <- "lints"
tmp <- withr::local_tempfile()
checkstyle_output(lints, tmp)

Expand Down
5 changes: 4 additions & 1 deletion tests/testthat/test-error.R
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ test_that("returns the correct linting", {
expect_lint('"\\R"', msg_escape_char)
expect_lint('"\\A"', msg_escape_char)
expect_lint('"\\z"', msg_escape_char)
placeholder_linter <- function(...) NULL
class(placeholder_linter) <- "linter"
attr(placeholder_linter, "name") <- "null"
expect_lint(
"a <- 1
function() {
b",
rex::rex("unexpected end of input"),
structure(function(...) NULL, class = "linter", name = "null")
placeholder_linter
)

linter <- equals_na_linter()
Expand Down
3 changes: 2 additions & 1 deletion tests/testthat/test-methods.R
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ test_that("as.data.frame.lints", {
)

# Convert lints to data.frame
lints <- structure(list(l1, l2), class = "lints")
lints <- list(l1, l2)
class(lints) <- "lints"
df <- as.data.frame(lints)
expect_s3_class(df, "data.frame")

Expand Down
85 changes: 36 additions & 49 deletions tests/testthat/test-rstudio_markers.R
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,15 @@ test_that("it returns markers which match lints", {
mockery::stub(rstudio_source_markers, "rstudioapi::callFun", function(...) list(...))
mockery::stub(rstudio_source_markers, "rstudioapi::executeCommand", function(...) NULL)

lint1 <- structure(
list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
)
),
class = "lints"
)
lint1 <- list(Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
))
class(lint1) <- "lints"
lint1[[1L]]$linter <- "linter_name"

marker1 <- rstudio_source_markers(lint1)
Expand All @@ -27,26 +23,24 @@ test_that("it returns markers which match lints", {
expect_identical(marker1$markers[[1L]]$column, lint1[[1L]]$column_number)
expect_identical(marker1$markers[[1L]]$message, paste0("[", lint1[[1L]]$linter, "] ", lint1[[1L]]$message))

lint2 <- structure(
list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
),
Lint(
filename = "test_file2",
line_number = 10L,
column_number = 1L,
type = "warning",
message = "test a message"
)
lint2 <- list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
),
class = "lints"
Lint(
filename = "test_file2",
line_number = 10L,
column_number = 1L,
type = "warning",
message = "test a message"
)
)
class(lint2) <- "lints"
lint2[[1L]]$linter <- "linter_name"
lint2[[2L]]$linter <- "linter_name"
marker2 <- rstudio_source_markers(lint2)
Expand All @@ -64,20 +58,16 @@ test_that("it prepends the package path if it exists", {
mockery::stub(rstudio_source_markers, "rstudioapi::callFun", function(...) list(...))
mockery::stub(rstudio_source_markers, "rstudioapi::executeCommand", function(...) NULL)

lint3 <- structure(
list(
Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
)
),
class = "lints",
path = "test"
)
lint3 <- list(Lint(
filename = "test_file",
line_number = 1L,
column_number = 2L,
type = "error",
line = "a line",
message = "hi"
))
class(lint3) <- "lints"
attr(lint3, "path") <- "test"
lint3[[1L]]$linter <- "linter_name"
marker3 <- rstudio_source_markers(lint3)
expect_identical(marker3$name, "lintr")
Expand All @@ -95,10 +85,7 @@ test_that("it returns an empty list of markers if there are no lints", {
mockery::stub(rstudio_source_markers, "rstudioapi::callFun", function(...) list(...))
mockery::stub(rstudio_source_markers, "rstudioapi::executeCommand", function(...) NULL)

lint4 <- structure(
list(),
class = "lints"
)
lint4 <- `class<-`(list(), "lints")
marker4 <- rstudio_source_markers(lint4)
expect_identical(marker4$name, "lintr")
expect_identical(marker4$markers, list())
Expand Down

0 comments on commit b49ee9d

Please sign in to comment.