[clang-tidy] Add a checker for swapped literal arguments.

[bkramer]

Summary

This looks for swapped arguments by looking at implicit conversions of
literal arguments

void Foo(int, double);
Foo(1.0, 3); // Most likely a bug

We do this by checking that the same argument types exist in literals and
the declaration of the function.

[djasper]

clang-tidy/misc/CMakeLists.txt

6

ImplicitArgumentConversion is not exactly what this does, I think. Maybe SwappedArgumentsCheck?

clang-tidy/misc/ImplicitArgumentConversion.cpp

33 ↗

(On Diff #11256)

Why do we only care about literals?

34 ↗

(On Diff #11256)

Maybe IgnoreParenImpCasts()?

52 ↗

(On Diff #11256)

Mhmm. This seems like a very crude way of doing this. Might be ok for a first iteration, but it seems like we could actually be much smarter and check if there is a re-ordering that would fit better (and even extend the analysis to more than two parameters, no)?

clang-tidy/misc/ImplicitArgumentConversion.h

18 ↗

(On Diff #11256)

This is VERY brief ;-)

[alexfh]

clang-tidy/misc/CMakeLists.txt

6

+1 for SwappedArgumentsCheck.

clang-tidy/misc/ImplicitArgumentConversion.cpp

52 ↗

(On Diff #11256)

Instead, I'd check if swapping each consecutive pair of arguments decreases the number of conversions by 2. It would also be nice to specify which pair of arguments is likely to be swapped.

[dblaikie]

Worth a compiler warning?

If we're talking literals, could we just warn whenever someone wrote a
literal with a mismatched parameter? (3.0 for an int parameter -
skipping 3e6 as being safe because people like to write big ints with
exponent notation even though it's actually a double)

Things like passing literal true/false for int parameters is also
often a bad sign... (I tried to implement bool literal conversion at
some point, but never got around to finishing it - it caught some
cases like this)

[thakis]

Wouldn't this make sense as a regular warning?

[djasper]

I think for this (and probably many future clang-tidy checks), the answer
"Does this make sense as a compiler warning?" is always: YES. However,
clang-tidy enables us to quickly develop and tune an implementation using
AST matchers, not worrying about slowing down the compiler and testing it
on a larger codebase in order establish whether a check/warning provides
sufficient benefit and even let users play around with it to get more
feedback. So, I see clang-tidy as a prototyping platform and good checks
will be turned into compiler warnings. At some stage we might want to think
about how we can simplify that process, i.e. how we can make porting a
clang-tidy check into a compiler warning easier, but IMO that stage isn't
necessarily now.

[thakis]

Makes sense, thanks.

[bkramer]

This is essentially a complete rewrite of the checker.

• Only compares pairs of arguments, and emits a swap fixit.
• Also looks at non-literal arguments. This increases false positives slightly but also catches a lot more cases.
• Renamed the checker.

[alexfh]

clang-tidy/misc/SwappedArgumentsCheck.cpp

55

IIUC, isWrittenInSameFile(l1, l2) will return true if l1 and l2 are macro locations. Am I missing something?

78

I'd prefer "const auto *" (or just "auto", if you want to save a few keystrokes).

91

Name-based approach may be used independently of argument types. I'd expect x/y, raw/column and other argument pairs using the same type to be swapped occasionally.

99

"Two matching implicit casts" isn't enough to understand what's wrong here. Maybe something like this

warning: potentially swapped arguments for parameter "x" of type double and parameter "y" of type int
note: argument "0" has type int, argument "10e6" has type double

?

test/clang-tidy/misc-swapped-arguments.cpp

2

Please add a test where the invocations are inside a template with a couple of instantiations.

[bkramer]

• Fix for crashes when the warning is within a macro (+ test)
• Use const auto *
• Wording tweak for warning message. Added types and SourceRanges to make the af

fected arguments stand out. Sadly we don't print them on the command line :(

[bkramer]

Updated for r212941.

[alexfh]

Looks good. Nice check!

[bkramer]

Committed in rL212942

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
+  SwappedArgumentsCheck.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
 #include "RedundantSmartptrGet.h"
+#include "SwappedArgumentsCheck.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "misc-argument-comment",
         new ClangTidyCheckFactory<ArgumentCommentCheck>());
     CheckFactories.addCheckFactory(
         "misc-bool-pointer-implicit-conversion",
         new ClangTidyCheckFactory<BoolPointerImplicitConversion>());
     CheckFactories.addCheckFactory(
         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
     CheckFactories.addCheckFactory(
+        "misc-swapped-arguments",
+        new ClangTidyCheckFactory<SwappedArgumentsCheck>());
+    CheckFactories.addCheckFactory(
         "misc-use-override",
         new ClangTidyCheckFactory<UseOverride>());
   }
 };
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<MiscModule>
 X("misc-module", "Adds miscellaneous lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MiscModule.
 volatile int MiscModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/misc/SwappedArgumentsCheck.h

+//===--- SwappedArgumentsCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Finds potentially swapped arguments by looking at implicit
+/// conversions.
+class SwappedArgumentsCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
+

clang-tidy/misc/SwappedArgumentsCheck.cpp

+//===--- SwappedArgumentsCheck.cpp - clang-tidy ---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SwappedArgumentsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/SmallPtrSet.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr().bind("call"), this);
+}
+
+/// \brief 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;
+}
+
+/// \brief 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;
+}
+
+/// \brief Get a StringRef representing a SourceRange.
+static StringRef getAsString(const MatchFinder::MatchResult &Result,
+                             SourceRange R) {
+  const SourceManager &SM = *Result.SourceManager;
+  // Don't even try to resolve macro or include contraptions. Not worth emitting
+  // a fixit for.
+  if (R.getBegin().isMacroID() ||
+      !SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))
+    return StringRef();
+
+  const char *Begin = SM.getCharacterData(R.getBegin());
+  const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken(
+      R.getEnd(), 0, SM, Result.Context->getLangOpts()));
+
+  return StringRef(Begin, End - Begin);
+}
+
+void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
+  auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
+
+  llvm::SmallPtrSet<const Expr *, 4> UsedArgs;
+  for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
+    const Expr *LHS = Call->getArg(I - 1);
+    const Expr *RHS = Call->getArg(I);
+
+    // Only need to check RHS, as LHS has already been covered. We don't want to
+    // emit two warnings for a single argument.
+    if (UsedArgs.count(RHS))
+      continue;
+
+    const auto *LHSCast = dyn_cast<ImplicitCastExpr>(ignoreNoOpCasts(LHS));
+    const auto *RHSCast = dyn_cast<ImplicitCastExpr>(ignoreNoOpCasts(RHS));
+
+    // Look if this is a potentially swapped argument pair. First look for
+    // implicit casts.
+    if (!LHSCast || !RHSCast || !isImplicitCastCandidate(LHSCast) ||
+        !isImplicitCastCandidate(RHSCast))
+      continue;
+
+    // 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())
+      continue;
+
+    // Emit a warning and fix-its that swap the arguments.
+    SourceRange LHSRange = LHS->getSourceRange(),
+                RHSRange = RHS->getSourceRange();
+    auto D =
+        diag(Call->getLocStart(), "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() << LHSRange << RHSRange;
+
+    StringRef RHSString = getAsString(Result, RHSRange);
+    StringRef LHSString = getAsString(Result, LHSRange);
+    if (!LHSString.empty() && !RHSString.empty()) {
+      D << FixItHint::CreateReplacement(
+               CharSourceRange::getTokenRange(LHSRange), RHSString)
+        << FixItHint::CreateReplacement(
+               CharSourceRange::getTokenRange(RHSRange), LHSString);
+    }
+
+    // Remember that we emitted a warning for this argument.
+    UsedArgs.insert(RHSCast);
+  }
+}
+
+} // namespace tidy
+} // namespace clang

test/clang-tidy/misc-swapped-arguments.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-swapped-arguments %t
+// REQUIRES: shell
+
+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-FIXES: F(4, 2.0)
+}
+
+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-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.
+// In macro, don't emit fixits.
+// CHECK-FIXES: F(b, M(Some, Function));
+
+#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.
+// 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
+}