From ba6af1c7a1eed6f4ce038e039844c5ace79e6073 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 5 Sep 2024 20:46:13 +0000 Subject: [PATCH] Find some = or -> assignments when discovering script globals --- NEWS.md | 1 + R/object_usage_linter.R | 4 +- tests/testthat/test-object_usage_linter.R | 108 ++++++++++------------ 3 files changed, 53 insertions(+), 60 deletions(-) diff --git a/NEWS.md b/NEWS.md index d2508f02d..3f22da7b5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -84,6 +84,7 @@ ### Lint accuracy fixes: removing false positives * `object_name_linter()` and `object_length_linter()` ignore {rlang} name injection like `x |> mutate("{new_name}" := foo(col))` (#1926, @MichaelChirico). No checking is applied in such cases. {data.table} in-place assignments like `DT[, "sPoNGeBob" := "friend"]` are still eligible for lints. +* `object_usage_linter()` finds global variables assigned with `=` or `->`, which avoids some issues around "undefined global variables" in scripts (#2654, @MichaelChirico). ## Notes diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 6b9466eff..1b7abce0b 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -148,8 +148,10 @@ get_assignment_symbols <- function(xml) { get_r_string(xml_find_all( xml, " - expr[LEFT_ASSIGN]/expr[1]/SYMBOL[1] | + expr[LEFT_ASSIGN or EQ_ASSIGN]/expr[1]/SYMBOL[1] | + expr[RIGHT_ASSIGN]/expr[2]/SYMBOL[1] | equal_assign/expr[1]/SYMBOL[1] | + expr_or_assign_or_help/expr[1]/SYMBOL[1] | expr[expr[1][SYMBOL_FUNCTION_CALL/text()='assign']]/expr[2]/* | expr[expr[1][SYMBOL_FUNCTION_CALL/text()='setMethod']]/expr[2]/* " diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index 299bd6e0e..355ea6328 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -748,6 +748,9 @@ test_that("symbols in formulas aren't treated as 'undefined global'", { }) test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { + linter <- object_usage_linter() + lint_msg <- "no visible binding for global variable 'column'" + expect_lint( trim_some(" foo <- function(x) { @@ -757,12 +760,8 @@ test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { ) } "), - list( - message = "no visible binding for global variable 'column'", - line_number = 4L, - column_number = 22L - ), - object_usage_linter() + list(lint_msg, line_number = 4L, column_number = 22L), + linter ) expect_lint( @@ -774,12 +773,8 @@ test_that("NSE-ish symbols after $/@ are ignored as sources for lints", { ) } "), - list( - message = "no visible binding for global variable 'column'", - line_number = 4L, - column_number = 22L - ), - object_usage_linter() + list(lint_msg, line_number = 4L, column_number = 22L), + linter ) }) @@ -798,53 +793,34 @@ test_that("functional lambda definitions are also caught", { }) test_that("messages without location info are repaired", { + linter <- object_usage_linter() + global_function_msg <- rex::rex("no visible global function definition for", anything) + global_variable_msg <- rex::rex("no visible binding for global variable", anything) + local_variable_msg <- rex::rex("local variable", anything, "assigned but may not be used") + # regression test for #1986 expect_lint( - trim_some(" - foo <- function() no_fun() - "), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() no_fun()", + list(global_function_msg, line_number = 1L, column_number = 19L), + linter ) expect_lint( - trim_some(" - foo <- function(a = no_fun()) a - "), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 1L, - column_number = 21L - ), - object_usage_linter() + "foo <- function(a = no_fun()) a", + list(global_function_msg, line_number = 1L, column_number = 21L), + linter ) expect_lint( - trim_some(" - foo <- function() no_global - "), - list( - message = rex::rex("no visible binding for global variable", anything), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() no_global", + list(global_variable_msg, line_number = 1L, column_number = 19L), + linter ) expect_lint( - trim_some(" - foo <- function() unused_local <- 42L - "), - list( - message = rex::rex("local variable", anything, "assigned but may not be used"), - line_number = 1L, - column_number = 19L - ), - object_usage_linter() + "foo <- function() unused_local <- 42L", + list(local_variable_msg, line_number = 1L, column_number = 19L), + linter ) # More complex case with two lints and missing location info @@ -854,17 +830,31 @@ test_that("messages without location info are repaired", { bar() "), list( - list( - message = rex::rex("local variable", anything, "assigned but may not be used"), - line_number = 1L, - column_number = 19L - ), - list( - message = rex::rex("no visible global function definition for", anything), - line_number = 2L, - column_number = 3L - ) + list(local_variable_msg, line_number = 1L, column_number = 19L), + list(global_function_msg, line_number = 2L, column_number = 3L) ), - object_usage_linter() + linter + ) +}) + +test_that("globals in scripts are found regardless of assignment operator", { + linter <- object_usage_linter() + + expect_lint( + trim_some(" + library(dplyr) + + global_const_eq = 5 + global_const_la <- 6 + 7 -> global_const_ra + + examplefunction <- function(df) { + df %>% + select(dist) %>% + mutate(power = global_const_eq + global_const_ra + global_const_la) + } + "), + NULL, + linter ) })