[clang] add warning on shifting boolean type

[angelomatnigoogle]

Summary

Warn on operations "b << x" and "b >> x"

Github issue #28334

[shafik]

Hey @aaron.ballman I made some suggestions in the bug report: https://github.com/llvm/llvm-project/issues/28334 do you think this is a reasonable approach?

[aaron.ballman]

Thank you for working on this! It seems that precommit CI found valid failures that should be addressed:

Clang :: AST/Interp/shifts.cpp
Clang :: Analysis/svalbuilder-simplify-no-crash.c
Clang :: CXX/over/over.built/p18.cpp
Clang :: Misc/warning-flags.c

Please be sure to also add a release note to clang/docs/ReleaseNotes.rst for the new warning flag. Also, can you try running this new diagnostic against some larger C and C++ code bases to see if it discovers any false or true positives?

clang/include/clang/Basic/DiagnosticSemaKinds.td

6606–6610

Reworded and combined into one diagnostic. I also changed the diagnostic group because "dependent" has specific meaning to C++ folks.

clang/lib/Sema/SemaExpr.cpp

11948–11956

With the change to the diagnostic wording, I think this function can go away entirely.

11953

One thing I wonder about is whether we can add a fix-it to this diagnostic to rewrite the expression to the alternative form. If that's easy to do, it might be a nice addition to the diagnostic. If it's at all a pain though, I don't insist.

clang/test/Sema/warn-shift-bool.cpp

2–3

7

You should also have a test case for >>

[angelomatnigoogle]

Consolidated shift-bool warning; fixed relevant unit tests

Thank you for the feedback, and pardon me for not getting back to this work sooner. I have adopted your code change suggestions with minor tweeks to get it working and passing the previously failing tests. Later this week I will double back and attempt to use FixItHint on this as well as to update ReleaseNotes and run clang against some larger code bases

[arsenm]

ping?

[aaron.ballman]

In D141414#4494064, @arsenm wrote:

ping?

Thank you for pinging this! I took a quick pass over the changes and they're moving in the right direction, but there are some new test failures that need addressing and some unfinished comments from the previous review.

clang/lib/Sema/SemaExpr.cpp

11955–11956

We try not to pass constant strings into diagnostics because that makes it harder for us to localize diagnostics in the future. Instead, these strings should be in the diagnostic wording itself (with a %select directive).

[NekoCdr]

@angelomatnigoogle are you still working on this patch?

[angelomatnigoogle]

Pardon me for losing sight on this change. Within the week, I will revisit this change based on the current outstanding comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_shift_lhs_negative : Warning<"shifting a negative signed value is undefined">,
   InGroup<DiagGroup<"shift-negative-value">>;
 def warn_shift_negative : Warning<"shift count is negative">,
   InGroup<DiagGroup<"shift-count-negative">>;
 def warn_shift_gt_typewidth : Warning<"shift count >= width of type">,
   InGroup<DiagGroup<"shift-count-overflow">>;
 def warn_shift_result_gt_typewidth : Warning<
   "signed shift result (%0) requires %1 bits to represent, but %2 only has "
   "%3 bits">, InGroup<DiagGroup<"shift-overflow">>;
+def warn_shift_on_bool_type : Warning<
+  "%select{left|right}0 shifting 'bool' will implicitly cast to 'int'; consider %select{%1|%2}0 "
+  "to avoid implicitly relying on the width of 'int'">, InGroup<DiagGroup<"shift-bool">>;
 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_precedence_bitwise_rel : Warning<
   "%0 has lower precedence than %1; %1 will be evaluated first">,
   InGroup<Parentheses>;
 def note_precedence_bitwise_first : Note<
   "place parentheses around the %0 expression to evaluate it first">;

clang/lib/Sema/SemaExpr.cpp

