[clang-tidy] Add new readability non-idiomatic static access check

Patch by: Lilla Barancsuk

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

llvm-svn: 310371

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

@@ -25,6 +25,7 @@ add_clang_library(clangTidyReadabilityModule
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

@@ -32,6 +32,7 @@
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
+#include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
@@ -70,6 +71,8 @@ public:
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
+    CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>(
+        "readability-static-accessed-through-instance");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(

clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

@@ -0,0 +1,90 @@
+//===--- StaticAccessedThroughInstanceCheck.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 "StaticAccessedThroughInstanceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+static unsigned getNameSpecifierNestingLevel(const QualType &QType) {
+  if (const ElaboratedType *ElType = QType->getAs<ElaboratedType>()) {
+    const NestedNameSpecifier *NestedSpecifiers = ElType->getQualifier();
+    unsigned NameSpecifierNestingLevel = 1;
+    do {
+      NameSpecifierNestingLevel++;
+      NestedSpecifiers = NestedSpecifiers->getPrefix();
+    } while (NestedSpecifiers);
+
+    return NameSpecifierNestingLevel;
+  }
+  return 0;
+}
+
+void StaticAccessedThroughInstanceCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "NameSpecifierNestingThreshold",
+                NameSpecifierNestingThreshold);
+}
+
+void StaticAccessedThroughInstanceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+                                      varDecl(hasStaticStorageDuration()))),
+                 unless(isInTemplateInstantiation()))
+          .bind("memberExpression"),
+      this);
+}
+
+void StaticAccessedThroughInstanceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MemberExpression =
+      Result.Nodes.getNodeAs<MemberExpr>("memberExpression");
+
+  if (MemberExpression->getLocStart().isMacroID())
+    return;
+
+  const Expr *BaseExpr = MemberExpression->getBase();
+
+  // Do not warn for overlaoded -> operators.
+  if (isa<CXXOperatorCallExpr>(BaseExpr))
+    return;
+
+  QualType BaseType =
+      BaseExpr->getType()->isPointerType()
+          ? BaseExpr->getType()->getPointeeType().getUnqualifiedType()
+          : BaseExpr->getType().getUnqualifiedType();
+
+  const ASTContext *AstContext = Result.Context;
+  PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
+  PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  std::string BaseTypeName =
+      BaseType.getAsString(PrintingPolicyWithSupressedTag);
+
+  SourceLocation MemberExprStartLoc = MemberExpression->getLocStart();
+  auto Diag =
+      diag(MemberExprStartLoc, "static member accessed through instance");
+
+  if (BaseExpr->HasSideEffects(*AstContext) ||
+      getNameSpecifierNestingLevel(BaseType) > NameSpecifierNestingThreshold)
+    return;
+
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getCharRange(MemberExprStartLoc,
+                                    MemberExpression->getMemberLoc()),
+      BaseTypeName + "::");
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.h

