From ac74b25bc4fcf3bec9e5077c3002d502de917659 Mon Sep 17 00:00:00 2001 From: Emmanuel Ferdman Date: Fri, 27 Feb 2026 22:18:44 +0200 Subject: [PATCH 1/7] Fix false positive in `if_switch_linter()` for empty strings Signed-off-by: Emmanuel Ferdman --- NEWS.md | 1 + R/if_switch_linter.R | 7 ++++++- tests/testthat/test-if_switch_linter.R | 7 +++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index f9e9f9585d..e1e5fc4dfb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ * Excluding `cyclocomp_linter()` in `available_linters()` or `linters_with_tags()`, which requires the weak dependency {cyclocomp}, no longer emits a warning (#2909, @MichaelChirico). * `repeat_linter()` no longer errors when `while` is in a column to the right of `}` (#2828, @MichaelChirico). * `undesirable_operator_linter(call_is_undesirable = FALSE)` now correctly skips prefix notation like `` `:::`(pkg, fun) `` (#2999, @emmanuel-ferdman). +* `if_switch_linter()` no longer produces a false positive when comparing to empty strings (`""` or `''`), which cannot be used as `switch()` case names (#2835, @emmanuel-ferdman). ## New and improved features diff --git a/R/if_switch_linter.R b/R/if_switch_linter.R index 815236de0b..287cd0f67a 100644 --- a/R/if_switch_linter.R +++ b/R/if_switch_linter.R @@ -159,7 +159,7 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) { - equal_str_cond <- "expr[1][EQ and expr/STR_CONST]" + equal_str_cond <- "expr[1][EQ and expr/STR_CONST[string-length(text()) > 2]]" if (max_branch_lines > 0L || max_branch_expressions > 0L) { complexity_cond <- xp_or(c( @@ -191,6 +191,10 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) # NB: IF AND {...} AND ELSE/... implies >= 3 equality conditions are present # .//expr/IF/...: the expr in `==` that's _not_ the STR_CONST # not(preceding::IF): prevent nested matches which might be incorrect globally + empty_string_cond <- paste0( + ".//ELSE/following-sibling::expr/IF/following-sibling::expr[1][EQ]", + "/expr/STR_CONST[string-length(text()) = 2]" + ) if_xpath <- glue(" //IF /parent::expr[ @@ -202,6 +206,7 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) and ELSE/following-sibling::expr[IF and {equal_str_cond}] ] and not({ max_lines_cond }) + and not({ empty_string_cond }) ] ") diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index a2165cc3a8..1f31e6eca5 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -19,6 +19,13 @@ test_that("if_switch_linter skips allowed usages", { expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2", linter) # still no third if() clause expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2 else 3", linter) + + # empty string comparisons can't use switch() + expect_no_lint("if (x == '') 1 else if (x == 'a') 2 else if (x == 'b') 3", linter) + expect_no_lint('if (x == "") 1 else if (x == "a") 2 else if (x == "b") 3', linter) + expect_no_lint("if (x == 'a') 1 else if (x == '') 2 else if (x == 'b') 3", linter) + expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2 else if (x == '') 3", linter) + expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2 else if (x == 'c') 3 else if (x == '') 4", linter) }) test_that("if_switch_linter blocks simple disallowed usages", { From fe5b810dc9efdcb81a4e2bf29bfc1ea1f8ef77e7 Mon Sep 17 00:00:00 2001 From: Emmanuel Ferdman Date: Sat, 28 Feb 2026 15:23:18 +0200 Subject: [PATCH 2/7] Fix false positive in `if_switch_linter()` for empty strings Signed-off-by: Emmanuel Ferdman --- NEWS.md | 2 +- R/if_switch_linter.R | 61 ++++++++++++++++++-------- tests/testthat/test-if_switch_linter.R | 13 ++++++ 3 files changed, 56 insertions(+), 20 deletions(-) diff --git a/NEWS.md b/NEWS.md index e1e5fc4dfb..fe8c821c1d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,7 +11,7 @@ * Excluding `cyclocomp_linter()` in `available_linters()` or `linters_with_tags()`, which requires the weak dependency {cyclocomp}, no longer emits a warning (#2909, @MichaelChirico). * `repeat_linter()` no longer errors when `while` is in a column to the right of `}` (#2828, @MichaelChirico). * `undesirable_operator_linter(call_is_undesirable = FALSE)` now correctly skips prefix notation like `` `:::`(pkg, fun) `` (#2999, @emmanuel-ferdman). -* `if_switch_linter()` no longer produces a false positive when comparing to empty strings (`""` or `''`), which cannot be used as `switch()` case names (#2835, @emmanuel-ferdman). +* `if_switch_linter()` no longer produces a false positive when comparing to empty strings (`""`, `''`, or raw strings like `R"()"`), which cannot be used as `switch()` case names (#2835, @emmanuel-ferdman). ## New and improved features diff --git a/R/if_switch_linter.R b/R/if_switch_linter.R index 287cd0f67a..c0b0201f5a 100644 --- a/R/if_switch_linter.R +++ b/R/if_switch_linter.R @@ -188,13 +188,6 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) switch_xpath <- NULL } - # NB: IF AND {...} AND ELSE/... implies >= 3 equality conditions are present - # .//expr/IF/...: the expr in `==` that's _not_ the STR_CONST - # not(preceding::IF): prevent nested matches which might be incorrect globally - empty_string_cond <- paste0( - ".//ELSE/following-sibling::expr/IF/following-sibling::expr[1][EQ]", - "/expr/STR_CONST[string-length(text()) = 2]" - ) if_xpath <- glue(" //IF /parent::expr[ @@ -206,27 +199,27 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) and ELSE/following-sibling::expr[IF and {equal_str_cond}] ] and not({ max_lines_cond }) - and not({ empty_string_cond }) ] ") - # not(. != .): don't match if there are _any_ expr which _don't_ match the top expr - equality_test_cond <- glue("self::*[ - .//expr/IF/following-sibling::{equal_str_cond}/expr[not(STR_CONST)] - != expr[1][EQ]/expr[not(STR_CONST)] - ]") - Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content bad_expr <- xml_find_all(xml, if_xpath) - expr_all_equal <- is.na(xml_find_first( - strip_comments_from_subtree(bad_expr), - equality_test_cond - )) + + bad_expr_clean <- strip_comments_from_subtree(bad_expr) + expr_all_equal <- vapply(bad_expr_clean, chain_vars_equal, logical(1L)) + bad_expr <- bad_expr[expr_all_equal] + + # Exclude empty strings, which can't be used as switch() case names + has_empty <- vapply(bad_expr, function(expr) { + str_nodes <- get_chain_strings(expr) + any(vapply(str_nodes, function(n) !nzchar(get_r_string(n)), logical(1L))) + }, logical(1L)) + bad_expr <- bad_expr[!has_empty] lints <- xml_nodes_to_lints( - bad_expr[expr_all_equal], + bad_expr, source_expression = source_expression, lint_message = paste( "Prefer switch() statements over repeated if/else equality tests,", @@ -251,3 +244,33 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) lints }) } + +get_chain_strings <- function(expr) { + str_nodes <- list() + first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr/STR_CONST") + if (!is.na(first)) str_nodes <- c(str_nodes, list(first)) + current <- expr + repeat { + else_if <- xml_find_first(current, "ELSE/following-sibling::expr[IF]") + if (is.na(else_if)) break + current <- else_if + str_node <- xml_find_first(current, "IF/following-sibling::expr[1][EQ]/expr/STR_CONST") + if (!is.na(str_node)) str_nodes <- c(str_nodes, list(str_node)) + } + str_nodes +} + +chain_vars_equal <- function(expr) { + var_nodes <- list() + first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") + if (!is.na(first)) var_nodes <- c(var_nodes, list(xml_text(first))) + current <- expr + repeat { + else_if <- xml_find_first(current, "ELSE/following-sibling::expr[IF]") + if (is.na(else_if)) break + current <- else_if + var_node <- xml_find_first(current, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") + if (!is.na(var_node)) var_nodes <- c(var_nodes, list(xml_text(var_node))) + } + length(unique(unlist(var_nodes))) == 1L +} diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index 1f31e6eca5..7d91278245 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -28,6 +28,19 @@ test_that("if_switch_linter skips allowed usages", { expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2 else if (x == 'c') 3 else if (x == '') 4", linter) }) +test_that("if_switch_linter handles raw empty strings", { + skip_unless_r(">= 4.0.0") + + linter <- if_switch_linter() + + # raw empty strings can't use switch() either + expect_no_lint('if (x == R"()") 1 else if (x == "a") 2 else if (x == "b") 3', linter) + expect_no_lint('if (x == r"[]") 1 else if (x == "a") 2 else if (x == "b") 3', linter) + expect_no_lint('if (x == R"{}") 1 else if (x == "a") 2 else if (x == "b") 3', linter) + expect_no_lint('if (x == R"--()--") 1 else if (x == "a") 2 else if (x == "b") 3', linter) + expect_no_lint('if (x == R"()") 1 else if (x == R"{}") 2 else if (x == R"[]") 3', linter) +}) + test_that("if_switch_linter blocks simple disallowed usages", { linter <- if_switch_linter() lint_msg <- rex::rex("Prefer switch() statements over repeated if/else equality tests") From 54be4df10f023b9bfd91537e8a2596cc9bbb8fa7 Mon Sep 17 00:00:00 2001 From: Emmanuel Ferdman Date: Sat, 28 Feb 2026 16:19:53 +0200 Subject: [PATCH 3/7] Fix false positive in `if_switch_linter()` for empty strings Signed-off-by: Emmanuel Ferdman --- tests/testthat/test-if_switch_linter.R | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index 7d91278245..7c44747286 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -29,8 +29,6 @@ test_that("if_switch_linter skips allowed usages", { }) test_that("if_switch_linter handles raw empty strings", { - skip_unless_r(">= 4.0.0") - linter <- if_switch_linter() # raw empty strings can't use switch() either From b2aa7590259e5b507ff116906088ba405ff66a0f Mon Sep 17 00:00:00 2001 From: Emmanuel Ferdman Date: Mon, 2 Mar 2026 20:40:45 +0200 Subject: [PATCH 4/7] Fix false positive in `if_switch_linter()` for empty strings Signed-off-by: Emmanuel Ferdman --- NEWS.md | 4 ++-- R/if_switch_linter.R | 16 +++++++++------- tests/testthat/test-if_switch_linter.R | 3 --- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/NEWS.md b/NEWS.md index fe8c821c1d..3682caa698 100644 --- a/NEWS.md +++ b/NEWS.md @@ -10,8 +10,6 @@ * Excluding `cyclocomp_linter()` in `available_linters()` or `linters_with_tags()`, which requires the weak dependency {cyclocomp}, no longer emits a warning (#2909, @MichaelChirico). * `repeat_linter()` no longer errors when `while` is in a column to the right of `}` (#2828, @MichaelChirico). -* `undesirable_operator_linter(call_is_undesirable = FALSE)` now correctly skips prefix notation like `` `:::`(pkg, fun) `` (#2999, @emmanuel-ferdman). -* `if_switch_linter()` no longer produces a false positive when comparing to empty strings (`""`, `''`, or raw strings like `R"()"`), which cannot be used as `switch()` case names (#2835, @emmanuel-ferdman). ## New and improved features @@ -53,6 +51,8 @@ ### Lint accuracy fixes: removing false positives +* `if_switch_linter()` no longer produces a false positive when comparing to empty strings (`""`, `''`, or raw strings like `R"()"`), which cannot be used as `switch()` case names (#2835, @emmanuel-ferdman). +* `undesirable_operator_linter(call_is_undesirable = FALSE)` now correctly skips prefix notation like `` `:::`(pkg, fun) `` (#2999, @emmanuel-ferdman). * `unnecessary_nesting_linter()` treats `=` assignment the same as `<-` for several pieces of logic (#2245 and #2829, @MichaelChirico). * `vector_logic_linter()` ignores scalar operators (`&&`/`||`) inside anonymous functions within `filter()`/`subset()` (#2935, @emmanuel-ferdman). diff --git a/R/if_switch_linter.R b/R/if_switch_linter.R index c0b0201f5a..ed3280650c 100644 --- a/R/if_switch_linter.R +++ b/R/if_switch_linter.R @@ -212,11 +212,11 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) bad_expr <- bad_expr[expr_all_equal] # Exclude empty strings, which can't be used as switch() case names - has_empty <- vapply(bad_expr, function(expr) { + nonempty <- vapply(bad_expr, function(expr) { str_nodes <- get_chain_strings(expr) - any(vapply(str_nodes, function(n) !nzchar(get_r_string(n)), logical(1L))) + all(vapply(str_nodes, function(n) nzchar(get_r_string(n)), logical(1L))) }, logical(1L)) - bad_expr <- bad_expr[!has_empty] + bad_expr <- bad_expr[nonempty] lints <- xml_nodes_to_lints( bad_expr, @@ -245,6 +245,7 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) }) } +# Extract STR_CONST nodes from equality conditions in an if/else if chain. get_chain_strings <- function(expr) { str_nodes <- list() first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr/STR_CONST") @@ -260,17 +261,18 @@ get_chain_strings <- function(expr) { str_nodes } +# Check that equality conditions in an if/else if chain use the same variable. chain_vars_equal <- function(expr) { - var_nodes <- list() + var_nodes <- character() first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") - if (!is.na(first)) var_nodes <- c(var_nodes, list(xml_text(first))) + if (!is.na(first)) var_nodes <- c(var_nodes, xml_text(first)) current <- expr repeat { else_if <- xml_find_first(current, "ELSE/following-sibling::expr[IF]") if (is.na(else_if)) break current <- else_if var_node <- xml_find_first(current, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") - if (!is.na(var_node)) var_nodes <- c(var_nodes, list(xml_text(var_node))) + if (!is.na(var_node)) var_nodes <- c(var_nodes, xml_text(var_node)) } - length(unique(unlist(var_nodes))) == 1L + length(unique(var_nodes)) == 1L } diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index 7c44747286..b9f59c19f2 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -32,9 +32,6 @@ test_that("if_switch_linter handles raw empty strings", { linter <- if_switch_linter() # raw empty strings can't use switch() either - expect_no_lint('if (x == R"()") 1 else if (x == "a") 2 else if (x == "b") 3', linter) - expect_no_lint('if (x == r"[]") 1 else if (x == "a") 2 else if (x == "b") 3', linter) - expect_no_lint('if (x == R"{}") 1 else if (x == "a") 2 else if (x == "b") 3', linter) expect_no_lint('if (x == R"--()--") 1 else if (x == "a") 2 else if (x == "b") 3', linter) expect_no_lint('if (x == R"()") 1 else if (x == R"{}") 2 else if (x == R"[]") 3', linter) }) From a8a43f13dc180a6d594437534650ab94cac1b940 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 2 Mar 2026 11:15:21 -0800 Subject: [PATCH 5/7] more raw string tests involving non-empty strings --- tests/testthat/test-if_switch_linter.R | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index b9f59c19f2..acbb12b410 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -17,8 +17,12 @@ test_that("if_switch_linter skips allowed usages", { # simple cases with two conditions might be more natural # without switch(); require at least three branches to trigger a lint expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2", linter) + # not thrown by raw strings + expect_no_lint("if (x == 'a') 1 else if (x == R'(b)') 2", linter) # still no third if() clause expect_no_lint("if (x == 'a') 1 else if (x == 'b') 2 else 3", linter) + # not thrown by raw strings + expect_no_lint("if (x == 'a') 1 else if (x == R'(b)') 2 else 3", linter) # empty string comparisons can't use switch() expect_no_lint("if (x == '') 1 else if (x == 'a') 2 else if (x == 'b') 3", linter) @@ -42,6 +46,8 @@ test_that("if_switch_linter blocks simple disallowed usages", { # anything with >= 2 equality statements is deemed switch()-worthy expect_lint("if (x == 'a') 1 else if (x == 'b') 2 else if (x == 'c') 3", lint_msg, linter) + # regardless of raw strings + expect_lint("if (x == 'a') 1 else if (x == r'(b)') 2 else if (x == R'--[c]--') 3", lint_msg, linter) # expressions are also OK expect_lint("if (foo(x) == 'a') 1 else if (foo(x) == 'b') 2 else if (foo(x) == 'c') 3", lint_msg, linter) # including when comments are present From a19a631fa93cbe61801f55378d5182e18d91932a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 2 Mar 2026 11:23:11 -0800 Subject: [PATCH 6/7] rename helpers --- R/if_switch_linter.R | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/R/if_switch_linter.R b/R/if_switch_linter.R index ed3280650c..6867410657 100644 --- a/R/if_switch_linter.R +++ b/R/if_switch_linter.R @@ -208,12 +208,12 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) bad_expr <- xml_find_all(xml, if_xpath) bad_expr_clean <- strip_comments_from_subtree(bad_expr) - expr_all_equal <- vapply(bad_expr_clean, chain_vars_equal, logical(1L)) + expr_all_equal <- vapply(bad_expr_clean, if_else_chain_expr_is_unique, logical(1L)) bad_expr <- bad_expr[expr_all_equal] # Exclude empty strings, which can't be used as switch() case names nonempty <- vapply(bad_expr, function(expr) { - str_nodes <- get_chain_strings(expr) + str_nodes <- if_else_chain_strings(expr) all(vapply(str_nodes, function(n) nzchar(get_r_string(n)), logical(1L))) }, logical(1L)) bad_expr <- bad_expr[nonempty] @@ -245,8 +245,8 @@ if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) }) } -# Extract STR_CONST nodes from equality conditions in an if/else if chain. -get_chain_strings <- function(expr) { +# Extract STR_CONST nodes from equality conditions in an if/else if chain +if_else_chain_strings <- function(expr) { str_nodes <- list() first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr/STR_CONST") if (!is.na(first)) str_nodes <- c(str_nodes, list(first)) @@ -261,18 +261,18 @@ get_chain_strings <- function(expr) { str_nodes } -# Check that equality conditions in an if/else if chain use the same variable. -chain_vars_equal <- function(expr) { - var_nodes <- character() +# Check that equality conditions in an if/else if chain use the same expression +if_else_chain_expr_is_unique <- function(expr) { + expr_nodes <- character() first <- xml_find_first(expr, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") - if (!is.na(first)) var_nodes <- c(var_nodes, xml_text(first)) + if (!is.na(first)) expr_nodes <- c(expr_nodes, xml_text(first)) current <- expr repeat { else_if <- xml_find_first(current, "ELSE/following-sibling::expr[IF]") if (is.na(else_if)) break current <- else_if - var_node <- xml_find_first(current, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") - if (!is.na(var_node)) var_nodes <- c(var_nodes, xml_text(var_node)) + expr_node <- xml_find_first(current, "IF/following-sibling::expr[1][EQ]/expr[not(STR_CONST)]") + if (!is.na(expr_node)) expr_nodes <- c(expr_nodes, xml_text(expr_node)) } - length(unique(var_nodes)) == 1L + length(unique(expr_nodes)) == 1L } From f0f8e57b80c2e65ed54b2c80c0aaf3d998c9390c Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 2 Mar 2026 11:25:48 -0800 Subject: [PATCH 7/7] include negative cases in multi-lint test --- tests/testthat/test-if_switch_linter.R | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index acbb12b410..9dcb055c6a 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -98,6 +98,20 @@ test_that("multiple lints have right metadata", { } else if (y == 'C') { do_C() } + if (z1 == 'a') { + do_a() + } else if (z2 == 'b') { + do_b() + } else if (z3 == 'c') { + do_c() + } + if (a == '1') { + do_1() + } else if (a == '2') { + do_2() + } else if (a == '') { + do_blank() + } }"), list( list(lint_msg, line_number = 2L),