Upstreaming linters from Google's internal linting suite

[MichaelChirico]

Internally at Google we use lintr to power our in-editor and CI-based R linting tools. Over the last year+ (in addition to working directly on lintr) we have been building out a set of linters that extends the suite offered by lintr. Currently, the package has >60 such linters, almost all of which we think are widely relevant. They have all been test on & applied to existing internal R code at Google.

We are happy to contribute any subset of these to lintr. Here is a brief summary of all of them; I can elaborate more as needed:

• AnyDuplicatedLinter - Require usage of anyDuplicated() > 0 over any(duplicated(.))
• AnyIsNALinter - Require usage of anyNA over any(is.na(.))
• ApplyAnonymousLinter - Block usage of anonymous functions in *apply functions when unnecessary New unnecessary_lambda_linter #1541
• BaseOverwriteLinter - Block assigning any variables whose name clashes with a 'base' R function New object_overwrite_linter #2307
• BooleanArithmeticLinter New boolean_arithmetic_linter #1579
• CharacterOnlyLinter - Enforce library calls using symbols Extend library_call_linter for character.only usage #2279
• ClassEqualsLinter - Block comparison of class with ==
• ComparisonNegationLinter New comparison_negation_linter #2272
• ConsecutiveMutateLinter New consecutive_mutate_linter #2305
• ConsecutiveSuppressionLinter New consecutive_suppression_linter #2306
• ElseSameLineLinter - Require else to come on the same line as
• EmptyAssignmentLinter - Block assignment of {} New empty_assignment_linter #1637
• ExpectComparisonLinter - Require usage of expect_gt(x, y) over expect_true(x > y) (and similar) Expect comparison #955
• ExpectIdenticalLinter - Require usage of expect_identical(x, y) where appropriate New expect_identical_linter #958
• ExpectIsLinter - Redirect away from deprecated testthat::expect_is() expect_is_linter, or just use undesirable_function_linter? #946
• ExpectLengthLinter - Require usage of expect_length(x, n) over expect_equal(length(x), n) New expect_length_linter #950
• ExpectNamedLinter - Require usage of expect_named(x, n) over expect_equal(names(x), n) New expect_named_linter #949
• ExpectNotLinter - Require usage of expect_false(.) over expect_true(!.) New expect_not_linter #945
• ExpectNullLinter - Require usage of expect_null(x) over expect_equal(x, NULL) and similar expect_null_linter #887
• ExpectS3ClassLinter - Require usage of expect_s3_class() or expect_s4_class() where appropriate. expect_s3_class_linter and expect_s4_class_linter #943
• ExpectS4ClassLinter - Require usage of expect_s4_class(x, k) over expect_true(is(x, k)) expect_s3_class_linter and expect_s4_class_linter #943
• ExpectTrueFalseAndConditionLinter - Force && conditions in expect_true(), expect_false() to be written separately New conjunct_expecation_linter #948
• ExpectTrueFalseLinter - Require usage of expect_true(x) over expect_equal(x, TRUE) New expect_true_false_linter #947
• ExpectTypeLinter - Require usage of expect_type(x, type) over expect_equal(typeof(x), type) expect_type_linter #924
• ExplicitReturnLinter Linter for explicit/implicit returns #2271
• FilterAndConditionLinter extend conjunct_test_linter for dplyr::filter() #2077
• FixedRegexLinter - Require usage of 'fixed=TRUE' in regular expressions where appropriate New fixed_regex_linter #1021
• ForLoopIndexLinter - Block usage of for loops directly overwriting the indexing variable New for_loop_index_linter #1629
• FunctionBraceLinter - Require multi-line functions to use braces
• GrepSubsetLinter - Require usage of grep(value = TRUE) over xgrep(x)
• IfelseCensorLinter - Block usage of ifelse where pmin or pmax is more appropriate
• IfElseMatchBracesLinter - Require both or neither if/else branches to use curly braces
• IfNotElseLinter if_not_else_linter() for "double negation" in if statements #2071
• IfSwitchLinter - Require usage of switch() over repeated if/else blocks New if_switch_linter #2304
• ImplicitElseReturnLinter New allow_implicit_else argument for return_linter #2321
• InnerCombineLinter - Require c() to be applied before relatively expensive vectorized functions
• InnerComparisonLinter - Require == to be used outside of sapply() when comparing to a constant Extend unnecessary_lambda_linter to look for "inner comparisons" #2300
• IsNumericLinter - Redirect is.numeric(x) || is.integer(x) to just use is.numeric(x) New is_numeric_linter #1635
• KeywordQuoteLinter - Block unnecessary quoting in calls keyword_quote_linter for unnecessary quoting #2030
• LengthLevelsLinter - Require usage of nlevels over length(levels(.)) length_levels_linter() for length(levels(x)) --> nlevels(x) #2051
• LengthsLinter New lengths_linter #1568
• ListComparisonLinter New list_comparison_linter #2293
• LiteralCoercionLinter - Require usage of correctly-typed literals over literal coercions
• MagrittrDotLinter New unnecessary_placeholder_linter #1656
• MultipleStopifnotLinter - Force consecutive stopifnot() calls into just one
• NestedIfelseLinter - Block usage of nested ifelse() calls
• NestedPipeLinter New nested_pipe_linter #2301
• NrowSubsetLinter New nrow_subset_linter #2298
• NumericLeadingZeroLinter - Require usage of a leading zero in all fractional numerics
• OneCallPipeLinter New one_call_pipe_linter #2294
• OuterNegationLinter - Require usage of !any(.) over all(!.), !all(.) over any(!.)
• PasteFilePathLinter - Block usage of paste() with sep='/' file path detection in paste_linter #2074
• PasteSepLinter - Block usage of paste() with sep=''
• PasteStrrepLinter - Require usage of strrep('.', n) over paste(rep('.', n), collapse = '') extend paste_linter() for strrep() equivalents #1652
• PasteToStringLinter - Block usage of paste() with collapse=', '
• PipeCallLinter - Force explicit calls in magrittr pipes New pipe_call_linter enforcing calls (not symbols) in magrittr pipes #801
• PipeReturnLinter New pipe_return_linter #2299
• RedundantEqualsLinter New redundant_equals_linter #1556
• RedundantIfelseLinter - Prevent ifelse() from being used to produce TRUE/FALSE
• RepLenLinter New rep_len_linter #2286
• ReturnAssignmentLinter New function_return_linter() #1569
• RequireNzcharLinter - Require usage of nzchar where appropriate New nzchar_linter #2275
• RoutineRegistrationLinter - Identify .Call usage to unregistered routines New routine_registration_linter #1669
• SampleIntLinter - Require usage of sample.int(n, m, ...) over sample(1:n, m, ...) New sample_int_linter #2274
• ScalarInLinter - Block usage like x %in% 'a' scalar_in_linter() for x %in% 1 #2084
• StopifnotAllLinter New stopifnot_all_linter #2273
• StopifnotAndConditionLinter - Force && conditions in stopifnot() to be written separately
• StopPasteLinter - Block usage of paste() and paste0() with messaging functions using ...
• StringsAsFactorsLinter - Identify cases where stringsAsFactors should be supplied explicitly
• SystemFileLinter - Block usage of file.path() with system.file()
• TerminalCloseLinter - Prohibit close() from terminating a function definition New terminal_close_linter #2276
• UnnecessaryNestingLinter New unnecessary_nesting_linter #2302
• UnreachableCodeLinter - Block unreachable code and comments following return statements
• VectorLogicLinter - Enforce usage of scalar && in conditional statements
• WhichGreplLinter - Require usage of grep over which(grepl(.)) New which_grepl_linter #2281
• YodaTestLinter - Block obvious 'yoda tests' New yoda_test_linter #957

