[clang-tidy] fix concat-nest-namespace fix hint remove the macro

Partially fixed [#60035](#60035)
This patch refactor the FixHint for concat-nest-namespace.
1. remove each namespace except the last non-nest namespace.
2. replace the last non-nest namespace with the new name.

It can remain the comment / pragma / macro between namespace and update the close comment.

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D147194

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp

@@ -7,10 +7,14 @@
 //===----------------------------------------------------------------------===//
 
 #include "ConcatNestedNamespacesCheck.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/Lexer.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/STLExtras.h"
 #include <algorithm>
+#include <optional>
 
 namespace clang::tidy::modernize {
 
@@ -20,6 +24,13 @@ static bool locationsInSameFile(const SourceManager &Sources,
          Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
 }
 
+static StringRef getRawStringRef(const SourceRange &Range,
+                                 const SourceManager &Sources,
+                                 const LangOptions &LangOpts) {
+  CharSourceRange TextRange = Lexer::getAsCharRange(Range, Sources, LangOpts);
+  return Lexer::getSourceText(TextRange, Sources, LangOpts);
+}
+
 static bool anonymousOrInlineNamespace(const NamespaceDecl &ND) {
   return ND.isAnonymousNamespace() || ND.isInlineNamespace();
 }
@@ -38,11 +49,49 @@ static bool alreadyConcatenated(std::size_t NumCandidates,
                                 const SourceManager &Sources,
                                 const LangOptions &LangOpts) {
   // FIXME: This logic breaks when there is a comment with ':'s in the middle.
-  CharSourceRange TextRange =
-      Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
-  StringRef CurrentNamespacesText =
-      Lexer::getSourceText(TextRange, Sources, LangOpts);
-  return CurrentNamespacesText.count(':') == (NumCandidates - 1) * 2;
+  return getRawStringRef(ReplacementRange, Sources, LangOpts).count(':') ==
+         (NumCandidates - 1) * 2;
+}
+
+static std::optional<SourceRange>
+getCleanedNamespaceFrontRange(const NamespaceDecl *ND, const SourceManager &SM,
+                              const LangOptions &LangOpts) {
+  // Front from namespace tp '{'
+  std::optional<Token> Tok =
+      ::clang::tidy::utils::lexer::findNextTokenSkippingComments(
+          ND->getLocation(), SM, LangOpts);
+  if (!Tok)
+    return std::nullopt;
+  while (Tok->getKind() != tok::TokenKind::l_brace) {
+    Tok = utils::lexer::findNextTokenSkippingComments(Tok->getEndLoc(), SM,
+                                                      LangOpts);
+    if (!Tok)
+      return std::nullopt;
+  }
+  return SourceRange{ND->getBeginLoc(), Tok->getEndLoc()};
+}
+
+static SourceRange getCleanedNamespaceBackRange(const NamespaceDecl *ND,
+                                                const SourceManager &SM,
+                                                const LangOptions &LangOpts) {
+  // Back from '}' to conditional '// namespace xxx'
+  const SourceRange DefaultSourceRange =
+      SourceRange{ND->getRBraceLoc(), ND->getRBraceLoc()};
+  SourceLocation Loc = ND->getRBraceLoc();
+  std::optional<Token> Tok =
+      utils::lexer::findNextTokenIncludingComments(Loc, SM, LangOpts);
+  if (!Tok)
+    return DefaultSourceRange;
+  if (Tok->getKind() != tok::TokenKind::comment)
+    return DefaultSourceRange;
+  SourceRange TokRange = SourceRange{Tok->getLocation(), Tok->getEndLoc()};
+  StringRef TokText = getRawStringRef(TokRange, SM, LangOpts);
+  std::string CloseComment = "namespace " + ND->getNameAsString();
+  // current fix hint in readability/NamespaceCommentCheck.cpp use single line
+  // comment
+  if (TokText != "// " + CloseComment && TokText != "//" + CloseComment)
+    return DefaultSourceRange;
+  return SourceRange{ND->getRBraceLoc(), Tok->getEndLoc()};
 }
 
 ConcatNestedNamespacesCheck::NamespaceString
@@ -65,11 +114,46 @@ void ConcatNestedNamespacesCheck::registerMatchers(
 }
 
 void ConcatNestedNamespacesCheck::reportDiagnostic(
-    const SourceRange &FrontReplacement, const SourceRange &BackReplacement) {
-  diag(Namespaces.front()->getBeginLoc(),
-       "nested namespaces can be concatenated", DiagnosticIDs::Warning)
-      << FixItHint::CreateReplacement(FrontReplacement, concatNamespaces())
-      << FixItHint::CreateReplacement(BackReplacement, "}");
+    const SourceManager &SM, const LangOptions &LangOpts) {
+  DiagnosticBuilder DB =
+      diag(Namespaces.front()->getBeginLoc(),
+           "nested namespaces can be concatenated", DiagnosticIDs::Warning);
+
+  SmallVector<SourceRange, 6> Fronts;
+  Fronts.reserve(Namespaces.size() - 1U);
+  SmallVector<SourceRange, 6> Backs;
+  Backs.reserve(Namespaces.size());
+
+  NamespaceDecl const *LastND = nullptr;
+
+  for (const NamespaceDecl *ND : Namespaces) {
+    if (ND->isNested())
+      continue;
+    LastND = ND;
+    std::optional<SourceRange> SR =
+        getCleanedNamespaceFrontRange(ND, SM, LangOpts);
+    if (!SR.has_value())
+      return;
+    Fronts.push_back(SR.value());
+    Backs.push_back(getCleanedNamespaceBackRange(ND, SM, LangOpts));
+  }
+  if (LastND == nullptr || Fronts.empty() || Backs.empty())
+    return;
+  // the last one should be handled specially
+  Fronts.pop_back();
+  SourceRange LastRBrace = Backs.pop_back_val();
+  NamespaceString ConcatNameSpace = concatNamespaces();
+
+  for (SourceRange const &Front : Fronts)
+    DB << FixItHint::CreateRemoval(Front);
+  DB << FixItHint::CreateReplacement(
+      SourceRange{LastND->getBeginLoc(), LastND->getLocation()},
+      ConcatNameSpace);
+  if (LastRBrace != SourceRange{LastND->getRBraceLoc(), LastND->getRBraceLoc()})
+    DB << FixItHint::CreateReplacement(LastRBrace,
+                                       ("} // " + ConcatNameSpace).str());
+  for (SourceRange const &Back : llvm::reverse(Backs))
+    DB << FixItHint::CreateRemoval(Back);
 }
 
 void ConcatNestedNamespacesCheck::check(
@@ -90,12 +174,10 @@ void ConcatNestedNamespacesCheck::check(
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
                                Namespaces.back()->getLocation());
-  SourceRange BackReplacement(Namespaces.back()->getRBraceLoc(),
-                              Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
                            getLangOpts()))
-    reportDiagnostic(FrontReplacement, BackReplacement);
+    reportDiagnostic(Sources, getLangOpts());
 
   Namespaces.clear();
 }

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h

@@ -29,8 +29,8 @@ class ConcatNestedNamespacesCheck : public ClangTidyCheck {
   using NamespaceContextVec = llvm::SmallVector<const NamespaceDecl *, 6>;
   using NamespaceString = llvm::SmallString<40>;
 
-  void reportDiagnostic(const SourceRange &FrontReplacement,
-                        const SourceRange &BackReplacement);
+  void reportDiagnostic(const SourceManager &Sources,
+                        const LangOptions &LangOpts);
   NamespaceString concatNamespaces();
   NamespaceContextVec Namespaces;
 };

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

@@ -75,6 +75,29 @@ SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
   return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
 }
 
