[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with `typedef` and `const &` diagnostics

[whisperity]

Summary

The base patch only deals with strict (canonical) type equality, which is merely a subset of all the dangerous function interfaces that we intend to find. In addition, in the base patch, canonical type equivalence is not diagnosed in a way that is immediately apparent to the user.

This patch extends the check with two features:

1. Proper typedef diagnostics and explanations to the user.
2. "Reference bind power" matching.

Case 1 will tell the user the common type of "SomeIntegerType" and "SomeOtherIntegerType" is "int" or "my_int" and "int" are the same. This makes the (otherwise, through CanonicalType desugaring already handled and diagnosed, but not explained) typedef case obvious to the user without having to invoke code comprehension tools.

Case 2 is a necessary addition because in every case someone encounters a function f(T t, const T& tr), any expression that might be passed to either can be passed to both. Thus, such adjacent parameter sequences should be matched. This is specifically modelled for const& only, as any other pair combination from "value, ref, qualified ref, rref" have some broad value category in which some actual swap of the arguments at a call would be a compiler error. (E.g. f(int, int&) and f(2, Val) breaks if swapped, similarly how rrefs cannot bind lvalues, etc.) This case broadens the result set, but only with the most dangerous and "Please think about this and refactor..." case.

(Note, that Case 2 does not model things like f(int, std::reference_wrapper<const int>).)

Due to the cases above always being a problem (and there isn't a good enough reason to say typedefs should not be diagnosed, everyone hates the weak typedef...), I decided not to make matching these configurable.

This patch also serves as a proof-of-concept on how I wish the check to evolve over some more subsequent patches, which all aim to introduce user-toggleable modelling.

Originally, the implementation of this feature was part of the very first patch related to this check, D69560 (diff 259320). It has been factored out for clarity and to allow more on-point discussion.

[aaron.ballman]

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

153–177

281–293

"Dissolve certain type sugars" has to be one of my new favorite comments. :-D

284

289

307–308

I think this is reasonable but it does raise a question about implicit conversions, which I suppose is more of a generic question about the check. Should the check consider these to be easily swappable parameter types?

struct S {
  S(int);
  operator int() const;
};

void func(S s, int i);

given that you can call func() like:

S s;
int i;

func(i, s);
func(s, i);

(Also, does the answer change if S has more types it can convert to/from? Or does the answer change if S can only convert from a type (or convert to a type) rather than convert both ways?)

324

It's not really unqualified since it could still have volatile (etc).

522–523

528–529

Might as well hoist this to be above the public access specifier (and can drop the private access specifier entirely at that point).

[whisperity]

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

153–177

How in the... Okay, I think I need to up my rebase game because I know for a fact that I fixed BrE to AmE in the parent patch and that patch was merged into the branch this patch was created from and this patch is created against the parent, obviously.

284

Is const respected properly in this case, will it not try to cast it away? Wow... I did not know this!

307–308

Yes, yes, yes and yes! That is the whole idea, that is one of the things in which we are "better" than the existing works... and one of the reasons behind this whole "argument swapping" deal is much worse in C++, than let's say Java. (A lot of existing literature deals with Java, or deals with C++ but only in terms of type equivalence, or not considering types -- only arg and param names -- at all.) That one shall be diagnosed, but the whole implicit conversion "mess" is coming in a later patch!
In fact, the patch for implicit conversions is already up, but in a will be reworked state.

Unfortunately, I can't seem to find a way to link a particular line of a particular diff, so let's do it manually. The ...-implicits.cpp test file in D75041 diff 315384 starting from line 95 shows this being dealt with.

Originally, implicit conversions were warned about when they were only unidirectional (i.e. int -> S possible, S -> int not possible, fn(int{}, S{}) warned). However, this created a lot of false positives even in the two or three projects I went over manually, so... for the time being, I reworked the patch (only changing like 2 lines at most!) to only deal with bidirectional implicit conversions. Because in case you say fn(5, 5) it still accepts, but if you have a valid s, fn(s, 5) is a compile error.
(You can check the earlier diff at the same location for this case.)

This becomes a deep rabbit hole way too fast, because one layer of the analysis is "warn about f(T1, T2) if f(T2, T1) is also possible". That's the idea I'm trying to sell right now. Saying "warn about f(T1, T2) if there exists some type Tx with which f(Tx, Tx) is possible" is a whooole lotta harder to handle. Maybe not impossible, but definitely harder, and with most likely introducing a plethora of weird false positives.

307–308

And once implicit conversions are in, a function definition like fn(int, const S&) will say that it can be mixed up because S::S(int) is a thing and then an S{} can be passed badly to a const S&.

There is also another, currently not yet uploaded part of the patch, which deals with whether fn(int, const int) (note no &!) should be deemed mixable. That one will most likely be behind a user configurable option... Is memcpy(void* dest, const void* source) mixable? Not if you have const void* as a local variable... Which, given the fact that you told me in D69560 that I should drop the const from local variables... means that I'll make the mistake of going memcpy(src, dst) more easily, than if I kept my consts on my locals. But this is some sort of a more philosophical question. C++CG says const T, T is not to be warned about. I say it's still an issue as every non-const implicitly converts to const. But that's why it's best to leave it up for the user to decide.

376

@aaron.ballman Should I say "invalid" here, or "sentinel" here? (Cf. the assert in sanitize().) It's more of a sentinel value... which is also invalid when you want to consider it a valid enum constant to be used for the intended purpose. Full 0...0 bit pattern means someone somewhere messed up great deal. 0...01 (little endian) bit pattern means we concluded there isn't a possible mix. Etc. with the other bits.

[whisperity]

• NFC Code style, formatting, wording, etc. changes as per review comments.
• NFC Removed a stale FIXME

[whisperity]

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

[aaron.ballman]

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

735–736

I think "with the same force" is going to be hard for users to make sense of. I'm at a bit of a loss for how to word it though. The issue is that T and const T& parameters *might* be easily swapped, so maybe it's best to call it out that way?

[whisperity]

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

734

This is a stale FIXME, the uniqueing of the emission is in the line right above...

735–736

This is a note tag to explain the reason behind the mix. The warning itself has been emitted before. How about

a T and const T& parameter accepts all values of T

?

[aaron.ballman]

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

734

Heh, I was sort of wondering about that. Please remove the FIXME before committing.

735–736

I think that'd be significantly more clear, yes!

[whisperity]

• Made the diagnostic message about "binding power" easier to understand.

[aaron.ballman]

LGTM!

[whisperity]

[NFC] Rebase.

[whisperity]

NFC: Rebase & update.

[whisperity]

NFC Fix lint.

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/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/SmallSet.h"
 
 #define DEBUG_TYPE "EasilySwappableParametersCheck"
 #include "llvm/Support/Debug.h"
 
 namespace optutils = clang::tidy::utils::options;
 
 /// The default value for the MinimumLength check option.
 static constexpr std::size_t DefaultMinimumLength = 2;

 } // namespace filter
 
 namespace model {
 
 /// The language features involved in allowing the mix between two parameters.
 enum class MixFlags : unsigned char {
   Invalid = 0, //< Sentinel bit pattern. DO NOT USE!
 
-  None = 1,      //< Mix between the two parameters is not possible.
-  Trivial = 2,   //< The two mix trivially, and are the exact same type.
-  Canonical = 4, //< The two mix because the types refer to the same
+  None = 1,           //< Mix between the two parameters is not possible.
+  Trivial = 2,        //< The two mix trivially, and are the exact same type.
+  Canonical = 4,      //< The two mix because the types refer to the same
                       // CanonicalType, but we do not elaborate as to how.
+  TypeAlias = 8,      //< The path from one type to the other involves
+                      // desugaring type aliases.
+  ReferenceBind = 16, //< The mix involves the binding power of "const &".
 
-  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/Canonical)
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/ReferenceBind)
 };
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 /// Returns whether the SearchedFlag is turned on in the Data.
 static inline bool hasFlag(MixFlags Data, MixFlags SearchedFlag) {
   assert(SearchedFlag != MixFlags::Invalid &&
          "can't be used to detect lack of all bits!");
 
   // "Data & SearchedFlag" would need static_cast<bool>() in conditions.
   return (Data & SearchedFlag) == SearchedFlag;
 }
 
 #ifndef NDEBUG
 
 // The modelling logic of this check is more complex than usual, and
 // potentially hard to understand without the ability to see into the
 // representation during the recursive descent. This debug code is only
 // compiled in 'Debug' mode, or if LLVM_ENABLE_ASSERTIONS config is turned on.
 
 /// Formats the MixFlags enum into a useful, user-readable representation.
 static inline std::string formatMixFlags(MixFlags F) {
   if (F == MixFlags::Invalid)
     return "#Inv!";
 
-  SmallString<4> Str{"---"};
+  SmallString<8> Str{"-----"};
 
   if (hasFlag(F, MixFlags::None))
     // Shows the None bit explicitly, as it can be applied in the recursion
     // even if other bits are set.
     Str[0] = '!';
   if (hasFlag(F, MixFlags::Trivial))
     Str[1] = 'T';
   if (hasFlag(F, MixFlags::Canonical))
     Str[2] = 'C';
+  if (hasFlag(F, MixFlags::TypeAlias))
+    Str[3] = 't';
+  if (hasFlag(F, MixFlags::ReferenceBind))
+    Str[4] = '&';
 
   return Str.str().str();
 }
 
 #else
 
 static inline std::string formatMixFlags(MixFlags F);
 
 #endif // NDEBUG
 
 /// Contains the metadata for the mixability result between two types,
 /// independently of which parameters they were calculated from.
 struct MixData {
+  /// The flag bits of the mix indicating what language features allow for it.
   MixFlags Flags;
 
+  /// A potentially calculated common underlying type after desugaring, that
+  /// both sides of the mix can originate from.
+  QualType CommonType;
+
   MixData(MixFlags Flags) : Flags(Flags) {}
+  MixData(MixFlags Flags, QualType CommonType)
+      : Flags(Flags), CommonType(CommonType) {}
 
   void sanitize() {
     assert(Flags != MixFlags::Invalid && "sanitize() called on invalid bitvec");
-    // TODO: There will be statements here in further extensions of the check.
+
+    if (hasFlag(Flags, MixFlags::None)) {
+      // If anywhere down the recursion a potential mix "path" is deemed
+      // impossible, throw away all the other bits because the mix is not
+      // possible.
+      Flags = MixFlags::None;
+      return;
+    }
+
+    if (Flags == MixFlags::Trivial)
+      return;
+
+    if (static_cast<bool>(Flags ^ MixFlags::Trivial))
+      // If the mix involves somewhere trivial equivalence but down the
+      // recursion other bit(s) were set, remove the trivial bit, as it is not
+      // trivial.
+      Flags &= ~MixFlags::Trivial;
+  }
+
+  MixData operator|(MixFlags EnableFlags) const {
+    return {Flags | EnableFlags, CommonType};
+  }
+  MixData &operator|=(MixFlags EnableFlags) {
+    Flags |= EnableFlags;
+    return *this;
   }
 };
 
 /// A named tuple that contains the information for a mix between two concrete
 /// parameters.
 struct Mix {
   const ParmVarDecl *First, *Second;
   MixData Data;
 
   Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data)
       : First(F), Second(S), Data(std::move(Data)) {}
 
   void sanitize() { Data.sanitize(); }
   MixFlags flags() const { return Data.Flags; }
