[clang-tidy] Ignore narrowing conversions in case of bitfields

[steakhal]

Summary

Bitfields are special. Due to integral promotion conv.prom/5 bitfield
member access expressions are frequently wrapped by an implicit cast to
int if that type can represent all the values of the bitfield.

Consider these examples:

struct SmallBitfield { unsigned int id : 4; };
x.id & 1;             (case-1)
x.id & 1u;            (case-2)
x.id << 1u;           (case-3)
(unsigned)x.id << 1;  (case-4)

Due to the promotion rules, we would get a warning for case-1. It's
debatable how useful this is, but the user at least has a convenient way of
'fixing' it by adding the u unsigned-suffix to the literal as
demonstrated by case-2. However, this won't work for shift operators like
the one in case-3. In case of a normal binary operator, both operands
contribute to the result type. However, the type of the shift expression is
the promoted type of the left operand. One could still suppress this
superfluous warning by explicitly casting the bitfield member access as
case-4 demonstrates, but why? The compiler already knew that the value from
the member access should safely fit into an int, why do we have this
warning in the first place? So, hereby we suppress this specific scenario, when a bitfield's value is implicitly cast to int (likely due to integral promotion).

Note that the e.g. bitshift operations might invoke unspecified/undefined
behavior, but that's another topic, this checker is about detecting
conversion-related defects.

Example AST for x.id << 1:

BinaryOperator 'int' '<<'
|-ImplicitCastExpr 'int' <IntegralCast>     <--- due to integral promotion
| `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
|   `-MemberExpr 'unsigned int' lvalue bitfield .id
|     `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
`-IntegerLiteral 'int' 1

[courbet]

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

104

There needs to be some kind of check of the size of the bit field vs the size of the integer, because struct SmallBitfield { unsigned int id : 31; }; should warn.

105

What is specific about shifting operators ? What about x+1 for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.

BinaryOperator <line:7:3, col:10> 'int' '+'
      |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
      | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
      |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
      |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
      `-IntegerLiteral <col:10> 'int' 1

106

There also needs to be some check of the constant value of the RHS, because x.id << 30 can actually overflow, so we want to warn in that case.

[whisperity]

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

87

(Terminology... 😕)

100

IntWidened? Just to point out that so far this is the only case we've found where the warning is superfluous.

[steakhal]

Thanks for the valuable feedback.

D114105#inline-1089282

Let me clarify my motivation, and why I'm interested in bitfields, conversions, and shift expressions.
Bitfields can be quite tricky.
It turns out doing anything useful with a bitfield will eventually result in an integral promotion, which mandates that the type must be 'int' if it fits, otherwise 'unsigned int' or no integral promotion happens otherwise.
conv.prom/5:

A prvalue for an integral bit-field ([class.bit]) can be converted to a prvalue of type int if int can represent all the values of the bit-field; otherwise, it can be converted to unsigned int if unsigned int can represent all the values of the bit-field. If the bit-field is larger yet, no integral promotion applies to it. If the bit-field has an enumerated type, it is treated as any other value of that type for promotion purposes.

This behavior really surprises developers, but the context in which it happens is mostly binary operators.
At first glance, those reports seem to be false-positives but they are actually true-positives - and we are happy catching them.
A recommended workaround to this problem could be simply supplying the proper integer literal constant, by adding the u unsigned-suffix.
This will turn the int operand into unsigned int, at which point the binary operator will suddenly do the right thing and promote the bitfield expression to unsigned int as well, and the warning disappears.
This workaround works just fine for the regular binary operators, whose result type is the promoted type of the operands.

In the expr.compound section, the shift expression is the only one whose return type depends only on the left operand, making my plotted workaround inapplicable for them.
Note that the compound shift assignment operator expression doesn't suffer from this behavior.
expr.ass/1:

[...] The result in all cases is a bit-field if the left operand is a bit-field.

Given that the compiler chose int because that would be able to hold the value of the underlying bitfield without information loss, I think it makes sense to ignore these cases.
Of course, the shift operation might cause issues, but that's I think a different topic.
We might be able to do the extra mile and check if the other operand is a constant expression and the represented value would cause UB or unspecified behavior at the shift operation, but that's not really a conversion issue, what this check supposed to look for.

x.id & 4u; // no-warning: promotion will force x.id to be unsigned
x.id << 4u; // we still have a warning! the type of the LHS is unaffected by the type of the RHS
(unsigned)x.id << 4; // looks weird, but does the right thing

That being said, I think the matcher should look through paren expressions but other than that I don't think there is an issue.
Alternatively, we could say that the users *must* use an explicit cast when loading from a bitfield in bitshifts to make their intention clear.
However, it doesn't feel natural and I cannot immediately see how frequently those reports would be true-positives.

WDYT? How should I proceed?

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

105

What is specific about shifting operators ? What about x+1 for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.

BinaryOperator <line:7:3, col:10> 'int' '+'
      |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
      | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
      |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
      |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
      `-IntegerLiteral <col:10> 'int' 1

