Invalid parentheses can cause lintr to error or hang

[kamilzyla]

Steps to reproduce

Run lintr::lint() on the following snippets.

This one throws Error in rep.int(character, length) : invalid 'times' value:

function() {)

This one hangs forever:

{ if (x) x()

Expected behavior

The function should exit cleanly and report a human-readable error.

Notes

Tested on version 3.0.0 released to CRAN as well as the development version at commit 6c3f0fd.

[AshesITR]

Thanks for reporting these two bugs.

1. is a bug in function_left_parentheses_linter() which fails to find the range end because the XML parse data contains only a single FUNCTION node. I think this (broken) edge case is better served by issuing a warning() in xml_nodes_to_lints() if for whatever reason, a range start or end is NA. @MichaelChirico WDYT? The alternative for this specific XPath would be to add a predicate FUNCTION[... and following-sibling::OP-LEFT-PAREN] which is unnecessary 99.999% of the time because we usually lint R code without parse errors, where this property of a FUNCTION node is guaranteed.
2. is a bug in getParseData(), which returns an invalid parse data frame (the parent column references an expression of id 14, which doesn't exist. We should be able to work around this by testing for this inconsistency.

[MichaelChirico]

Is there any reason to attempt running linters if get_source_expressions() returns an error?

file <- withr::local_tempfile(lines = "function() {)")
get_source_expressions(file)$error
# /tmp/RtmpiT5lal/file1b7cdf709f1728:1:13: error: [NA] unexpected ')'
# function() {)
#             ^

BTW, behavior on { if (x) x() is improved in current HEAD:

lint("{ if (x) x()\n")
# Error in generate_top_level_map(parsed_content) : 
#   Logical error: unassigned set did not shrink. Check file syntax and please report as a lintr bug.

[MichaelChirico]

Bumping to 3.1.0... the change to just not attempt linting for syntax errors feels like a big enough behavior change, and the current HEAD behavior (somewhat cryptic error, but does mention checking syntax) is not so catastrophic that I'm uncomfortable releasing with it.

[ywwry66]

The first error is affecting on-the-fly syntax checking. I am using Emacs ESS mode with flycheck package. When I type something like this in an R script:

foo <- function()

I get the Error in rep.int(character, length) : invalid 'times' value.

In fact, flycheck returns
Suspicious state from syntax checker r-lintr: Flycheck checker r-lintr returned 1, but its output contained no errors: Error in rep.int(character, length) : invalid 'times' value Calls: <Anonymous> ... print.lint -> cat -> highlight_string -> fill_with -> paste0 Execution halted
which renders it barely usable.

Before version 3.0.0, the same code yields

/Users/ruiyangwu/test1.R:1:16: style: Place a space before left parenthesis, except in a function call.
foo <- function()
               ^
/Users/ruiyangwu/test1.R:1:17: error: unexpected end of input
foo <- function()
                ^

[MichaelChirico]

@ywwry66 can you confirm you're on the latest dev version?

[ywwry66]

Yes I just tested a few minutes ago using

devtools::install_github("r-lib/lintr")

As comparison, I installed version 2.0.1 using

devtools::install_github("r-lib/lintr", ref="01e57c2")

[MichaelChirico]

Oh @ywwry66 actually that's #763.

This works:

x = lint("foo <- function()\n")

Printing fails:

print(x)
# Error in rep.int(character, length) : invalid 'times' value

[ywwry66]

You are right. So I guess that also applies to OP's first bug:

x = lint("function() {)\n")

works just the same. And it is the printing method that fails.

Looking at the object x, the culprit seems to be the NA in ranges of the first linter:

List of 3
 $ <text>:List of 8
  ..$ filename     : chr "<text>"
  ..$ line_number  : int 1
  ..$ column_number: int 9
  ..$ type         : chr "style"
  ..$ message      : chr "Remove spaces before the left parenthesis in a function call."
  ..$ line         : chr "function() {)"
  ..$ ranges       :List of 1
  .. ..$ : int [1:2] 9 NA
  ..$ linter       : chr "function_left_parentheses_linter"
  ..- attr(*, "class")= chr [1:2] "lint" "list"
 $ <text>:List of 8
  ..$ filename     : chr "<text>"
  ..$ line_number  : int 1
  ..$ column_number: int 12
  ..$ type         : chr "style"
  ..$ message      : chr "Opening curly braces should never go on their own line and should always be followed by a new line."
  ..$ line         : chr "function() {)"
  ..$ ranges       :List of 1
  .. ..$ : int [1:2] 12 12
  ..$ linter       : chr "brace_linter"
  ..- attr(*, "class")= chr [1:2] "lint" "list"
 $ <text>:List of 8
  ..$ filename     : chr "<text>"
  ..$ line_number  : int 1
  ..$ column_number: int 13
  ..$ type         : chr "error"
  ..$ message      : chr "unexpected ')'"
  ..$ line         : chr "function() {)"
  ..$ ranges       : NULL
  ..$ linter       : chr "error"
  ..- attr(*, "class")= chr [1:2] "lint" "list"
 - attr(*, "class")= chr [1:2] "lints" "list"