From 4270c613af42b666033e4754e89cb2c88e588ef0 Mon Sep 17 00:00:00 2001
From: Michael Chirico <chiricom@google.com>
Date: Sun, 27 Mar 2022 01:20:51 -0700
Subject: [PATCH] New function_brace_linter (#987)

---
 DESCRIPTION                                 |  1 +
 NAMESPACE                                   |  1 +
 R/function_brace_linter.R                   | 28 +++++++++++++++++++++
 R/zzz.R                                     |  1 +
 inst/lintr/linters.csv                      |  1 +
 man/default_linters.Rd                      |  3 ++-
 man/function_brace_linter.Rd                | 18 +++++++++++++
 man/linters.Rd                              | 13 +++++-----
 man/readability_linters.Rd                  |  1 +
 man/style_linters.Rd                        |  1 +
 tests/testthat/default_linter_testcode.R    |  4 +++
 tests/testthat/test-function_brace_linter.R | 22 ++++++++++++++++
 12 files changed, 87 insertions(+), 7 deletions(-)
 create mode 100644 R/function_brace_linter.R
 create mode 100644 man/function_brace_linter.Rd
 create mode 100644 tests/testthat/test-function_brace_linter.R

diff --git a/DESCRIPTION b/DESCRIPTION
index 96ed7d829..74b58355e 100644
--- a/DESCRIPTION
+++ b/DESCRIPTION
@@ -79,6 +79,7 @@ Collate:
     'expect_type_linter.R'
     'extract.R'
     'extraction_operator_linter.R'
+    'function_brace_linter.R'
     'function_left_parentheses.R'
     'get_source_expressions.R'
     'ids_with_token.R'
diff --git a/NAMESPACE b/NAMESPACE
index 45eea7854..87767d60b 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -45,6 +45,7 @@ export(expect_s4_class_linter)
 export(expect_true_false_linter)
 export(expect_type_linter)
 export(extraction_operator_linter)
+export(function_brace_linter)
 export(function_left_parentheses_linter)
 export(get_source_expressions)
 export(ids_with_token)
diff --git a/R/function_brace_linter.R b/R/function_brace_linter.R
new file mode 100644
index 000000000..2ad87c8fa
--- /dev/null
+++ b/R/function_brace_linter.R
@@ -0,0 +1,28 @@
+#' Require multi-line functions to use braces
+#'
+#' This linter catches function definitions spanning multiple lines of code
+#'   that aren't wrapped in braces
+#'
+#' @evalRd rd_tags("function_brace_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+function_brace_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$xml_parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    bad_expr_xpath <- "//expr[FUNCTION and @line1 != @line2 and not(expr[OP-LEFT-BRACE])]"
+    bad_expr <- xml2::xml_find_all(xml, bad_expr_xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      lint_message = "Any function spanning multiple lines must use curly braces.",
+      type = "warning"
+    ))
+  })
+}
diff --git a/R/zzz.R b/R/zzz.R
index cf9f25655..a7f47747c 100644
--- a/R/zzz.R
+++ b/R/zzz.R
@@ -89,6 +89,7 @@ default_linters <- with_defaults(
   commented_code_linter(),
   cyclocomp_linter(),
   equals_na_linter(),
+  function_brace_linter(),
   function_left_parentheses_linter(),
   if_else_match_braces_linter(),
   infix_spaces_linter(),
diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv
index 96bb25ff5..9ac2b29cc 100644
--- a/inst/lintr/linters.csv
+++ b/inst/lintr/linters.csv
@@ -23,6 +23,7 @@ expect_s4_class_linter,package_development best_practices
 expect_true_false_linter,package_development best_practices readability
 expect_type_linter,package_development best_practices
 extraction_operator_linter,style best_practices
+function_brace_linter,default style readability
 function_left_parentheses_linter,style readability default
 if_else_match_braces_linter,default style readability
 implicit_integer_linter,style consistency best_practices
diff --git a/man/default_linters.Rd b/man/default_linters.Rd
index 906aa122c..f2cc959ec 100644
--- a/man/default_linters.Rd
+++ b/man/default_linters.Rd
@@ -5,7 +5,7 @@
 \alias{default_linters}
 \title{Default linters}
 \format{
-An object of class \code{list} of length 27.
+An object of class \code{list} of length 28.
 }
 \usage{
 default_linters
@@ -30,6 +30,7 @@ The following linters are tagged with 'default':
 \item{\code{\link{commented_code_linter}}}
 \item{\code{\link{cyclocomp_linter}}}
 \item{\code{\link{equals_na_linter}}}
+\item{\code{\link{function_brace_linter}}}
 \item{\code{\link{function_left_parentheses_linter}}}
 \item{\code{\link{if_else_match_braces_linter}}}
 \item{\code{\link{infix_spaces_linter}}}
diff --git a/man/function_brace_linter.Rd b/man/function_brace_linter.Rd
new file mode 100644
index 000000000..76606108b
--- /dev/null
+++ b/man/function_brace_linter.Rd
@@ -0,0 +1,18 @@
+% Generated by roxygen2: do not edit by hand
+% Please edit documentation in R/function_brace_linter.R
+\name{function_brace_linter}
+\alias{function_brace_linter}
+\title{Require multi-line functions to use braces}
+\usage{
+function_brace_linter()
+}
+\description{
+This linter catches function definitions spanning multiple lines of code
+that aren't wrapped in braces
+}
+\seealso{
+\link{linters} for a complete list of linters available in lintr.
+}
+\section{Tags}{
+\link[=default_linters]{default}, \link[=readability_linters]{readability}, \link[=style_linters]{style}
+}
diff --git a/man/linters.Rd b/man/linters.Rd
index 8d8bcfae4..e551d85e9 100644
--- a/man/linters.Rd
+++ b/man/linters.Rd
@@ -17,17 +17,17 @@ Documentation for linters is structured into tags to allow for easier discovery.
 \section{Tags}{
 The following tags exist:
 \itemize{
-\item{\link[=best_practices_linters]{best_practices} (26 linters)}
+\item{\link[=best_practices_linters]{best_practices} (27 linters)}
 \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)}
 \item{\link[=configurable_linters]{configurable} (16 linters)}
-\item{\link[=consistency_linters]{consistency} (10 linters)}
+\item{\link[=consistency_linters]{consistency} (11 linters)}
 \item{\link[=correctness_linters]{correctness} (7 linters)}
-\item{\link[=default_linters]{default} (27 linters)}
-\item{\link[=efficiency_linters]{efficiency} (9 linters)}
+\item{\link[=default_linters]{default} (28 linters)}
+\item{\link[=efficiency_linters]{efficiency} (10 linters)}
 \item{\link[=package_development_linters]{package_development} (13 linters)}
-\item{\link[=readability_linters]{readability} (29 linters)}
+\item{\link[=readability_linters]{readability} (30 linters)}
 \item{\link[=robustness_linters]{robustness} (11 linters)}
-\item{\link[=style_linters]{style} (33 linters)}
+\item{\link[=style_linters]{style} (34 linters)}
 }
 }
 \section{Linters}{
@@ -57,6 +57,7 @@ The following linters exist:
 \item{\code{\link{expect_true_false_linter}} (tags: best_practices, package_development, readability)}
 \item{\code{\link{expect_type_linter}} (tags: best_practices, package_development)}
 \item{\code{\link{extraction_operator_linter}} (tags: best_practices, style)}
+\item{\code{\link{function_brace_linter}} (tags: default, readability, style)}
 \item{\code{\link{function_left_parentheses_linter}} (tags: default, readability, style)}
 \item{\code{\link{if_else_match_braces_linter}} (tags: default, readability, style)}
 \item{\code{\link{implicit_integer_linter}} (tags: best_practices, consistency, style)}
diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd
index 1a0619063..12d6ba036 100644
--- a/man/readability_linters.Rd
+++ b/man/readability_linters.Rd
@@ -21,6 +21,7 @@ The following linters are tagged with 'readability':
 \item{\code{\link{expect_named_linter}}}
 \item{\code{\link{expect_not_linter}}}
 \item{\code{\link{expect_true_false_linter}}}
+\item{\code{\link{function_brace_linter}}}
 \item{\code{\link{function_left_parentheses_linter}}}
 \item{\code{\link{if_else_match_braces_linter}}}
 \item{\code{\link{infix_spaces_linter}}}
diff --git a/man/style_linters.Rd b/man/style_linters.Rd
index b1df97fc7..ba523553c 100644
--- a/man/style_linters.Rd
+++ b/man/style_linters.Rd
@@ -18,6 +18,7 @@ The following linters are tagged with 'style':
 \item{\code{\link{commented_code_linter}}}
 \item{\code{\link{cyclocomp_linter}}}
 \item{\code{\link{extraction_operator_linter}}}
+\item{\code{\link{function_brace_linter}}}
 \item{\code{\link{function_left_parentheses_linter}}}
 \item{\code{\link{if_else_match_braces_linter}}}
 \item{\code{\link{implicit_integer_linter}}}
diff --git a/tests/testthat/default_linter_testcode.R b/tests/testthat/default_linter_testcode.R
index 0d8cc3f4f..a22b1e9f1 100644
--- a/tests/testthat/default_linter_testcode.R
+++ b/tests/testthat/default_linter_testcode.R
@@ -29,6 +29,10 @@ someComplicatedFunctionWithALongCamelCaseName <- function(x)
 # vector_logic
 if (1 & 2) FALSE else TRUE
 
+# function_brace
+my_metric <- function(x)
+  sum(x) + prod(x)
+
 # no_tab
 # pipe_continuation
 # seq_linter
diff --git a/tests/testthat/test-function_brace_linter.R b/tests/testthat/test-function_brace_linter.R
new file mode 100644
index 000000000..e18fb4eb6
--- /dev/null
+++ b/tests/testthat/test-function_brace_linter.R
@@ -0,0 +1,22 @@
+test_that("function_brace_linter skips allowed usages", {
+  expect_lint("function(x) 4", NULL, function_brace_linter())
+
+  lines <- trim_some("
+    function(x) {
+      x + 4
+    }
+  ")
+  expect_lint(lines, NULL, function_brace_linter())
+})
+
+test_that("function_brace_linter blocks disallowed usage", {
+  lines <- trim_some("
+    function(x)
+      x+4
+  ")
+  expect_lint(
+    lines,
+    rex::rex("Any function spanning multiple lines must use curly braces."),
+    function_brace_linter()
+  )
+})