Add `repeat_linter` for infinite loops #2107

MEO265 · 2023-09-05T18:46:36Z

PR resolves #2106

codecov-commenter · 2023-09-05T18:51:21Z

Codecov Report

Merging #2107 (ab17c87) into main (fa914d0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ab17c87 differs from pull request most recent head 5d95612. Consider uploading reports for the commit 5d95612 to get more accurate results

@@           Coverage Diff           @@
##             main    #2107   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         119      120    +1     
  Lines        5228     5240   +12     
=======================================
+ Hits         5210     5222   +12     
  Misses         18       18

Files Changed	Coverage Δ
R/repeat_linter.R	`100.00% <100.00%> (ø)`

📢 Have feedback on the report? Share it here.

MichaelChirico · 2023-09-05T18:57:10Z

R/repeat_linter.R

@@ -0,0 +1,46 @@
+#' Repeat linter
+#'
+#' Check that `while` is not used for infinite loops.


wording suggestion here. this could read as "any infinite while loop is detected" which is broader than the actual linter. maybe "simple infinite loops", or just mention while(TRUE) here

MichaelChirico · 2023-09-05T18:59:06Z

R/repeat_linter.R

+#' @export
+repeat_linter <- function() {
+  xpath <- "
+  //WHILE[following-sibling::*[1][self::OP-LEFT-PAREN] and


first condition looks redundant to me. is it possible to write a while loop without '('? if you're worried about 'while' being used in other contexts, while is a reserved word so it will show up as a SYMBOL IINM (worth adding a test for this to be sure)

while is a reserved word so it will show up as a SYMBOL IINM (worth adding a test for this to be sure)

When I try to create a test for it, the parser always flags an error. I would skip the tests if you don't have a suggestion

This works: parse(text = "`while`(TRUE, NULL)"), but actually resolves as equivalent to while (TRUE) NULL.

We can actually get into really funky territory here:

`while` <- identity while(TRUE) # [1] TRUE while (FALSE) { } # Error in while (FALSE) { : unused argument ({ # })

I'm OK to skip tests.

MichaelChirico · 2023-09-05T19:00:30Z

R/repeat_linter.R

+  xpath <- "
+  //WHILE[following-sibling::*[1][self::OP-LEFT-PAREN] and
+  following-sibling::*[2][self::expr[NUM_CONST[text()='TRUE'] and count(*)=1]]
+  and following-sibling::*[3][self::OP-RIGHT-PAREN]]"


this is also redundant. if you're worried about more complicated expressions in the condition, your test for NUM_CONST is implicitly handling that -- TRUE would show up at further nesting of in that case.

MichaelChirico · 2023-09-05T19:01:42Z

R/repeat_linter.R

