[clang-tidy] bugprone-assert-side-effect non-const operator methods

[schenker]

With this PR, bugprone-assert-side-effect reports non-const calls to << and >> operators and does no longer consider any const operator calls to have side effects.

E.g. the following snippet is now reported and was previously not:

std::stringstream ss;
assert(ss << 1);

The following snippet was previously a false-positive and is not reported anymore:

struct {
  int operator+=(int i) const { return i; }
} t;
assert(t += 1);

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: None (schenker)

Changes

With this PR, bugprone-assert-side-effect assumes that operator methods that are not marked as const have side effects. This matches the existing rule for non-operator methods.

E.g. the following snippet is now reported and was previously not:

std::stringstream ss;
assert(ss << 1);

---

Full diff: https://github.com/llvm/llvm-project/pull/71974.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp (+12-1)

diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
index 07a987359d4d8d7..599f5ac70e7a229 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
@@ -41,6 +41,10 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
   }
 
   if (const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
+    if (const auto *FuncDecl = OpCallExpr->getDirectCallee())
+      if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
+        return !MethodDecl->isConst();
+
     OverloadedOperatorKind OpKind = OpCallExpr->getOperator();
     return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
            OpKind == OO_MinusEqual || OpKind == OO_StarEqual ||
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
index 6c41e1e320adeac..49d3d456deb9d35 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -11,7 +11,7 @@ class MyClass {
 
   MyClass &operator=(const MyClass &rhs) { return *this; }
 
-  int operator-() { return 1; }
+  int operator-() const { return 1; }
 
   operator bool() const { return true; }
 
@@ -84,5 +84,16 @@ int main() {
   msvc_assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds
 
+  struct {
+    int operator<<(int i) const { return i; }
+  } constOp;
+  assert(constOp << 1);
+
+  struct {
+    int operator<<(int i) { return i; }
+  } nonConstOp;
+  assert(nonConstOp << 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+
   return 0;
 }

[PiotrZSL]

Update documentation of this check to mention this, and update release notes.
Code looks fine.

[PiotrZSL]

One more question, there is option for this check:
"CheckFunctionCalls
Whether to treat non-const member and non-member functions as they produce side effects. Disabled by default because it can increase the number of false positive warnings."

Won't this overlap this change ?

[PiotrZSL]

I'm not sure now if this wont cause too much false-positives, after all this is why CheckFunctionCalls option were added. Sometimes object can have 2 same operators, one const and one not, in such case depend on "this" type, non-const could be used, this would be false-positive.

[schenker]

I'm not sure now if this wont cause too much false-positives, after all this is why CheckFunctionCalls option were added. Sometimes object can have 2 same operators, one const and one not, in such case depend on "this" type, non-const could be used, this would be false-positive.

I adapted the tests to handle the const overload situation.

But you are right, I misunderstood CheckFunctionCalls and things are trickier than I thought. I still think it could make sense to treat method calls and method operator calls the same way.

I will rethink my implementation, thanks for reviewing!

[schenker]

@PiotrZSL I changed some things, please take another look.

All const operator calls are now considered to be side-effect free. This also fixes some false-positives, e.g.

struct t {
  int operator+=(int i) const { return i; }
} t;
assert(t += 1);

is no longer reported.

I added << and >> to the list of operators with side-effects.
Only const calls are reported, so

std::stringstream ss;
assert(ss << 1);

is reported, but

assert(5 << 1);

is not.

There is no overlap with CheckFunctionCalls, CXXOperatorCallExprs are not affected by the flag.

[PiotrZSL]

LGTM, rebase code before merging

[PiotrZSL]

Code is fine and can be merged, unless you want it merged as "[email protected]" you may need to disable email privacy options or try to squash all commits in one.

[schenker]

Thanks for pointing out, I squashed and changed the settings.