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

[barancsuk]

Summary

Checks for member expressions that access static members through instances, and
replaces them with the corresponding expressions that use a more readable :: operator.

Example:

The following code:

struct C {
  static void foo();
  static int x;
};

C *c1=new C();
c1->foo();
c1->x;

is changed to:

C::foo();
C::x;

[Eugene.Zelenko]

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

7

Is :: operator really?

[Eugene.Zelenko]

It also seems that your code is not based on trunk, since Release Notes were cleared when version was changed to 6. Please update.

[aaron.ballman]

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

23

Why not use isStaticStorageClass() here as well?

clang-tidy/readability/StaticAccessedThroughInstanceCheck.h

20

:: isn't an operator -- I would say: "and replaces them with uses of the appropriate qualified-id."

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

7

Same wording suggestion here as above.

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

34

This fix-it worries me because it changes the semantics of the code. The function f() is no longer called, and so this isn't a valid source transformation.

137

Please add some tests where the replacement is somewhat more complex, like:

namespace N {
struct S {
  struct T {
    struct U {
      static int x;
     };
  };
};
}

// Complex replacement
void f(N::S::T::U u) {
  u.x = 12; // N::S::T::U::x = 12;
}

Note that this is a case where it's possible that the code becomes *less* readable rather than more readable. I am fine without having any configuration right now, but we may want to consider whether the nesting level should factor in to whether we think this is more or less readable.

[baloghadamsoftware]

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

34

Maybe the correct fix would be here f(1, 2, 3, 4); C::x;

[xazax.hun]

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

34

Maybe for now we should just skip this cases. Expr::HasSideEffects might be a good candidate to filter these cases.

[baloghadamsoftware]

I am not sure whether we should cope with such ugly things as overloaded member access operators with side effects, but they can also cause troubles using this fix:

#include <iostream>

using std::cout;
using std::endl;

struct C {
  static int N;
  int n = 0;
};

int C::N = 0;

struct Cptr {
  C* c;

  explicit Cptr(C *cc): c(cc) {}

  C* operator->() {
    ++c->n;
    return c;
  }
};

int main() {
  Cptr cp(new C);
  cout<<"n: "<<cp->n<<endl;
  cp->N = 10;
  cout<<"n: "<<cp->n<<endl;
}

Output:

n: 1
n: 3

After the fix, the output changes to:

n: 1
n: 2

[baloghadamsoftware]

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

34

I think including the expression with side effect before the member access (as I suggested) is not more complicated than skipping these cases.

[aaron.ballman]

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

34

Please ensure you handle the more complex cases then, such as:

struct S {
  static int X;
};

void g(int &);
S h();

void f(S s) {
  g(h().X);
  if ([s]() { return s; }().X == 15)
    ;
}

[barancsuk]

Address review comments

[barancsuk]

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

23

Unfortunately, isStaticStorageClass() misses variable declarations that do not contain the static keyword, including definitions of static variables outside of their class.
However, hasStaticStorageDuration() has no problem finding all static variable declarations correctly.

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

7

The suggested wording is indeed a clearer one, and I rewrote the descriptions and the documentation accordingly, however I found, that in the C++ International Standard :: is referred to as a scope (resolution) operator.

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

34

Expressions with side effects introduce some troublesome cases.
For example, in the following code, the static member expression, h().x does not get evaluated if a() returns false.

struct C {
  static int x;
};

void j(int);
int k(bool);
bool a();
C h();

void g(){
  j(k(a() && h().x));
}

Maybe for now, the check should filter these expressions with side effects.
They might better be addressed in a follow-up patch.

[aaron.ballman]

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

23

Under what circumstances would that make a difference? If the variable is declared within the class with the static storage specifier, that declaration should be sufficient for the check, regardless of how the definition looks, no?

If it does make a difference, can you add a test case that demonstrates that?

31

No else after a return.

80

No need for the extra parens here.

clang-tidy/readability/StaticAccessedThroughInstanceCheck.h

20

Reflow the comments.

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

34

