Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
MichaelChirico opened this issue May 20, 2022 · 12 comments · Fixed by #1691
Closed

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

MichaelChirico opened this issue May 20, 2022 · 12 comments · Fixed by #1691
Labels
feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Collaborator

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
@AshesITR
Copy link
Collaborator

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

@MichaelChirico
Copy link
Collaborator Author

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.

@AshesITR
Copy link
Collaborator

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

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented May 20, 2022

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

@AshesITR
Copy link
Collaborator

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.

@MichaelChirico
Copy link
Collaborator Author

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

@AshesITR
Copy link
Collaborator

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

@MichaelChirico
Copy link
Collaborator Author

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

@AshesITR
Copy link
Collaborator

Okay, so the argument could be named allow_colon = FALSE

@AshesITR AshesITR added the feature a feature request or enhancement label May 22, 2022
@maelle
Copy link
Contributor

maelle commented Aug 30, 2022

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

@MichaelChirico
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants