Ignore symbols used for subsetting in object_name_linter

[AshesITR]

• implement fix
• add regression test
• add NEWS bullet

fixes #582

[AshesITR]

The XPath is similar to T_and_F_symbol linter, but it uses the ancestor axis instead of parent as that caused false positives.
Will check if T_and_F_symbol needs a fix as well.

[MichaelChirico]

I have a feeling ancestor may be too aggressive without more conditions. Running the comparison script now to try and confirm.

[AshesITR]

Sounds good. I tested "//parent::expr" before, which produced false positives (detecting symbols on the other side of assigments).
"ancestor" was the only axis that passed all the tests.

[MichaelChirico]

Attaching some comparison results:

On this branch, not HEAD
On HEAD, not this branch

[AshesITR]

The lints absent in this PR look good. All of them are either a generic defined in the package or a dollar-subset expression.

I plan to take a look at the new lints later.

Found a new false positive:

values[[ as.character(colIterator) ]] <<- content

shouldn't lint because values is the assignment target.

[AshesITR]

Updated XPath to exclude parts of [ and [[ indexing expressions and added appropriate tests.

[AshesITR]

This case necessitates the ancestor axis for the not() expression.

[AshesITR]

Currently rerunning on all the packages with new lints from your sample, @MichaelChirico.

[AshesITR]

I checked against the follwing packages and found no erroneous added nor removed lints.
Please take another look, I think we can merge.

List of tested Packages

assertthat bravo broom callr canvasXpress catdap cli clipr ClusterBootstrap CorrectedFDR cpp11 crayon crochet DBI DEEVD desc devtools dfoptim Dodge epsiwal etma evaluate fcros forcats generics ggplot2 gramEvol gtable HiddenMarkov highr HLSM hms htmlwidgets httr idr ineq knitr labeling leontief LogrankA malani mgcv modelr msBP munsell noncompliance paramtest peakRAM pillar pkgbuild pkgload plyr PPQplan processx progress ps R6 rappdirs RColorBrewer Rcpp RcppArmadillo RcppEigen readr readxl rematch2 remotes reprex Rgb rmarkdown rprojroot rstudioapi rvest scales selectr seqmon SetRank shiny shinyAce space sra stringr tidyselect tidyverse uFTIR VecStatGraphs2D viridisLite waldo withr yarr

[MichaelChirico]

as it's written, this is a totally different expression -- is there some relationship between the LEFT_ASSIGN (etc) ancestor and the OP-LEFT-BRACKET ancestor? I'm wondering if it's safer to express both conditions at once (it might not be).

Anyway I am running on a sample of 500 packages now, let's see what comes out.

[AshesITR]

The problem with combining it the conditions need to be nested, i.e. there may be no such ancestor before reaching the (first) assignment ancestor.

I couldn't get it to work in a single condition but if you habe an idea for a different one, I'm happy to try it.

The tests are pretty comprehensive now. The only missing bits are nested expressions with on-the-fly assignments, but I checked a few of these interactively, e.g. fun(BAD <- 42).

[MichaelChirico]

On the large sample of packages, still turning up a lot of new-not-old and old-not-new lints. Probably too big to attach here... took a random sample of 500 from each:

main_not_pr.txt
pr_not_main.txt

[AshesITR]

Manually inspected main_not_pr.txt.

I found one class of false negatives that need fixing:

'badName' <- 42
"badName" <- 42

Manually inspected pr_not_main.txt

I found no false positives.
The new lints are of the form setter(badName) <- 42 or subset assignments like badName[good_name] <- 42.

I'll go ahead and try to fix the false negatives and add tests for them.

[MichaelChirico]

Good stuff. Glad the comparison script has been so helpful to draw out edge cases. LGTM now.