From 082bf7f637164086f5cf3135dc892f0ca5191b50 Mon Sep 17 00:00:00 2001 From: Benjamin Kramer Date: Mon, 14 Jul 2014 14:24:30 +0000 Subject: [PATCH] [clang-tidy] Add a checker for swapped arguments. This looks for swapped arguments by looking at implicit conversions of arguments void Foo(int, double); Foo(1.0, 3); // Most likely a bug llvm-svn: 212942 --- .../clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 4 + .../clang-tidy/misc/SwappedArgumentsCheck.cpp | 125 ++++++++++++++++++ .../clang-tidy/misc/SwappedArgumentsCheck.h | 30 +++++ .../clang-tidy/misc-swapped-arguments.cpp | 44 ++++++ 5 files changed, 204 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/misc-swapped-arguments.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 233604ca7c80..aee804956a52 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -5,6 +5,7 @@ add_clang_library(clangTidyMiscModule BoolPointerImplicitConversion.cpp MiscTidyModule.cpp RedundantSmartptrGet.cpp + SwappedArgumentsCheck.cpp UseOverride.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index ee992928057d..669ad0811123 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -13,6 +13,7 @@ #include "ArgumentCommentCheck.h" #include "BoolPointerImplicitConversion.h" #include "RedundantSmartptrGet.h" +#include "SwappedArgumentsCheck.h" #include "UseOverride.h" namespace clang { @@ -30,6 +31,9 @@ public: CheckFactories.addCheckFactory( "misc-redundant-smartptr-get", new ClangTidyCheckFactory()); + CheckFactories.addCheckFactory( + "misc-swapped-arguments", + new ClangTidyCheckFactory()); CheckFactories.addCheckFactory( "misc-use-override", new ClangTidyCheckFactory()); diff --git a/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.cpp b/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.cpp new file mode 100644 index 000000000000..18edd91d8965 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.cpp @@ -0,0 +1,125 @@ +//===--- 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(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("call"); + + llvm::SmallPtrSet 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(ignoreNoOpCasts(LHS)); + const auto *RHSCast = dyn_cast(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 diff --git a/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.h b/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.h new file mode 100644 index 000000000000..9dbdad18d7f0 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.h @@ -0,0 +1,30 @@ +//===--- 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 + diff --git a/clang-tools-extra/test/clang-tidy/misc-swapped-arguments.cpp b/clang-tools-extra/test/clang-tidy/misc-swapped-arguments.cpp new file mode 100644 index 000000000000..1d43a49e9ee6 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-swapped-arguments.cpp @@ -0,0 +1,44 @@ +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-swapped-arguments %t +// REQUIRES: shell + +void F(int, double); + +int SomeFunction(); + +template +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 +}