[courbet]

Thanks for the details, this explains the motivation well. I think the key point is the combination of:

It turns out doing anything useful with a bitfield will eventually result in an integral promotion, which mandates that the type must be 'int' if it fits
conv.prom/5:

A prvalue for an integral bit-field ([class.bit]) can be converted to a prvalue of type int if int can represent all the values of the bit-field; otherwise, it can be converted to unsigned int if unsigned int can represent all the values of the bit-field. If the bit-field is larger yet, no integral promotion applies to it. If the bit-field has an enumerated type, it is treated as any other value of that type for promotion purposes.

and

shift expression is the only one whose return type depends only on the left operand

So adding that as a comment above ShiftingWidenedBitfieldValue would help.

Maybe also add a note in the comment that we are not interested in whether the shift itself might overflow.

[steakhal]

• added more tests
• add a detailed comment about the situation we suppress
• see through parenExprs

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

105

I'm not sure if this approach is the right approach.
In that sense, we should not act differently on shift operations, rather just ignore member access expressions implicitly promoted to int.
This is what we are actually looking for. I would rather pursue this direction, but it would mean that we suppress a larger set of reports that we currently have.

What's your opinion on this @courbet @aaron.ballman?

[steakhal]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

63

Outdated comment; I'll skim through and make sure these reflect the current status.

[steakhal]

Bitshifts have nothing to do with this issue, thus I'm simply matching for bitfield -> int implicit casts due to promotions.

[steakhal]

I think it's in pretty good shape, please have a look to help me move this forward.

[courbet]

Yes, I think the new approach is what we want. The comments also make it much clearer.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

1

Can you add a test with:

void takesInt(int);

...

takesInt(x.id);

I think this suffers from the same problem currently.

[steakhal]

Added the test_parameter_passing() tests, demonstrating the implicit conversion triggered by parameter passing.
It turns out my previous revision suppressed a useful report.
Now, I fixed that and we get a warning for take<int>(x.id) // warn when x is CompleteBitfield with 32 bitwidth.
Aside from this, no behavioral change happened compared to the previous revision.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

1

Sure, great idea! It actually revealed a logical flaw. Thanks.

[courbet]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

63

[nit] newline after take

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
   // We attempt to address common incidents of compound expressions with
   // `IsIgnoredTypeTwoLevelsDeep`, allowing binary expressions that have one
   // operand of the ignored types and the other operand of another integer type.
   const auto IsIgnoredTypeTwoLevelsDeep =
       anyOf(IsConversionFromIgnoredType,
             binaryOperator(hasOperands(IsConversionFromIgnoredType,
                                        hasType(isInteger()))));
 
