[clang-tidy] Improve "common type" diagnostic output in 'bugprone-easily-swappable-parameters'

[whisperity]

Summary

Make the check handle cases of the "common type" involved in the mix being non-trivial, e.g. pointers, references, attributes, these things coming from typedefs, etc. This results in clearer diagnostics that have more coverage in their explanation, such as saying const int & as common type instead of just int.

[aaron.ballman]

LGTM!

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

struct MixData {
   }
 
   /// Add the specified flag bits to the flags.
   MixData &operator|=(MixFlags EnableFlags) {
     Flags |= EnableFlags;
     return *this;
   }
 
-  /// Add the specified qualifiers to the common type in the Mix.
-  MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const {
+  template <class F> MixData withCommonTypeTransformed(F &&Func) const {
     if (CommonType.isNull())
       return *this;
 
-    QualType NewCommonType = Ctx.getQualifiedType(CommonType, Quals);
+    QualType NewCommonType = Func(CommonType);
 
     if (CreatedFromOneWayConversion) {
       MixData M{Flags, Conversion};
       M.CommonType = NewCommonType;
       return M;
     }
+
     return {Flags, NewCommonType, Conversion, ConversionRTL};
   }
 };
 
 /// A named tuple that contains the information for a mix between two concrete
 /// parameters.
 struct Mix {
   const ParmVarDecl *First, *Second;
const ParmVarDecl *getLastParam() const {
     assert(!Mixes.empty());
     return Mixes.back().Second;
   }
 };
 
 /// Helper enum for the recursive calls in the modelling that toggle what kinds
 /// of implicit conversions are to be modelled.
 enum class ImplicitConversionModellingMode : unsigned char {
-  /// No implicit conversions are modelled.
+  ///< No implicit conversions are modelled.
   None,
 
-  /// The full implicit conversion sequence is modelled.
+  ///< The full implicit conversion sequence is modelled.
   All,
 
-  /// Only model a unidirectional implicit conversion and within it only one
+  ///< Only model a unidirectional implicit conversion and within it only one
   /// standard conversion sequence.
   OneWaySingleStandardOnly
 };
 
 static MixData
 isLRefEquallyBindingToType(const TheCheck &Check,
                            const LValueReferenceType *LRef, QualType Ty,
                            const ASTContext &Ctx, bool IsRefRHS,
                            ImplicitConversionModellingMode ImplicitMode);
 
 static MixData
 approximateImplicitConversion(const TheCheck &Check, QualType LType,
                               QualType RType, const ASTContext &Ctx,
                               ImplicitConversionModellingMode ImplicitMode);
 
 static inline bool isUselessSugar(const Type *T) {
   return isa<AttributedType, DecayedType, ElaboratedType, ParenType>(T);
 }
 
+namespace {
+
+struct NonCVRQualifiersResult {
+  /// True if the types are qualified in a way that even after equating or
+  /// removing local CVR qualification, even if the unqualified types
+  /// themselves would mix, the qualified ones don't, because there are some
+  /// other local qualifiers that are not equal.
+  bool HasMixabilityBreakingQualifiers;
+
+  /// The set of equal qualifiers between the two types.
+  Qualifiers CommonQualifiers;
+};
+
+} // namespace
+
 /// Returns if the two types are qualified in a way that ever after equating or
 /// removing local CVR qualification, even if the unqualified types would mix,
 /// the qualified ones don't, because there are some other local qualifiers
 /// that aren't equal.