+  QualType commonUnderlyingType() const { return Data.CommonType; }
 };
 
 // NOLINTNEXTLINE(misc-redundant-expression): Seems to be a bogus warning.
 static_assert(std::is_trivially_copyable<Mix>::value &&
                   std::is_trivially_move_constructible<Mix>::value &&
                   std::is_trivially_move_assignable<Mix>::value,
               "Keep frequently used data simple!");
 
const ParmVarDecl *getLastParam() const {
     // finds something that can not be mixed with the rest, by going *forward*
     // in the list of parameters. So at any moment of break, the RHS of the last
     // element of the mix vector is also the last element of the mixing range.
     assert(!Mixes.empty());
     return Mixes.back().Second;
   }
 };
 
+static MixData isLRefEquallyBindingToType(const TheCheck &Check,
+                                          const LValueReferenceType *LRef,
+                                          QualType Ty, const ASTContext &Ctx,
+                                          bool IsRefRHS);
+
 /// Approximate the way how LType and RType might refer to "essentially the
 /// same" type, in a sense that at a particular call site, an expression of
 /// type LType and RType might be successfully passed to a variable (in our
 /// specific case, a parameter) of type RType and LType, respectively.
 /// Note the swapped order!
 ///
 /// The returned data structure is not guaranteed to be properly set, as this
 /// function is potentially recursive. It is the caller's responsibility to
 /// call sanitize() on the result once the recursion is over.
 static MixData calculateMixability(const TheCheck &Check, const QualType LType,
                                    const QualType RType,
                                    const ASTContext &Ctx) {
   LLVM_DEBUG(llvm::dbgs() << ">>> calculateMixability for LType:\n";
              LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n";
              RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
 
   if (LType == RType) {
     LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Trivial equality.\n");
-    return {MixFlags::Trivial};
+    return {MixFlags::Trivial, LType};
   }
 
-  // TODO: Implement more elaborate logic, such as typedef, implicit
-  // conversions, etc.
+  // Dissolve certain type sugars that do not affect the mixability of one type
+  // with the other, and also do not require any sort of elaboration for the
+  // user to understand.
+  if (isa<ParenType>(LType.getTypePtr())) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is ParenType.\n");
+    return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),
+                               RType, Ctx);
+  }
+  if (isa<ParenType>(RType.getTypePtr())) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is ParenType.\n");
+    return calculateMixability(Check, LType,
+                               RType.getSingleStepDesugaredType(Ctx), Ctx);
+  }
+
+  // Dissolve typedefs.
+  if (const auto *LTypedef = LType->getAs<TypedefType>()) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n");
+    return calculateMixability(Check, LTypedef->desugar(), RType, Ctx) |
+           MixFlags::TypeAlias;
+  }
+  if (const auto *RTypedef = RType->getAs<TypedefType>()) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is typedef.\n");
+    return calculateMixability(Check, LType, RTypedef->desugar(), Ctx) |
+           MixFlags::TypeAlias;
+  }
+
+  // At a particular call site, what could be passed to a 'T' or 'const T' might
+  // also be passed to a 'const T &' without the call site putting a direct
+  // side effect on the passed expressions.
+  if (const auto *LRef = LType->getAs<LValueReferenceType>()) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is &.\n");
+    return isLRefEquallyBindingToType(Check, LRef, RType, Ctx, false) |
+           MixFlags::ReferenceBind;
+  }
+  if (const auto *RRef = RType->getAs<LValueReferenceType>()) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is &.\n");
+    return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true) |
+           MixFlags::ReferenceBind;
+  }
 
   // If none of the previous logic found a match, try if Clang otherwise
   // believes the types to be the same.
   if (LType.getCanonicalType() == RType.getCanonicalType()) {
     LLVM_DEBUG(llvm::dbgs()
                << "<<< calculateMixability. Same CanonicalType.\n");
-    return {MixFlags::Canonical};
+    return {MixFlags::Canonical, LType.getCanonicalType()};
   }
 
   LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. No match found.\n");
   return {MixFlags::None};
 }
 
