[clang-tidy] Suppress reports to similarly used parameters in 'bugprone-easily-swappable-parameters'

[whisperity]

Summary

There are several types of functions and various reasons why some "swappable parameters" cannot be fixed with changing the parameters' types, etc. The most common example might be int min(int a, int b)... no matter what you do, the two parameters must remain the same type.

The filtering heuristic implemented in this patch deals with trying to find such functions during the modelling and building of the swappable parameter range. If the parameter currently scrutinised matches either of the predicates below, it will be regarded as not swappable even if the type of the parameter matches.

On average, 50% of the results are silenced by this heuristic (over a set of test projects we investigated). This heuristic has the possibility of producing false negatives, and no path-sensitive analysis is made, as usual in Tidy. However, the reduction of the findings greatly allows developers to see the more useful results about the dodgiest interfaces first and not be deafened by the noise.

The modelling "similarly used parameters" is toggleable by the user as a checker option. Due to the massive benefits this option brings for users who run this check for the first time on their project, the default is to have this turned on.

• Common expression: if both parameters are found in a common expression somewhere in the function, they are silenced. This heuristic is an absolute blast because it deals with arithmetics, comparisons, forwarding function calls, etc. out of the box at the same time.
• Returns: if both parameters are returned from different statements of the function, they are silenced. This deals with "switch-like" functions.
• Pass to same function: if both parameters are passed to the same function('s overload) to the same index, they are silenced. This deals with "dispatch-like" functions.
• Same member access: if the same member field is accessed or method is called on the two parameters, even if distinct expressions, they are "used in a similar fashion" and are silenced.

[whisperity]

This patch will be replaced with the refactoring of the base check.

[whisperity]

• Rebased over updated, rewritten, newer base patch version.
• Added the heuristic for "same member access" to the patch (which was so far only on my development branch).

[whisperity]

• Fixed an issue of the "same function" heuristic that blew up if functions are forward declared.
• [NFC] Optimised "relatedness" is to be checked first (as it's a trivial double hashmap lookup) before the recursive descent of type checking.

[whisperity]

• [NFC] Reformat the documentation so option values are in single backticks.

[aaron.ballman]

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

481

I *think* you could run into this assert with K&R C function where there is a FunctionDecl to get back to but the decl claims it has no params because it has no prototype (while the call expression actually has the arguments). However, there may be other code that protects us from this case.

532–536

Is this a FIXME because there's a matcher issue this works around or should it be an assertion?

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

43

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst

90

[whisperity]

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

532–536

I was unable to create a test for this case. This part of the code, while I was twisting my head left and right, is one of the strictest remnant of the old version of the check... There definitely was a project in the test set back in December 2019 which made the check crash or emit massive false positives, and this thing was added to guard against that.

At a hunch, I think maybe a lambda returning one of its captures (which are, AFAIK, ParmVarDecls of the constructor!) could be what caused the crash/false positive. I will check this out in detail soon.

[whisperity]

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

481

At face value, I would say the fact that a K&R function has no params declared means that the matcher above will not be able to do forEachArgumentWithParam, because the argument vector is N long, but the parameter vector is empty.

Nevertheless, it won't hurt to add a test case for this though.

[whisperity]

• Added some test cases to ensure edge cases won't crash.

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

481

No assertion, no crash. But unfortunately, we generate a false positive. But we don't crash. I added a test for this, just in case.

532–536

I do not remember why I thought this makes crashes because I tried and no crashing now (which is good), but lambdas defined inside a function's body is definitely a good reproducer for when this is hit.

Rephrased the comment, and added a test about this.

[aaron.ballman]

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

481

Thanks for adding the test case! I think a false positive is reasonable enough given how rarely I expect people using K&R C function declarations will worry about swapped arguments.

519

Question: would it make sense to (someday, not now) consider output parameters similar to return statements? e.g.,

void int_swap(int &foo, int &bar) {
  int temp = foo;
  foo = bar;
  bar = temp;
}

As a question for today: should co_return should be handled similarly as return?

532–536

Thank you for the clarification and test coverage!

553–556

I don't think there's a need to make these optional when we're using the Enabled flag.

[whisperity]

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

519

1. Maybe. Unfortunately, one needs to be careful with that. Originally I did implement a "5th" heuristic that dealt with ignoring parameters that had the same builtin operator used on them. However, while it silenced a few false positives, it started creating massive amounts of false negatives.

(In the concrete example, I think foo = bar will silence them because "appears in the same expression".)

2. I don't know. It would require a deeper understanding of Coroutines themselves, and how people use them.

553–556

The original idea was that each heuristic could be individually toggled by the user... But I think we can agree that would be a bad idea and just overengineering.

[whisperity]

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

519

If you don't mind me posting images, I can show a direct example. Things where the inability to track data flow really bites us in the backside.

Examples from Apache Xerces:

Here's a false positive that would be silenced by the logic of "using the same operateur on the two params".

And here is a false negative from the exact same logic.

Maybe it could be some solace that we're restricting to non-const-lvalue-references (and pointers-to-non-const ??) and only assignment (only assignment?!))...

[aaron.ballman]

LGTM once the optional bits are fixed up.

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

519

Ah, point #1 is an excellent point. As for #2, I think we can handle it in a follow-up, but I believe co_return follows the same usage patterns as return, generally.

553–556

Yeah, I think it's probably better to drop the optionals here -- we can revisit the decision later if there's user feedback requesting finer granularity here.

[whisperity]

NFC Turned some Optional<T> into just T.

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

519

Added a TODO for co_return, so we don't forget.

[whisperity]

NFC: Rebase & update.

While this patch conceptually only depends on D69560, the diff itself has become linear during the code review and subsequent updates, and as such, must be applied on top of D75041.

[whisperity]

NC Rebase & more buildbot shenanigans so we can get a pass!

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

public:
   const std::size_t MinimumLength;
 
   /// The parameter names (as written in the source text) to be ignored.
   const std::vector<std::string> IgnoredParameterNames;
 
   /// The parameter typename suffixes (as written in the source code) to be
   /// ignored.
   const std::vector<std::string> IgnoredParameterTypeSuffixes;
+
+  /// If enabled, diagnostics for parameters that are used together in a
+  /// similar way will not be presented.
+  const bool SuppressParametersUsedTogether;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EASILYSWAPPABLEPARAMETERSCHECK_H

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

 //===--- EasilySwappableParametersCheck.cpp - clang-tidy ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "EasilySwappableParametersCheck.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
 #define DEBUG_TYPE "EasilySwappableParametersCheck"
 #include "llvm/Support/Debug.h"
 
 namespace optutils = clang::tidy::utils::options;
 
optutils::serializeStringList({"bool",
                                    "ConstIterator",
                                    "RandomIt",
                                    "randomit",
                                    "random_iterator",
                                    "ReverseIt",
                                    "reverse_iterator",
                                    "reverse_const_iterator"});
 
+/// The default value for suppressing diagnostics about parameters that are
+/// used together.
+static constexpr bool DefaultSuppressParametersUsedTogether = true;
+
 #ifndef NDEBUG
 
 template <std::size_t Width>
 static inline std::string formatBitsImpl(std::size_t Num) {
   constexpr std::size_t WidthWithPadding = Width + (Width / 4);
   char S[WidthWithPadding];
   for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) {
     if (CurPos % 5 == 0) {

 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 using TheCheck = EasilySwappableParametersCheck;
 
 namespace filter {
+class SimilarlyUsedParameterPairSuppressor;
+
 static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node);
+static bool
+isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,
+                         const ParmVarDecl *Param1, const ParmVarDecl *Param2);
 } // namespace filter
 
 namespace model {
 
 /// The language features involved in allowing the mix between two parameters.
 enum MixFlags : unsigned char {
   MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!
 
LLVM_DEBUG(llvm::dbgs()
                << "<<< calculateMixability. Same CanonicalType.\n");
     return {MIX_Canonical};
   }
 
   LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. No match found.\n");
   return {MIX_None};
 }
 
-static MixableParameterRange modelMixingRange(const TheCheck &Check,
-                                              const FunctionDecl *FD,
-                                              std::size_t StartIndex) {
+static MixableParameterRange modelMixingRange(
+    const TheCheck &Check, const FunctionDecl *FD, std::size_t StartIndex,
+    const filter::SimilarlyUsedParameterPairSuppressor &UsageBasedSuppressor) {
   std::size_t NumParams = FD->getNumParams();
   assert(StartIndex < NumParams && "out of bounds for start");
   const ASTContext &Ctx = FD->getASTContext();
 
   MixableParameterRange Ret;
   // A parameter at index 'StartIndex' had been trivially "checked".
   Ret.NumParamsChecked = 1;
 
for (std::size_t I = StartIndex + 1; I < NumParams; ++I) {
     // Now try to go forward and build the range of [Start, ..., I, I + 1, ...]
     // parameters that can be messed up at a call site.
     MixableParameterRange::MixVector MixesOfIth;
     for (std::size_t J = StartIndex; J < I; ++J) {
       const ParmVarDecl *Jth = FD->getParamDecl(J);
       LLVM_DEBUG(llvm::dbgs()
                  << "Check mix of #" << J << " against #" << I << "...\n");
 
+      if (isSimilarlyUsedParameter(UsageBasedSuppressor, Ith, Jth)) {
+        // Consider the two similarly used parameters to not be possible in a
+        // mix-up at the user's request, if they enabled this heuristic.
+        LLVM_DEBUG(llvm::dbgs() << "Parameters #" << I << " and #" << J
+                                << " deemed related, ignoring...\n");
+
+        // If the parameter #I and #J mixes, then I is mixable with something
+        // in the current range, so the range has to be broken and I not
+        // included.
+        MixesOfIth.clear();
+        break;
+      }
+
       Mix M{Jth, Ith,
             calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx)};
       LLVM_DEBUG(llvm::dbgs() << "Mix flags (raw)           : "
                               << formatBits(M.flags()) << '\n');
       M.sanitize();
       LLVM_DEBUG(llvm::dbgs() << "Mix flags (after sanitize): "
                               << formatBits(M.flags()) << '\n');
 
for (std::size_t I = StartIndex + 1; I < NumParams; ++I) {
     ++Ret.NumParamsChecked; // Otherwise a new param was iterated.
   }
 
   return Ret;
 }
 
 } // namespace model
 
+/// Matches DeclRefExprs and their ignorable wrappers to ParmVarDecls.
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<Stmt>, paramRefExpr) {
+  return expr(ignoringParenImpCasts(ignoringElidableConstructorCall(
+      declRefExpr(to(parmVarDecl().bind("param"))))));
+}
+
 namespace filter {
 
 /// Returns whether the parameter's name or the parameter's type's name is
 /// configured by the user to be ignored from analysis and diagnostic.
 static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node) {
   LLVM_DEBUG(llvm::dbgs() << "Checking if '" << Node->getName()
                           << "' is ignored.\n");
 
if (llvm::any_of(Check.IgnoredParameterTypeSuffixes,
                      })) {
       LLVM_DEBUG(llvm::dbgs() << "\tType suffix ignored.\n");
       return true;
     }
   }
 
   return false;
 }