-static bool hasNonCVRMixabilityBreakingQualifiers(const ASTContext &Ctx,
-                                                  QualType LType,
-                                                  QualType RType) {
-  LLVM_DEBUG(
-      llvm::dbgs() << ">>> hasNonCVRMixabilityBreakingQualifiers for LType:\n";
+static NonCVRQualifiersResult
+getNonCVRQualifiers(const ASTContext &Ctx, QualType LType, QualType RType) {
+  LLVM_DEBUG(llvm::dbgs() << ">>> getNonCVRQualifiers for LType:\n";
              LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n";
              RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
   Qualifiers LQual = LType.getLocalQualifiers(),
              RQual = RType.getLocalQualifiers();
 
   // Strip potential CVR. That is handled by the check option QualifiersMix.
   LQual.removeCVRQualifiers();
   RQual.removeCVRQualifiers();
 
-  Qualifiers CommonQuals = Qualifiers::removeCommonQualifiers(LQual, RQual);
-  (void)CommonQuals;
+  NonCVRQualifiersResult Ret;
+  Ret.CommonQualifiers = Qualifiers::removeCommonQualifiers(LQual, RQual);
 
   LLVM_DEBUG(llvm::dbgs() << "--- hasNonCVRMixabilityBreakingQualifiers. "
                              "Removed common qualifiers: ";
-             CommonQuals.print(llvm::dbgs(), Ctx.getPrintingPolicy());
+             Ret.CommonQualifiers.print(llvm::dbgs(), Ctx.getPrintingPolicy());
              llvm::dbgs() << "\n\tremaining on LType: ";
              LQual.print(llvm::dbgs(), Ctx.getPrintingPolicy());
              llvm::dbgs() << "\n\tremaining on RType: ";
              RQual.print(llvm::dbgs(), Ctx.getPrintingPolicy());
              llvm::dbgs() << '\n';);
 
-  // If there are any remaining qualifiers, consider the two types distinct.
-  return LQual.hasQualifiers() || RQual.hasQualifiers();
+  // If there are no other non-cvr non-common qualifiers left, we can deduce
+  // that mixability isn't broken.
+  Ret.HasMixabilityBreakingQualifiers =
+      LQual.hasQualifiers() || RQual.hasQualifiers();
+
+  return Ret;
 }
 
 /// 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!
 ///
calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
   }
   if (isUselessSugar(RType.getTypePtr())) {
     LLVM_DEBUG(llvm::dbgs()
                << "--- calculateMixability. RHS is useless sugar.\n");
     return calculateMixability(
         Check, LType, RType.getSingleStepDesugaredType(Ctx), Ctx, ImplicitMode);
   }
 
+  const auto *LLRef = LType->getAs<LValueReferenceType>();
+  const auto *RLRef = RType->getAs<LValueReferenceType>();
+  if (LLRef && RLRef) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS and RHS are &.\n");
+
+    return calculateMixability(Check, LLRef->getPointeeType(),
+                               RLRef->getPointeeType(), Ctx, ImplicitMode)
+        .withCommonTypeTransformed(
+            [&Ctx](QualType QT) { return Ctx.getLValueReferenceType(QT); });
+  }
   // 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>()) {
+  if (LLRef) {
     LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is &.\n");
-    return isLRefEquallyBindingToType(Check, LRef, RType, Ctx, false,
+    return isLRefEquallyBindingToType(Check, LLRef, RType, Ctx, false,
                                       ImplicitMode) |
            MixFlags::ReferenceBind;
   }
-  if (const auto *RRef = RType->getAs<LValueReferenceType>()) {
+  if (RLRef) {
     LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is &.\n");
-    return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true,
+    return isLRefEquallyBindingToType(Check, RLRef, LType, Ctx, true,
                                       ImplicitMode) |
            MixFlags::ReferenceBind;
   }
 
   if (LType->getAs<TypedefType>()) {
     LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n");
     return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),
                                RType, Ctx, ImplicitMode) |
            MixFlags::TypeAlias;
   }
   if (RType->getAs<TypedefType>()) {
     LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is typedef.\n");
     return calculateMixability(Check, LType,
                                RType.getSingleStepDesugaredType(Ctx), Ctx,
                                ImplicitMode) |
            MixFlags::TypeAlias;
   }
 
   // A parameter of type 'cvr1 T' and another of potentially differently
   // qualified 'cvr2 T' may bind with the same power, if the user so requested.
   //
   // Whether to do this check for the inner unqualified types.
   bool CompareUnqualifiedTypes = false;
-  // Should the result have a common inner type qualified for diagnosis?
-  bool RequalifyUnqualifiedMixabilityResult = false;
   if (LType.getLocalCVRQualifiers() != RType.getLocalCVRQualifiers()) {
     LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
       llvm::dbgs() << "--- calculateMixability. LHS has CVR-Qualifiers: ";
       Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
           .print(llvm::dbgs(), Ctx.getPrintingPolicy());
       llvm::dbgs() << '\n';
     });
     LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) {
       llvm::dbgs() << "--- calculateMixability. RHS has CVR-Qualifiers: ";
       Qualifiers::fromCVRMask(RType.getLocalCVRQualifiers())
           .print(llvm::dbgs(), Ctx.getPrintingPolicy());
       llvm::dbgs() << '\n';
     });
 
     if (!Check.QualifiersMix) {
       LLVM_DEBUG(llvm::dbgs()
                  << "<<< calculateMixability. QualifiersMix turned off - not "
                     "mixable.\n");
       return {MixFlags::None};
     }
 
     CompareUnqualifiedTypes = true;
   }
+  // Whether the two types had the same CVR qualifiers.
+  bool OriginallySameQualifiers = false;
   if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() &&
       LType.getLocalCVRQualifiers() != 0) {
     LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
       llvm::dbgs()
           << "--- calculateMixability. LHS and RHS have same CVR-Qualifiers: ";
       Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
           .print(llvm::dbgs(), Ctx.getPrintingPolicy());
       llvm::dbgs() << '\n';
     });
 
     CompareUnqualifiedTypes = true;
