[Clang] add -Wshift-bool warning to handle shifting of bool

[a-tarasyuk]

Fixes #28334

---

This PR introduces the -Wshift-bool warning to detect and warn against shifting bool values using the << or >> operators. Shifting a bool implicitly converts it to an int, which can lead to unintended behavior.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #28334

---

This PR introduces the -Wshift-bool warning to detect and warn against shifting bool values using the << or >> operators. Shifting a bool implicitly converts it to an int, which can lead to unintended behavior.

---

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

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaExpr.cpp (+6)
• (added) clang/test/Sema/shift-bool.cpp (+21)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6056a6964fbcc..d623d7daf05ea 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -138,6 +138,7 @@ Improvements to Clang's diagnostics
 - Fixed a bug where Clang's Analysis did not correctly model the destructor behavior of ``union`` members (#GH119415).
 - A statement attribute applied to a ``case`` label no longer suppresses
   'bypassing variable initialization' diagnostics (#84072).
+- The ``-Wshift-bool`` warning has been added to warn about shifting a ``boolean``. (#GH28334)
 
 Improvements to Clang's time-trace
 ----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 05e39899e6f25..193aea3a8dfc3 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -461,6 +461,8 @@ def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
 def DanglingGsl : DiagGroup<"dangling-gsl">;
 def ReturnStackAddress : DiagGroup<"return-stack-address">;
+def ShiftBool: DiagGroup<"shift-bool">;
+
 // Name of this warning in GCC
 def : DiagGroup<"return-local-addr", [ReturnStackAddress]>;
 def Dangling : DiagGroup<"dangling", [DanglingAssignment,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c4f0fc55b4a38..754cf1e14799b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7115,6 +7115,9 @@ def warn_shift_result_sets_sign_bit : Warning<
   "signed shift result (%0) sets the sign bit of the shift expression's "
   "type (%1) and becomes negative">,
   InGroup<DiagGroup<"shift-sign-overflow">>, DefaultIgnore;
+def warn_shift_bool : Warning<
+  "%select{left|right}0 shifting a `bool` implicitly converts it to 'int'">,
+  InGroup<ShiftBool>, DefaultIgnore;
 
 def warn_precedence_bitwise_rel : Warning<
   "%0 has lower precedence than %1; %1 will be evaluated first">,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 3cd4010740d19..0816403b2df2a 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -11246,6 +11246,12 @@ static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,
   if (S.getLangOpts().OpenCL)
     return;
 
+  if (LHS.get()->IgnoreParenImpCasts()->getType()->isBooleanType()) {
+    S.Diag(Loc, diag::warn_shift_bool)
+        << (Opc == BO_Shr) /*left|right*/ << LHS.get()->getSourceRange();
+    return;
+  }
+
   // Check right/shifter operand
   Expr::EvalResult RHSResult;
   if (RHS.get()->isValueDependent() ||
diff --git a/clang/test/Sema/shift-bool.cpp b/clang/test/Sema/shift-bool.cpp
new file mode 100644
index 0000000000000..dfbc1a1e73307
--- /dev/null
+++ b/clang/test/Sema/shift-bool.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fsyntax-only -Wshift-bool -verify %s
+
+void t() {
+  int x = 10;
+  bool y = true;
+
+  bool a = y << x; // expected-warning {{left shifting a `bool` implicitly converts it to 'int'}}
+  bool b = y >> x; // expected-warning {{right shifting a `bool` implicitly converts it to 'int'}}
+
+  bool c = false << x; // expected-warning {{left shifting a `bool` implicitly converts it to 'int'}}
+  bool d = false >> x; // expected-warning {{right shifting a `bool` implicitly converts it to 'int'}}
+
+  bool e = y << 5; // expected-warning {{left shifting a `bool` implicitly converts it to 'int'}}
+  bool f = y >> 5; // expected-warning {{right shifting a `bool` implicitly converts it to 'int'}}
+
+  bool g = y << -1; // expected-warning {{left shifting a `bool` implicitly converts it to 'int'}}
+  bool h = y >> -1; // expected-warning {{right shifting a `bool` implicitly converts it to 'int'}}
+
+  bool i = y << 0; // expected-warning {{left shifting a `bool` implicitly converts it to 'int'}}
+  bool j = y >> 0; // expected-warning {{right shifting a `bool` implicitly converts it to 'int'}}
+}

[Fznamznon]

I wonder if we should also suggest to change the shift to b & !x for right shifts?

[a-tarasyuk]

@Fznamznon we can make a suggestion, however, I'm not confident about one for the right shift.

[Fznamznon]

Should we only warn on right shifts as the issue comments suggest?

[a-tarasyuk]

@Fznamznon do you mean omitting the handling of the right shift? I suppose both << and >> convert the type to int., so handling both seem appropriate. Or does it not?

[Fznamznon]

Yeah, they both convert to int. However a right shift of a bool will often result in zero. A left shift though could have been intended. Weird but intended.
I don't really have a strong opinion on this, but original comments from @zygoloid in #28334 (comment) suggests so. I wanted to consider all the options.

[a-tarasyuk]

Thanks. I agree that it’s important to discuss the right shift behavior before moving forward with these changes. If we decide to remove the right shift, I’ll update the PR.