+
+/// This namespace contains the implementations for the suppression of
+/// diagnostics from similaly used ("related") parameters.
+namespace relatedness_heuristic {
+
+static constexpr std::size_t SmallDataStructureSize = 4;
+
+template <typename T, std::size_t N = SmallDataStructureSize>
+using ParamToSmallSetMap =
+    llvm::DenseMap<const ParmVarDecl *, llvm::SmallSet<T, N>>;
+
+/// Returns whether the sets mapped to the two elements in the map have at
+/// least one element in common.
+template <typename MapTy, typename ElemTy>
+bool lazyMapOfSetsIntersectionExists(const MapTy &Map, const ElemTy &E1,
+                                     const ElemTy &E2) {
+  auto E1Iterator = Map.find(E1);
+  auto E2Iterator = Map.find(E2);
+  if (E1Iterator == Map.end() || E2Iterator == Map.end())
+    return false;
+
+  for (const auto &E1SetElem : E1Iterator->second)
+    if (llvm::find(E2Iterator->second, E1SetElem) != E2Iterator->second.end())
+      return true;
+
+  return false;
+}
+
+/// Implements the heuristic that marks two parameters related if there is
+/// a usage for both in the same strict expression subtree. A strict
+/// expression subtree is a tree which only includes Expr nodes, i.e. no
+/// Stmts and no Decls.
+class AppearsInSameExpr : public RecursiveASTVisitor<AppearsInSameExpr> {
+  using Base = RecursiveASTVisitor<AppearsInSameExpr>;
+
+  const FunctionDecl *const FD;
+  const Expr *CurrentExprOnlyTreeRoot = nullptr;
+  llvm::DenseMap<const ParmVarDecl *,
+                 llvm::SmallPtrSet<const Expr *, SmallDataStructureSize>>
+      ParentExprsForParamRefs;
+
+public:
+  explicit AppearsInSameExpr(const FunctionDecl *FD) : FD(FD) {
+    TraverseFunctionDecl(const_cast<FunctionDecl *>(FD));
+  }
+
+  bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+    return lazyMapOfSetsIntersectionExists(ParentExprsForParamRefs,
+                                           Param1, Param2);
+  }
+
+  bool TraverseDecl(Decl *D) {
+    CurrentExprOnlyTreeRoot = nullptr;
+    return Base::TraverseDecl(D);
+  }
+
+  bool TraverseStmt(Stmt *S, DataRecursionQueue *Queue = nullptr) {
+    if (auto *E = dyn_cast_or_null<Expr>(S)) {
+      bool RootSetInCurrentStackFrame = false;
+      if (!CurrentExprOnlyTreeRoot) {
+        CurrentExprOnlyTreeRoot = E;
+        RootSetInCurrentStackFrame = true;
+      }
+
+      bool Ret = Base::TraverseStmt(S);
+
+      if (RootSetInCurrentStackFrame)
+        CurrentExprOnlyTreeRoot = nullptr;
+
+      return Ret;
+    }
+
+    // A Stmt breaks the strictly Expr subtree.
+    CurrentExprOnlyTreeRoot = nullptr;
+    return Base::TraverseStmt(S);
+  }
+
+  bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+    if (!CurrentExprOnlyTreeRoot)
+      return true;
+
+    if (auto *PVD = dyn_cast<ParmVarDecl>(DRE->getDecl()))
+      if (llvm::find(FD->parameters(), PVD))
+        ParentExprsForParamRefs[PVD].insert(CurrentExprOnlyTreeRoot);
+
+    return true;
+  }
+};
+
+/// Implements the heuristic that marks two parameters related if there are
+/// two separate calls to the same function (overload) and the parameters are
+/// passed to the same index in both calls, i.e f(a, b) and f(a, c) passes
+/// b and c to the same index (2) of f(), marking them related.
+class PassedToSameFunction {
+  ParamToSmallSetMap<std::pair<const FunctionDecl *, unsigned>> TargetParams;
+
+public:
+  explicit PassedToSameFunction(const FunctionDecl *FD) {
+    auto ParamsAsArgsInFnCalls =
+        match(functionDecl(forEachDescendant(
+                  callExpr(forEachArgumentWithParam(
+                               paramRefExpr(), parmVarDecl().bind("passed-to")))
+                      .bind("call-expr"))),
+              *FD, FD->getASTContext());
+    for (const auto &Match : ParamsAsArgsInFnCalls) {
+      const auto *PassedParamOfThisFn = Match.getNodeAs<ParmVarDecl>("param");
+      const auto *CE = Match.getNodeAs<CallExpr>("call-expr");
+      const auto *PassedToParam = Match.getNodeAs<ParmVarDecl>("passed-to");
+      assert(PassedParamOfThisFn && CE && PassedToParam);
+
+      const FunctionDecl *CalledFn = CE->getDirectCallee();
+      if (!CalledFn)
+        continue;
+
+      llvm::Optional<unsigned> TargetIdx;
+      unsigned NumFnParams = CalledFn->getNumParams();
+      for (unsigned Idx = 0; Idx < NumFnParams; ++Idx)
+        if (CalledFn->getParamDecl(Idx) == PassedToParam)
+          TargetIdx.emplace(Idx);
+
+      assert(TargetIdx.hasValue() && "Matched, but didn't find index?");
+      TargetParams[PassedParamOfThisFn].insert(
+          {CalledFn->getCanonicalDecl(), *TargetIdx});
+    }
+  }
+
+  bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+    return lazyMapOfSetsIntersectionExists(TargetParams, Param1, Param2);
+  }
+};
+
+/// Implements the heuristic that marks two parameters related if the same
+/// member is accessed (referred to) inside the current function's body.
+class AccessedSameMemberOf {
+  ParamToSmallSetMap<const Decl *> AccessedMembers;
+
+public:
+  explicit AccessedSameMemberOf(const FunctionDecl *FD) {
+    auto MembersCalledOnParams = match(
+        functionDecl(forEachDescendant(
+            memberExpr(hasObjectExpression(paramRefExpr())).bind("mem-expr"))),
+        *FD, FD->getASTContext());
+
+    for (const auto &Match : MembersCalledOnParams) {
+      const auto *AccessedParam = Match.getNodeAs<ParmVarDecl>("param");
+      const auto *ME = Match.getNodeAs<MemberExpr>("mem-expr");
+      assert(AccessedParam && ME);
+      AccessedMembers[AccessedParam].insert(
+          ME->getMemberDecl()->getCanonicalDecl());
+    }
+  }
+
+  bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+    return lazyMapOfSetsIntersectionExists(AccessedMembers, Param1, Param2);
+  }
+};
+
+/// Implements the heuristic that marks two parameters related if different
+/// ReturnStmts return them from the function.
+class Returned {
+  llvm::SmallVector<const ParmVarDecl *, SmallDataStructureSize> ReturnedParams;
+
+public:
+  explicit Returned(const FunctionDecl *FD) {
+    auto ParamReturns = match(functionDecl(forEachDescendant(
+                                  returnStmt(hasReturnValue(paramRefExpr())))),
+                              *FD, FD->getASTContext());
+    for (const auto &Match : ParamReturns) {
+      const auto *ReturnedParam = Match.getNodeAs<ParmVarDecl>("param");
+      assert(ReturnedParam);
+
+      if (find(FD->parameters(), ReturnedParam) == FD->param_end())
+        // Returning an Expr to a ParmVarDecl that **isn't** parameter of the
+        // function should not be possible. Make sure a mistake of the matcher
+        // does **not** interfere with the heuristic's result.
+        continue;
+
+      ReturnedParams.emplace_back(ReturnedParam);
+    }
+  }
+
+  bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+    return llvm::find(ReturnedParams, Param1) != ReturnedParams.end() &&
+           llvm::find(ReturnedParams, Param2) != ReturnedParams.end();
+  }
+};
+
+} // namespace relatedness_heuristic
+
+/// Helper class that is used to detect if two parameters of the same function
+/// are used in a similar fashion, to suppress the result.
+class SimilarlyUsedParameterPairSuppressor {
+  const bool Enabled;
+  llvm::Optional<relatedness_heuristic::AppearsInSameExpr> SameExpr;
+  llvm::Optional<relatedness_heuristic::PassedToSameFunction> PassToFun;
+  llvm::Optional<relatedness_heuristic::AccessedSameMemberOf> SameMember;
+  llvm::Optional<relatedness_heuristic::Returned> Returns;
+
+public:
+  SimilarlyUsedParameterPairSuppressor(const FunctionDecl *FD, bool Enable)
+      : Enabled(Enable) {
+    if (!Enable)
+      return;
+
+    SameExpr.emplace(FD);
+    PassToFun.emplace(FD);
+    SameMember.emplace(FD);
+    Returns.emplace(FD);
+  }
+
+  /// Returns whether the specified two parameters are deemed similarly used
+  /// or related by the heuristics.
+  bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+    if (!Enabled)
+      return false;
+
+    LLVM_DEBUG(llvm::dbgs()
+               << "::: Matching similar usage / relatedness heuristic...\n");
+
+    if (SameExpr && (*SameExpr)(Param1, Param2)) {
+      LLVM_DEBUG(llvm::dbgs() << "::: Used in the same expression.\n");
+      return true;
+    }
+
+    if (PassToFun && (*PassToFun)(Param1, Param2)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "::: Passed to same function in different calls.\n");
+      return true;
+    }
+
+    if (SameMember && (*SameMember)(Param1, Param2)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "::: Same member field access or method called.\n");
+      return true;
+    }
+
+    if (Returns && (*Returns)(Param1, Param2)) {
+      LLVM_DEBUG(llvm::dbgs() << "::: Both parameter returned.\n");
+      return true;
+    }
+
+    LLVM_DEBUG(llvm::dbgs() << "::: None.\n");
+    return false;
+  }
+};
+
+// (This function hoists the call to operator() of the wrapper, so we do not
+// need to define the previous class at the top of the file.)
+static bool
+isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,
+                         const ParmVarDecl *Param1, const ParmVarDecl *Param2) {
+  return Suppressor(Param1, Param2);
+}
+
 } // namespace filter
 
 /// Matches functions that have at least the specified amount of parameters.
 AST_MATCHER_P(FunctionDecl, parameterCountGE, unsigned, N) {
   return Node.getNumParams() >= N;
 }
 
 /// Matches *any* overloaded unary and binary operators.
