New linter for `if(isTRUE(x))` and `if(isFALSE(x))`

[eitsupi]

Since if(isTRUE(x)) is often safer than the following expressions (In R 4.2, an error occurs if the length of x is greater than 1), it may be useful to have a linter that can detect these.

if(x) {...}
if(x == TRUE) {...}

[AshesITR]

if (x) is not equivalent to if (isTRUE(x)) though, e.g. if x is a positive integer. if (x == TRUE) is a more clear-cut case, same for if (x == FALSE).

[eitsupi]

Thank you for your reply and for explaining the details. My concern is that beginners, including myself, may be using these without being fully aware of the differences and it could be helpful if lintr could suggest a better way to write.

I am assuming something similar to what shellcheck detects for single quotes. (As noted in the documentation, double-quotes and single quotes have different meanings in shell scripts, and users who understand the behavior well enough will intentionally use the two.)

This suggestion is primarily meant to help newbies who assume single and double quotes are basically the same, like in Python and JavaScript. It's not at all meant to discourage experienced users from using single quotes in general. If you are well aware of the difference, please do not hesitate to permanently disable this suggestion with disable=SC2016 in your .shellcheckrc.

https://github.com/koalaman/shellcheck/wiki/SC2016

[IndrajeetPatil]

Agreed that these should lint:

library(lintr)

lint(text = "if (x == TRUE) 1L",
     linters = linters_with_tags(tags = NULL))

lint(text = "if (x == FALSE) 0L",
     linters = linters_with_tags(tags = NULL))

^{Created on 2022-08-26 with reprex v2.0.2}

[MichaelChirico]

FWIW, we have a linter for this already (slated for upstreaming eventually). Any ==TRUE/FALSE expression is linted.

One tougher case is data.table subsetting, where the overloading of the i argument means we have to take care:

DT[is_treatment == TRUE]
# WRONG, not equivalent
DT[is_treatment]
# correct -- data.table uses () to signal "not a join"
DT[(is_treatment)]

[IndrajeetPatil]

Nice!

Should this get the "google-linter" label then?

[MichaelChirico]

up to OP -- originally the requested linter is slightly different. but if this format is satisfactory they ok

[eitsupi]

originally the requested linter is slightly different. but if this format is satisfactory they ok

I think it would be great if it were implemented. I hope this issue can be used for it.

[IndrajeetPatil]

@MichaelChirico Not sure how much it would be to port this over from Google, but, for now, marking this for 3.0.2. Feel free to remove it if you think that's too optimistic.

[MichaelChirico]

That's fine, I don't think there's any rush for 3.0.2. Porting is mostly tedium to switch internal idioms to match lintr style, and more importantly reviewer bandwidth.