[clang-tidy] false positive narrowing conversion

Let's consider the following code from the issue #139467:

void test(int cond, char c) {
    char ret = cond > 0 ? ':' : c;
}

Initializer of 'ret' looks the following:

-ImplicitCastExpr 'char' <IntegralCast>
 `-ConditionalOperator 'int'
   |-BinaryOperator 'int' '>'
   | |-ImplicitCastExpr 'int' <LValueToRValue>
   | | `-DeclRefExpr 'int' lvalue ParmVar 'cond' 'int'
   | `-IntegerLiteral 'int' 0
   |-CharacterLiteral 'int' 58
   `-ImplicitCastExpr 'int' <IntegralCast>
     `-ImplicitCastExpr 'char' <LValueToRValue>
       `-DeclRefExpr 'char' lvalue ParmVar 'c' 'char'

So it could be seen that 'RHS' of the conditional operator is
DeclRefExpr 'c' which is casted to 'int' and then the whole conditional expression is casted to 'char'.
But this last conversion is not narrowing, because 'RHS' was 'char' _initially_.
We should just remove the cast from 'char' to 'int' before the narrowing conversion check.

Author: AndreyG

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

@@ -553,15 +553,23 @@ bool NarrowingConversionsCheck::handleConditionalOperator(
     // We have an expression like so: `output = cond ? lhs : rhs`
     // From the point of view of narrowing conversion we treat it as two
     // expressions `output = lhs` and `output = rhs`.
-    handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
-                         *CO->getLHS());
-    handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), Lhs,
-                         *CO->getRHS());
+    handleConditionalOperatorArgument(Context, Lhs, CO->getLHS());
+    handleConditionalOperatorArgument(Context, Lhs, CO->getRHS());
     return true;
   }
   return false;
 }
 
+void NarrowingConversionsCheck::handleConditionalOperatorArgument(
+    const ASTContext &Context, const Expr &Lhs, const Expr *Arg) {
+  if (const auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(Arg)) {
+    if (!Arg->getIntegerConstantExpr(Context)) {
+      Arg = ICE->getSubExpr();
+    }
+  }
+  handleBinaryOperator(Context, Arg->getExprLoc(), Lhs, *Arg);
+}
+
 void NarrowingConversionsCheck::handleImplicitCast(
     const ASTContext &Context, const ImplicitCastExpr &Cast) {
   if (Cast.getExprLoc().isMacroID())

clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h

@@ -85,6 +85,8 @@ class NarrowingConversionsCheck : public ClangTidyCheck {
   bool handleConditionalOperator(const ASTContext &Context, const Expr &Lhs,
                                  const Expr &Rhs);
 
+  void handleConditionalOperatorArgument(const ASTContext &Context, const Expr &Lhs,
+                                         const Expr *Arg);
   void handleImplicitCast(const ASTContext &Context,
                           const ImplicitCastExpr &Cast);
 

clang-tools-extra/docs/ReleaseNotes.rst

@@ -182,6 +182,10 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`bugprone-narrowing-conversions
+  <clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing
+  false positive from analysis of a conditional expression in C.
+
 - Improved :doc:`bugprone-crtp-constructor-accessibility
   <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` check by fixing
   false positives on deleted constructors that cannot be used to construct

clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c

@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
+
+char test_char(int cond, char c) {
+	char ret = cond > 0 ? ':' : c;
+	return ret;
+}
+
+short test_short(int cond, short s) {
+	short ret = cond > 0 ? ':' : s;
+	return ret;
+}
+
+int test_int(int cond, int i) {
+	int ret = cond > 0 ? ':' : i;
+	return ret;
+}
+
+void test(int cond, int i) {
+  char x = cond > 0 ? ':' : i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]
+}
+

clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp

@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
+
+char test_char(int cond, char c) {
+	char ret = cond > 0 ? ':' : c;
+	return ret;
+}
+
+short test_short(int cond, short s) {
+	short ret = cond > 0 ? ':' : s;
+	return ret;
+}
+
+int test_int(int cond, int i) {
+	int ret = cond > 0 ? ':' : i;
+	return ret;
+}
+
+void test(int cond, int i) {
+  char x = cond > 0 ? ':' : i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]
+}
+