[clang-tidy] new check 'readability-isolate-declaration'

Summary:
This patch introduces a new clang-tidy check that matches on all `declStmt` that declare more then one variable
and transform them into one statement per declaration if possible.

It currently only focusses on variable declarations but should be extended to cover more kinds of declarations in the future.
It is related to https://reviews.llvm.org/D27621 and does use it's extensive test-suite. Thank you to firolino for his work!

Reviewers: rsmith, aaron.ballman, alexfh, hokein, kbobyrev

Reviewed By: aaron.ballman

Subscribers: ZaMaZaN4iK, mgehre, nemanjai, kbarton, lebedev.ri, Eugene.Zelenko, mgorny, xazax.hun, cfe-commits

Tags: #clang-tools-extra

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

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@345735 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 319 insertions, 0 deletions

diff --git a/clang-tidy/readability/CMakeLists.txt b/clang-tidy/readability/CMakeLists.txt
index bd010af9..dace226a 100644
--- a/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tidy/readability/CMakeLists.txt
@@ -11,6 +11,7 @@ add_clang_library(clangTidyReadabilityModule
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
diff --git a/clang-tidy/readability/IsolateDeclarationCheck.cpp b/clang-tidy/readability/IsolateDeclarationCheck.cpp
new file mode 100644
index 00000000..3155be39
--- /dev/null
+++ b/clang-tidy/readability/IsolateDeclarationCheck.cpp
@@ -0,0 +1,279 @@
+//===--- IsolateDeclarationCheck.cpp - clang-tidy -------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "IsolateDeclarationCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::tidy::utils::lexer;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); }
+AST_MATCHER(DeclStmt, onlyDeclaresVariables) {
+  return llvm::all_of(Node.decls(), [](Decl *D) { return isa<VarDecl>(D); });
+}
+} // namespace
+
+void IsolateDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declStmt(allOf(onlyDeclaresVariables(), unless(isSingleDecl()),
+                     hasParent(compoundStmt())))
+          .bind("decl_stmt"),
+      this);
+}
+
+static SourceLocation findStartOfIndirection(SourceLocation Start,
+                                             int Indirections,
+                                             const SourceManager &SM,
+                                             const LangOptions &LangOpts) {
+  assert(Indirections >= 0 && "Indirections must be non-negative");
+  if (Indirections == 0)
+    return Start;
+
+  // Note that the post-fix decrement is necessary to perform the correct
+  // number of transformations.
+  while (Indirections-- != 0) {
+    Start = findPreviousAnyTokenKind(Start, SM, LangOpts, tok::star, tok::amp);
+    if (Start.isInvalid() || Start.isMacroID())
+      return SourceLocation();
+  }
+  return Start;
+}
+
+static bool isMacroID(SourceRange R) {
+  return R.getBegin().isMacroID() || R.getEnd().isMacroID();
+}
+
+/// This function counts the number of written indirections for the given
+/// Type \p T. It does \b NOT resolve typedefs as it's a helper for lexing
+/// the source code.
+/// \see declRanges
+static int countIndirections(const Type *T, int Indirections = 0) {
+  if (T->isFunctionPointerType()) {
+    const auto *Pointee = T->getPointeeType()->castAs<FunctionType>();
+    return countIndirections(
+        Pointee->getReturnType().IgnoreParens().getTypePtr(), ++Indirections);
+  }
+
+  // Note: Do not increment the 'Indirections' because it is not yet clear
+  // if there is an indirection added in the source code of the array
+  // declaration.
+  if (const auto *AT = dyn_cast<ArrayType>(T))
+    return countIndirections(AT->getElementType().IgnoreParens().getTypePtr(),
+                             Indirections);
+
+  if (isa<PointerType>(T) || isa<ReferenceType>(T))
+    return countIndirections(T->getPointeeType().IgnoreParens().getTypePtr(),
+                             ++Indirections);
+
+  return Indirections;
+}
+
+static bool typeIsMemberPointer(const Type *T) {
+  if (isa<ArrayType>(T))
+    return typeIsMemberPointer(T->getArrayElementTypeNoTypeQual());
+
+  if ((isa<PointerType>(T) || isa<ReferenceType>(T)) &&
+      isa<PointerType>(T->getPointeeType()))
+    return typeIsMemberPointer(T->getPointeeType().getTypePtr());
+
+  return isa<MemberPointerType>(T);
+}
+
+/// This function tries to extract the SourceRanges that make up all
+/// declarations in this \c DeclStmt.
+///
+/// The resulting vector has the structure {UnderlyingType, Decl1, Decl2, ...}.
+/// Each \c SourceRange is of the form [Begin, End).
+/// If any of the create ranges is invalid or in a macro the result will be
+/// \c None.
+/// If the \c DeclStmt contains only one declaration, the result is \c None.
+/// If the \c DeclStmt contains declarations other than \c VarDecl the result
+/// is \c None.
+///
+/// \code
+///    int * ptr1 = nullptr, value = 42;
+/// // [  ][              ] [         ] - The ranges here are inclusive
+/// \endcode
+/// \todo Generalize this function to take other declarations than \c VarDecl.
+static Optional<std::vector<SourceRange>>
+declRanges(const DeclStmt *DS, const SourceManager &SM,
+           const LangOptions &LangOpts) {
+  std::size_t DeclCount = std::distance(DS->decl_begin(), DS->decl_end());
+  if (DeclCount < 2)
+    return None;
+
+  if (rangeContainsExpansionsOrDirectives(DS->getSourceRange(), SM, LangOpts))
+    return None;
+
+  // The initial type of the declaration and each declaration has it's own
+  // slice. This is necessary, because pointers and references bind only
+  // to the local variable and not to all variables in the declaration.
+  // Example: 'int *pointer, value = 42;'
+  std::vector<SourceRange> Slices;
+  Slices.reserve(DeclCount + 1);
+
+  // Calculate the first slice, for now only variables are handled but in the
+  // future this should be relaxed and support various kinds of declarations.
+  const auto *FirstDecl = dyn_cast<VarDecl>(*DS->decl_begin());
+
+  if (FirstDecl == nullptr)
+    return None;
+
+  // FIXME: Member pointers are not transformed correctly right now, that's
+  // why they are treated as problematic here.
+  if (typeIsMemberPointer(FirstDecl->getType().IgnoreParens().getTypePtr()))
+    return None;
+
+  // Consider the following case: 'int * pointer, value = 42;'
+  // Created slices (inclusive)    [  ][       ] [         ]
+  // Because 'getBeginLoc' points to the start of the variable *name*, the
+  // location of the pointer must be determined separatly.
+  SourceLocation Start = findStartOfIndirection(
+      FirstDecl->getLocation(),
+      countIndirections(FirstDecl->getType().IgnoreParens().getTypePtr()), SM,
+      LangOpts);
+
+  // Fix function-pointer declarations that have a '(' in front of the
+  // pointer.
+  // Example: 'void (*f2)(int), (*g2)(int, float) = gg;'
+  // Slices:   [   ][        ] [                     ]
+  if (FirstDecl->getType()->isFunctionPointerType())
+    Start = findPreviousTokenKind(Start, SM, LangOpts, tok::l_paren);
+
+  // It is popssible that a declarator is wrapped with parens.
+  // Example: 'float (((*f_ptr2)))[42], *f_ptr3, ((f_value2)) = 42.f;'
+  // The slice for the type-part must not contain these parens. Consequently
+  // 'Start' is moved to the most left paren if there are parens.
+  while (true) {
+    if (Start.isInvalid() || Start.isMacroID())
+      break;
+
+    Token T = getPreviousToken(Start, SM, LangOpts);
+    if (T.is(tok::l_paren)) {
+      Start = findPreviousTokenStart(Start, SM, LangOpts);
+      continue;
+    }
+    break;
+  }
+
+  SourceRange DeclRange(DS->getBeginLoc(), Start);
+  if (DeclRange.isInvalid() || isMacroID(DeclRange))
+    return None;
+
+  // The first slice, that is prepended to every isolated declaration, is
+  // created.
+  Slices.emplace_back(DeclRange);
+
+  // Create all following slices that each declare a variable.
+  SourceLocation DeclBegin = Start;
+  for (const auto &Decl : DS->decls()) {
+    const auto *CurrentDecl = cast<VarDecl>(Decl);
+
+    // FIXME: Member pointers are not transformed correctly right now, that's
+    // why they are treated as problematic here.
+    if (typeIsMemberPointer(CurrentDecl->getType().IgnoreParens().getTypePtr()))
+      return None;
+
+    SourceLocation DeclEnd =
+        CurrentDecl->hasInit()
+            ? findNextTerminator(CurrentDecl->getInit()->getEndLoc(), SM,
+                                 LangOpts)
+            : findNextTerminator(CurrentDecl->getEndLoc(), SM, LangOpts);
+
+    SourceRange VarNameRange(DeclBegin, DeclEnd);
+    if (VarNameRange.isInvalid() || isMacroID(VarNameRange))
+      return None;
+
+    Slices.emplace_back(VarNameRange);
+    DeclBegin = DeclEnd.getLocWithOffset(1);
+  }
+  return Slices;
+}
+
+static Optional<std::vector<StringRef>>
+collectSourceRanges(llvm::ArrayRef<SourceRange> Ranges, const SourceManager &SM,
+                    const LangOptions &LangOpts) {
+  std::vector<StringRef> Snippets;
+  Snippets.reserve(Ranges.size());
+
+  for (const auto &Range : Ranges) {
+    CharSourceRange CharRange = Lexer::getAsCharRange(
+        CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
+        LangOpts);
+
+    if (CharRange.isInvalid())
+      return None;
+
+    bool InvalidText = false;
+    StringRef Snippet =
+        Lexer::getSourceText(CharRange, SM, LangOpts, &InvalidText);
+
+    if (InvalidText)
+      return None;
+
+    Snippets.emplace_back(Snippet);
+  }
+
+  return Snippets;
+}
+
+/// Expects a vector {TypeSnippet, Firstdecl, SecondDecl, ...}.
+static std::vector<std::string>
+createIsolatedDecls(llvm::ArrayRef<StringRef> Snippets) {
+  // The first section is the type snippet, which does not make a decl itself.
+  assert(Snippets.size() > 2 && "Not enough snippets to create isolated decls");
+  std::vector<std::string> Decls(Snippets.size() - 1);
+
+  for (std::size_t I = 1; I < Snippets.size(); ++I)
+    Decls[I - 1] = Twine(Snippets[0])
+                       .concat(Snippets[0].endswith(" ") ? "" : " ")
+                       .concat(Snippets[I].ltrim())
+                       .concat(";")
+                       .str();
+
+  return Decls;
+}
+
+void IsolateDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt");
+
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(),
+           "multiple declarations in a single statement reduces readability");
+
+  Optional<std::vector<SourceRange>> PotentialRanges =
+      declRanges(WholeDecl, *Result.SourceManager, getLangOpts());
+  if (!PotentialRanges)
+    return;
+
+  Optional<std::vector<StringRef>> PotentialSnippets = collectSourceRanges(
+      *PotentialRanges, *Result.SourceManager, getLangOpts());
+
+  if (!PotentialSnippets)
+    return;
+
+  std::vector<std::string> NewDecls = createIsolatedDecls(*PotentialSnippets);
+  std::string Replacement = llvm::join(
+      NewDecls,
+      (Twine("\n") + Lexer::getIndentationForLine(WholeDecl->getBeginLoc(),
+                                                  *Result.SourceManager))
+          .str());
+
+  Diag << FixItHint::CreateReplacement(WholeDecl->getSourceRange(),
+                                       Replacement);
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tidy/readability/IsolateDeclarationCheck.h b/clang-tidy/readability/IsolateDeclarationCheck.h
new file mode 100644
index 00000000..b7f4793d
--- /dev/null
+++ b/clang-tidy/readability/IsolateDeclarationCheck.h
@@ -0,0 +1,36 @@
+//===--- IsolateDeclarationCheck.h - clang-tidy -----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// This check diagnoses all DeclStmt's declaring more than one variable and
+/// tries to refactor the code to one statement per declaration.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-declaration.html
+class IsolateDeclarationCheck : public ClangTidyCheck {
+public:
+  IsolateDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
diff --git a/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tidy/readability/ReadabilityTidyModule.cpp
index ca49f6f1..5855bb07 100644
--- a/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
@@ -67,6 +68,8 @@ public:
         "readability-implicit-bool-conversion");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<IsolateDeclarationCheck>(
+        "readability-isolate-declaration");
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(