Add library call linter (#2027)

* add library call linter

* tests

* correct lint findings

* fix lint results

* return last node rather than last element

* remove debug code

* review comments

* add explicit L for integers

* indent XPath one more level

* add readability

* added news

---------

Co-authored-by: Nicholas Masel <[email protected]>
Co-authored-by: AshesITR <[email protected]>
Co-authored-by: Michael Chirico <[email protected]>

DESCRIPTION

@@ -114,6 +114,7 @@ Collate:
     'is_lint_level.R'
     'is_numeric_linter.R'
     'lengths_linter.R'
+    'library_call_linter.R'
     'line_length_linter.R'
     'lint.R'
     'linter_tag_docs.R'

NAMESPACE

@@ -78,6 +78,7 @@ export(inner_combine_linter)
 export(is_lint_level)
 export(is_numeric_linter)
 export(lengths_linter)
+export(library_call_linter)
 export(line_length_linter)
 export(lint)
 export(lint_dir)

NEWS.md

@@ -4,6 +4,11 @@
 
 * `inner_combine_linter()` no longer throws on length-1 calls to `c()` like `c(exp(2))` or `c(log(3))` (#2017, @MichaelChirico). Such usage is discouraged by `unnecessary_concatenation_linter()`, but `inner_combine_linter()` _per se_ does not apply.
 
+## New and improved features
+
+* `library_call_linter()` can detect if all library calls are not at the top of your script (#2027, @nicholas-masel).
+
+
 ## Changes to defaults
 
 * `assignment_linter()` lints the {magrittr} assignment pipe `%<>%` (#2008, @MichaelChirico). This can be deactivated by setting the new argument `allow_pipe_assign` to `TRUE`.

R/library_call_linter.R

@@ -0,0 +1,75 @@
+#' Library call linter
+#'
+#' Force library calls to all be at the top of the script.
+#'
+#' @examples
+#' # will produce lints
+#' lint(
+#'   text = "
+#'     library(dplyr)
+#'     print('test')
+#'     library(tidyr)
+#'   ",
+#'   linters = library_call_linter()
+#' )
+#'
+#' lint(
+#'   text = "
+#'     library(dplyr)
+#'     print('test')
+#'     library(tidyr)
+#'     library(purrr)
+#'   ",
+#'   linters = library_call_linter()
+#' )
+#'
+#' # okay
+#' lint(
+#'   text = "
+#'     library(dplyr)
+#'     print('test')
+#'   ",
+#'   linters = library_call_linter()
+#' )
+#'
+#' lint(
+#'   text = "
+#'     # comment
+#'     library(dplyr)
+#'   ",
+#'   linters = library_call_linter()
+#' )
+#'
+#' @evalRd rd_tags("library_call_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+library_call_linter <- function() {
+
+  xpath <- "
+    (//SYMBOL_FUNCTION_CALL[text() = 'library'])[last()]
+      /preceding::expr
+      /SYMBOL_FUNCTION_CALL[text() != 'library'][last()]
+      /following::expr[SYMBOL_FUNCTION_CALL[text() = 'library']]
+  "
+
+  Linter(function(source_expression) {
+    if (!is_lint_level(source_expression, "file")) {
+      return(list())
+    }
+
+    xml <- source_expression$full_xml_parsed_content
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    if (length(bad_expr) == 0L) {
+      return(list())
+    }
+
+    xml_nodes_to_lints(
+      bad_expr,
+      source_expression = source_expression,
+      lint_message = "Move all library calls to the top of the script.",
+      type = "warning"
+    )
+  })
+}

inst/lintr/linters.csv

@@ -42,6 +42,7 @@ infix_spaces_linter,style readability default configurable
 inner_combine_linter,efficiency consistency readability
 is_numeric_linter,readability best_practices consistency
 lengths_linter,efficiency readability best_practices
+library_call_linter,style best_practices readability
 line_length_linter,style readability default configurable
 literal_coercion_linter,best_practices consistency efficiency
 matrix_apply_linter,readability efficiency

tests/testthat/test-library_call_linter.R

@@ -0,0 +1,99 @@
+test_that("library_call_linter skips allowed usages", {
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+    "),
+    NULL,
+    library_call_linter()
+  )
+
+  expect_lint("print('test')",
+    NULL,
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      # comment
+      library(dplyr)
+    "),
+    NULL,
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      print('test')
+      # library(dplyr)
+    "),
+    NULL,
+    library_call_linter()
+  )
+})
+
+test_that("library_call_linter warns on disallowed usages", {
+  lint_message <- rex::rex("Move all library calls to the top of the script.")
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+      library(tidyr)
+    "),
+    lint_message,
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+      library(tidyr)
+      library(purrr)
+    "),
+    list(
+      list(lint_message, line_number = 3L, column_number = 1L),
+      list(lint_message, line_number = 4L, column_number = 1L)
+    ),
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+      print('test')
+      library(tidyr)
+    "),
+    lint_message,
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+      library(tidyr)
+      print('test')
+    "),
+    lint_message,
+    library_call_linter()
+  )
+
+  expect_lint(
+    trim_some("
+      library(dplyr)
+      print('test')
+      library(tidyr)
+      print('test')
+      library(purrr)
+    "),
+    list(
+      list(lint_message, line_number = 3L, column_number = 1L),
+      list(lint_message, line_number = 5L, column_number = 1L)
+    ),
+    library_call_linter()
+  )
+})