diff --git a/DESCRIPTION b/DESCRIPTION index 6e9bcfa02..c68a79082 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -138,6 +138,7 @@ Collate: 'namespace.R' 'namespace_linter.R' 'nested_ifelse_linter.R' + 'nested_pipe_linter.R' 'nonportable_path_linter.R' 'nrow_subset_linter.R' 'numeric_leading_zero_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 604679f23..e7c9514f3 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -99,6 +99,7 @@ export(missing_package_linter) export(modify_defaults) export(namespace_linter) export(nested_ifelse_linter) +export(nested_pipe_linter) export(no_tab_linter) export(nonportable_path_linter) export(nrow_subset_linter) diff --git a/NEWS.md b/NEWS.md index 868503157..f4190a054 100644 --- a/NEWS.md +++ b/NEWS.md @@ -31,6 +31,7 @@ * `which_grepl_linter()` for discouraging `which(grepl(ptn, x))` in favor of directly using `grep(ptn, x)` (part of #884, @MichaelChirico). * `list_comparison_linter()` for discouraging comparisons on the output of `lapply()`, e.g. `lapply(x, sum) > 10` (part of #884, @MichaelChirico). * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico). +* `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico). * `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (part of #884, @MichaelChirico). * `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico). diff --git a/R/nested_pipe_linter.R b/R/nested_pipe_linter.R new file mode 100644 index 000000000..d53c9a16b --- /dev/null +++ b/R/nested_pipe_linter.R @@ -0,0 +1,86 @@ +#' Block usage of pipes nested inside other calls +#' +#' Nesting pipes harms readability; extract sub-steps to separate variables, +#' append further pipeline steps, or otherwise refactor such usage away. +#' +#' @param allow_inline Logical, default `TRUE`, in which case only "inner" +#' pipelines which span more than one line are linted. If `FALSE`, even +#' "inner" pipelines that fit in one line are linted. +#' @param allow_outer_calls Character vector dictating which "outer" +#' calls to exempt from the requirement to unnest (see examples). Defaults +#' to [try()], [tryCatch()], and [withCallingHandlers()]. +#' +#' @examples +#' # will produce lints +#' code <- "df1 %>%\n inner_join(df2 %>%\n select(a, b)\n )" +#' writeLines(code) +#' lint( +#' text = code, +#' linters = nested_pipe_linter() +#' ) +#' +#' lint( +#' text = "df1 %>% inner_join(df2 %>% select(a, b))", +#' linters = nested_pipe_linter(allow_inline = FALSE) +#' ) +#' +#' lint( +#' text = "tryCatch(x %>% filter(grp == 'a'), error = identity)", +#' linters = nested_pipe_linter(allow_outer_calls = character()) +#' ) +#' +#' # okay +#' lint( +#' text = "df1 %>% inner_join(df2 %>% select(a, b))", +#' linters = nested_pipe_linter() +#' ) +#' +#' code <- "df1 %>%\n inner_join(df2 %>%\n select(a, b)\n )" +#' writeLines(code) +#' lint( +#' text = code, +#' linters = nested_pipe_linter(allow_outer_calls = "inner_join") +#' ) +#' +#' lint( +#' text = "tryCatch(x %>% filter(grp == 'a'), error = identity)", +#' linters = nested_pipe_linter() +#' ) +#' +#' @evalRd rd_tags("nested_pipe_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +nested_pipe_linter <- function( + allow_inline = TRUE, + allow_outer_calls = c("try", "tryCatch", "withCallingHandlers")) { + multiline_and <- if (allow_inline) "@line1 != @line2 and" else "" + xpath <- glue(" + (//PIPE | //SPECIAL[{ xp_text_in_table(magrittr_pipes) }]) + /parent::expr[{multiline_and} preceding-sibling::expr/SYMBOL_FUNCTION_CALL[ + not({ xp_text_in_table(allow_outer_calls) }) + and ( + text() != 'switch' + or parent::expr + /following-sibling::expr[1] + /*[self::PIPE or self::SPECIAL[{ xp_text_in_table(magrittr_pipes) }]] + ) + ]] + ") + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + bad_expr <- xml_find_all(xml, xpath) + + xml_nodes_to_lints( + bad_expr, + source_expression = source_expression, + lint_message = "Don't nest pipes inside other calls.", + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 4c1b31300..1a5fe5787 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -56,6 +56,7 @@ missing_argument_linter,correctness common_mistakes configurable missing_package_linter,robustness common_mistakes namespace_linter,correctness robustness configurable executing nested_ifelse_linter,efficiency readability +nested_pipe_linter,readability consistency configurable no_tab_linter,style consistency deprecated nonportable_path_linter,robustness best_practices configurable nrow_subset_linter,efficiency consistency best_practices diff --git a/man/configurable_linters.Rd b/man/configurable_linters.Rd index 0f5adb229..ac0be3120 100644 --- a/man/configurable_linters.Rd +++ b/man/configurable_linters.Rd @@ -30,6 +30,7 @@ The following linters are tagged with 'configurable': \item{\code{\link{line_length_linter}}} \item{\code{\link{missing_argument_linter}}} \item{\code{\link{namespace_linter}}} +\item{\code{\link{nested_pipe_linter}}} \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{object_length_linter}}} \item{\code{\link{object_name_linter}}} diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd index 24ae08f8d..5ae4303ab 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -26,6 +26,7 @@ The following linters are tagged with 'consistency': \item{\code{\link{keyword_quote_linter}}} \item{\code{\link{length_levels_linter}}} \item{\code{\link{literal_coercion_linter}}} +\item{\code{\link{nested_pipe_linter}}} \item{\code{\link{nrow_subset_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} \item{\code{\link{nzchar_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index ba0644116..361ec1a7d 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -19,8 +19,8 @@ The following tags exist: \itemize{ \item{\link[=best_practices_linters]{best_practices} (62 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (10 linters)} -\item{\link[=configurable_linters]{configurable} (35 linters)} -\item{\link[=consistency_linters]{consistency} (28 linters)} +\item{\link[=configurable_linters]{configurable} (36 linters)} +\item{\link[=consistency_linters]{consistency} (29 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=deprecated_linters]{deprecated} (4 linters)} @@ -28,7 +28,7 @@ The following tags exist: \item{\link[=executing_linters]{executing} (6 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} -\item{\link[=readability_linters]{readability} (61 linters)} +\item{\link[=readability_linters]{readability} (62 linters)} \item{\link[=regex_linters]{regex} (4 linters)} \item{\link[=robustness_linters]{robustness} (17 linters)} \item{\link[=style_linters]{style} (39 linters)} @@ -92,6 +92,7 @@ The following linters exist: \item{\code{\link{missing_package_linter}} (tags: common_mistakes, robustness)} \item{\code{\link{namespace_linter}} (tags: configurable, correctness, executing, robustness)} \item{\code{\link{nested_ifelse_linter}} (tags: efficiency, readability)} +\item{\code{\link{nested_pipe_linter}} (tags: configurable, consistency, readability)} \item{\code{\link{nonportable_path_linter}} (tags: best_practices, configurable, robustness)} \item{\code{\link{nrow_subset_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{numeric_leading_zero_linter}} (tags: consistency, readability, style)} diff --git a/man/nested_pipe_linter.Rd b/man/nested_pipe_linter.Rd new file mode 100644 index 000000000..1f5eb03e3 --- /dev/null +++ b/man/nested_pipe_linter.Rd @@ -0,0 +1,68 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/nested_pipe_linter.R +\name{nested_pipe_linter} +\alias{nested_pipe_linter} +\title{Block usage of pipes nested inside other calls} +\usage{ +nested_pipe_linter( + allow_inline = TRUE, + allow_outer_calls = c("try", "tryCatch", "withCallingHandlers") +) +} +\arguments{ +\item{allow_inline}{Logical, default \code{TRUE}, in which case only "inner" +pipelines which span more than one line are linted. If \code{FALSE}, even +"inner" pipelines that fit in one line are linted.} + +\item{allow_outer_calls}{Character vector dictating which "outer" +calls to exempt from the requirement to unnest (see examples). Defaults +to \code{\link[=try]{try()}}, \code{\link[=tryCatch]{tryCatch()}}, and \code{\link[=withCallingHandlers]{withCallingHandlers()}}.} +} +\description{ +Nesting pipes harms readability; extract sub-steps to separate variables, +append further pipeline steps, or otherwise refactor such usage away. +} +\examples{ +# will produce lints +code <- "df1 \%>\%\n inner_join(df2 \%>\%\n select(a, b)\n )" +writeLines(code) +lint( + text = code, + linters = nested_pipe_linter() +) + +lint( + text = "df1 \%>\% inner_join(df2 \%>\% select(a, b))", + linters = nested_pipe_linter(allow_inline = FALSE) +) + +lint( + text = "tryCatch(x \%>\% filter(grp == 'a'), error = identity)", + linters = nested_pipe_linter(allow_outer_calls = character()) +) + +# okay +lint( + text = "df1 \%>\% inner_join(df2 \%>\% select(a, b))", + linters = nested_pipe_linter() +) + +code <- "df1 \%>\%\n inner_join(df2 \%>\%\n select(a, b)\n )" +writeLines(code) +lint( + text = code, + linters = nested_pipe_linter(allow_outer_calls = "inner_join") +) + +lint( + text = "tryCatch(x \%>\% filter(grp == 'a'), error = identity)", + linters = nested_pipe_linter() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=configurable_linters]{configurable}, \link[=consistency_linters]{consistency}, \link[=readability_linters]{readability} +} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 101346b55..67c08d265 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -42,6 +42,7 @@ The following linters are tagged with 'readability': \item{\code{\link{line_length_linter}}} \item{\code{\link{matrix_apply_linter}}} \item{\code{\link{nested_ifelse_linter}}} +\item{\code{\link{nested_pipe_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} \item{\code{\link{object_length_linter}}} \item{\code{\link{object_overwrite_linter}}} diff --git a/tests/testthat/test-nested_pipe_linter.R b/tests/testthat/test-nested_pipe_linter.R new file mode 100644 index 000000000..1e1679238 --- /dev/null +++ b/tests/testthat/test-nested_pipe_linter.R @@ -0,0 +1,192 @@ +test_that("nested_pipe_linter skips allowed usages", { + linter <- nested_pipe_linter() + + expect_lint("a %>% b() %>% c()", NULL, linter) + + expect_lint( + trim_some(" + foo <- function(x) { + out <- a %>% b() + return(out) + } + "), + NULL, + linter + ) + + # pipes fitting on one line can be ignored + expect_lint( + "bind_rows(a %>% select(b), c %>% select(b))", + NULL, + linter + ) + + # switch outputs are OK + expect_lint("switch(x, a = x %>% foo())", NULL, linter) + # final position is an output position + expect_lint("switch(x, a = x, x %>% foo())", NULL, linter) + + # inline switch inputs are not linted + expect_lint( + trim_some(" + switch( + x %>% foo(), + a = x + ) + "), + NULL, + linter + ) +}) + +patrick::with_parameters_test_that( + "allow_outer_calls defaults are ignored by default", + expect_lint( + trim_some(sprintf(outer_call, fmt = " + %s( + x %%>%% + foo() + ) + ")), + NULL, + nested_pipe_linter() + ), + .test_name = c("try", "tryCatch", "withCallingHandlers"), + outer_call = c("try", "tryCatch", "withCallingHandlers") +) + +test_that("nested_pipe_linter blocks simple disallowed usages", { + linter <- nested_pipe_linter() + linter_inline <- nested_pipe_linter(allow_inline = FALSE) + lint_msg <- rex::rex("Don't nest pipes inside other calls.") + + expect_lint( + "bind_rows(a %>% select(b), c %>% select(b))", + list(lint_msg, lint_msg), + linter_inline + ) + + expect_lint( + trim_some(" + print( + a %>% + filter(b > c) + ) + "), + lint_msg, + linter + ) + + # switch inputs are linted + expect_lint( + trim_some(" + switch( + x %>% + foo(), + a = x + ) + "), + lint_msg, + linter + ) + + expect_lint( + trim_some(" + switch( + x %>% foo(), + a = x + ) + "), + lint_msg, + linter_inline + ) +}) + +test_that("allow_outer_calls= argument works", { + expect_lint( + trim_some(" + try( + x %>% + foo() + ) + "), + rex::rex("Don't nest pipes inside other calls."), + nested_pipe_linter(allow_outer_calls = character()) + ) + + expect_lint( + trim_some(" + print( + x %>% + foo() + ) + "), + NULL, + nested_pipe_linter(allow_outer_calls = "print") + ) +}) + +test_that("Native pipes are handled as well", { + skip_if_not_r_version("4.1.0") + + linter <- nested_pipe_linter() + linter_inline <- nested_pipe_linter(allow_inline = FALSE) + lint_msg <- rex::rex("Don't nest pipes inside other calls.") + + expect_lint( + "bind_rows(a |> select(b), c |> select(b))", + NULL, + linter + ) + expect_lint( + "bind_rows(a |> select(b), c |> select(b))", + list(lint_msg, lint_msg), + linter_inline + ) + + expect_lint( + trim_some(" + print( + a |> + filter(b > c) + ) + "), + lint_msg, + linter + ) +}) + +test_that("lints vectorize", { + lint_msg <- rex::rex("Don't nest pipes inside other calls.") + + lines <- trim_some("{ + bind_rows( + a %>% select(b), + c %>% + select(d), + e %>% + select(f) %>% + filter(g > 0), + h %>% filter(i < 0) + ) + }") + expect_lint( + lines, + list( + list(lint_msg, line_number = 4L), + list(lint_msg, line_number = 6L) + ), + nested_pipe_linter() + ) + + expect_lint( + lines, + list( + list(lint_msg, line_number = 3L), + list(lint_msg, line_number = 4L), + list(lint_msg, line_number = 6L), + list(lint_msg, line_number = 9L) + ), + nested_pipe_linter(allow_inline = FALSE) + ) +})