[clang-tidy] Improve build-in type handling in bugprone-swapped-arguments

[PiotrZSL]

Summary

Improved detection of argument swaps involving integral and floating-point
types by enhancing handling of implicit conversions. Now implicit casts
from float to double are also considered, same for integers.
Improved documentation.

Fixes: #62926

[PiotrZSL]

Ping

[carlosgalvezp]

LGTM, thank you for the fix!

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

 #include "clang/Tooling/FixIt.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::bugprone {
 
 void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(callExpr().bind("call"), this);
+  Finder->addMatcher(callExpr(unless(isInTemplateInstantiation())).bind("call"),
+                     this);
 }
 
 /// Look through lvalue to rvalue and nop casts. This filters out
 /// implicit conversions that have no effect on the input but block our view for
 /// other implicit casts.
 static const Expr *ignoreNoOpCasts(const Expr *E) {
   if (auto *Cast = dyn_cast<CastExpr>(E))
     if (Cast->getCastKind() == CK_LValueToRValue ||
         Cast->getCastKind() == CK_NoOp)
       return ignoreNoOpCasts(Cast->getSubExpr());
   return E;
 }
 
 /// Restrict the warning to implicit casts that are most likely
 /// accidental. User defined or integral conversions fit in this category,
 /// lvalue to rvalue or derived to base does not.
 static bool isImplicitCastCandidate(const CastExpr *Cast) {
   return Cast->getCastKind() == CK_UserDefinedConversion ||
          Cast->getCastKind() == CK_FloatingToBoolean ||
          Cast->getCastKind() == CK_FloatingToIntegral ||
          Cast->getCastKind() == CK_IntegralToBoolean ||
          Cast->getCastKind() == CK_IntegralToFloating ||
          Cast->getCastKind() == CK_MemberPointerToBoolean ||
-         Cast->getCastKind() == CK_PointerToBoolean;
+         Cast->getCastKind() == CK_PointerToBoolean ||
+         (Cast->getCastKind() == CK_IntegralCast &&
+          Cast->getSubExpr()->getType()->isBooleanType());
+}
+
+static bool areTypesSemiEqual(const QualType L, const QualType R) {
+  if (L == R)
+    return true;
+
+  if (!L->isBuiltinType() || !R->isBuiltinType())
+    return false;
+
+  return (L->isFloatingType() && R->isFloatingType()) ||
+         (L->isIntegerType() && R->isIntegerType()) ||
+         (L->isBooleanType() && R->isBooleanType());
+}
+
+static bool areArgumentsPotentiallySwapped(const QualType LTo,
+                                           const QualType RTo,
+                                           const QualType LFrom,
+                                           const QualType RFrom) {
+  if (LTo == RTo || LFrom == RFrom)
+    return false;
+
+  const bool REq = areTypesSemiEqual(RTo, LFrom);
+  if (LTo == RFrom && REq)
+    return true;
+
+  bool LEq = areTypesSemiEqual(LTo, RFrom);
+  if (RTo == LFrom && LEq)
+    return true;
+
+  if (REq && LEq && !areTypesSemiEqual(RTo, LTo))
+    return true;
+
+  return false;
 }
 
 void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
   const ASTContext &Ctx = *Result.Context;
   const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
 
   llvm::SmallPtrSet<const Expr *, 4> UsedArgs;
   for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
     // If the types that go into the implicit casts match the types of the other
     // argument in the declaration there is a high probability that the
     // arguments were swapped.
     // TODO: We could make use of the edit distance between the argument name
     // and the name of the passed variable in addition to this type based
     // heuristic.
     const Expr *LHSFrom = ignoreNoOpCasts(LHSCast->getSubExpr());
     const Expr *RHSFrom = ignoreNoOpCasts(RHSCast->getSubExpr());
-    if (LHS->getType() == RHS->getType() ||
-        LHS->getType() != RHSFrom->getType() ||
-        RHS->getType() != LHSFrom->getType())
+    if (!areArgumentsPotentiallySwapped(LHS->getType(), RHS->getType(),
+                                        LHSFrom->getType(), RHSFrom->getType()))
       continue;
 
     // Emit a warning and fix-its that swap the arguments.
     diag(Call->getBeginLoc(), "argument with implicit conversion from %0 "
                               "to %1 followed by argument converted from "
                               "%2 to %3, potentially swapped arguments.")
-        << LHS->getType() << LHSFrom->getType() << RHS->getType()
-        << RHSFrom->getType()
-        << tooling::fixit::createReplacement(*LHS, *RHS, Ctx)
+        << LHSFrom->getType() << LHS->getType() << RHSFrom->getType()
+        << RHS->getType() << tooling::fixit::createReplacement(*LHS, *RHS, Ctx)
         << tooling::fixit::createReplacement(*RHS, *LHS, Ctx);
 
     // Remember that we emitted a warning for this argument.
     UsedArgs.insert(RHSCast);
   }
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- Extend :doc:`bugprone-unused-return-value
   with specified return types using the ``CheckedReturnTypes`` option.
 
 - Improved :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>` check. Detect uses and moves in
   constructor initializers. Correctly handle constructor arguments as being
   sequenced when constructor call is written as list-initialization. Understand
   that there is a sequence point between designated initializers.
 
