lintr icon indicating copy to clipboard operation
lintr copied to clipboard

implicit_integer_linter shouldn't lint 1:10 (optionally?)

Open MichaelChirico opened this issue 2 years ago • 9 comments

lintr::lint("1:10\n", lintr::implicit_integer_linter())
# <text>:1:2: style: Integers should not be implicit. Use the form 1L for integers or 1.0 for doubles.
# 1:10
# ~^
# <text>:1:5: style: Integers should not be implicit. Use the form 1L for integers or 1.0 for doubles.
# 1:10
#   ~~^

but typeof(1:10) is "integer", and the performance is essentially identical:

microbenchmark::microbenchmark(
  1L:10L,
  1L:10,
  1:10L,
  1:10,
  times = 1e6
)

# Unit: nanoseconds
#    expr min  lq     mean median  uq      max neval
#  1L:10L 199 242 479.8983    257 284 89303201 1e+06
#   1L:10 197 237 376.0418    251 278 10444161 1e+06
#   1:10L 198 239 354.6013    252 280 10678523 1e+06
#    1:10 190 233 405.9966    246 274 14666175 1e+06

MichaelChirico avatar May 20 '22 07:05 MichaelChirico

implicit_integer_linter isn't really about performance, more about explicit typing.

AshesITR avatar May 20 '22 15:05 AshesITR

right -- the more important point is that identical(1:10, 1L:10L). the performance bit is just demonstrating that there's no performance reason to prefer one or the other either.

MichaelChirico avatar May 20 '22 15:05 MichaelChirico

Is there a philosophical difference to identical(1.0, 1)?

AshesITR avatar May 20 '22 15:05 AshesITR

to me, it's different -- almost everywhere in R, 1 actually means 1(double) (according to the parser). but with 1:10 it's actually 1(integer) (in effect, i.e., at runtime).

for 1.0 v 1, as I understand it we only like 1.0 as a way of signalling "yes, this is a double, it doesn't need to be 1L" to the linter.

put another way -- the linter wants 1 to be 1L whenever possible. it aims to set the default numeric literal to integer, and only allow doubles with decimal points. but for 1:10, 1 is already (in effect) integral, so there's no need to intervene.

that's how I see it at least.

PS I used to be a stickler about writing 1L:10L but got sick of the extra characters and especially at the cost of typing difficulty -- typing 1L:10L requires some deft coordination to get the shift key pressed in exactly the right places

MichaelChirico avatar May 20 '22 16:05 MichaelChirico

I tend to rarely need the : with literals, usually seq_*() functions provide a nicer solution.

To me the improved clarity of intent is worth the extra effort spent typing.

: is super odd anyway, compare 1.0:3.5 with 1.5:3 for example.

AshesITR avatar May 20 '22 16:05 AshesITR

are you OK with adding an option to disable for : (it can be off by default)

MichaelChirico avatar May 20 '22 17:05 MichaelChirico

Yes, that'd be okay. What about other such cases, e.g. seq_len(42) and seq.int()?

AshesITR avatar May 20 '22 17:05 AshesITR

personally I use L equivalents for those cases. i don't have any justification for the difference :)

MichaelChirico avatar May 20 '22 17:05 MichaelChirico

Okay, so the argument could be named allow_colon = FALSE

AshesITR avatar May 20 '22 18:05 AshesITR

And what about, say, list1[[1]]?

maelle avatar Aug 30 '22 13:08 maelle

Revisiting this...

Here in the source, the arguments to a:b are explicitly converted with asReal():

https://github.com/r-devel/r-svn/blob/f941f28d2db0f3ecbe4ae355a6b39d4f5612e592/src/main/seq.c#L166-L167

So 1:10 is consistent with what the code is actually doing / doesn't require casting.

MichaelChirico avatar Sep 21 '22 15:09 MichaelChirico

And what about, say, list1[[1]]?

Maelle, I think that's one of the more common places where I think of this linter as "good", I definitely think it should lint. My thinking is leaving this linter off is the better option if you don't think it should lint...

Please open a separate issue if you'd like to discuss more, this one will close with #1691

MichaelChirico avatar Oct 13 '22 02:10 MichaelChirico