finer-tuned handling of ambiguous XML nodes for infix operators (#2115)

* finer-tuned handling of ambiguous XML nodes for infix operators

* tidy

* adjust NEWS

* extra newline

* centralize exact tag match logic

* shrink diff

* simplify undesirable operator case

---------

Co-authored-by: AshesITR <[email protected]>

Author: MichaelChirico

NEWS.md

@@ -1,5 +1,9 @@
 # lintr (development version)
 
+## Breaking changes
+
+* `infix_spaces_linter()` distinguishes `<-`, `:=`, `<<-` and `->`, `->>`, i.e. `infix_spaces_linter(exclude_operators = "->")` will no longer exclude `->>` (#2115, @MichaelChirico). This change is breaking for users relying on manually-supplied `exclude_operators` containing `"<-"` to also exclude `:=` and `<<-`. The fix is to manually supply `":="` and `"<<-"` as well. We don't expect this change to affect many users, the fix is simple, and the new behavior is much more transparent, so we are including this breakage in a minor release.
+
 ## Bug fixes
 
 * `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.

R/infix_spaces_linter.R

@@ -6,9 +6,8 @@
 #' @param exclude_operators Character vector of operators to exclude from consideration for linting.
 #'   Default is to include the following "low-precedence" operators:
 #'   `+`, `-`, `~`, `>`, `>=`, `<`, `<=`, `==`, `!=`, `&`, `&&`, `|`, `||`, `<-`, `:=`, `<<-`, `->`, `->>`,
-#'   `=`, `/`, `*`, and any infix operator (exclude infixes by passing `"%%"`). Note that `<-`, `:=`, and `<<-`
-#'   are included/excluded as a group (indicated by passing `"<-"`), as are `->` and `->>` (_viz_, `"->"`),
-#'   and that `=` for assignment and for setting arguments in calls are treated the same.
+#'   `=`, `/`, `*`, and any infix operator (exclude infixes by passing `"%%"`). Note that `=` for assignment
+#'   and for setting arguments in calls are treated the same.
 #' @param allow_multiple_spaces Logical, default `TRUE`. If `FALSE`, usage like `x  =  2` will also be linted;
 #'   excluded by default because such usage can sometimes be used for better code alignment, as is allowed
 #'   by the style guide.
@@ -65,9 +64,8 @@ infix_spaces_linter <- function(exclude_operators = NULL, allow_multiple_spaces
     lint_message <- "Put exactly one space on each side of infix operators."
   }
 
-  infix_tokens <- infix_metadata[
-    infix_metadata$low_precedence & !infix_metadata$string_value %in% exclude_operators,
-    "xml_tag"
+  infix_tokens <- infix_metadata$xml_tag_exact[
+    infix_metadata$low_precedence & !infix_metadata$string_value %in% exclude_operators
   ]
 
   # NB: preceding-sibling::* and not preceding-sibling::expr because

R/shared_constants.R

@@ -130,8 +130,11 @@ infix_metadata <- data.frame(stringsAsFactors = FALSE, matrix(byrow = TRUE, ncol
   "OR",              "|",
   "AND2",            "&&",
   "OR2",             "||",
-  "LEFT_ASSIGN",     "<-",  # also includes := and <<-
-  "RIGHT_ASSIGN",    "->",  # also includes ->>
+  "LEFT_ASSIGN",     "<-",
+  "LEFT_ASSIGN",     ":=",
+  "LEFT_ASSIGN",     "<<-",
+  "RIGHT_ASSIGN",    "->",
+  "RIGHT_ASSIGN",    "->>",
   "EQ_ASSIGN",       "=",
   "EQ_SUB",          "=",   # in calls: foo(x = 1)
   "EQ_FORMALS",      "=",   # in definitions: function(x = 1)
@@ -140,7 +143,8 @@ infix_metadata <- data.frame(stringsAsFactors = FALSE, matrix(byrow = TRUE, ncol
   "OP-SLASH",        "/",
   "OP-STAR",         "*",
   "OP-COMMA",        ",",
-  "OP-CARET",        "^",   # also includes **
+  "OP-CARET",        "^",
+  "OP-CARET",        "**",
   "OP-AT",           "@",
   "OP-EXCLAMATION",  "!",
   "OP-COLON",        ":",
@@ -168,16 +172,19 @@ infix_metadata$unary <- infix_metadata$xml_tag %in% c("OP-PLUS", "OP-MINUS", "OP
 # high-precedence operators are ignored by this linter; see
 #   https://style.tidyverse.org/syntax.html#infix-operators
 infix_metadata$low_precedence <- infix_metadata$string_value %in% c(
-  "+", "-", "~", ">", ">=", "<", "<=", "==", "!=", "&", "&&", "|", "||", "<-", "->", "=", "%%", "/", "*", "|>"
+  "+", "-", "~", ">", ">=", "<", "<=", "==", "!=", "&", "&&", "|", "||",
+  "<-", ":=", "<<-", "->", "->>", "=", "%%", "/", "*", "|>"
 )
 # comparators come up in several lints
 infix_metadata$comparator <- infix_metadata$string_value %in% c("<", "<=", ">", ">=", "==", "!=")
 
-# undesirable_operator_linter needs to distinguish
-infix_overload <- data.frame(
-  exact_string_value = c("<-", "<<-", "=", "->", "->>", "^", "**"),
-  xml_tag = rep(c("LEFT_ASSIGN", "RIGHT_ASSIGN", "OP-CARET"), c(3L, 2L, 2L)),
-  stringsAsFactors = FALSE
+# these XML nodes require checking the text() to disambiguate multiple operators using the same tag
+infix_metadata$ambiguous_tag <- infix_metadata$xml_tag %in% infix_metadata$xml_tag[duplicated(infix_metadata$xml_tag)]
+infix_metadata$xml_tag_exact <- infix_metadata$xml_tag
+infix_metadata$xml_tag_exact[infix_metadata$ambiguous_tag] <- sprintf(
+  "%s[text() = '%s']",
+  infix_metadata$xml_tag_exact[infix_metadata$ambiguous_tag],
+  infix_metadata$string_value[infix_metadata$ambiguous_tag]
 )
 
 # functions equivalent to base::ifelse() for linting purposes

R/undesirable_operator_linter.R

@@ -46,24 +46,11 @@ undesirable_operator_linter <- function(op = default_undesirable_operators) {
   if (is.null(names(op)) || !all(nzchar(names(op))) || length(op) == 0L) {
     stop("'op' should be a non-empty named character vector; use missing elements to indicate default messages.")
   }
-  undesirable_operator_metadata <- merge(
-    # infix must be handled individually below; non-assignment `=` are always OK
-    infix_metadata[
-      infix_metadata$string_value != "%%" & !infix_metadata$xml_tag %in% c("EQ_SUB", "EQ_FORMALS"),
-    ],
-    infix_overload,
-    by = "xml_tag", all.x = TRUE
-  )
-
-  included_operators <- undesirable_operator_metadata$string_value %in% names(op) |
-    undesirable_operator_metadata$exact_string_value %in% names(op)
-  operator_nodes <- undesirable_operator_metadata$xml_tag[included_operators]
-  needs_exact_string <- !is.na(undesirable_operator_metadata$exact_string_value[included_operators])
-  operator_nodes[needs_exact_string] <- sprintf(
-    "%s[text() = '%s']",
-    operator_nodes[needs_exact_string],
-    undesirable_operator_metadata$exact_string_value[included_operators][needs_exact_string]
-  )
+  # infix must be handled individually below; non-assignment `=` are always OK
+  operator_nodes <- infix_metadata$xml_tag_exact[
+    infix_metadata$string_value %in% setdiff(names(op), "%%") &
+      !infix_metadata$xml_tag %in% c("EQ_SUB", "EQ_FORMALS")
+  ]
 
   is_infix <- startsWith(names(op), "%")
   if (any(is_infix)) {

man/infix_spaces_linter.Rd

@@ -70,6 +70,8 @@ test_that("The three `=` are all linted", {
 })
 
 test_that("exclude_operators works", {
+  lint_msg <- rex::rex("Put spaces around all infix operators.")
+
   expect_lint("a+b", NULL, infix_spaces_linter(exclude_operators = "+"))
   expect_lint(
     trim_some("
@@ -80,10 +82,13 @@ test_that("exclude_operators works", {
     infix_spaces_linter(exclude_operators = c("+", "-"))
   )
 
-  # grouped operators
-  expect_lint("a<<-1", NULL, infix_spaces_linter(exclude_operators = "<-"))
-  expect_lint("a:=1", NULL, infix_spaces_linter(exclude_operators = "<-"))
-  expect_lint("a->>1", NULL, infix_spaces_linter(exclude_operators = "->"))
+  # operators match on text, not hidden node
+  expect_lint("a<<-1", lint_msg, infix_spaces_linter(exclude_operators = "<-"))
+  expect_lint("a<<-1", NULL, infix_spaces_linter(exclude_operators = "<<-"))
+  expect_lint("a:=1", lint_msg, infix_spaces_linter(exclude_operators = "<-"))
+  expect_lint("a:=1", NULL, infix_spaces_linter(exclude_operators = ":="))
+  expect_lint("a->>1", lint_msg, infix_spaces_linter(exclude_operators = "->"))
+  expect_lint("a->>1", NULL, infix_spaces_linter(exclude_operators = "->>"))
   expect_lint("a%any%1", NULL, infix_spaces_linter(exclude_operators = "%%"))
   expect_lint("function(a=1) { }", NULL, infix_spaces_linter(exclude_operators = "="))
   expect_lint("foo(a=1)", NULL, infix_spaces_linter(exclude_operators = "="))