[clang-tidy] Handle expr with side-effects in readability-static-accessed-through-instance (#90736)

Improved readability-static-accessed-through-instance check to 
support expressions with side-effects.

Originally calls to overloaded operator were
ignored by check, in fear of possible side-effects.

This change remove that restriction, and enables
fix-its for expressions with side-effect via
--fix-notes.

Closes #75163

Author: PiotrZSL

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

@@ -59,10 +59,6 @@ void StaticAccessedThroughInstanceCheck::check(
 
   const Expr *BaseExpr = MemberExpression->getBase();
 
-  // Do not warn for overloaded -> operators.
-  if (isa<CXXOperatorCallExpr>(BaseExpr))
-    return;
-
   const QualType BaseType =
       BaseExpr->getType()->isPointerType()
           ? BaseExpr->getType()->getPointeeType().getUnqualifiedType()
@@ -89,17 +85,30 @@ void StaticAccessedThroughInstanceCheck::check(
     return;
 
   SourceLocation MemberExprStartLoc = MemberExpression->getBeginLoc();
-  auto Diag =
-      diag(MemberExprStartLoc, "static member accessed through instance");
-
-  if (BaseExpr->HasSideEffects(*AstContext) ||
-      getNameSpecifierNestingLevel(BaseType) > NameSpecifierNestingThreshold)
-    return;
+  auto CreateFix = [&] {
+    return FixItHint::CreateReplacement(
+        CharSourceRange::getCharRange(MemberExprStartLoc,
+                                      MemberExpression->getMemberLoc()),
+        BaseTypeName + "::");
+  };
+
+  {
+    auto Diag =
+        diag(MemberExprStartLoc, "static member accessed through instance");
+
+    if (getNameSpecifierNestingLevel(BaseType) > NameSpecifierNestingThreshold)
+      return;
+
+    if (!BaseExpr->HasSideEffects(*AstContext,
+                                  /* IncludePossibleEffects =*/true)) {
+      Diag << CreateFix();
+      return;
+    }
+  }
 
-  Diag << FixItHint::CreateReplacement(
-      CharSourceRange::getCharRange(MemberExprStartLoc,
-                                    MemberExpression->getMemberLoc()),
-      BaseTypeName + "::");
+  diag(MemberExprStartLoc, "member base expression may carry some side effects",
+       DiagnosticIDs::Level::Note)
+      << BaseExpr->getSourceRange() << CreateFix();
 }
 
 } // namespace clang::tidy::readability

clang-tools-extra/docs/ReleaseNotes.rst

@@ -352,6 +352,11 @@ Changes in existing checks
   <clang-tidy/checks/readability/redundant-inline-specifier>` check to properly
   emit warnings for static data member with an in-class initializer.
 
+- Improved :doc:`readability-static-accessed-through-instance
+  <clang-tidy/checks/readability/static-accessed-through-instance>` check to
+  support calls to overloaded operators as base expression and provide fixes to
+  expressions with side-effects.
+
 - Improved :doc:`readability-static-definition-in-anonymous-namespace
   <clang-tidy/checks/readability/static-definition-in-anonymous-namespace>`
   check by resolving fix-it overlaps in template code by disregarding implicit

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

@@ -35,3 +35,6 @@ is changed to:
   C::E1;
   C::E2;
 
+The `--fix` commandline option provides default support for safe fixes, whereas
+`--fix-notes` enables fixes that may replace expressions with side effects,
+potentially altering the program's behavior.

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

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t -- -- -isystem %S/Inputs/static-accessed-through-instance
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t -- --fix-notes -- -isystem %S/Inputs/static-accessed-through-instance
 #include <__clang_cuda_builtin_vars.h>
 
 enum OutEnum {
@@ -47,7 +47,8 @@ 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;{{$}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: member base expression may carry some side effects
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
 }
 
 int i(int &);
@@ -59,20 +60,23 @@ 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));{{$}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: note: member base expression may carry some side effects
+  // CHECK-FIXES: {{^}}  j(i(C::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));{{$}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:14: note: member base expression may carry some side effects
+  // CHECK-FIXES: {{^}}  j(k(a() && C::x));{{$}}
 
   if ([c]() {
         c.ns();
         return c;
       }().x == 15)
     ;
   // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
-  // CHECK-FIXES: {{^}}  if ([c]() {{{$}}
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: note: member base expression may carry some side effects
+  // CHECK-FIXES: {{^}}  if (C::x == 15){{$}}
 }
 
 // Nested specifiers
@@ -261,8 +265,11 @@ struct Qptr {
 };
 
 int func(Qptr qp) {
-  qp->y = 10; // OK, the overloaded operator might have side-effects.
-  qp->K = 10; //
+  qp->y = 10;
+  qp->K = 10;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance]
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: member base expression may carry some side effects
+  // CHECK-FIXES: {{^}}  Q::K = 10;
 }
 
 namespace {
@@ -380,3 +387,20 @@ namespace PR51861 {
     // CHECK-FIXES: {{^}}    PR51861::Foo::getBar();{{$}}
   }
 }
+
+namespace PR75163 {
+  struct Static {
+    static void call();
+  };
+
+  struct Ptr {
+    Static* operator->();
+  };
+
+  void test(Ptr& ptr) {
+    ptr->call();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: static member accessed through instance [readability-static-accessed-through-instance]
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: note: member base expression may carry some side effects
+    // CHECK-FIXES: {{^}}    PR75163::Static::call();{{$}}
+  }
+}