diff --git a/DESCRIPTION b/DESCRIPTION index 8b20c69ed..1f7264286 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -170,6 +170,7 @@ Collate: 'string_boundary_linter.R' 'strings_as_factors_linter.R' 'system_file_linter.R' + 'terminal_close_linter.R' 'trailing_blank_lines_linter.R' 'trailing_whitespace_linter.R' 'tree_utils.R' diff --git a/NAMESPACE b/NAMESPACE index f1ea599a2..9da6d5205 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -133,6 +133,7 @@ export(stopifnot_all_linter) export(string_boundary_linter) export(strings_as_factors_linter) export(system_file_linter) +export(terminal_close_linter) export(todo_comment_linter) export(trailing_blank_lines_linter) export(trailing_whitespace_linter) diff --git a/NEWS.md b/NEWS.md index ee042c597..4f53beb0e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,7 @@ * `stopifnot_all_linter()` discourages tests with `all()` like `stopifnot(all(x > 0))`; `stopifnot()` runs `all()` itself, and uses a better error message (part of #884, @MichaelChirico). * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico). +* `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup. ### Lint accuracy fixes: removing false positives diff --git a/R/terminal_close_linter.R b/R/terminal_close_linter.R new file mode 100644 index 000000000..f860a3429 --- /dev/null +++ b/R/terminal_close_linter.R @@ -0,0 +1,25 @@ +#' Prohibit close() from terminating a function definition +#' +#' Functions that end in `close(x)` are almost always better written by using +#' `on.exit(close(x))` close to where `x` is defined and/or opened. +#' +#' @evalRd rd_tags("terminal_close_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +terminal_close_linter <- make_linter_from_xpath( + xpath = " + //FUNCTION + /following-sibling::expr + /expr[last()][ + expr/SYMBOL_FUNCTION_CALL[text() = 'close'] + or expr[ + SYMBOL_FUNCTION_CALL[text() = 'return'] + and following-sibling::expr/expr/SYMBOL_FUNCTION_CALL[text() = 'close'] + ] + ] + ", + lint_message = paste( + "Use on.exit(close(x)) to close connections instead of", + "running it as the last call in a function." + ) +) diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index e5e763c4f..275c1863d 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -90,6 +90,7 @@ string_boundary_linter,readability efficiency configurable strings_as_factors_linter,robustness system_file_linter,consistency readability best_practices T_and_F_symbol_linter,style readability robustness consistency best_practices default +terminal_close_linter,best_practices robustness todo_comment_linter,style configurable trailing_blank_lines_linter,style default trailing_whitespace_linter,style default configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 7022070d1..9baf4f3bc 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -57,6 +57,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{stopifnot_all_linter}}} \item{\code{\link{system_file_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} +\item{\code{\link{terminal_close_linter}}} \item{\code{\link{undesirable_function_linter}}} \item{\code{\link{undesirable_operator_linter}}} \item{\code{\link{unnecessary_lambda_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 21947f999..c960b5d00 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,7 +17,7 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (54 linters)} +\item{\link[=best_practices_linters]{best_practices} (55 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (8 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} \item{\link[=consistency_linters]{consistency} (23 linters)} @@ -29,7 +29,7 @@ The following tags exist: \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} \item{\link[=readability_linters]{readability} (56 linters)} -\item{\link[=robustness_linters]{robustness} (14 linters)} +\item{\link[=robustness_linters]{robustness} (15 linters)} \item{\link[=style_linters]{style} (38 linters)} } } @@ -120,6 +120,7 @@ The following linters exist: \item{\code{\link{strings_as_factors_linter}} (tags: robustness)} \item{\code{\link{system_file_linter}} (tags: best_practices, consistency, readability)} \item{\code{\link{T_and_F_symbol_linter}} (tags: best_practices, consistency, default, readability, robustness, style)} +\item{\code{\link{terminal_close_linter}} (tags: best_practices, robustness)} \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)} diff --git a/man/robustness_linters.Rd b/man/robustness_linters.Rd index 9581e5d7f..499965308 100644 --- a/man/robustness_linters.Rd +++ b/man/robustness_linters.Rd @@ -24,6 +24,7 @@ The following linters are tagged with 'robustness': \item{\code{\link{seq_linter}}} \item{\code{\link{strings_as_factors_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} +\item{\code{\link{terminal_close_linter}}} \item{\code{\link{undesirable_function_linter}}} \item{\code{\link{undesirable_operator_linter}}} } diff --git a/man/terminal_close_linter.Rd b/man/terminal_close_linter.Rd new file mode 100644 index 000000000..207518708 --- /dev/null +++ b/man/terminal_close_linter.Rd @@ -0,0 +1,18 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/terminal_close_linter.R +\name{terminal_close_linter} +\alias{terminal_close_linter} +\title{Prohibit close() from terminating a function definition} +\usage{ +terminal_close_linter() +} +\description{ +Functions that end in \code{close(x)} are almost always better written by using +\code{on.exit(close(x))} close to where \code{x} is defined and/or opened. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=robustness_linters]{robustness} +} diff --git a/tests/testthat/test-terminal_close_linter.R b/tests/testthat/test-terminal_close_linter.R new file mode 100644 index 000000000..697804f68 --- /dev/null +++ b/tests/testthat/test-terminal_close_linter.R @@ -0,0 +1,74 @@ +test_that("terminal_close_linter skips allowed cases", { + linter <- terminal_close_linter() + + lines <- trim_some(" + foo <- function(bar) { + tmp <- tempfile() + on.exit(close(tmp)) + writeLines(bar, tmp) + return(invisible()) + } + ") + expect_lint(lines, NULL, linter) + + lines <- trim_some(" + foo <- function(bar) { + close <- bar + 1 + return(close) + } + ") + expect_lint(lines, NULL, linter) + + lines <- trim_some(" + foo <- function(bar) { + close <- bar + 1 + close + } + ") + expect_lint(lines, NULL, linter) +}) + +test_that("terminal_close_linter blocks simple cases", { + linter <- terminal_close_linter() + lint_msg <- rex::rex("Use on.exit(close(x)) to close connections") + + expect_lint( + trim_some(" + foo <- function(bar) { + tmp <- tempfile() + writeLines(bar, tmp) + return(close(tmp)) + } + "), + list(lint_msg, line_number = 4L, column_number = 3L), + linter + ) + + expect_lint( + trim_some(" + foo <- function(bar) { + tmp <- tempfile() + writeLines(bar, tmp) + close(tmp) + } + "), + list(lint_msg, line_number = 4L, column_number = 3L), + linter + ) + + # When multiple terminations happen, only lint the one + expect_lint( + trim_some(" + foo <- function(bar) { + tmp1 <- tempfile() + tmp2 <- tempfile() + writeLines(bar, tmp1) + writeLines(bar, tmp2) + close(tmp1) + close(tmp2) + } + "), + list(lint_msg, line_number = 7L, column_number = 3L), + linter + ) +})