+std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+                               const LangOptions &LangOpts) {
+  // `Lexer::findNextToken` will ignore comment
+  if (Start.isMacroID())
+    return std::nullopt;
+  Start = Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts);
+  // Break down the source location.
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Start);
+  bool InvalidTemp = false;
+  StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+  if (InvalidTemp)
+    return std::nullopt;
+  // Lex from the start of the given location.
+  Lexer L(SM.getLocForStartOfFile(LocInfo.first), LangOpts, File.begin(),
+          File.data() + LocInfo.second, File.end());
+  L.SetCommentRetentionState(true);
+  // Find the token.
+  Token Tok;
+  L.LexFromRawLexer(Tok);
+  return Tok;
+}
+
 std::optional<Token>
 findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM,
                               const LangOptions &LangOpts) {

clang-tools-extra/clang-tidy/utils/LexerUtils.h

@@ -85,6 +85,10 @@ SourceLocation findNextAnyTokenKind(SourceLocation Start,
   }
 }
 
+std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+                               const LangOptions &LangOpts);
+
 // Finds next token that's not a comment.
 std::optional<Token> findNextTokenSkippingComments(SourceLocation Start,
                                                    const SourceManager &SM,

clang-tools-extra/docs/ReleaseNotes.rst

@@ -297,6 +297,10 @@ Changes in existing checks
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
+- Fixed an issue in :doc:`modernize-concat-nested-namespaces
+  <clang-tidy/checks/modernize/concat-nested-namespaces>` when using macro between 
+  namespace declarations could result incorrect fix.
+
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.

clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h

@@ -5,4 +5,4 @@ void t();
 } // namespace nn2
 } // namespace nn1
 // CHECK-FIXES: void t();