@@ -0,0 +1,43 @@
+//===--- StaticAccessedThroughInstanceCheck.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_STATIC_ACCESSED_THROUGH_INSTANCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_ACCESSED_THROUGH_INSTANCE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \@brief Checks for member expressions that access static members through
+/// instances and replaces them with uses of the appropriate qualified-id.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-accessed-through-instance.html
+class StaticAccessedThroughInstanceCheck : public ClangTidyCheck {
+public:
+  StaticAccessedThroughInstanceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        NameSpecifierNestingThreshold(
+            Options.get("NameSpecifierNestingThreshold", 3)) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const unsigned NameSpecifierNestingThreshold;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_ACCESSED_THROUGH_INSTANCE_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -71,6 +71,12 @@ Improvements to clang-tidy
       ``AllowConditionalPointerCasts`` -> ``AllowPointerConditions``.
 
 
+- New `readability-static-accessed-through-instance
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-accessed-through-instance.html>`_ check
+
+  Finds member expressions that access static members through instances and
+  replaces them with uses of the appropriate qualified-id.
+
 Improvements to include-fixer
 -----------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -178,5 +178,6 @@ Clang-Tidy Checks
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
+   readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

clang-tools-extra/docs/clang-tidy/checks/readability-static-accessed-through-instance.rst

@@ -0,0 +1,30 @@
+.. title:: clang-tidy - readability-static-accessed-through-instance
+
+readability-static-accessed-through-instance
+============================================
+
+Checks for member expressions that access static members through instances, and
+replaces them with uses of the appropriate qualified-id.
+
+Example:
+
+The following code:
+
+.. code-block:: c++
+
+  struct C {
+    static void foo();
+    static int x;
+  };
+
+  C *c1 = new C();
+  c1->foo();
+  c1->x;
+
+is changed to:
+
+.. code-block:: c++
+
+  C::foo();
+  C::x;
+

clang-tools-extra/test/clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp

@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t -- -config="{CheckOptions: [{key: readability-static-accessed-through-instance.NameSpecifierNestingThreshold, value: 4}]}" --
+
+// Nested specifiers
+namespace M {
+namespace N {
+struct V {
+  static int v;
+  struct T {
+    static int t;
+    struct U {
+      static int u;
+    };
+  };
+};
+}
+}
+
+void f(M::N::V::T::U u) {
+  M::N::V v;
+  v.v = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  M::N::V::v = 12;{{$}}
+
+  M::N::V::T w;
+  w.t = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  M::N::V::T::t = 12;{{$}}
+
+  // u.u is not changed, because the nesting level is over 4
+  u.u = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  u.u = 12;{{$}}
+}

clang-tools-extra/test/clang-tidy/readability-static-accessed-through-instance.cpp

@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+  static void foo();
+  static int x;
+  int nsx;
+  void mf() {
+    (void)&x;    // OK, x is accessed inside the struct.
+    (void)&C::x; // OK, x is accessed using a qualified-id.
+    foo();       // OK, foo() is accessed inside the struct.
+  }
+  void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+  void foo();
+  int x;
+};
+
+template <typename T> struct CT {
+  static T foo();
+  static T x;
+  int nsx;
+  void mf() {
+    (void)&x;    // OK, x is accessed inside the struct.
+    (void)&C::x; // OK, x is accessed using a qualified-id.
+    foo();       // OK, foo() is accessed inside the struct.
+  }
+};
+
+// Expressions with side effects
+C &f(int, int, int, int);
+void g() {
+  f(1, 2, 3, 4).x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+  j(i(h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  j(i(h().x));{{$}}
+
+  // The execution of h() depends on the return value of a().
+  j(k(a() && h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+  // CHECK-FIXES: {{^}}  j(k(a() && h().x));{{$}}
+
+  if ([c]() {
+        c.ns();
+        return c;
+      }().x == 15)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+  static int v;
+  struct T {
+    static int t;
+    struct U {
+      static int u;
+    };
+  };
+};
+}
+
+void f(N::V::T::U u) {
+  N::V v;
+  v.v = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::v = 12;{{$}}
+
+  N::V::T w;
+  w.t = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::T::t = 12;{{$}}
+
+  // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+  u.u = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  u.u = 12;{{$}}
+
+  using B = N::V::T::U;
+  B b;
+  b.u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  B::u;{{$}}
+}
+
+// Templates
+template <typename T> T CT<T>::x;
+
+template <typename T> struct CCT {
+  T foo();
+  T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template <typename T> void f(T t, C c) {
+  t.x; // OK, t is a template parameter.
+  c.x; // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 1{{$}}
+}
+
+template <int N> struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template <int N> void h() {
+  S<N> sN;
+  sN.x; // OK, value of N affects whether x is static or not.
+
+  S<2> s2;
+  s2.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+  C *c1 = new C();
+  c1->foo(); // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 1{{$}}
+  c1->x; // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 2{{$}}
+  c1->nsx; // OK, nsx is a non-static member.
+
+  const C *c2 = new C();
+  c2->foo(); // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 2{{$}}
+
+  C::foo(); // OK, foo() is accessed using a qualified-id.
+  C::x;     // OK, x is accessed using a qualified-id.
+
+  D d;
+  d.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::foo();{{$}}
+  d.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::x;{{$}}
+
+  E e;
+  e.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  E::foo();{{$}}
+  e.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  E::x;{{$}}
+
+  CC *cc = new CC;
+
+  f(*c1, *c1);
+  f(*cc, *c1);
+
+  // Macros: OK, macros are not checked.
+  FOO((*c1));
+  X((*c1));
+  FOO((*cc));
+  X((*cc));
+
+  // Templates
+  CT<int> ct;
+  ct.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  CT<int>::foo();{{$}}
+  ct.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  CT<int>::x;{{$}}
+  ct.nsx; // OK, nsx is a non-static member
+
+  CCT<int> cct;
+  cct.foo(); // OK, CCT has no static members.
+  cct.x;     // OK, CCT has no static members.
+
+  h<4>();
+}
+
+// Overloaded member access operator
+struct Q {
+  static int K;
+  int y = 0;
+};
+
+int Q::K = 0;
+
+struct Qptr {
+  Q *q;
+
+  explicit Qptr(Q *qq) : q(qq) {}
+
+  Q *operator->() {
+    ++q->y;
+    return q;
+  }
+};
+
+int func(Qptr qp) {
+  qp->y = 10; // OK, the overloaded operator might have side-effects.
+  qp->K = 10; //
+}