[clang-tools-extra] [clang-tidy] Avoid token merging in redundant-parentheses fix-its (PR #202365)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Zeyi Xu (zeyi2)

<details>
<summary>Changes</summary>

The readability-redundant-parentheses check emitted fix-its that simply removed both parentheses. Tools that apply those fix-its directly could join adjacent tokens and produce invalid code, e.g. `return(0)` becoming `return0`.

Replace the opening parenthesis with a space when removing it would merge identifier characters across the removed token.

AI Usage: Test assisted by Codex.
Closes https://github.com/llvm/llvm-project/issues/185108

---
Full diff: https://github.com/llvm/llvm-project/pull/202365.diff


3 Files Affected:

- (modified) clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp (+20-1) 
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+23) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7-3) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
index c177c07b95a75..639e183f434b2 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
@@ -13,6 +13,7 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Lex/Lexer.h"
 #include <cassert>
 
 using namespace clang::ast_matchers;
@@ -34,6 +35,23 @@ AST_MATCHER(ParenExpr, isInMacro) {
 
 } // namespace
 
+static FixItHint createSpacedRemoval(SourceLocation Loc,
+                                     const SourceManager &SM,
+                                     const LangOptions &LangOpts) {
+  if (Loc.isValid() && !Loc.isMacroID()) {
+    auto LocInfo = SM.getDecomposedLoc(Loc);
+    bool Invalid = false;
+    StringRef Buffer = SM.getBufferData(LocInfo.first, &Invalid);
+    if (!Invalid && LocInfo.second > 0 && LocInfo.second + 1 < Buffer.size() &&
+        Lexer::isAsciiIdentifierContinueChar(Buffer[LocInfo.second - 1],
+                                             LangOpts) &&
+        Lexer::isAsciiIdentifierContinueChar(Buffer[LocInfo.second + 1],
+                                             LangOpts))
+      return FixItHint::CreateReplacement(SourceRange(Loc, Loc), " ");
+  }
+  return FixItHint::CreateRemoval(Loc);
+}
+
 RedundantParenthesesCheck::RedundantParenthesesCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
@@ -66,7 +84,8 @@ void RedundantParenthesesCheck::registerMatchers(MatchFinder *Finder) {
 void RedundantParenthesesCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *PE = Result.Nodes.getNodeAs<ParenExpr>("dup");
   diag(PE->getBeginLoc(), "redundant parentheses around expression")
-      << FixItHint::CreateRemoval(PE->getLParen())
+      << createSpacedRemoval(PE->getLParen(), *Result.SourceManager,
+                             getLangOpts())
       << FixItHint::CreateRemoval(PE->getRParen());
 }
 
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 4258f7faf34fd..6d91ac1ef1e8e 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -359,6 +359,29 @@ TEST(DiagnosticsTest, ClangTidy) {
                "function 'bar' is within a recursive call chain"))));
 }
 
+TEST(DiagnosticsTest, ClangTidyRedundantParenthesesFix) {
+  Annotations Test(R"cpp(
+    int func() {
+      return$lparen[[(]]0$rparen[[)]];
+    }
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  TU.ClangTidyProvider = addTidyChecks("readability-redundant-parentheses");
+
+  clangd::Fix ExpectedFix;
+  ExpectedFix.Message = "redundant parentheses around expression";
+  ExpectedFix.Edits.push_back(TextEdit{Test.range("lparen"), " "});
+  ExpectedFix.Edits.push_back(TextEdit{Test.range("rparen"), ""});
+
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      ifTidyChecks(ElementsAre(AllOf(
+          Diag(Test.range("lparen"), "redundant parentheses around expression"),
+          diagSource(Diag::ClangTidy),
+          diagName("readability-redundant-parentheses"),
+          withFix(equalToFix(ExpectedFix))))));
+}
+
 TEST(DiagnosticsTest, ClangTidyEOF) {
   // clang-format off
   Annotations Test(R"cpp(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9703bb8f17208..13d771ec821bf 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -802,9 +802,13 @@ Changes in existing checks
   macros that may expand differently in other configurations.
 
 - Improved :doc:`readability-redundant-parentheses
-  <clang-tidy/checks/readability/redundant-parentheses>` check by fixing a
-  false positive for parentheses present around an overloaded operator in the
-  context of a binary operation.
+  <clang-tidy/checks/readability/redundant-parentheses>` check:
+
+  - Fixed a false positive for parentheses present around an overloaded operator
+    in the context of a binary operation.
+
+  - Fixed a bug where clients that apply fix-its without :program:`clang-tidy`'s
+    cleanup could produce invalid code by joining adjacent tokens.
 
 - Improved :doc:`readability-redundant-preprocessor
   <clang-tidy/checks/readability/redundant-preprocessor>` check by fixing a

``````````

</details>


https://github.com/llvm/llvm-project/pull/202365