seq_linter gives wrong message

[MichaelChirico]

writeLines('mtcars[1:min(8, nrow(mtcars)), ]', tmp <- tempfile())
lintr::lint(tmp, seq_linter)
# /tmp/RtmpWXD4Q3/file4c4595f9321d2:1:8: warning: Avoid 1:min expressions, use seq_len. Disable this with a comment: `# nolint` (one line) or `# lintr: disable` (multiple lines). See go/tidy-style#linting for more.
# mtcars[1:min(8, nrow(mtcars)), ]
#        ^

I think this is incorrect because ! min %in% bad_funcs, i.e. it's not the 1:min that's triggering the lint, it's the use of nrow in 1:.

[AshesITR]

Can you check if this issue occurs in the master branch? I can't reproduce with that.

[MichaelChirico]

Same for me on current master. sessionInfo() in details:

``` R version 4.0.0 (2020-04-24) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Linux Mint 19.3 Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.7.1 LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.7.1 locale: [1] LC_CTYPE=en_SG.UTF-8 LC_NUMERIC=C LC_TIME=en_SG.UTF-8 [4] LC_COLLATE=en_SG.UTF-8 LC_MONETARY=en_SG.UTF-8 LC_MESSAGES=en_SG.UTF-8 [7] LC_PAPER=en_SG.UTF-8 LC_NAME=C LC_ADDRESS=C [10] LC_TELEPHONE=C LC_MEASUREMENT=en_SG.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] lintr_2.0.1.9000 loaded via a namespace (and not attached): [1] ps_1.4.0 crayon_1.3.4 withr_2.3.0 rprojroot_1.3-2 assertthat_0.2.1 [6] R6_2.4.1 backports_1.1.10 magrittr_1.5 stringi_1.5.3 lazyeval_0.2.2 [11] remotes_2.2.0 rstudioapi_0.11 callr_3.5.0 rex_1.2.0 xml2_1.3.2 [16] cyclocomp_1.1.0 desc_1.2.0 tools_4.0.0 stringr_1.4.0 xfun_0.18 [21] compiler_4.0.0 processx_3.4.4 xmlparsedata_1.0.4 knitr_1.30 ```

[russHyde]

How should the warning message be worded here?

Avoid expressions of the form `object[1:xxx, ]`
Avoid expressions of the form `[1:min(8, nrow(mtcars)), ]`
Avoid using the range operator when subsetting.
... alternatives ...

[MichaelChirico]

I think something like the 3rd one is closest to best. I might say

Mixing : with nrow is risky because it is prone to errors when nrow is 0.

[AshesITR]

I like your suggestion, but would leave out the "is risky" part.

Mixing : with nrow is prone to errors when nrow is 0.

[IndrajeetPatil]

Just updating the reprex with the current status of this linter:

library(lintr)

lint(
  text = "mtcars[1:min(8, nrow(mtcars)), ]",
  linters = seq_linter()
)
#> <text>:1:8: warning: [seq_linter] 1:min(...) is likely to be wrong in the empty edge case. Use seq_len(min(...)) instead.
#> mtcars[1:min(8, nrow(mtcars)), ]
#>        ^~~~~~~~~~~~~~~~~~~~~~

^{Created on 2022-07-29 by the reprex package (v2.0.1.9000)}