Skip to content

Commit de947c7

Browse files
trevorldtrevorld
authored
feat: vector_logic_linter() (#111)
1 parent 9ead6e5 commit de947c7

8 files changed

Lines changed: 360 additions & 0 deletions

File tree

‎NAMESPACE‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export(todo_comment_linter)
6565
export(undesirable_function_linter)
6666
export(undesirable_operator_linter)
6767
export(unnecessary_nesting_linter)
68+
export(vector_logic_linter)
6869
export(which_grepl_linter)
6970
importFrom(data.table,":=")
7071
importFrom(data.table,.BY)

‎NEWS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
+ `expect_s3_class_linter()` (@trevorld, #110)
88
+ `expect_s4_class_linter()` (@trevorld, #109)
99
+ `nzchar_linter()` (@trevorld, #102)
10+
+ `vector_logic_linter()` (@trevorld, #111)
1011

1112
* New vignette "Tips and tricks" that lists some solutions for problems one may
1213
encounter when writing new rules (#94).

‎R/linters_factory.R‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -888,6 +888,24 @@ makeActiveBinding(
888888
)
889889

890890

891+
#' @inherit lintr::vector_logic_linter title
892+
#' @description
893+
#' See <https://lintr.r-lib.org/reference/vector_logic_linter>.
894+
895+
#' @usage vector_logic_linter
896+
#' @name vector_logic_linter
897+
#' @export
898+
#' @return The name of the linter
899+
NULL
900+
makeActiveBinding(
901+
'vector_logic_linter',
902+
function() {
903+
function() 'vector_logic'
904+
},
905+
env = environment()
906+
)
907+
908+
891909
#' @inherit lintr::which_grepl_linter title
892910
#' @description
893911
#' See <https://lintr.r-lib.org/reference/which_grepl_linter>.

‎R/list-linters.R‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ list_linters <- function(path = ".") {
6262
# TODO: I think it should be removed eventually, ast-grep tools is just not
6363
# the right tool for this. For now, I just leave it opt-in.
6464
# "unreachable_code",
65+
"vector_logic",
6566
"which_grepl"
6667
)
6768
keep_or_exclude_testthat_rules(path, out)

‎inst/config.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,5 @@ keep:
5151
- unnecessary_nesting
5252
# This can be activated, but it is slow.
5353
# - unreachable_code
54+
- vector_logic
5455
- which_grepl

‎inst/rules/builtin/vector_logic.yml‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
# TODO: subsetting expressions for filter/subset
2+
3+
id: vector_logic-1
4+
language: r
5+
severity: warning
6+
rule:
7+
all:
8+
- pattern: "$LEFT & $RIGHT"
9+
- any:
10+
- inside:
11+
any:
12+
- kind: if_statement
13+
- kind: while_statement
14+
field: condition
15+
stopBy:
16+
any:
17+
- kind: call
18+
- kind: subset
19+
- inside:
20+
any:
21+
- pattern: expect_true($X)
22+
- pattern: expect_false($X)
23+
- pattern: testthat::expect_true($X)
24+
- pattern: testthat::expect_false($X)
25+
- pattern: tinytest::expect_true($X)
26+
- pattern: tinytest::expect_false($X)
27+
field: arguments
28+
stopBy:
29+
any:
30+
- kind: call
31+
- kind: subset
32+
constraints:
33+
LEFT:
34+
not:
35+
any:
36+
- regex: "^as.raw\\("
37+
- regex: "^as.hexmode\\("
38+
- regex: "^as.octmode\\("
39+
- kind: integer
40+
- kind: float
41+
- regex: string
42+
RIGHT:
43+
not:
44+
any:
45+
- regex: "^as.raw\\("
46+
- regex: "^as.hexmode\\("
47+
- regex: "^as.octmode\\("
48+
- kind: integer
49+
- kind: float
50+
- kind: string
51+
message: 'Use `&&` in conditional expressions.'
52+
53+
---
54+
55+
id: vector_logic-2
56+
language: r
57+
severity: warning
58+
rule:
59+
all:
60+
- pattern: "$LEFT | $RIGHT"
61+
- any:
62+
- inside:
63+
any:
64+
- kind: if_statement
65+
- kind: while_statement
66+
field: condition
67+
stopBy:
68+
any:
69+
- kind: call
70+
- kind: subset
71+
- inside:
72+
any:
73+
- pattern: expect_true($X)
74+
- pattern: expect_false($X)
75+
- pattern: testthat::expect_true($X)
76+
- pattern: testthat::expect_false($X)
77+
- pattern: tinytest::expect_true($X)
78+
- pattern: tinytest::expect_false($X)
79+
field: arguments
80+
stopBy:
81+
any:
82+
- kind: call
83+
- kind: subset
84+
constraints:
85+
LEFT:
86+
not:
87+
any:
88+
- regex: "^as.raw\\("
89+
- regex: "^as.hexmode\\("
90+
- regex: "^as.octmode\\("
91+
- kind: integer
92+
- kind: float
93+
- kind: string
94+
RIGHT:
95+
not:
96+
any:
97+
- regex: "^as.raw\\("
98+
- regex: "^as.hexmode\\("
99+
- regex: "^as.octmode\\("
100+
- kind: integer
101+
- kind: float
102+
- kind: string
103+
message: 'Use `||` in conditional expressions.'

‎man/vector_logic_linter.Rd‎

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎tests/testthat/test-vector_logic.R‎

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
test_that("vector_logic_linter skips allowed usages", {
2+
linter <- vector_logic_linter()
3+
4+
expect_no_lint("if (TRUE) 5 else if (TRUE) 2", linter)
5+
expect_no_lint("if (TRUE || FALSE) 1; while (TRUE && FALSE) 2", linter)
6+
7+
# function calls and extractions may aggregate to scalars -- only catch
8+
# usages at the highest logical level
9+
expect_no_lint("if (agg_function(x & y)) 1", linter)
10+
expect_no_lint("if (DT[x | y, cond]) 1", linter)
11+
12+
# don't match potentially OK usages nested within calls
13+
expect_no_lint("if (TRUE && any(TRUE | FALSE)) 4", linter)
14+
# even if the usage is nested in those calls (b/181915948)
15+
expect_no_lint("if (TRUE && any(TRUE | FALSE | TRUE)) 4", linter)
16+
17+
# don't match potentially OK usages in the branch itself
18+
expect_no_lint(
19+
trim_some(
20+
"
21+
if (TRUE) {
22+
x | y
23+
}
24+
"
25+
),
26+
linter
27+
)
28+
29+
# valid nested usage within aggregator
30+
expect_no_lint("testthat::expect_false(any(TRUE | TRUE))", linter)
31+
})
32+
33+
test_that("vector_logic_linter blocks simple disallowed usages", {
34+
linter <- vector_logic_linter()
35+
36+
expect_lint(
37+
"if (TRUE & FALSE) 1",
38+
"Use `&&` in conditional expressions.",
39+
linter
40+
)
41+
expect_lint(
42+
"while (TRUE | TRUE) 2",
43+
"Use `||` in conditional expressions.",
44+
linter
45+
)
46+
})
47+
48+
test_that("vector_logic_linter detects nested conditions", {
49+
linter <- vector_logic_linter()
50+
51+
expect_lint(
52+
"if (TRUE & TRUE || FALSE) 4",
53+
"Use `&&` in conditional expressions.",
54+
linter
55+
)
56+
expect_lint(
57+
"if (TRUE && (TRUE | FALSE)) 4",
58+
"Use `||` in conditional expressions.",
59+
linter
60+
)
61+
})
62+
63+
test_that("vector_logic_linter catches usages in expect_true()/expect_false()", {
64+
linter <- vector_logic_linter()
65+
and_msg <- "Use `&&` in conditional expressions."
66+
or_msg <- "Use `||` in conditional expressions."
67+
68+
expect_lint("expect_true(TRUE & FALSE)", and_msg, linter)
69+
expect_lint("expect_false(TRUE | TRUE)", or_msg, linter)
70+
71+
# ditto with namespace qualification
72+
expect_lint("testthat::expect_true(TRUE & FALSE)", and_msg, linter)
73+
expect_lint("testthat::expect_false(TRUE | TRUE)", or_msg, linter)
74+
})
75+
76+
test_that("vector_logic_linter doesn't get mixed up from complex usage", {
77+
expect_no_lint(
78+
trim_some(
79+
"
80+
if (a) {
81+
expect_true(ok)
82+
x <- 2
83+
a | b
84+
}
85+
"
86+
),
87+
vector_logic_linter()
88+
)
89+
})
90+
91+
test_that("vector_logic_linter recognizes some false positives around bitwise &/|", {
92+
linter <- vector_logic_linter()
93+
94+
expect_no_lint("if (info & as.raw(12)) { }", linter)
95+
expect_no_lint("if (as.raw(12) & info) { }", linter)
96+
expect_no_lint("if (info | as.raw(12)) { }", linter)
97+
expect_no_lint("if (info & as.octmode('100')) { }", linter)
98+
expect_no_lint("if (info | as.octmode('011')) { }", linter)
99+
expect_no_lint("if (info & as.hexmode('100')) { }", linter)
100+
expect_no_lint("if (info | as.hexmode('011')) { }", linter)
101+
# implicit as.octmode() coercion
102+
expect_no_lint("if (info & '100') { }", linter)
103+
expect_no_lint("if (info | '011') { }", linter)
104+
expect_no_lint("if ('011' | info) { }", linter)
105+
106+
# further nesting
107+
expect_no_lint("if ((info & as.raw(12)) == as.raw(12)) { }", linter)
108+
expect_no_lint("if ((info | as.raw(12)) == as.raw(12)) { }", linter)
109+
expect_no_lint('if ((mode & "111") != as.octmode("111")) { }', linter)
110+
expect_no_lint('if ((mode | "111") != as.octmode("111")) { }', linter)
111+
expect_no_lint('if ((mode & "111") != as.hexmode("111")) { }', linter)
112+
expect_no_lint('if ((mode | "111") != as.hexmode("111")) { }', linter)
113+
})
114+
115+
# TODO: subsetting expressions for filter/subset
116+
#
117+
# test_that("incorrect subset/filter usage is caught", {
118+
# linter <- vector_logic_linter()
119+
# and_msg <- "Use `&` in subsetting expressions"
120+
# or_msg <- "Use `|` in subsetting expressions"
121+
#
122+
# expect_lint("filter(x, y && z)", and_msg, linter)
123+
# expect_lint("filter(x, y || z)", or_msg, linter)
124+
# expect_lint("subset(x, y && z)", and_msg, linter)
125+
# expect_lint("subset(x, y || z)", or_msg, linter)
126+
#
127+
# expect_lint("x %>% filter(y && z)", and_msg, linter)
128+
# expect_lint(
129+
# "filter(x, a & b, c | d, e && f)",
130+
# list(and_msg, column_number = 27L),
131+
# linter
132+
# )
133+
# })
134+
#
135+
# test_that("native pipe usage is caught in subset/filter logic", {
136+
# skip_if_not_r_version("4.1.0")
137+
#
138+
# expect_lint(
139+
# "x |> filter(y && z)",
140+
# "Use `&` in subsetting",
141+
# vector_logic_linter()
142+
# )
143+
# })
144+
#
145+
# test_that("subsetting logic handles nesting", {
146+
# linter <- vector_logic_linter()
147+
# and_msg <- "Use `&` in subsetting expressions"
148+
# or_msg <- "Use `|` in subsetting expressions"
149+
#
150+
# expect_lint("filter(x, a & b || c)", or_msg, linter)
151+
# expect_lint("filter(x, a && b | c)", and_msg, linter)
152+
#
153+
# # adversarial commenting
154+
# expect_lint(
155+
# trim_some(
156+
# "
157+
# filter(x, a #comment
158+
# && b | c)
159+
# "
160+
# ),
161+
# and_msg,
162+
# linter
163+
# )
164+
#
165+
# expect_lint(
166+
# trim_some(
167+
# "
168+
# filter(x, a && #comment
169+
# b | c)
170+
# "
171+
# ),
172+
# and_msg,
173+
# linter
174+
# )
175+
#
176+
# # but not valid usage
177+
# expect_no_lint("filter(x, y < mean(y, na.rm = AA && BB))", linter)
178+
# expect_no_lint("subset(x, y < mean(y, na.rm = AA && BB) & y > 0)", linter)
179+
# expect_no_lint("subset(x, y < x[y > 0, drop = AA && BB, y])", linter)
180+
# })
181+
#
182+
# test_that("filter() handling is conservative about stats::filter()", {
183+
# linter <- vector_logic_linter()
184+
# and_msg <- "Use `&` in subsetting expressions"
185+
#
186+
# # NB: this should be invalid, filter= is a vector argument
187+
# expect_no_lint("stats::filter(x, y && z)", linter)
188+
# # The only logical argument to stats::filter(), exclude by keyword
189+
# expect_no_lint("filter(x, circular = y && z)", linter)
190+
# # But presence of circular= doesn't invalidate lint
191+
# expect_lint("filter(x, circular = TRUE, y && z)", and_msg, linter)
192+
# expect_lint("filter(x, y && z, circular = TRUE)", and_msg, linter)
193+
# expect_no_lint(
194+
# trim_some(
195+
# "
196+
# filter(x, circular # comment
197+
# = y && z)
198+
# "
199+
# ),
200+
# linter
201+
# )
202+
# expect_no_lint(
203+
# trim_some(
204+
# "
205+
# filter(x, circular = # comment
206+
# y && z)
207+
# "
208+
# ),
209+
# linter
210+
# )
211+
# expect_no_lint(
212+
# trim_some(
213+
# "
214+
# filter(x, circular # comment
215+
# = # comment
216+
# y && z)
217+
# "
218+
# ),
219+
# linter
220+
# )
221+
# })

0 commit comments

Comments
 (0)