EasilySwappableParametersCheck::EasilySwappableParametersCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       MinimumLength(clampMinimumLength(
           Options.get("MinimumLength", DefaultMinimumLength))),
       IgnoredParameterNames(optutils::parseStringList(
           Options.get("IgnoredParameterNames", DefaultIgnoredParameterNames))),
       IgnoredParameterTypeSuffixes(optutils::parseStringList(
           Options.get("IgnoredParameterTypeSuffixes",
-                      DefaultIgnoredParameterTypeSuffixes))) {}
+                      DefaultIgnoredParameterTypeSuffixes))),
+      SuppressParametersUsedTogether(
+          Options.get("SuppressParametersUsedTogether",
+                      DefaultSuppressParametersUsedTogether)) {}
 
 void EasilySwappableParametersCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "MinimumLength", MinimumLength);
   Options.store(Opts, "IgnoredParameterNames",
                 optutils::serializeStringList(IgnoredParameterNames));
   Options.store(Opts, "IgnoredParameterTypeSuffixes",
                 optutils::serializeStringList(IgnoredParameterTypeSuffixes));
+  Options.store(Opts, "SuppressParametersUsedTogether",
+                SuppressParametersUsedTogether);
 }
 
 void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {
   const auto BaseConstraints = functionDecl(
       // Only report for definition nodes, as fixing the issues reported
       // requires the user to be able to change code.
       isDefinition(), parameterCountGE(MinimumLength),
       unless(isOverloadedUnaryOrBinaryOperator()));
void EasilySwappableParametersCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("func");
   assert(FD);
 
   const PrintingPolicy &PP = FD->getASTContext().getPrintingPolicy();
   std::size_t NumParams = FD->getNumParams();
   std::size_t MixableRangeStartIndex = 0;
 
+  // Spawn one suppressor and if the user requested, gather information from
+  // the AST for the parameters' usages.
+  filter::SimilarlyUsedParameterPairSuppressor UsageBasedSuppressor{
+      FD, SuppressParametersUsedTogether};
+
   LLVM_DEBUG(llvm::dbgs() << "Begin analysis of " << getName(FD) << " with "
                           << NumParams << " parameters...\n");
   while (MixableRangeStartIndex < NumParams) {
     if (filter::isIgnoredParameter(*this,
                                    FD->getParamDecl(MixableRangeStartIndex))) {
       LLVM_DEBUG(llvm::dbgs()
                  << "Parameter #" << MixableRangeStartIndex << " ignored.\n");
       ++MixableRangeStartIndex;
       continue;
     }
 
-    model::MixableParameterRange R =
-        model::modelMixingRange(*this, FD, MixableRangeStartIndex);
+    model::MixableParameterRange R = model::modelMixingRange(
+        *this, FD, MixableRangeStartIndex, UsageBasedSuppressor);
     assert(R.NumParamsChecked > 0 && "Ensure forward progress!");
     MixableRangeStartIndex += R.NumParamsChecked;
     if (R.NumParamsChecked < MinimumLength) {
       LLVM_DEBUG(llvm::dbgs() << "Ignoring range of " << R.NumParamsChecked
                               << " lower than limit.\n");
       continue;
     }
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst

.. option:: IgnoredParameterTypeSuffixes
     ``bool``, ``It``, ``Iterator``, ``InputIt``, ``ForwardIt``, ``BidirIt``,
     ``RandomIt``, ``random_iterator``, ``ReverseIt``, ``reverse_iterator``,
     ``reverse_const_iterator``, ``RandomIt``, ``random_iterator``,
     ``ReverseIt``, ``reverse_iterator``, ``reverse_const_iterator``,
     ``Const_Iterator``, ``ConstIterator``.
     In addition, ``_Bool`` (but not ``_bool``) is also part of the default
     value.
 
+.. option:: SuppressParametersUsedTogether
+
+    Suppresses diagnostics about parameters that are used together or in a
+    similar fashion inside the function's body.
+    Defaults to ``true``.
+    A value of ``false`` will turn off the heuristic.
+
+    Currently, the following heuristics are implemented, either of which
+    will suppress the warning about the parameter pair involved:
+
+    * The parameters are used in the same expression, e.g. ``f(a, b)`` or
+      ``a < b``.
+    * The parameters are further passed to the same function to the same
+      parameter of that function, of the same overload.
+      E.g. ``f(a, 1)`` and ``f(b, 2)`` to some ``f(T, int)``.
+
+      .. note::
+
+        The check does not perform path-sensitive analysis, and as such,
+        "same function" in this context means the same function declaration.
+        If the same member function of a type on two distinct instances are
+        called with the parameters, it will still be regarded as
+        "same function".
+
+    * The same member field is accessed, or member method is called of the
+      two parameters, e.g. ``a.foo()`` and ``b.foo()``.
+    * Separate ``return`` statements return either of the parameters on
+      different code paths.
 
 Limitations
 -----------
 
 **This check is designed to check function signatures!**
 
 The check does not investigate functions that are generated by the compiler
 in a context that is only determined from a call site.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp

 // RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
 // RUN:   -config='{CheckOptions: [ \
 // RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
 // RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: "\"\";Foo;Bar"}, \
-// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"} \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
 // RUN:  ]}' --
 
 void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.
 
 void nothingIgnored(int I, int J) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'nothingIgnored' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
 // CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in the range is 'I'
 // CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'J'

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp

 // RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
 // RUN:   -config='{CheckOptions: [ \
 // RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
 // RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
 // RUN:  ]}' --
 
 #define assert(X) ((void)(X))
 
 void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
 
 struct S {};
 

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp

 // RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
 // RUN:   -config='{CheckOptions: [ \
 // RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
 // RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
 
 int magic(int Left, int Right, int X, int Y) { return 0; }
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters of 'magic' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
 // CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in the range is 'Left'
 // CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'Y'

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.cpp

+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 1}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 1} \
+// RUN:  ]}' --
+
+namespace std {
+template <typename T>
+T max(const T &A, const T &B);
+} // namespace std
+
+bool coin();
+void f(int);
+void g(int);
+void h(int, int);
+void i(int, bool);
+void i(int, char);
+
+struct Tmp {
+  int f(int);
+  int g(int, int);
+};
+
+struct Int {
+  int I;
+};
+
+void compare(int Left, int Right) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'compare' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'Left'
+// CHECK-MESSAGES: :[[@LINE-3]]:28: note: the last parameter in the range is 'Right'
+
+int decideSequence(int A, int B) {
+  if (A)
+    return 1;
+  if (B)
+    return 2;
+  return 3;
+}
+// CHECK-MESSAGES: :[[@LINE-7]]:20: warning: 2 adjacent parameters of 'decideSequence' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-8]]:24: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-9]]:31: note: the last parameter in the range is 'B'
+
+int myMax(int A, int B) { // NO-WARN: Appears in same expression.
+  return A < B ? A : B;
+}
+
+int myMax2(int A, int B) { // NO-WARN: Appears in same expression.
+  if (A < B)
+    return A;
+  return B;
+}
+
+int myMax3(int A, int B) { // NO-WARN: Appears in same expression.
+  return std::max(A, B);
+}
+
+int binaryToUnary(int A, int) {
+  return A;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:19: warning: 2 adjacent parameters of 'binaryToUnary' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-5]]:29: note: the last parameter in the range is '<unnamed>'
+
+int randomReturn1(int A, int B) { // NO-WARN: Appears in same expression.
+  return coin() ? A : B;
+}
+
+int randomReturn2(int A, int B) { // NO-WARN: Both parameters returned.
+  if (coin())
+    return A;
+  return B;
+}
+
+int randomReturn3(int A, int B) { // NO-WARN: Both parameters returned.
+  bool Flip = coin();
+  if (Flip)
+    return A;
+  Flip = coin();
+  if (Flip)
+    return B;
+  Flip = coin();
+  if (!Flip)
+    return 0;
+  return -1;
+}
+
+void passthrough1(int A, int B) { // WARN: Different functions, different params.
+  f(A);
+  g(B);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough1' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough2(int A, int B) { // NO-WARN: Passed to same index of same function.
+  f(A);
+  f(B);
+}
+
+void passthrough3(int A, int B) { // NO-WARN: Passed to same index of same funtion.
+  h(1, A);
+  h(1, B);
+}
+
+void passthrough4(int A, int B) { // WARN: Different index used.
+  h(1, A);
+  h(B, 2);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough4' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough5(int A, int B) { // WARN: Different function overload.
+  i(A, false);
+  i(A, '\0');
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough5' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough6(int A, int B) { // NO-WARN: Passed to same index of same function.
+  Tmp Temp;
+  Temp.f(A);
+  Temp.f(B);
+}
+
+void passthrough7(int A, int B) { // NO-WARN: Passed to same index of same function.
+  // Clang-Tidy isn't path sensitive, the fact that the two objects we call the
+  // function on is different is not modelled.
+  Tmp Temp1, Temp2;
+  Temp1.f(A);
+  Temp2.f(B);
+}
+
+void passthrough8(int A, int B) { // WARN: Different functions used.
+  f(A);
+  Tmp{}.f(B);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough8' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+// Test that the matching of "passed-to-function" is done to the proper node.
+// Put simply, this test should not crash here.
+void forwardDeclared(int X);
+
+void passthrough9(int A, int B) { // NO-WARN: Passed to same index of same function.
+  forwardDeclared(A);
+  forwardDeclared(B);
+}
+
+void forwardDeclared(int X) {}
+
+void passthrough10(int A, int B) { // NO-WARN: Passed to same index of same function.
+  forwardDeclared(A);
+  forwardDeclared(B);
+}
+
+bool compare1(Int I, Int J) { // NO-WARN: Same member accessed.
+  int Val1 = I.I;
+  int Val2 = J.I;
+  return Val1 < Val2;
+}
+
+bool compare2(Tmp T1, Tmp T2) { // NO-WARN: Same member accessed.
+  int Val1 = T1.g(0, 1);
+  int Val2 = T2.g(2, 3);
+  return Val1 < Val2;
+}
+
+bool compare3(Tmp T1, Tmp T2) { // WARN: Different member accessed.
+  int Val1 = T1.f(0);
+  int Val2 = T2.g(1, 2);
+  return Val1 < Val2;
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:15: warning: 2 adjacent parameters of 'compare3' of similar type ('Tmp')
+// CHECK-MESSAGES: :[[@LINE-6]]:19: note: the first parameter in the range is 'T1'
+// CHECK-MESSAGES: :[[@LINE-7]]:27: note: the last parameter in the range is 'T2'
+
+int rangeBreaker(int I, int J, int K, int L, int M, int N) {
+  // (I, J) swappable.
+
+  if (J == K) // (J, K) related.
+    return -1;
+
+  if (K + 2 > Tmp{}.f(K))
+    return M;
+
+  // (K, L, M) swappable.
+
+  return N; // (M, N) related.
+}
+// CHECK-MESSAGES: :[[@LINE-13]]:18: warning: 2 adjacent parameters of 'rangeBreaker' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-14]]:22: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-15]]:29: note: the last parameter in the range is 'J'
+// CHECK-MESSAGES: :[[@LINE-16]]:32: warning: 3 adjacent parameters of 'rangeBreaker' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-17]]:36: note: the first parameter in the range is 'K'
+// CHECK-MESSAGES: :[[@LINE-18]]:50: note: the last parameter in the range is 'M'