What do you think is the best way to decide about which are in/out of scope for lintr? Does it make sense to delay this until 3.0 is released, or push to include with the major version bump?

[jimhester]

I think it would be great to include these in the 3.0 release. It would also give people more incentive to upgrade.

It might also be worthwhile to package these as a separate object, so it would be easy to use them all as google_linters like we currently have for default_linters.

As to which of them should be on by default I leave that to others to discuss.

[AshesITR]

That sounds really great! I also like the idea of adding google_linters. In the same spirit, should we add all_linters as well?
At the moment, non-default linters are somewhat hard to discover. We ended up going through the entire ?linters manpage and look at all linters to decide whether we might want to use it.
With all_linters the workflow for deciding on a subset could be to first enable all and then disable those that actually cause unwanted lints in the code.

To decide on the defaults, it will probably help to look at the concrete implementations.

Curious side question regarding ScalarInLinter: That construct is my go-to way to check if x == "a" when x is known to contain NAs. Is there a better, equally efficient, way to do this?
Illustrative code:

x <- c(NA, "a", "b", "c", NA)
x == "a" # c(NA, TRUE, FALSE, FALSE, NA)
x %in% "a" # c(FALSE, TRUE, FALSE, FALSE, FALSE) -- much safer for e.g. indexing

[MichaelChirico]

no, it's a known false positive of the linter. we decided it's better for readability to treat NA handling explicitly, if so intended, and put that caveat in the lint message:

!is.na(x) & x == "a"

[MichaelChirico]

@AshesITR with #958 we'll finish up the current set of 13 testthat-related linters we have.

I think now is a good time to think if we want to triage the prep for lintr 3.0.0 -- have have O(100) linters ready for upstreaming, which would put us at about June to finish, assuming we keep up the current (quite high) pace of new PRs.

Should we prioritize instead? Pick some set of particularly neat/high-impact linters to include in the 3.0.0 release, then focus on tidying up 3.0.0 and putting it on CRAN before returning to new google linters?

[AshesITR]

Sounds good to me.
Do you have any linters in mind that fire frequently and could be considered high-impact?
We could aim for a 3.0.1 release containing the rest of the google linters.

[AshesITR]

@MichaelChirico I edited your OP with a checklist and linked all PRs / issues I was aware of.
Are there new linters to be upstreamed or is the list complete?

[LukasDSauer]

Hello! I am interested in applying the Google R Style Guide to one of our packages. Thus, I am thrilled to see that you consider supplying a lintr::google_linters list to the lintr package. Is this list in planning and will it be added to the package in the near future?

[MichaelChirico]

there are a slew already added -- see #962

[LukasDSauer]

Hi Michael and thank you for the quick reply. As far as I understand, #962 contains a list of Google linters already added to lintr. Jim Hester suggested above that available Google linters could be packaged in a separate object:

It might also be worthwhile to package these as a separate object, so it would be easy to use them all as google_linters like we currently have for default_linters.

Has this already been done or would I have to include all of the linters in #962 one-by-one?

[MichaelChirico]

With #2321, that's it!

Special shout-out to @AshesITR and @IndrajeetPatil for your time and insights during the review process! The suite is now much better thanks to you! Thanks also to others who have filed follow-ups/tested the new linters on your own projects.