-// CHECK-FIXES-NEXT: } // namespace nn1
+// CHECK-FIXES-NEXT: } // namespace nn1::nn2

clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp

@@ -143,7 +143,7 @@ void t() {}
 #endif
 } // namespace n40
 } // namespace n39
-// CHECK-FIXES: }
+// CHECK-FIXES: } // namespace n39::n40
 
 namespace n41 {
 namespace n42 {
@@ -154,7 +154,59 @@ void t() {}
 #endif
 } // namespace n42
 } // namespace n41
-// CHECK-FIXES: }
+// CHECK-FIXES: } // namespace n41::n42
+
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace n43 {
+#define N43_INNER
+namespace n44 {
+void foo() {}
+} // namespace n44
+#undef N43_INNER
+} // namespace n43
+// CHECK-FIXES: #define N43_INNER
+// CHECK-FIXES: namespace n43::n44 {
+// CHECK-FIXES: } // namespace n43::n44
+// CHECK-FIXES: #undef N43_INNER
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace n45{
+#define N45_INNER
+namespace n46
+{
+#pragma clang diagnostic push
+namespace n47 {
+void foo() {}
+} // namespace n47
+#pragma clang diagnostic pop
+} //namespace n46
+#undef N45_INNER
+} //namespace n45
+// CHECK-FIXES: #define N45_INNER
+// CHECK-FIXES: #pragma clang diagnostic push
+// CHECK-FIXES: namespace n45::n46::n47 {
+// CHECK-FIXES: } // namespace n45::n46::n47
+// CHECK-FIXES: #pragma clang diagnostic pop
+// CHECK-FIXES: #undef N45_INNER
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace avoid_add_close_comment {
+namespace inner {
+void foo() {}
+}
+}
+// CHECK-FIXES: namespace avoid_add_close_comment::inner {
+// CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace avoid_change_close_comment {
+namespace inner {
+void foo() {}
+} // namespace inner and other comments
+} // namespace avoid_change_close_comment and other comments
+// CHECK-FIXES: namespace avoid_change_close_comment::inner {
+// CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner
 
 int main() {
   n26::n27::n28::n29::n30::t();