From 6b4059a5fd13a5f30391524991868040df2a85f0 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 4 Aug 2025 15:13:56 +0200 Subject: [PATCH 1/7] Add tests for is.unsorted() recommendation on identical() --- tests/testthat/test-sort_linter.R | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index 15d8ab209e..c054cc3db3 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -108,15 +108,19 @@ test_that("sort_linter blocks simple disallowed usages", { sorted_msg <- rex::rex("Use !is.unsorted(x) to test the sortedness of a vector.") expect_lint("sort(x) == x", sorted_msg, linter) + expect_lint("identical(x, sort(x))", sorted_msg, linter) # argument order doesn't matter expect_lint("x == sort(x)", sorted_msg, linter) + expect_lint("identical(sort(x), x)", sorted_msg, linter) # inverted version expect_lint("sort(x) != x", unsorted_msg, linter) + expect_lint("!identical(x, sort(x))", unsorted_msg, linter) # expression matching expect_lint("sort(foo(x)) == foo(x)", sorted_msg, linter) + expect_lint("identical(foo(x), sort(foo(x))", sorted_msg, linter) }) test_that("lints vectorize", { From 7fdade77c5db0e9283d4d51b06760165d8a86245 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Mon, 4 Aug 2025 15:14:14 +0200 Subject: [PATCH 2/7] Add new XPath components for identical() --- R/sort_linter.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/R/sort_linter.R b/R/sort_linter.R index aa66ece89a..15ac789d13 100644 --- a/R/sort_linter.R +++ b/R/sort_linter.R @@ -85,12 +85,11 @@ sort_linter <- function() { sorted_xpath <- " parent::expr[not(SYMBOL_SUB)] /parent::expr[ - (EQ or NE) + (EQ or NE or expr/SYMBOL_FUNCTION_CALL/text() = 'identical') and expr/expr = expr ] " - arguments_xpath <- ".//SYMBOL_SUB[text() = 'method' or text() = 'decreasing' or text() = 'na.last']" From fe8d654507066277b70bac253d19ba8e566c24e0 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Tue, 5 Aug 2025 17:24:42 +0200 Subject: [PATCH 3/7] Split XPaths for idential() and == --- R/sort_linter.R | 25 +++++++++++++++++++++++-- tests/testthat/test-sort_linter.R | 2 +- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/R/sort_linter.R b/R/sort_linter.R index 15ac789d13..abff5d8dec 100644 --- a/R/sort_linter.R +++ b/R/sort_linter.R @@ -85,7 +85,14 @@ sort_linter <- function() { sorted_xpath <- " parent::expr[not(SYMBOL_SUB)] /parent::expr[ - (EQ or NE or expr/SYMBOL_FUNCTION_CALL/text() = 'identical') + (EQ or NE) + and expr/expr = expr + ] + " + sorted_identical_xpath <- " + parent::expr[not(SYMBOL_SUB)] + /parent::expr[ + expr/SYMBOL_FUNCTION_CALL[text() = 'identical'] and expr/expr = expr ] " @@ -141,8 +148,22 @@ sort_linter <- function() { "Use is.unsorted(x) to test the unsortedness of a vector." ) + sorted_identical_expr <- xml_find_all(xml_calls, sorted_identical_xpath) + is_negated <- !is.na( + xml_find_first(sorted_identical_expr, "preceding-sibling::*[not(self::COMMENT)][1][self::OP-EXCLAMATION]") + ) + + lint_message <- c( + lint_message, + ifelse( + is_negated, + "Use is.unsorted(x) to test the unsortedness of a vector.", + "Use !is.unsorted(x) to test the sortedness of a vector." + ) + ) + sorted_lints <- xml_nodes_to_lints( - sorted_expr, + combine_nodesets(sorted_expr, sorted_identical_expr), source_expression = source_expression, lint_message = lint_message, type = "warning" diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index c054cc3db3..02dd9bda77 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -120,7 +120,7 @@ test_that("sort_linter blocks simple disallowed usages", { # expression matching expect_lint("sort(foo(x)) == foo(x)", sorted_msg, linter) - expect_lint("identical(foo(x), sort(foo(x))", sorted_msg, linter) + expect_lint("identical(foo(x), sort(foo(x)))", sorted_msg, linter) }) test_that("lints vectorize", { From 9be67fe4bfaa7acb6318aae3bbf0112258e92ee8 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Tue, 5 Aug 2025 17:38:56 +0200 Subject: [PATCH 4/7] Add NEWS item --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index d7e38ccabc..b92a8ca3d2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -48,6 +48,7 @@ * `object_usage_linter()` lints missing packages that may cause false positives (#2872, @AshesITR) * New argument `include_s4_slots` for the `xml_find_function_calls()` entry in the `get_source_expressions()` to govern whether calls of the form `s4Obj@fun()` are included in the result (#2820, @MichaelChirico). * `sprintf_linter()` lints `sprintf()` and `gettextf()` calls when a constant string is passed to `fmt` (#2894, @Bisaloo). +* `sort_linter()` recommends usage of `!is.unsorted(x)` over `identical(x, sort(x))` (#2921, @Bisaloo). ### New linters From fe6f8e1935a72f084b15e6031978b964c7596945 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Wed, 22 Oct 2025 18:42:22 +0200 Subject: [PATCH 5/7] Use expect_no_lint() for false positives --- tests/testthat/test-sort_linter.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index 02dd9bda77..c02c098f09 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -1,17 +1,17 @@ test_that("sort_linter skips allowed usages", { linter <- sort_linter() - expect_lint("order(y)", NULL, linter) + expect_no_lint("order(y)", linter) - expect_lint("y[order(x)]", NULL, linter) + expect_no_lint("y[order(x)]", linter) # If another function is intercalated, don't fail - expect_lint("x[c(order(x))]", NULL, linter) + expect_no_lint("x[c(order(x))]", linter) - expect_lint("x[order(y, x)]", NULL, linter) - expect_lint("x[order(x, y)]", NULL, linter) + expect_no_lint("x[order(y, x)]", linter) + expect_no_lint("x[order(x, y)]", linter) # pretty sure this never makes sense, but test anyway - expect_lint("x[order(y, na.last = x)]", NULL, linter) + expect_no_lint("x[order(y, na.last = x)]", linter) }) From 67086a90b56c61582228b4f3b346fa8edb231798 Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Wed, 22 Oct 2025 18:53:00 +0200 Subject: [PATCH 6/7] Add false positive tests for is.unsorted --- tests/testthat/test-sort_linter.R | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index c02c098f09..90af645384 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -12,6 +12,10 @@ test_that("sort_linter skips allowed usages", { expect_no_lint("x[order(x, y)]", linter) # pretty sure this never makes sense, but test anyway expect_no_lint("x[order(y, na.last = x)]", linter) + + # is.unsorted false positives + expect_no_lint("identical(sort(x), y)", linter) + expect_no_lint("identical(sort(foo(x)), x)", linter) }) From 11aa2bac4c4e0fb1c3be4f1ca2d746f6397fd0ea Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Wed, 22 Oct 2025 19:02:29 +0200 Subject: [PATCH 7/7] Add one expression matching test --- tests/testthat/test-sort_linter.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index 90af645384..d9536c599b 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -125,6 +125,7 @@ test_that("sort_linter blocks simple disallowed usages", { # expression matching expect_lint("sort(foo(x)) == foo(x)", sorted_msg, linter) expect_lint("identical(foo(x), sort(foo(x)))", sorted_msg, linter) + expect_lint("identical(sort(foo(x)), foo(x))", sorted_msg, linter) }) test_that("lints vectorize", {