-
Notifications
You must be signed in to change notification settings - Fork 187
Writing robust, performant linters
Michael Chirico edited this page Dec 15, 2023
·
8 revisions
The {lintr} codebase has a lot of accumulated knowledge about how to write robust and fast linters. This Wiki exists as a repository for tidbits on these topics.
It exists as a Wiki to make editing it open to all and with low overhead.
- When writing a test for logical constants like
TRUE
orFALSE
, if you want the condition to match the shorthandsT
andF
, note that the former is aNUM_CONST
while the latter is aSYMBOL
(c.f.getParseData(parse(text = "TRUE; T"))
). - Keep pipes (
%>%
,|>
) in mind when writing lints based on positional logic (e.g. if it's a lint for the 2nd argument to meet some condition, that will usually become the 1st argument inside a pipe chain). - The magrittr pipe
%>%
and the "native pipe"|>
show up differently on the parse tree:SPECIAL
andPIPE
, respectively. Note that all infix operators (e.g.%%
,%in%
,%*%
) show up asSPECIAL
, so you'll need to test thetext()
as well for magrittr pipes. - Often, it's better to anchor on
EQ_SUB
instead ofSYMBOL_SUB
when writing conditions around named arguments. The latter need not be present in all cases, e.g. infoo("a" = 1)
, which is valid R code, the parse tree will have aSTR_CONST
for"a"
, not aSYMBOL_SUB
. - Be wary of
*
searches likepreceding-sibling::*[1]
. Are you sure everything counts? One common mistake is to include<COMMENT>
nodes here, so the XPath lands on a comment instead of the intended expression. Exclude such comments likepreceding-sibling::*[not(self::COMMENT)][1]
. - Also be wary of
expr
searches likepreceding-sibling::expr[1]
. Are=
assignment expressions excluded intentionally? Note that depending on the R version, the expressiona = 1
will not show up like<expr><expr><SYMBOL>a</SYMBOL></expr><EQ_ASSIGN>=</EQ_ASSIGN><expr><NUM_CONST>1</NUM_CONST></expr></expr>
(if we swapEQ_ASSIGN
toLEFT_ASSIGN
, that's howa <- 1
would appear). The outermost<expr>
may be<equal_assign>
or<expr_or_assign_or_help>
instead.preceding-sibling::expr[1]
will thus skip such an assignment, which is often a mistake.
- Avoid
//*
XPaths like the plague! At least in the current {xml2}, it is almost always slower than alternatives. A good example is https://github.com/r-lib/lintr/pull/2025, which shows a 3x speed-up from avoiding//*
even though the replacement is a long, inefficient-seeming chain of//A[expr] | //B[expr]
-style repetitive expressions. - Similarly, avoid
//expr
XPaths. See https://github.com/r-lib/lintr/issues/1358 -- more than 1/3 of all nodes are<expr>
, so//expr
only eliminates a relatively small portion of the parse tree. The more specific a node you can anchor on, the better, but the difference among nodes besides<expr>
is not as important, so err on the side of readability/comprehensibility. - If you use
//SYMBOL_FUNCTION_CALL
as an entry point, use thexml_find_function_calls()
helper instead, because it returns cached results much faster, especially when testing for multiple options oftext() = 'foo'
.