From d3e6f094998477b3316d9638a2a9a17b6d4f881e Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Sat, 1 Jul 2023 11:55:14 +0200 Subject: [PATCH 1/6] add failing tests --- tests/testthat/test-object_usage_linter.R | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index d00f6c1ec2..be2ec12b8d 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -765,3 +765,42 @@ test_that("functional lambda definitions are also caught", { object_usage_linter() ) }) + +test_that("messages without location info are repaired", { + # 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() + ) + + 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() + ) + + 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() + ) +}) From cae45d1acee9ab6cf37bc5a847e765875ffe4c7b Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Sat, 1 Jul 2023 12:09:29 +0200 Subject: [PATCH 2/6] fix #1986, update NEWS --- NEWS.md | 8 +++++--- R/object_usage_linter.R | 10 +++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/NEWS.md b/NEWS.md index f844aa081f..faa64ceb9b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -22,8 +22,8 @@ * `assignment_linter()` no longer lints assignments in braces that include comments when `allow_trailing = FALSE` (#1701, @ashbaldry) * `object_usage_linter()` - + No longer silently ignores usage warnings that don't contain a quoted name (#1714, @AshesITR) - + No longer fails on code with comments inside a multi-line call to `glue::glue()` (#1919, @MichaelChirico) + + No longer silently ignores usage warnings that don't contain a quoted name (#1714, @AshesITR) + + No longer fails on code with comments inside a multi-line call to `glue::glue()` (#1919, @MichaelChirico) * `namespace_linter()` correctly recognizes backticked operators to be exported from respective namespaces (like `` rlang::`%||%` ``) (#1752, @IndrajeetPatil) @@ -40,7 +40,9 @@ * `object_name_linter()` allows all S3 group Generics (see `?base::groupGeneric`) and S3 generics defined in a different file in the same package (#1808, #1841, @AshesITR) -* `object_usage_linter()` improves identification of the exact source of a lint for undefined variables in expressions with where the variable is used as a symbol in a usual way, for example in a formula or in an extraction with `$` (#1914, @MichaelChirico). +* `object_usage_linter()` improves identification of the exact source of a lint + + for undefined variables in expressions with where the variable is used as a symbol in a usual way, for example in a formula or in an extraction with `$` (#1914, @MichaelChirico). + + for general usage warnings without location info (#1986, @AshesITR) * `function_left_parentheses_linter()` produces a more specific lint (and no longer fails) when the opening parenthesis is on a different line than `function` or the call name (#1953, @MichaelChirico). Thanks also to @IndrajeetPatil and @lorenzwalthert for identifying a regression in the initial fix, #1963. diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 05c6209500..274b9fa085 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -282,10 +282,10 @@ parse_check_usage <- function(expression, "'", capture(name = "name", anything), "'", - anything + zero_or_more(any, type = "lazy") ) ), - line_info + or(line_info, end) ) ) @@ -301,7 +301,11 @@ parse_check_usage <- function(expression, # nocov end res <- res[!missing, ] - res$line1 <- as.integer(res$line1) + start_line - 1L + res$line1 <- ifelse( + nzchar(res$line1), + as.integer(res$line1) + start_line - 1L, + start_line + ) res$line2 <- ifelse( nzchar(res$line2), as.integer(res$line2) + start_line - 1L, From ee5316686a1df8e9426cf7ff6f806b76e5b6acac Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Sat, 1 Jul 2023 19:24:18 +0200 Subject: [PATCH 3/6] also test error in formal argument --- tests/testthat/test-object_usage_linter.R | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index be2ec12b8d..07499dbbce 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -780,6 +780,18 @@ test_that("messages without location info are repaired", { object_usage_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() + ) + expect_lint( trim_some(" foo <- function() no_global From 8f08e0276f32d593ba0f2c0b41b08dda81c92b5c Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Sat, 1 Jul 2023 19:49:41 +0200 Subject: [PATCH 4/6] better fallback --- R/object_usage_linter.R | 7 ++++++- tests/testthat/test-object_usage_linter.R | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 274b9fa085..4ef0bc1b51 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -99,6 +99,7 @@ object_usage_linter <- function(interpret_glue = TRUE, skip_with = TRUE) { known_used_symbols = known_used_symbols, declared_globals = declared_globals, start_line = as.integer(xml2::xml_attr(fun_assignment, "line1")), + end_line = as.integer(xml2::xml_attr(fun_assignment, "line2")), skip_with = skip_with ) @@ -245,6 +246,7 @@ parse_check_usage <- function(expression, known_used_symbols = character(), declared_globals = character(), start_line = 1L, + end_line = 1L, skip_with = TRUE) { vals <- list() @@ -304,7 +306,7 @@ parse_check_usage <- function(expression, res$line1 <- ifelse( nzchar(res$line1), as.integer(res$line1) + start_line - 1L, - start_line + NA_integer_ ) res$line2 <- ifelse( nzchar(res$line2), @@ -312,6 +314,9 @@ parse_check_usage <- function(expression, res$line1 ) + res$line1[is.na(res$line1)] <- start_line + res$line2[is.na(res$line2)] <- end_line + res } diff --git a/tests/testthat/test-object_usage_linter.R b/tests/testthat/test-object_usage_linter.R index 07499dbbce..4056124d31 100644 --- a/tests/testthat/test-object_usage_linter.R +++ b/tests/testthat/test-object_usage_linter.R @@ -815,4 +815,25 @@ test_that("messages without location info are repaired", { ), object_usage_linter() ) + + # More complex case with two lints and missing location info + expect_lint( + trim_some(" + foo <- function() a <- + 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 + ) + ), + object_usage_linter() + ) }) From 26fac56b1cb9039d4dc6c740d7400e910b5bccb6 Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Sat, 1 Jul 2023 19:53:23 +0200 Subject: [PATCH 5/6] nolint newly surfaced codetools false-positives --- R/quotes_linter.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/quotes_linter.R b/R/quotes_linter.R index 3820fc881a..1392a33f91 100644 --- a/R/quotes_linter.R +++ b/R/quotes_linter.R @@ -47,7 +47,7 @@ quotes_linter <- function(delimiter = c('"', "'")) { single_quote, end ) - lint_message <- "Only use double-quotes." # nolint: object_usage_linter. An apparent codetools bug. + lint_message <- "Only use double-quotes." # nolint: object_usage. An apparent codetools bug. } else { quote_regex <- rex( start, @@ -73,8 +73,8 @@ quotes_linter <- function(delimiter = c('"', "'")) { quote_matches, function(id) { with(content[str_idx[id], ], { - line <- source_expression$file_lines[[line1]] - col2 <- if (line1 == line2) col2 else nchar(line) + line <- source_expression$file_lines[[line1]] # nolint: object_usage. Codetools bug + col2 <- if (line1 == line2) col2 else nchar(line) # nolint: object_usage. Codetools bug Lint( filename = source_expression$filename, line_number = line1, From 500d1375613291d2f9c20a2acee961a6bbdfba05 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 1 Jul 2023 16:28:09 -0700 Subject: [PATCH 6/6] Update NEWS.md --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index ea433bed54..61b78c7565 100644 --- a/NEWS.md +++ b/NEWS.md @@ -43,7 +43,7 @@ * `object_usage_linter()` improves identification of the exact source of a lint + for undefined variables in expressions with where the variable is used as a symbol in a usual way, for example in a formula or in an extraction with `$` (#1914, @MichaelChirico). - + for general usage warnings without location info (#1986, @AshesITR) + + for general usage warnings without location info (#1986 and #1917, @AshesITR) * `function_left_parentheses_linter()` produces a more specific lint (and no longer fails) when the opening parenthesis is on a different line than `function` or the call name (#1953, @MichaelChirico). Thanks also to @IndrajeetPatil and @lorenzwalthert for identifying a regression in the initial fix, #1963.