S3 inheritance should be ignored

[bmwilly]

Since 27a5d95, S3 objects are incorrectly linted

Reproducible example:

$ cat ~/tmp/ex.R
f <- function(x, ...) UseMethod("f", x)
f.character <- function(x) x
$ Rscript -e 'lintr::lint("~/tmp/ex.R")'
~/tmp/ex.R:2:1: style: Variable or function name should be snake_case.
f.character <- function(x) x
^~~~~~~~~~~

[jimhester]

@fangly Can you look into this regression and add an additional test for it, thanks!

[fangly]

To enfore a given naming style, e.g. "snake_case":

• Before: You would have used multiple_dots_linter() and camel_case_linter(). All names with a single dot were ignored, whether they corresponded to S3 generics or not (e.g. "my.function" was wrongly ignored). Also, names with multiple dot were linted, whether they corresponded to S3 generics or not (e.g. "as.data.frame.data.frame" would have been wrongly linted).
• Now: The idea is to use a single linter, object_name_linter("snake_case"). This function lints names regardless of the number of does, and is aware of base S3 generics but not aware of user-defined generics.

In short, before, you got some false positive and some false negative lints, and now you should only get some false positives. False positives are annoying, but you can ignore them with a "#nolint" comment. False negatives are more problematic, because as a user, you cannot do anything against them.

Workarounds and solutions:

1. As mentionned above, you could add "#nolint" comments on lines where you declare S3 methods. It is not ideal if you have many S3 methods,
2. A simple lintr fix:
   2.1. Have object_name_linter() explicitly ignore single dotted names, like in the old behavior. As explained in the examples above, this is not foolproof.
   2.2. Add a "lax" option to object_name_linter(). Just like in absolute_path_linter(), it would default to TRUE and allow users to select whether they prefer to be strict (and lint too much) or lax (and lint too little).
3. Ideally, lintr would have a mechanism to whitelist user-defined generics. This is difficult to do well because a S3 generic could be declared after a S3 method declaration or even in a different file. We also have to keep in mind that lintr works on a package level with lint_package() and on a single file level with lint(). Working on a package level with the NAMESPACE file would not be ideal, because not all S3 methods need to be exported.

What are your thoughts?

[jimhester]

We could load the package namespace like was done previously and get the defined generics that way. The downside is if the project is not a package or if the package is not installed the generics won't be found.

Alternatively we can parse all project code regardless of which file is being linted and whitelist S3 generics. This approach would also be useful for the object_usage_linter(), we could just look for symbol assignment in the package / file namespace.

The latter can be done by using the call_names() definition and looking for length(call_names(f, "UseMethod")) > 0 where f is the parsed function definition.

[bfgray3]

I'd like to add my vote for this issue after encountering a lint for a data.frame S3 method.

[jimhester]

This seems to be fixed in the current master.

[dpprdan]

It seems that this is not fixed for data.frame S3 methods (and potentially other classes with a dot in their name):

$ cat lintr.data.frame.R
f <- function(x, ...) UseMethod("f", x)
f.data.frame <- function(x) x
$ Rscript -e 'lintr::lint("lintr.data.frame.R")'
lintr.data.frame.R:2:1: style: Variable and function name style should be snake_case.
f.data.frame <- function(x) x
^~~~~~~~~~~~

This is from current master (d86407d).

[dpprdan]

@jimhester Regarding my comment above, would you consider reopening this or should I rather open a new issue?