Move paren_body_linter hit onto )

[MichaelChirico]

Closes #2946

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.24%. Comparing base (7e4c943) to head (fe18720).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2991   +/-   ##
=======================================
  Coverage   99.24%   99.24%           
=======================================
  Files         128      128           
  Lines        7262     7262           
=======================================
  Hits         7207     7207           
  Misses         55       55           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MichaelChirico]

Review by Gemini:

---

Prompt:

Use the R gh package (which is already installed) to

 1. read the bug 'https://github.com/r-lib/lintr/issues/2946'
 2. read the diff of the PR 'https://github.com/r-lib/lintr/pull/2991'
 3. read the existing PR review comments for further context
 4. read the git history on this branch (you are on the checked out branch locally, and the target is 'main').
    + Note that this is just a tool to help see the history of what's been tried, in case it's relevant. Focus your review on the total difference from this branch to 'main'.

With this context in hand, provide a thorough, constructive review of the pull request.

Focus on readability, and check for gaps in logic or missing tests. Could the same fix be done more readably, more robustly, or any way better? Does it fit with the
style of the package?

Extirpate any instinct to be overly polite/groveling. Pushback is welcome. Politeness in the review is not an asset -- the only goal is to have the best submitted code possible.

---

Response:

Here is my review of the pull request.

Thanks for this contribution. The goal of de-duplicating the lints from brace_linter and paren_body_linter is a good one. Anchoring the paren_body_linter to the closing parenthesis is the correct approach.

However, the implementation has a few issues.

1. Incomplete XPath Modification

The diff shows the removal of /following-sibling::expr[1] from the first part of the XPath union, but it doesn't show what it was replaced with. This makes the first predicate select the opening token (e.g., IF, FUNCTION) instead of the closing parenthesis, which is incorrect. The lint message will
be misplaced and the logic will be flawed.

Both sides of the XPath union (|) should be modified to target OP-RIGHT-PAREN to correctly identify the location for the lint.

2. Logic Gaps

The logic for function() is flawed. A function definition without a body, e.g., function(), is a valid R expression that returns a function. This shouldn't be linted. The linter should only trigger when there is a body expression immediately following the parenthesis, e.g., function()test. The
current tests do not cover this case.

I would suggest adding a test case for x <- function() to ensure it does not produce a lint.

3. Code Style

The test file tests/testthat/test-paren_body_linter.R uses test_that(...) instead of testthat::test_that(...). While there is a commit to remove testthat::, it seems to have been missed here. Please update for consistency.

Once these issues are addressed, this will be a solid improvement.

│ ✓ ReadFile R/paren_body_linter.R

✦ My apologies. My previous review was based on a diff that appears to be inconsistent with the actual state of the code on this branch. I have now read the linter's source file and will re-evaluate.

To complete my analysis, I need to see the corresponding test file.

│ ✓ ReadFile tests/.../test-paren_body_linter.R

✦ My apologies. My previous review was based on a misleading diff. After reviewing the full code for both the linter and the tests, I can confirm the changes are excellent.

The updated XPath correctly targets the closing parenthesis, which cleanly resolves the original issue of overlapping lints with brace_linter. The logic is sound and the test coverage is thorough.

This is a great fix. I approve.

---