+repeat_linter <- function() {
+  xpath <- "
+  //WHILE[following-sibling::*[1][self::OP-LEFT-PAREN] and
+  following-sibling::*[2][self::expr[NUM_CONST[text()='TRUE'] and count(*)=1]]


I would simplify this as following-sibling::expr[1]/NUM_CONST[...]

the node structure of while() loops is pretty simple in terms of what can parse, so no need to be overly defensive IMO.

I implemented it as suggested but still had to test that an expression existed at all

MichaelChirico · 2023-09-05T19:03:03Z

R/repeat_linter.R

+    lints <- xml_nodes_to_lints(
+      xml_find_all(xml, xpath),
+      source_expression = source_expression,
+      lint_message = "'while (TRUE)' is not recommended for infinite loops. Use 'repeat' instead.",


We prefer the actionable bit first (there's an issue about doing so consistently in the tracker somewhere...). here that would be "Use repeat instead of while(TRUE) for infinite loops"

MichaelChirico · 2023-09-05T19:07:11Z

tests/testthat/test-repeat_linter.R

more suggested test cases:

# while as a function call `while`(TRUE) # TRUE is the expression while (j < 5) TRUE # TRUE in a more complicated condition while (TRUE && j < 5) { ... } # sensitivity to '{' while (TRUE) NULL while (j < 5) NULL

Also should test for lint locations, especially with multiline code and multiple lints in one expression.

{ while (TRUE) { } while (TRUE) { } }

MichaelChirico

The code looks good and works, thanks! My comments are for polish/completeness.

MEO265 · 2023-09-05T19:10:50Z

The code looks good and works, thanks! My comments are for polish/completeness.

Understood. I'm always happy about comments that help to improve.

MichaelChirico · 2023-09-05T21:32:32Z

R/repeat_linter.R

+#' @export
+repeat_linter <- function() {
+  xpath <- "
+  //*[(self::WHILE or self::expr[SYMBOL_FUNCTION_CALL[.='`while`']])


//* is really troublesome for XPath -- they are evaluated greedily and so this linter will be really expensive on files with a lot of expressions. See https://github.com/r-lib/lintr/wiki/Writing-robust,-performant-linters.

I really think we can ignore the `while` case unless there's a specific request for it -- that's a really unusual way to write a while loop. Otherwise, we would usually write this as //WHILE[...] | //SYMBOL_FUNCTION_CALL[text() = 'while']/parent::expr[...].

(TIL you can use . = '...' to check the text on a node btw! Nice find. I still like text() = '...' for expressiveness, . = '...' is a little bit harder to reason about)

I will ignore the `while` case.
I misunderstood you I thought you would have wished for this case by mentioning it as a possible test here

MichaelChirico · 2023-09-06T16:58:33Z

@MEO265 FYI, I edited the PR description. "Solves" is not the ideal keyword since it won't auto-close the issue -- see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue.

MEO265 · 2023-09-06T19:17:25Z

@MEO265 FYI, I edited the PR description. "Solves" is not the ideal keyword since it won't auto-close the issue -- see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue.

Thanks for the info.

MEO265 · 2023-09-07T14:28:30Z

Can someone tell me why pipeline "check-link-rot" fails and whether I can influence it at all?

MichaelChirico · 2023-09-07T14:31:08Z

Can someone tell me why pipeline "check-link-rot" fails and whether I can influence it at all?

ignore it, it's been happening to other builds as well, that link appears fragile.

MEO265 · 2023-09-07T16:33:24Z

@MichaelChirico I looked at your changes and will try to do the same next time

MEO265 added 2 commits September 5, 2023 20:36

Add repeat_linter

676a35e

Add repeat_linter to NEWS

6bc66af

MichaelChirico reviewed Sep 5, 2023

View reviewed changes

MEO265 and others added 9 commits September 5, 2023 21:56

Update description

296793e

Update lint_message

22072a2

Add more tests

d112eec

Simplify and enhance the XPath expression

3836d7f

Update doc

dfc807e

Merge branch 'main' into repeat_linter

5870bcc

Fix XPath expression

17e15d5

Add multiline test

1a7f510

extra newline

3ec302f

MichaelChirico reviewed Sep 5, 2023

View reviewed changes

MEO265 added 3 commits September 6, 2023 21:50

Ignore ` while (TRUE) `

1d6d1c4

Merge branch 'main' into repeat_linter

1a42030

Fix merge fails

4fac026

MichaelChirico added 2 commits September 7, 2023 09:21

redundant condition

f6915df

use intermediate variable for easier debugging

e8887e4

style

44a1f7d

MichaelChirico previously approved these changes Sep 7, 2023

View reviewed changes

update test metadata

5d95612

MichaelChirico dismissed their stale review via 5d95612 September 7, 2023 16:53

MichaelChirico approved these changes Sep 7, 2023

View reviewed changes

MichaelChirico closed this Sep 7, 2023

MichaelChirico reopened this Sep 7, 2023

MichaelChirico merged commit 99fdd85 into r-lib:main Sep 7, 2023
41 of 42 checks passed

MEO265 deleted the repeat_linter branch September 7, 2023 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `repeat_linter` for infinite loops #2107

Add `repeat_linter` for infinite loops #2107

MEO265 commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading

MichaelChirico Sep 5, 2023

MichaelChirico Sep 5, 2023

MEO265 Sep 5, 2023 •

edited

Loading

MichaelChirico Sep 5, 2023

MichaelChirico Sep 5, 2023

MichaelChirico Sep 5, 2023 •

edited

Loading

MEO265 Sep 5, 2023

MichaelChirico Sep 5, 2023

MichaelChirico Sep 5, 2023

AshesITR Sep 5, 2023

MichaelChirico left a comment

MEO265 commented Sep 5, 2023

MichaelChirico Sep 5, 2023

MEO265 Sep 6, 2023

MichaelChirico commented Sep 6, 2023

MEO265 commented Sep 6, 2023

MEO265 commented Sep 7, 2023

MichaelChirico commented Sep 7, 2023

MEO265 commented Sep 7, 2023

Add repeat_linter for infinite loops #2107

Add repeat_linter for infinite loops #2107

Conversation

MEO265 commented Sep 5, 2023 • edited Loading

codecov-commenter commented Sep 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MEO265 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico left a comment

Choose a reason for hiding this comment

MEO265 commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico commented Sep 6, 2023

MEO265 commented Sep 6, 2023

MEO265 commented Sep 7, 2023

MichaelChirico commented Sep 7, 2023

MEO265 commented Sep 7, 2023

Add `repeat_linter` for infinite loops #2107

Add `repeat_linter` for infinite loops #2107

MEO265 commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading

MEO265 Sep 5, 2023 •

edited

Loading

MichaelChirico Sep 5, 2023 •

edited

Loading