+  // Bitfields are special. Due to integral promotion [conv.prom/5] bitfield
+  // member access expressions are frequently wrapped by an implicit cast to
+  // `int` if that type can represent all the values of the bitfield.
+  //
+  // Consider these examples:
+  //   struct SmallBitfield { unsigned int id : 4; };
+  //   x.id & 1;             (case-1)
+  //   x.id & 1u;            (case-2)
+  //   x.id << 1u;           (case-3)
+  //   (unsigned)x.id << 1;  (case-4)
+  //
+  // Due to the promotion rules, we would get a warning for case-1. It's
+  // debatable how useful this is, but the user at least has a convenient way of
+  // //fixing// it by adding the `u` unsigned-suffix to the literal as
+  // demonstrated by case-2. However, this won't work for shift operators like
+  // the one in case-3. In case of a normal binary operator, both operands
+  // contribute to the result type. However, the type of the shift expression is
+  // the promoted type of the left operand. One could still suppress this
+  // superfluous warning by explicitly casting the bitfield member access as
+  // case-4 demonstrates, but why? The compiler already knew that the value from
+  // the member access should safely fit into an `int`, why do we have this
+  // warning in the first place? So, hereby we suppress this specific scenario.
+  //
+  // Note that the bitshift operation might invoke unspecified/undefined
+  // behavior, but that's another topic, this checker is about detecting
+  // conversion-related defects.
+  //
+  // Example AST for `x.id << 1`:
+  //   BinaryOperator 'int' '<<'
+  //   |-ImplicitCastExpr 'int' <IntegralCast>
+  //   | `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
+  //   |   `-MemberExpr 'unsigned int' lvalue bitfield .id
+  //   |     `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
+  //   `-IntegerLiteral 'int' 1
+  const auto ImplicitIntWidenedBitfieldValue = implicitCastExpr(
+      hasCastKind(CK_IntegralCast), hasType(asString("int")),
+      has(castExpr(hasCastKind(CK_LValueToRValue),
+                   has(ignoringParens(
+                       memberExpr(hasDeclaration(fieldDecl(isBitField()))))))));
+
   // Casts:
   //   i = 0.5;
   //   void f(int); f(0.5);
   Finder->addMatcher(
       traverse(TK_AsIs, implicitCastExpr(
                             hasImplicitDestinationType(
                                 hasUnqualifiedDesugaredType(builtinType())),
                             hasSourceExpression(hasType(
                                 hasUnqualifiedDesugaredType(builtinType()))),
                             unless(hasSourceExpression(IsCeilFloorCallExpr)),
                             unless(hasParent(castExpr())),
                             WarnWithinTemplateInstantiation
                                 ? stmt()
                                 : stmt(unless(isInTemplateInstantiation())),
                             IgnoreConversionFromTypes.empty()
                                 ? castExpr()
                                 : castExpr(unless(hasSourceExpression(
-                                      IsIgnoredTypeTwoLevelsDeep))))
+                                      IsIgnoredTypeTwoLevelsDeep))),
+                            unless(ImplicitIntWidenedBitfieldValue))
                             .bind("cast")),
       this);
 
   // Binary operators:
   //   i += 0.5;
   Finder->addMatcher(
       binaryOperator(
           isAssignmentOperator(),

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN:   -std=c++17 -- -target x86_64-unknown-linux
+
+#define CHAR_BITS 8
+static_assert(sizeof(unsigned int) == 32 / CHAR_BITS);
+
+template <typename T, typename U>
+struct is_same {
+  static constexpr bool value = false;
+};
+template <typename T>
+struct is_same<T, T> {
+  static constexpr bool value = true;
+};
+
+template <typename T, typename U>
+static constexpr bool is_same_v = is_same<T, U>::value;
+
+struct NoBitfield {
+  unsigned int id;
+};
+struct SmallBitfield {
+  unsigned int id : 4;
+};
+
+struct BigBitfield {
+  unsigned int id : 31;
+};
+struct CompleteBitfield {
+  unsigned int id : 32;
+};
+
+int example_warning(unsigned x) {
+  // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  return x;
+}
+
+void test_binary_and(SmallBitfield x) {
+  static_assert(is_same_v<decltype(x.id & 1), int>);
+  static_assert(is_same_v<decltype(x.id & 1u), unsigned>);
+
+  x.id & 1;
+  x.id & 1u;
+
+  1 & x.id;
+  1u & x.id;
+}
+
+void test_binary_or(SmallBitfield x) {
+  static_assert(is_same_v<decltype(x.id | 1), int>);
+  static_assert(is_same_v<decltype(x.id | 1u), unsigned>);
+
+  x.id | 1;
+  x.id | 1u;
+
+  1 | x.id;
+  1u | x.id;
+}
+
+void test(NoBitfield x) {
+  static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+  static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+  static_assert(is_same_v<decltype(x.id + 1), unsigned>);
+  static_assert(is_same_v<decltype(x.id + 1u), unsigned>);
+
+  x.id << 1;
+  x.id << 1u;
+  x.id >> 1;
+  x.id >> 1u;
+  x.id + 1;
+  x.id + 1u;
+
+  1 << x.id;
+  1u << x.id;
+  1 >> x.id;
+  1u >> x.id;
+  1 + x.id;
+  1u + x.id;
+}
+
+void test(SmallBitfield x) {
+  static_assert(is_same_v<decltype(x.id << 1), int>);
+  static_assert(is_same_v<decltype(x.id << 1u), int>);
+
+  x.id << 1;
+  x.id << 1u;
+  x.id >> 1;
+  x.id >> 1u;
+
+  x.id + 1;
+  x.id + 1u;
+
+  1 << x.id;
+  1u << x.id;
+  1 >> x.id;
+  1u >> x.id;
+
+  1 + x.id;
+  1u + x.id;
+}
+
+void test(BigBitfield x) {
+  static_assert(is_same_v<decltype(x.id << 1), int>);
+  static_assert(is_same_v<decltype(x.id << 1u), int>);
+
+  x.id << 1;
+  x.id << 1u;
+  x.id >> 1;
+  x.id >> 1u;
+
+  x.id + 1;
+  x.id + 1u;
+
+  1 << x.id;
+  1u << x.id;
+  1 >> x.id;
+  1u >> x.id;
+
+  1 + x.id;
+  1u + x.id;
+}
+
+void test(CompleteBitfield x) {
+  static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+  static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+
+  x.id << 1;
+  x.id << 1u;
+  x.id >> 1;
+  x.id >> 1u;
+
+  x.id + 1;
+  x.id + 1u;
+
+  1 << x.id;
+  1u << x.id;
+  1 >> x.id;
+  1u >> x.id;
+
+  1 + x.id;
+  1u + x.id;
+}
+
+void test_parens(SmallBitfield x) {
+  static_assert(is_same_v<decltype(x.id << (2)), int>);
+  static_assert(is_same_v<decltype(((x.id)) << (2)), int>);
+  x.id << (2);
+  ((x.id)) << (2);
+
+  static_assert(is_same_v<decltype((2) << x.id), int>);
+  static_assert(is_same_v<decltype((2) << ((x.id))), int>);
+  (2) << x.id;
+  (2) << ((x.id));
+}