if (!LHSType->isVLSTBuiltinType()) {
     QualType VecTy =
         S.Context.getScalableVectorType(RHSEleType, VecSize.getKnownMinValue());
     RHS = S.ImpCastExprToType(RHS.get(), VecTy, CK_VectorSplat);
   }
 
   return LHSType;
 }
 
 // C99 6.5.7
 QualType Sema::CheckShiftOperands(ExprResult &LHS, ExprResult &RHS,
                                   SourceLocation Loc, BinaryOperatorKind Opc,
                                   bool IsCompAssign) {
   checkArithmeticNull(*this, LHS, RHS, Loc, /*IsCompare=*/false);
+  if (LHS.get()->IgnoreParenImpCasts()->getType()->isBooleanType())
+    Diag(Loc, diag::warn_shift_on_bool_type) << (Opc == BO_Shr)
+      << "'bool && (shift_count | desired_type_width - 1)'"
+      << "'bool & !shift_count'";
 
   // Vector shifts promote their scalar inputs to vector type.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType()) {
     if (LangOpts.ZVector) {
       // The shift operators for the z vector extensions work basically
       // like general shifts, except that neither the LHS nor the RHS is
       // allowed to be a "vector bool".

clang/test/AST/Interp/shifts.cpp

constexpr void test() { // ref-error {{constexpr function never produces a constant expression}} \
     lli = 1LL << (sizeof(long long) * __CHAR_BIT__ - 2);
   }
 
   static_assert(1 << 4 == 16, "");
   constexpr unsigned m = 2 >> 1;
   static_assert(m == 1, "");
   constexpr unsigned char c = 0 << 8;
   static_assert(c == 0, "");
-  static_assert(true << 1, "");
+
+  constexpr unsigned b = true << 1; // expected-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}} \
+                                    // cxx17-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}} \
+                                    // ref-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}} \
+                                    // ref-cxx17-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}}
+  static_assert(b, ""); 
+
   static_assert(1 << (__INT_WIDTH__ +1) == 0, "");  // expected-error {{not an integral constant expression}} \
                                                     // expected-note {{>= width of type 'int'}} \
                                                     // cxx17-error {{not an integral constant expression}} \
                                                     // cxx17-note {{>= width of type 'int'}} \
                                                     // ref-error {{not an integral constant expression}} \
                                                     // ref-note {{>= width of type 'int'}} \
                                                     // ref-cxx17-error {{not an integral constant expression}} \
                                                     // ref-cxx17-note {{>= width of type 'int'}}

clang/test/Analysis/svalbuilder-simplify-no-crash.c

 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -verify
 
 // Here, we test that svalbuilder simplification does not produce any
 // assertion failure.
 
 void crashing(long a, _Bool b) {
   (void)(a & 1 && 0);
   b = a & 1;
-  (void)(b << 1); // expected-warning{{core.UndefinedBinaryOperatorResult}}
+  (void)(b << 1); // expected-warning{{core.UndefinedBinaryOperatorResult}} \
+                  // expected-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}}
 }

clang/test/CXX/over/over.built/p18.cpp

void f(int i, float f, bool b, int* pi, T* pt, T t) {
   (void)(3 | f);  // expected-error {{invalid operands}}
   (void)(3 | b);
   (void)(3 | pi); // expected-error {{invalid operands}}
   (void)(3 | pt); // FIXME
   (void)(3 | t);
 
   (void)(i << 3);
   (void)(f << 3);  // expected-error {{invalid operands}}
-  (void)(b << 3);
+  (void)(b << 3);  // expected-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}}
   (void)(pi << 3); // expected-error {{invalid operands}}
   (void)(pt << 3); // FIXME
   (void)(t << 3);
   (void)(3 << i);
   (void)(3 << f);  // expected-error {{invalid operands}}
   (void)(3 << b);
   (void)(3 << pi); // expected-error {{invalid operands}}
   (void)(3 << pt); // FIXME
   (void)(3 << t);
 
   (void)(i >> 3);
   (void)(f >> 3);  // expected-error {{invalid operands}}
-  (void)(b >> 3);
+  (void)(b >> 3);  // expected-warning {{right shifting 'bool' will implicitly cast to 'int'; consider 'bool & !shift_count' to avoid implicitly relying on the width of 'int'}}
   (void)(pi >> 3); // expected-error {{invalid operands}}
   (void)(pt >> 3); // FIXME
   (void)(t >> 3);
   (void)(3 >> i);
   (void)(3 >> f);  // expected-error {{invalid operands}}
   (void)(3 >> b);
   (void)(3 >> pi); // expected-error {{invalid operands}}
   (void)(3 >> pt); // FIXME
   (void)(3 >> t);
 }

clang/test/Sema/warn-shift-bool.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int f(int x) {
+  const bool b = 1;
+  const bool c = b << x; // expected-warning {{left shifting 'bool' will implicitly cast to 'int'; consider 'bool && (shift_count | desired_type_width - 1)' to avoid implicitly relying on the width of 'int'}}
+  const bool d = b >> x; // expected-warning {{right shifting 'bool' will implicitly cast to 'int'; consider 'bool & !shift_count' to avoid implicitly relying on the width of 'int'}}
+  return 0;
+}