r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.19k stars 184 forks source link

object_name_linter() misses S3 generics declared with `=` assignment #2507

Open klmr opened 8 months ago

klmr commented 8 months ago

Consider the following code:

f = function(x) {
  UseMethod("f")
}

g <- function(x) {
  UseMethod("f")
}

f.default <- function(x) {}

g.default <- function(x) {}

running lintr::lint() on this file with the default linters configuration gives the following output:

/path/test.r:1:3: style: [assignment_linter] Use <-, not =, for assignment.
f = function(x) {
  ^
/path/test.r:9:1: style: [object_name_linter] Variable and function name style should match snake_case or symbols.
f.default <- function(x) {}
^~~~~~~~~

The object_name_linter warning is unexpected: f is declared as a generic. In fact, directly calling declared_s3_generics() on the XML parse tree of the above code returns "g" where it should return c("f", "g").

MichaelChirico commented 8 months ago

Nice find! This should be relatively easy to fix, are you interested in trying a PR? Should be a matter of including EQ_ASSIGN where only LEFT_ASSIGN is assumed now. There's also a relevant note in the wiki

klmr commented 8 months ago

Unfortunately the fix is not quite that easy for somebody who isn’t fluent in XPath, since EQ_ASSIGN is already included:

https://github.com/r-lib/lintr/blob/11eae86373104d813c55d85cf1538108eb40082c/R/declared_functions.R#L7

(I tried changing it to ./EQ_ASSIGN because I am unsure about precedence, but this didn’t fix it.)

klmr commented 8 months ago

Ah, I guess this note in the Wiki might address this:

The outermost <expr> may be <equal_assign> or <expr_or_assign_or_help> instead.

MichaelChirico commented 8 months ago

Ah, I guess this note in the Wiki might address this:

The outermost <expr> may be <equal_assign> or <expr_or_assign_or_help> instead.

yep! it's possible we missed this in this case and only looked for