add xml_find_function_calls() helper to source expressions

[AshesITR]

fixes #2350
fixes #2338

• Implement helper
• Document
• Add NEWS bullet
• Use in our linters
• Compare performance
• Add section in custom linters vignette

Filing for early feedback and discussion

[codecov-commenter]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (d1491c2) 98.64% compared to head (3766ac6) 98.53%.

❗ Current head 3766ac6 differs from pull request most recent head 451f409. Consider uploading reports for the commit 451f409 to get more accurate results

Files	Patch %	Lines
R/make_linter_from_xpath.R	57.14%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
- Coverage   98.64%   98.53%   -0.12%     
==========================================
  Files         125      125              
  Lines        5629     5609      -20     
==========================================
- Hits         5553     5527      -26     
- Misses         76       82       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelChirico]

this should probably get a section in the custom linters vignette, I don't see that on your TODO right now

[AshesITR]

so far 118s on main vs 113s here for

devtools::load_all()
system.time(lintr::lint_package())

So 5 secs savings ATM with only some linters converted.

[AshesITR]

We have quite a few //SYMBOL_FUNCTION_CALL[text() = ...]/path/1 | //SYMBOL_FUNCTION_CALL[text() = ...]/path/2 XPaths.
ATM, I convert them using self:: and the union of the two function calls examined.

It might be worth splitting these into to separate paths with separate calls to xml_find_function_call(). Not sure about the performance gains of that.

[AshesITR]

@MichaelChirico PTAL for another round.
The technical part of this PR is done I think.

If you approve of the changes so far, I would flesh out the documentation.

[MichaelChirico]

@MichaelChirico PTAL for another round. The technical part of this PR is done I think.

If you approve of the changes so far, I would flesh out the documentation.

I was also hoping for the vignette change to serve as a high-level overview for guiding the review 😅

It would help to leave that in the main PR comment anyway for future reference on such a big change.

For now, I'll try reading the tests and see if that suffices.

[MichaelChirico]

so far 118s on main vs 113s here for

devtools::load_all()
system.time(lintr::lint_package())

So 5 secs savings ATM with only some linters converted.

Overall change looks pretty good, I think we're close enough to ready that we can re-run the benchmark + start on user-facing docs.

[AshesITR]

Another performance timing on lintr itself is 117s [main] vs 101s [here] => 14% Speedup 🥳

LMK if you want more comparisons before merging. Otherwise, I think it's good to go.

[MichaelChirico]

Might be worth benchmarking on base R as well, here's a snippet I tried:

system.time(withr::with_dir(
  "~/svn/r-devel/src/library",
  for (pkg in list.files()) if (file.exists(file.path(pkg, "DESCRIPTION")))
    lint_package(pkg, linters = linters)
))

But linters includes cyclocomp_linter() which probably isn't a good idea, wheels were spinning interminably. So maybe a benchmark on a "reasonable" set of linters instead of the full suite.

Approved now if you want to forego that.

[MichaelChirico]

OK, I'm seeing 27% improvement on r-devel using our suite (minus cyclocomp_linter()):

Setup

get_linters <- \() modify_defaults(
  eval(parse(text = read.dcf(".lintr", "linters"))),
  cyclocomp_linter = NULL
)
linters <- get_linters(); system.time(withr::with_dir(
  "~/svn/r-devel/src/library",
  for (pkg in list.files()) if (file.exists(file.path(pkg, "DESCRIPTION")))
    lint_package(pkg, linters = linters)
))

Timings

# main
    user   system  elapsed 
2239.148  327.508 2666.586 

# current branch
    user   system  elapsed 
1828.817   88.002 1949.443 

[AshesITR]

Thanks @MichaelChirico for taking this to the finish line 👍🏻

[MichaelChirico]

looks like this snuck in without test coverage