-    RequalifyUnqualifiedMixabilityResult = true;
+    OriginallySameQualifiers = true;
   }
 
   if (CompareUnqualifiedTypes) {
-    if (hasNonCVRMixabilityBreakingQualifiers(Ctx, LType, RType)) {
+    NonCVRQualifiersResult AdditionalQuals =
+        getNonCVRQualifiers(Ctx, LType, RType);
+    if (AdditionalQuals.HasMixabilityBreakingQualifiers) {
       LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Additional "
                                  "non-equal incompatible qualifiers.\n");
       return {MixFlags::None};
     }
 
     MixData UnqualifiedMixability =
         calculateMixability(Check, LType.getLocalUnqualifiedType(),
-                            RType.getLocalUnqualifiedType(), Ctx, ImplicitMode);
+                            RType.getLocalUnqualifiedType(), Ctx, ImplicitMode)
+            .withCommonTypeTransformed([&AdditionalQuals, &Ctx](QualType QT) {
+              // Once the mixability was deduced, apply the qualifiers common
+              // to the two type back onto the diagnostic printout.
+              return Ctx.getQualifiedType(QT, AdditionalQuals.CommonQualifiers);
+            });
 
-    if (!RequalifyUnqualifiedMixabilityResult)
+    if (!OriginallySameQualifiers)
+      // User-enabled qualifier change modelled for the mix.
       return UnqualifiedMixability | MixFlags::Qualifiers;
 
-    // Apply the same qualifier back into the found common type if we found
-    // a common type between the unqualified versions.
-    return UnqualifiedMixability.qualify(LType.getLocalQualifiers(), Ctx);
+    // Apply the same qualifier back into the found common type if they were
+    // the same.
+    return UnqualifiedMixability.withCommonTypeTransformed(
+        [&Ctx, LType](QualType QT) {
+          return Ctx.getQualifiedType(QT, LType.getLocalQualifiers());
+        });
   }
 
   // Certain constructs match on the last catch-all getCanonicalType() equality,
   // which is perhaps something not what we want. If this variable is true,
   // the canonical type equality will be ignored.
   bool RecursiveReturnDiscardingCanonicalType = false;
 
   if (LType->isPointerType() && RType->isPointerType()) {
     // If both types are pointers, and pointed to the exact same type,
     // LType == RType took care of that. Try to see if the pointee type has
     // some other match. However, this must not consider implicit conversions.
     LLVM_DEBUG(llvm::dbgs()
                << "--- calculateMixability. LHS and RHS are Ptrs.\n");
-    MixData MixOfPointee = calculateMixability(
-        Check, LType->getPointeeType(), RType->getPointeeType(), Ctx,
-        ImplicitConversionModellingMode::None);
+    MixData MixOfPointee =
+        calculateMixability(Check, LType->getPointeeType(),
+                            RType->getPointeeType(), Ctx,
+                            ImplicitConversionModellingMode::None)
+            .withCommonTypeTransformed(
+                [&Ctx](QualType QT) { return Ctx.getPointerType(QT); });
     if (hasFlag(MixOfPointee.Flags,
                 MixFlags::WorkaroundDisableCanonicalEquivalence))
       RecursiveReturnDiscardingCanonicalType = true;
 
     MixOfPointee.sanitize();
     if (MixOfPointee.indicatesMixability()) {
       LLVM_DEBUG(llvm::dbgs()
                  << "<<< calculateMixability. Pointees are mixable.\n");
for (const model::Mix &M : R.Mixes) {
       std::string LTypeStr = LType.getAsString(PP);
       std::string RTypeStr = 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 (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr)
+        if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) {
           if (hasFlag(M.flags(), MixFlags::Qualifiers))
             DiagText = "after resolving type aliases, '%0' and '%1' share a "
                        "common type";
           else
             DiagText =
                 "after resolving type aliases, '%0' and '%1' are the same";
-        else if (!CommonType.isNull()) {
+        } else if (!CommonType.isNull()) {
           DiagText = "after resolving type aliases, the common type of '%0' "
                      "and '%1' is '%2'";
           ExplicitlyPrintCommonType = true;
         }
 
         auto Diag =
             diag(LVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note)
             << LTypeStr << RTypeStr;

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

 
 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]]:37: note: 'int' and 'ICRTy' parameters accept and bind the same kind of values
 // CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, 'int' and 'MyIntCRTy' are the same
 // CHECK-MESSAGES: :[[@LINE-6]]:52: note: 'int' and 'MyIntCRTy' parameters accept and bind the same kind of values
-// CHECK-MESSAGES: :[[@LINE-7]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'int'
-// CHECK-MESSAGES: :[[@LINE-8]]:52: note: 'ICRTy' and 'MyIntCRTy' parameters accept and bind the same kind of values
+// CHECK-MESSAGES: :[[@LINE-7]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'const int &'
 
 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-3]]:70: note: the first parameter in the range is 'One'
 // CHECK-MESSAGES: :[[@LINE-3]]:70: note: the last parameter in the range is 'Two'
 
 void attributedParam1Typedef(const __attribute__((address_space(256))) int *One,
                              const __attribute__((address_space(256))) MyInt1 *Two) {}
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: 2 adjacent parameters of 'attributedParam1Typedef' of similar type are
 // CHECK-MESSAGES: :[[@LINE-3]]:77: note: the first parameter in the range is 'One'
 // CHECK-MESSAGES: :[[@LINE-3]]:80: note: the last parameter in the range is 'Two'
-// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, the common type of 'const __attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'const __attribute__((address_space(256))) int'
-// FIXME: The last diagnostic line is a bit bad: the common type should be a
-// pointer type -- it is not clear right now, how it would be possible to
-// properly wire a logic in that fixes it.
+// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, 'const __attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' are the same
+
+void attributedParam1TypedefRef(
+    const __attribute__((address_space(256))) int &OneR,
+    __attribute__((address_space(256))) MyInt1 &TwoR) {}
+// NO-WARN: One is CVR-qualified, the other is not.
+
+void attributedParam1TypedefCRef(
+    const __attribute__((address_space(256))) int &OneR,
+    const __attribute__((address_space(256))) MyInt1 &TwoR) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 2 adjacent parameters of 'attributedParam1TypedefCRef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the first parameter in the range is 'OneR'
+// CHECK-MESSAGES: :[[@LINE-3]]:55: note: the last parameter in the range is 'TwoR'
+// CHECK-MESSAGES: :[[@LINE-5]]:5: note: after resolving type aliases, 'const __attribute__((address_space(256))) int &' and 'const __attribute__((address_space(256))) MyInt1 &' are the same
 
 void attributedParam2(__attribute__((address_space(256))) int *One,
                       const __attribute__((address_space(256))) MyInt1 *Two) {}
 // NO-WARN: One is CVR-qualified, the other is not.
 
 void attributedParam3(const int *One,
                       const __attribute__((address_space(256))) MyInt1 *Two) {}
 // NO-WARN: One is attributed, the other is not.
 
 void attributedParam4(const __attribute__((address_space(512))) int *One,
                       const __attribute__((address_space(256))) MyInt1 *Two) {}
 // NO-WARN: Different value of the attribute.

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

 
 template <typename T>
 void copy(const T *Dest, T *Source) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 2 adjacent parameters of 'copy' of similar type are
 // CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'Dest'
 // CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'Source'
 // CHECK-MESSAGES: :[[@LINE-4]]:26: note: 'const T *' and 'T *' parameters accept and bind the same kind of values
 
+void attributedParam1TypedefRef(
+    const __attribute__((address_space(256))) int &OneR,
+    __attribute__((address_space(256))) MyInt1 &TwoR) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 2 adjacent parameters of 'attributedParam1TypedefRef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the first parameter in the range is 'OneR'
+// CHECK-MESSAGES: :[[@LINE-3]]:49: note: the last parameter in the range is 'TwoR'
+// CHECK-MESSAGES: :[[@LINE-5]]:5: note: after resolving type aliases, the common type of 'const __attribute__((address_space(256))) int &' and '__attribute__((address_space(256))) MyInt1 &' is '__attribute__((address_space(256))) int &'
+// CHECK-MESSAGES: :[[@LINE-5]]:5: note: 'const __attribute__((address_space(256))) int &' and '__attribute__((address_space(256))) MyInt1 &' parameters accept and bind the same kind of values
+
 void attributedParam2(__attribute__((address_space(256))) int *One,
                       const __attribute__((address_space(256))) MyInt1 *Two) {}
 // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam2' of similar type are
 // CHECK-MESSAGES: :[[@LINE-3]]:64: note: the first parameter in the range is 'One'
 // CHECK-MESSAGES: :[[@LINE-3]]:73: note: the last parameter in the range is 'Two'
-// CHECK-MESSAGES: :[[@LINE-5]]:23: note: after resolving type aliases, the common type of '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: after resolving type aliases, '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' share a common type
 // CHECK-MESSAGES: :[[@LINE-5]]:23: note: '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' parameters accept and bind the same kind of values
-// FIXME: The last diagnostic line is a bit bad: the common type should be a
-// pointer type -- it is not clear right now, how it would be possible to
-// properly wire a logic in that fixes it.