+/// Calculates if the reference binds an expression of the given type. This is
+/// true iff 'LRef' is some 'const T &' type, and the 'Ty' is 'T' or 'const T'.
+static MixData isLRefEquallyBindingToType(const TheCheck &Check,
+                                          const LValueReferenceType *LRef,
+                                          QualType Ty, const ASTContext &Ctx,
+                                          bool IsRefRHS) {
+  LLVM_DEBUG(llvm::dbgs() << ">>> isLRefEquallyBindingToType for LRef:\n";
+             LRef->dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand Type:\n";
+             Ty.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
+
+  QualType ReferredType = LRef->getPointeeType();
+  if (!ReferredType.isLocalConstQualified()) {
+    LLVM_DEBUG(llvm::dbgs()
+               << "<<< isLRefEquallyBindingToType. Not const ref.\n");
+    return {MixFlags::None};
+  };
+
+  QualType NonConstReferredType = ReferredType;
+  NonConstReferredType.removeLocalConst();
+  if (ReferredType == Ty || NonConstReferredType == Ty) {
+    LLVM_DEBUG(
+        llvm::dbgs()
+        << "<<< isLRefEquallyBindingToType. Type of referred matches.\n");
+    return {MixFlags::Trivial, ReferredType};
+  }
+
+  LLVM_DEBUG(
+      llvm::dbgs()
+      << "--- isLRefEquallyBindingToType. Checking mix for underlying type.\n");
+  return IsRefRHS ? calculateMixability(Check, Ty, NonConstReferredType, Ctx)
+                  : calculateMixability(Check, NonConstReferredType, Ty, Ctx);
+}
+
 static MixableParameterRange modelMixingRange(const TheCheck &Check,
                                               const FunctionDecl *FD,
                                               std::size_t StartIndex) {
   std::size_t NumParams = FD->getNumParams();
   assert(StartIndex < NumParams && "out of bounds for start");
   const ASTContext &Ctx = FD->getASTContext();
 
   MixableParameterRange Ret;
for (std::size_t J = StartIndex; J < I; ++J) {
       Mix M{Jth, Ith,
             calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx)};
       LLVM_DEBUG(llvm::dbgs() << "Mix flags (raw)           : "
                               << formatMixFlags(M.flags()) << '\n');
       M.sanitize();
       LLVM_DEBUG(llvm::dbgs() << "Mix flags (after sanitize): "
                               << formatMixFlags(M.flags()) << '\n');
 
       assert(M.flags() != MixFlags::Invalid && "All flags decayed!");
 
       if (M.flags() != MixFlags::None)
         MixesOfIth.emplace_back(std::move(M));
     }
 
     if (MixesOfIth.empty()) {
       // If there weren't any new mixes stored for Ith, the range is
       // [Start, ..., I].

 /// Returns the diagnostic-friendly name of the node, or a constant value.
 static SmallString<64> getNameOrUnnamed(const NamedDecl *ND) {
   auto Name = getName(ND);
   if (Name.empty())
     Name = "<unnamed>";
   return Name;
 }
 
+/// Returns whether a particular Mix between two parameters should have the
+/// types involved diagnosed to the user. This is only a flag check.
+static inline bool needsToPrintTypeInDiagnostic(const model::Mix &M) {
+  return static_cast<bool>(M.flags() & (model::MixFlags::TypeAlias |
+                                        model::MixFlags::ReferenceBind));
+}
+
+namespace {
+
+/// Retains the elements called with and returns whether the call is done with
+/// a new element.
+template <typename E, std::size_t N> class InsertOnce {
+  llvm::SmallSet<E, N> CalledWith;
+
+public:
+  bool operator()(E El) { return CalledWith.insert(std::move(El)).second; }
+
+  bool calledWith(const E &El) const { return CalledWith.contains(El); }
+};
+
+struct SwappedEqualQualTypePair {
+  QualType LHSType, RHSType;
+
+  bool operator==(const SwappedEqualQualTypePair &Other) const {
+    return (LHSType == Other.LHSType && RHSType == Other.RHSType) ||
+           (LHSType == Other.RHSType && RHSType == Other.LHSType);
+  }
+
+  bool operator<(const SwappedEqualQualTypePair &Other) const {
+    return LHSType < Other.LHSType && RHSType < Other.RHSType;
+  }
+};
+
+struct TypeAliasDiagnosticTuple {
+  QualType LHSType, RHSType, CommonType;
+
+  bool operator==(const TypeAliasDiagnosticTuple &Other) const {
+    return CommonType == Other.CommonType &&
+           ((LHSType == Other.LHSType && RHSType == Other.RHSType) ||
+            (LHSType == Other.RHSType && RHSType == Other.LHSType));
+  }
+
+  bool operator<(const TypeAliasDiagnosticTuple &Other) const {
+    return CommonType < Other.CommonType && LHSType < Other.LHSType &&
+           RHSType < Other.RHSType;
+  }
+};
+
+/// Helper class to only emit a diagnostic related to MixFlags::TypeAlias once.
+class UniqueTypeAliasDiagnosticHelper
+    : public InsertOnce<TypeAliasDiagnosticTuple, 8> {
+  using Base = InsertOnce<TypeAliasDiagnosticTuple, 8>;
+
+public:
+  /// Returns whether the diagnostic for LHSType and RHSType which are both
+  /// referring to CommonType being the same has not been emitted already.
+  bool operator()(QualType LHSType, QualType RHSType, QualType CommonType) {
+    if (CommonType.isNull() || CommonType == LHSType || CommonType == RHSType)
+      return Base::operator()({LHSType, RHSType, {}});
+
+    TypeAliasDiagnosticTuple ThreeTuple{LHSType, RHSType, CommonType};
+    if (!Base::operator()(ThreeTuple))
+      return false;
+
+    bool AlreadySaidLHSAndCommonIsSame = calledWith({LHSType, CommonType, {}});
+    bool AlreadySaidRHSAndCommonIsSame = calledWith({RHSType, CommonType, {}});
+    if (AlreadySaidLHSAndCommonIsSame && AlreadySaidRHSAndCommonIsSame) {
+      // "SomeInt == int" && "SomeOtherInt == int" => "Common(SomeInt,
+      // SomeOtherInt) == int", no need to diagnose it. Save the 3-tuple only
+      // for shortcut if it ever appears again.
+      return false;
+    }
+
+    return true;
+  }
+};
+
+} // namespace
+
 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(
void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(BaseConstraints, isExplicitTemplateSpecialization())
           .bind("func"),
       this);
 }
 
 void EasilySwappableParametersCheck::check(
     const MatchFinder::MatchResult &Result) {
+  using namespace model;
+  using namespace filter;
+
   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;
 
   LLVM_DEBUG(llvm::dbgs() << "Begin analysis of " << getName(FD) << " with "
                           << NumParams << " parameters...\n");
   while (MixableRangeStartIndex < NumParams) {
-    if (filter::isIgnoredParameter(*this,
-                                   FD->getParamDecl(MixableRangeStartIndex))) {
+    if (isIgnoredParameter(*this, FD->getParamDecl(MixableRangeStartIndex))) {
       LLVM_DEBUG(llvm::dbgs()
                  << "Parameter #" << MixableRangeStartIndex << " ignored.\n");
       ++MixableRangeStartIndex;
       continue;
     }
 
-    model::MixableParameterRange R =
-        model::modelMixingRange(*this, FD, MixableRangeStartIndex);
+    MixableParameterRange R =
+        modelMixingRange(*this, FD, MixableRangeStartIndex);
     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;
     }
 
+    bool NeedsAnyTypeNote = llvm::any_of(R.Mixes, needsToPrintTypeInDiagnostic);
     const ParmVarDecl *First = R.getFirstParam(), *Last = R.getLastParam();
     std::string FirstParamTypeAsWritten = First->getType().getAsString(PP);
     {
-      StringRef DiagText = "%0 adjacent parameters of %1 of similar type "
-                           "('%2') are easily swapped by mistake";
-      // TODO: This logic will get extended here with future flags.
+      StringRef DiagText;
+
+      if (NeedsAnyTypeNote)
+        DiagText = "%0 adjacent parameters of %1 of similar type are easily "
+                   "swapped by mistake";
+      else
+        DiagText = "%0 adjacent parameters of %1 of similar type ('%2') are "
+                   "easily swapped by mistake";
 
       auto Diag = diag(First->getOuterLocStart(), DiagText)
-                  << static_cast<unsigned>(R.NumParamsChecked) << FD
-                  << FirstParamTypeAsWritten;
+                  << static_cast<unsigned>(R.NumParamsChecked) << FD;
+      if (!NeedsAnyTypeNote)
+        Diag << FirstParamTypeAsWritten;
 
       CharSourceRange HighlightRange = CharSourceRange::getTokenRange(
           First->getBeginLoc(), Last->getEndLoc());
       Diag << HighlightRange;
     }
 
     // There is a chance that the previous highlight did not succeed, e.g. when
     // the two parameters are on different lines. For clarity, show the user
     // the involved variable explicitly.
     diag(First->getLocation(), "the first parameter in the range is '%0'",
          DiagnosticIDs::Note)
         << getNameOrUnnamed(First)
         << CharSourceRange::getTokenRange(First->getLocation(),
                                           First->getLocation());
     diag(Last->getLocation(), "the last parameter in the range is '%0'",
          DiagnosticIDs::Note)
         << getNameOrUnnamed(Last)
         << CharSourceRange::getTokenRange(Last->getLocation(),
                                           Last->getLocation());
+
+    // Helper classes to silence elaborative diagnostic notes that would be
+    // too verbose.
+    UniqueTypeAliasDiagnosticHelper UniqueTypeAlias;
+    InsertOnce<SwappedEqualQualTypePair, 8> UniqueBindPower;
+
+    for (const Mix &M : R.Mixes) {
+      assert(M.flags() >= MixFlags::Trivial &&
+             "Sentinel or false mix in result.");
+
+      if (needsToPrintTypeInDiagnostic(M)) {
+        // Typedefs might result in the type of the variable needing to be
+        // emitted to a note diagnostic, so prepare it.
+        const ParmVarDecl *LVar = M.First;
+        const ParmVarDecl *RVar = M.Second;
+        QualType LType = LVar->getType();
+        QualType RType = RVar->getType();
+        QualType CommonType = M.commonUnderlyingType();
+        std::string LTypeAsWritten = LType.getAsString(PP);
+        std::string RTypeAsWritten = RType.getAsString(PP);
+        std::string CommonTypeStr = CommonType.getAsString(PP);
+
+        if (hasFlag(M.flags(), MixFlags::TypeAlias) &&
+            UniqueTypeAlias(LType, RType, CommonType)) {
+          StringRef DiagText;
+          bool ExplicitlyPrintCommonType = false;
+          if (LTypeAsWritten == CommonTypeStr ||
+              RTypeAsWritten == CommonTypeStr)
+            DiagText =
+                "after resolving type aliases, '%0' and '%1' are the same";
+          else {
+            DiagText = "after resolving type aliases, the common type of '%0' "
+                       "and '%1' is '%2'";
+            ExplicitlyPrintCommonType = true;
+          }
+
+          auto Diag =
+              diag(LVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note)
+              << LTypeAsWritten << RTypeAsWritten;
+          if (ExplicitlyPrintCommonType)
+            Diag << CommonTypeStr;
+        }
+
+        if (hasFlag(M.flags(), MixFlags::ReferenceBind) &&
+            UniqueBindPower({LType, RType})) {
+          StringRef DiagText = "'%0' and '%1' parameters accept and bind the "
+                               "same kind of values";
+          diag(RVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note)
+              << LTypeAsWritten << RTypeAsWritten;
+        }
+      }
+    }
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

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

.. code-block:: c++
     int add(T X, U Y) { return X + Y };
 
     void TheseAreNotWarnedAbout() {
         printf("%d %d\n", 1, 2);   // Two ints passed, they could be swapped.
         someOldCFunction(1, 2, 3); // Similarly, multiple ints passed.
 
         add(1, 2); // Instantiates 'add<int, int>', but that's not a user-defined function.
     }
+
+Due to the limitation above, parameters which type are further dependent upon
+template instantiations to *prove* that they mix with another parameter's is
+not diagnosed.
+
+.. code-block:: c++
+
+    template <typename T>
+    struct Vector {
+      typedef T element_type;
+    };
+
+    // Diagnosed: Explicit instantiation was done by the user, we can prove it
+    // is the same type.
+    void Explicit(int A, Vector<int>::element_type B) { /* ... */ }
+
+    // Diagnosed: The two parameter types are exactly the same.
+    template <typename T>
+    void Exact(typename Vector<T>::element_type A,
+               typename Vector<T>::element_type B) { /* ... */ }
+
+    // Skipped: The two parameters are both 'T' but we can not prove this
+    // without actually instantiating.
+    template <typename T>
+    void FalseNegative(T A, typename Vector<T>::element_type B) { /* ... */ }

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

 // CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'I1'
 // CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'I2'
 
 void typedefAndTypedef2(MyInt2 I1, MyInt2 I2) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'typedefAndTypedef2' of similar type ('MyInt2')
 // CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'I1'
 // CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'I2'
 
-void throughTypedef(int I, MyInt1 J) {}
-// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'throughTypedef' of similar type ('int')
-// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in the range is 'I'
-// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in the range is 'J'
-
-void betweenTypedef(MyInt1 I, MyInt2 J) {}
-// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'betweenTypedef' of similar type ('MyInt1')
-// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'I'
-// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in the range is 'J'
+void typedefMultiple(MyInt1 I1, MyInt2 I2x, MyInt2 I2y) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 adjacent parameters of 'typedefMultiple' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I1'
+// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the last parameter in the range is 'I2y'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: after resolving type aliases, the common type of 'MyInt1' and 'MyInt2' is 'int'
+
+void throughTypedef1(int I, MyInt1 J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent parameters of 'throughTypedef1' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:26: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:36: note: the last parameter in the range is 'J'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: after resolving type aliases, 'int' and 'MyInt1' are the same
+
+void betweenTypedef2(MyInt1 I, MyInt2 J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent parameters of 'betweenTypedef2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:39: note: the last parameter in the range is 'J'
+// CHECK-MESSAGES: :[[@LINE-4]]:22: note: after resolving type aliases, the common type of 'MyInt1' and 'MyInt2' is 'int'
+
+typedef MyInt2 MyInt2b;
+
+void typedefChain(int I, MyInt1 MI1, MyInt2 MI2, MyInt2b MI2b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 4 adjacent parameters of 'typedefChain' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:23: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:58: note: the last parameter in the range is 'MI2b'
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: after resolving type aliases, 'int' and 'MyInt1' are the same
+// CHECK-MESSAGES: :[[@LINE-5]]:19: note: after resolving type aliases, 'int' and 'MyInt2' are the same
+// CHECK-MESSAGES: :[[@LINE-6]]:19: note: after resolving type aliases, 'int' and 'MyInt2b' are the same
 
 typedef long MyLong1;
 using MyLong2 = long;
 
-void throughTypedefToOtherType(MyInt1 I, MyLong1 J) {} // NO-WARN: Not the same type.
+void throughTypedefToOtherType(MyInt1 I, MyLong1 J) {} // NO-WARN: int and long.
 
 void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
 
 void qualified2(int I, volatile int VI) {} // NO-WARN: Not the same type.
 
 void qualified3(int *IP, const int *CIP) {} // NO-WARN: Not the same type.
 
 void qualified4(const int CI, const long CL) {} // NO-WARN: Not the same type.
 
 using CInt = const int;
 
 void qualifiedThroughTypedef1(int I, CInt CI) {} // NO-WARN: Not the same type.
 
-void qualifiedThroughTypedef2(CInt CI1, const int CI2) {} // NO-WARN: Not the same type.
-// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 2 adjacent parameters of 'qualifiedThroughTypedef2' of similar type ('CInt')
+void qualifiedThroughTypedef2(CInt CI1, const int CI2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 2 adjacent parameters of 'qualifiedThroughTypedef2' of similar type are
 // CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'CI1'
 // CHECK-MESSAGES: :[[@LINE-3]]:51: note: the last parameter in the range is 'CI2'
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: after resolving type aliases, 'CInt' and 'const int' are the same
 
-void reference1(int I, int &IR) {} // NO-WARN: Not the same type.
+void qualifiedThroughTypedef3(CInt CI1, const MyInt1 CI2, const int CI3) {} // NO-WARN: Not the same type.
 
-void reference2(int I, const int &CIR) {} // NO-WARN: Not the same type.
-
-void reference3(int I, int &&IRR) {} // NO-WARN: Not the same type.
-
-void reference4(int I, const int &&CIRR) {} // NO-WARN: Not the same type.
+void qualifiedThroughTypedef4(CInt CI1, const MyInt1 CI2, const MyInt2 CI3) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:41: warning: 2 adjacent parameters of 'qualifiedThroughTypedef4' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:54: note: the first parameter in the range is 'CI2'
+// CHECK-MESSAGES: :[[@LINE-3]]:72: note: the last parameter in the range is 'CI3'
+// CHECK-MESSAGES: :[[@LINE-4]]:41: note: after resolving type aliases, the common type of 'const MyInt1' and 'const MyInt2' is 'int'
+
+void reference1(int I, int &IR) {} // NO-WARN: Distinct semantics when called.
+
+void reference2(int I, const int &CIR) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'reference2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in the range is 'CIR'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: 'int' and 'const int &' parameters accept and bind the same kind of values
+
+void reference3(int I, int &&IRR) {} // NO-WARN: Distinct semantics when called.
+
+void reference4(int I, const int &&CIRR) {} // NO-WARN: Distinct semantics when called.
+
+void reference5(const int CI, const int &CIR) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'reference5' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in the range is 'CI'
+// CHECK-MESSAGES: :[[@LINE-3]]:42: note: the last parameter in the range is 'CIR'
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: 'const int' and 'const int &' parameters accept and bind the same kind of values
+
+void reference6(int I, const int &CIR, int J, const int &CJR) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 4 adjacent parameters of 'reference6' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:58: note: the last parameter in the range is 'CJR'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: 'int' and 'const int &' parameters accept and bind the same kind of values
+
+using ICRTy = const int &;
+using MyIntCRTy = const MyInt1 &;
+
+void referenceThroughTypedef(int I, ICRTy Builtin, MyIntCRTy MyInt) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: 3 adjacent parameters of 'referenceThroughTypedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:34: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:62: note: the last parameter in the range is 'MyInt'
+// CHECK-MESSAGES: :[[@LINE-4]]:30: note: after resolving type aliases, the common type of 'int' and 'ICRTy' is 'const int'
+// CHECK-MESSAGES: :[[@LINE-5]]:37: note: 'int' and 'ICRTy' parameters accept and bind the same kind of values
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: after resolving type aliases, 'int' and 'MyIntCRTy' are the same
+// CHECK-MESSAGES: :[[@LINE-7]]:52: note: 'int' and 'MyIntCRTy' parameters accept and bind the same kind of values
+// CHECK-MESSAGES: :[[@LINE-8]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'int'
+// CHECK-MESSAGES: :[[@LINE-9]]:52: note: 'ICRTy' and 'MyIntCRTy' parameters accept and bind the same kind of values
+
+short const typedef int unsigned Eldritch;
+typedef const unsigned short Holy;
+
+void collapse(Eldritch Cursed, Holy Blessed) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'collapse' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'Cursed'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in the range is 'Blessed'
+// CHECK-MESSAGES: :[[@LINE-4]]:15: note: after resolving type aliases, the common type of 'Eldritch' and 'Holy' is 'const unsigned short'
+
+void collapseAndTypedef(Eldritch Cursed, const Holy &Blessed) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'collapseAndTypedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:34: note: the first parameter in the range is 'Cursed'
+// CHECK-MESSAGES: :[[@LINE-3]]:54: note: the last parameter in the range is 'Blessed'
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: after resolving type aliases, the common type of 'Eldritch' and 'const Holy &' is 'const unsigned short'
+// CHECK-MESSAGES: :[[@LINE-5]]:42: note: 'Eldritch' and 'const Holy &' parameters accept and bind the same kind of values
 
 template <typename T1, typename T2>
 struct Pair {};
 
 void templateParam1(Pair<int, int> P1, Pair<int, int> P2) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'templateParam1' of similar type ('Pair<int, int>')
 // CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'P1'
 // CHECK-MESSAGES: :[[@LINE-3]]:55: note: the last parameter in the range is 'P2'

 template <typename T, typename... Us>
 void templateVariadic2(T TVar, Us... UVars) {} // NO-WARN: Distinct types in primary template.
 
 template <>
 void templateVariadic2(int TVar, int UVars1, int UVars2) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 3 adjacent parameters of 'templateVariadic2<int, int, int>' of similar type ('int')
 // CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'TVar'
 // CHECK-MESSAGES: :[[@LINE-3]]:50: note: the last parameter in the range is 'UVars2'
+
+template <typename T>
+using TwoOf = Pair<T, T>;
+
+void templateAndAliasTemplate(Pair<int, int> P, TwoOf<int> I) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 2 adjacent parameters of 'templateAndAliasTemplate' of similar type ('Pair<int, int>')
+// CHECK-MESSAGES: :[[@LINE-2]]:46: note: the first parameter in the range is 'P'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'I'
+
+template <typename T>
+struct Vector {
+  typedef T element_type;
+  typedef T &reference_type;
+  typedef const T const_element_type;
+  typedef const T &const_reference_type;
+};
+
+void memberTypedef(int I, Vector<int>::element_type E) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 2 adjacent parameters of 'memberTypedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:53: note: the last parameter in the range is 'E'
+// CHECK-MESSAGES: :[[@LINE-4]]:20: note: after resolving type aliases, 'int' and 'Vector<int>::element_type' are the same
+
+template <typename T>
+void memberTypedefDependent1(T T1, typename Vector<T>::element_type T2) {} // NO-WARN: Dependent name is not instantiated and resolved against other type.
+
+template <typename T>
+void memberTypedefDependent2(typename Vector<T>::element_type E1,
+                             typename Vector<T>::element_type E2) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: 2 adjacent parameters of 'memberTypedefDependent2' of similar type ('typename Vector<T>::element_type')
+// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the first parameter in the range is 'E1'
+// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the last parameter in the range is 'E2'
+
+template <typename T>
+void memberTypedefDependentReference1(
+    typename Vector<T>::element_type E,
+    typename Vector<T>::const_element_type &R) {} // NO-WARN: Not instantiated.
+
+template <typename T>
+void memberTypedefDependentReference2(
+    typename Vector<T>::element_type E,
+    typename Vector<T>::const_reference_type R) {} // NO-WARN: Not instantiated.
+
+template <typename T>
+void memberTypedefDependentReference3(
+    typename Vector<T>::element_type E,
+    const typename Vector<T>::element_type &R) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 2 adjacent parameters of 'memberTypedefDependentReference3' of similar type are
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the first parameter in the range is 'E'
+// CHECK-MESSAGES: :[[@LINE-3]]:45: note: the last parameter in the range is 'R'
+// CHECK-MESSAGES: :[[@LINE-4]]:5: note: 'typename Vector<T>::element_type' and 'const typename Vector<T>::element_type &' parameters accept and bind the same kind of values