+- Improved :doc:`bugprone-swapped-arguments
+  <clang-tidy/checks/bugprone/swapped-arguments>` by enhancing handling of
+  implicit conversions, resulting in better detection of argument swaps
+  involving integral and floating-point types.
+
 - Deprecated :doc:`cert-dcl21-cpp
   <clang-tidy/checks/cert/dcl21-cpp>` check.
 
 - Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`. Please use
   :doc:`cppcoreguidelines-use-default-member-init
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>` instead.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst

 .. title:: clang-tidy - bugprone-swapped-arguments
 
 bugprone-swapped-arguments
 ==========================
 
-Finds potentially swapped arguments by looking at implicit conversions.
+Finds potentially swapped arguments by examining implicit conversions.
+It analyzes the types of the arguments being passed to a function and compares
+them to the expected types of the corresponding parameters. If there is a
+mismatch or an implicit conversion that indicates a potential swap, a warning
+is raised.
+
+.. code-block:: c++
+
+  void printNumbers(int a, float b);
+
+  int main() {
+    // Swapped arguments: float passed as int, int as float)
+    printNumbers(10.0f, 5);
+    return 0;
+  }
+
+Covers a wide range of implicit conversions, including:
+- User-defined conversions
+- Conversions from floating-point types to boolean or integral types
+- Conversions from integral types to boolean or floating-point types
+- Conversions from boolean to integer types or floating-point types
+- Conversions from (member) pointers to boolean
+
+It is important to note that for most argument swaps, the types need to match
+exactly. However, there are exceptions to this rule. Specifically, when the
+swapped argument is of integral type, an exact match is not always necessary.
+Implicit casts from other integral types are also accepted. Similarly, when
+dealing with floating-point arguments, implicit casts between different
+floating-point types are considered acceptable.
+
+To avoid confusion, swaps where both swapped arguments are of integral types or
+both are of floating-point types do not trigger the warning. In such cases, it's
+assumed that the developer intentionally used different integral or
+floating-point types and does not raise a warning. This approach prevents false
+positives and provides flexibility in handling situations where varying integral
+or floating-point types are intentionally utilized.

clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp

 // RUN: %check_clang_tidy %s bugprone-swapped-arguments %t
 
 void F(int, double);
 
 int SomeFunction();
 
 template <typename T, typename U>
 void G(T a, U b) {
   F(a, b); // no-warning
   F(2.0, 4);
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // CHECK-FIXES: F(4, 2.0)
 }
 
+void funShortFloat(short, float);
+void funFloatFloat(float, float);
+void funBoolShort(bool, short);
+void funBoolFloat(bool, float);
+
 void foo() {
   F(1.0, 3);
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // CHECK-FIXES: F(3, 1.0)
 
 #define M(x, y) x##y()
 
   double b = 1.0;
   F(b, M(Some, Function));
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // CHECK-FIXES: F(M(Some, Function), b);
 
 #define N F(b, SomeFunction())
 
   N;
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // In macro, don't emit fixits.
 // CHECK-FIXES: #define N F(b, SomeFunction())
 
   G(b, 3);
   G(3, 1.0);
   G(0, 0);
 
   F(1.0, 1.0);    // no-warning
   F(3, 1.0);      // no-warning
   F(true, false); // no-warning
   F(0, 'c');      // no-warning
 
 #define APPLY(f, x, y) f(x, y)
   APPLY(F, 1.0, 3);
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // CHECK-FIXES: APPLY(F, 3, 1.0);
 
 #define PARAMS 1.0, 3
 #define CALL(P) F(P)
   CALL(PARAMS);
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments.
 // In macro, don't emit fixits.
+
+  funShortFloat(5.0, 1U);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'short' followed by argument converted from 'unsigned int' to 'float', potentially swapped arguments.
+  // CHECK-FIXES: funShortFloat(1U, 5.0);
+  funShortFloat(5.0, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'short' followed by argument converted from 'int' to 'float', potentially swapped arguments.
+  // CHECK-FIXES: funShortFloat(1, 5.0);
+  funShortFloat(5.0f, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'float' to 'short' followed by argument converted from 'int' to 'float', potentially swapped arguments.
+  // CHECK-FIXES: funShortFloat(1, 5.0f);
+
+  funFloatFloat(1.0f, 2.0);
+
+  funBoolShort(1U, true);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'unsigned int' to 'bool' followed by argument converted from 'bool' to 'short', potentially swapped arguments.
+  // CHECK-FIXES: funBoolShort(true, 1U);
+
+  funBoolFloat(1.0, true);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'bool' followed by argument converted from 'bool' to 'float', potentially swapped arguments.
+  // CHECK-FIXES: funBoolFloat(true, 1.0);
 }