Maybe for now, the check should filter these expressions with side effects.
They might better be addressed in a follow-up patch.

I think that's a good approach.

91

It'd be good to have an additional RUN line that sets the nesting level to something other than the default and ensure the behavior is consistent.

[barancsuk]

Further reviews addressed

[barancsuk]

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

23

isStaticStorageClass() does fail for the current test cases.

The reason for that is the function hasDecl(), that, for static variables with multiple declarations, may return a declaration that does not contain the static keyword.
For example, it finds int C::x = 0; rather than static int x;
Then isStaticStorageClass() discards it.

However, hasStaticStorageDuration() works fine, because all declarations are static storage duration, regardless of whether the static keyword is present or not.

[aaron.ballman]

LGTM!

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

23

Hmmm, that's unexpected (though not incorrect, from looking at the matcher implementations). Thank you for the explanation, I think it's fine for your patch.

[alexfh]

A few late comments.

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

28–29 ↗

(On Diff #110209)

This may be confusing as to whether the check removes the struct definition (and the definition of c1) or not.

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

11

typo: "Thefollowing"

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

55

Line wrapping gone bad. I guess you can just remove the second line. Same below.

[barancsuk]

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

55

I think the line wrapping and the typo have already been corrected in a later version.

[Eugene.Zelenko]

Please look at recently filed PR40544.

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
+#include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "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>(
         "readability-named-parameter");
     CheckFactories.registerCheck<NonConstParameterCheck>(
         "readability-non-const-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");

clang-tidy/readability/StaticAccessedThroughInstanceCheck.h

+//===--- 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 calls to the preferred, more readable `::` operator.
+///
+/// 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) {}
+  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_STATIC_ACCESSED_THROUGH_INSTANCE_H

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

+//===--- 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 {
+
+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();
+
+  QualType BaseType =
+      BaseExpr->getType()->isPointerType()
+          ? BaseExpr->getType().getUnqualifiedType()->getPointeeType()
+          : BaseExpr->getType().getUnqualifiedType();
+
+  PrintingPolicy PrintingPolicyWithSupressedTag(Result.Context->getLangOpts());
+  PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  std::string BaseTypeName =
+      BaseType.getAsString(PrintingPolicyWithSupressedTag);
+
+  diag(MemberExpression->getLocStart(),
+       "static member accessed through instance")
+      << FixItHint::CreateReplacement(
+          CharSourceRange::getCharRange(MemberExpression->getLocStart(),
+                                        MemberExpression->getMemberLoc()),
+          BaseTypeName + "::");
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- Added `ParameterThreshold` to `readability-function-size
 
   Finds functions that have more than `ParameterThreshold` parameters and emits a warning.
 
 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
+- 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 the corresponding expressions that use a more readable scope operator.
+
 - Support clang-formatting of the code around applied fixes (``-format-style``
   command-line option).
 
 - New `hicpp` module
 
   Adds checks that implement the `High Integrity C++ Coding Standard <http://www.codingstandard.com/section/index/>`_ and other safety
   standards. Many checks are aliased to other modules.
 

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    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

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

+.. 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 the corresponding expressions that use a more readable `::` operator.
+
+Example:
+
+Thefollowing 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;
+

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

+// 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 the scope operator.
+    foo();       // OK, foo() is accessed inside the struct.
+  }
+};
+
+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 the scope operator.
+    foo();       // OK, foo() is accessed inside the struct.
+  }
+};
+
+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: {{^}}  C::x;{{$}}
+}
+
+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;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+}
+
+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 accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+  C *c1 = new C();
+  c1->foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::foo();{{$}}
+  c1->x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+  c1->nsx; // OK, nsx is a non-static member.
+
+  C::foo(); // OK, foo() is accessed using the scope operator.
+  C::x;     // OK, x is accessed using the scope operator.
+
+  D d;
+  d.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  D::foo();{{$}}
+  d.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  D::x;{{$}}
+
+  E e;
+  e.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  E::foo();{{$}}
+  e.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // 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 accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  CT<int>::foo();{{$}}
+  ct.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // 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>();
+}