[clang-tidy] Fix crash and handle AttributedTypes in 'bugprone-easily-swappable-parameters'

[whisperity]

Summary

@vabridgers identified a way to crash the check by running on code that involve AttributedTypes. This patch fixes the check to first and foremost not crash, but also improves the logic handling qualifiers.

If the types contain any additional (not just CVR) qualifiers that are not the same, they will not be deemed mixable. The logic for CVR-mixing, and the QualifiersMix check option remain unchanged.

[aaron.ballman]

The rename from CommonType to CoreType could probably be done as a separate NFC to make the review a bit easier, if we decide that's a good terminology switch. I'm not certain switching from common to core makes things more clear though; what's the distinction trying to be made?

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

476–477

2199

I don't know that "core type" is a term of art that will convey much meaning to users in a diagnostic message. Do we need to change the wording of the diagnostic?

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

362–364

Yeah, the diagnostic here is a bit unfortunate. This was an existing deficiency with the check though, right? I assume it also impacts references and not just pointers?

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

122

Should probably have a FIXME comment here as well given the lack of a pointer.

[vabridgers]

This change fixed the downstream crash. Thanks for the quick turnaround!

[whisperity]

• Remove orthogonal/half-baked changes.
• Fix a mistake in the logic changing only a temporary state.
• Add a missing FIXME.

[whisperity]

In D106361#2890458, @aaron.ballman wrote:

The rename from CommonType to CoreType could probably be done as a separate NFC to make the review a bit easier, if we decide that's a good terminology switch. I'm not certain switching from common to core makes things more clear though; what's the distinction trying to be made?

Not much, just tried to dance around the fact that the diagnostic emitted for this is, put simply, crappy. I'm coming up with a better solution, and indeed, let's fix the issue at hand now and focus on the improvements later.

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

476–477

Actually, the suggestion is also bad. getQualifiers() returns a copy on which adding is a moot operation... Turns out you can use ASTContext to create a specifically qualified type for you.

Now if there was a way to express this in the type of getQualifiers(), to warn you "Don't make the mistake of thinking this would CHANGE anything!"...

476–477

Sorry, I mean, not the suggestion per se, but these 4 new lines of code contained multiple issues.

2199

The wording needs to be changed, but not in this particular way. I started working on a follow-up refactoring patch in general and some changes made it into this diff. I'll revert these from this patch and will work everything related to that into a new one.

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

362–364

I will look into this (and improving the diagnostics in general) in a follow-up patch.

[whisperity]

NFC Fix bad phrasing in comments (e.g. still mentioning core type and having a misleading negated sentence.)

[aaron.ballman]

LGTM!

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

476–477

Ah, this looks much better, thank you!

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 {
-    SplitQualType Split = CommonType.split();
-    Split.Quals.addQualifiers(Quals);
-    QualType CommonType{Split.Ty, Split.Quals.getAsOpaqueValue()};
+  MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const {
+    if (CommonType.isNull())
+      return *this;
+
+    QualType NewCommonType = Ctx.getQualifiedType(CommonType, Quals);
 
     if (CreatedFromOneWayConversion) {
       MixData M{Flags, Conversion};
-      M.CommonType = CommonType;
+      M.CommonType = NewCommonType;
       return M;
     }
-    return {Flags, CommonType, Conversion, ConversionRTL};
+    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;
   MixData Data;
isLRefEquallyBindingToType(const TheCheck &Check,
                            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<DecayedType, ElaboratedType, ParenType>(T);
+  return isa<AttributedType, DecayedType, ElaboratedType, ParenType>(T);
+}
+
+/// 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";
+      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;
+
+  LLVM_DEBUG(llvm::dbgs() << "--- hasNonCVRMixabilityBreakingQualifiers. "
+                             "Removed common qualifiers: ";
+             CommonQuals.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();
 }
 
 /// 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, QualType LType, QualType RType,
                     const ASTContext &Ctx,
                     ImplicitConversionModellingMode ImplicitMode) {
   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';);
-
-  // 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 == RType) {
     LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Trivial equality.\n");
     return {MixFlags::Trivial, LType};
   }
 
   // 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.
calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
   }
   if (const auto *RRef = RType->getAs<LValueReferenceType>()) {
     LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is &.\n");
     return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true,
                                       ImplicitMode) |
            MixFlags::ReferenceBind;
   }
 
-  // Dissolve typedefs after the qualifiers outside the typedef are dealt with.
   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 is CVR.\n");
-    LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) llvm::dbgs()
-               << "--- calculateMixability. RHS is CVR.\n");
+    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.\n");
+                 << "<<< calculateMixability. QualifiersMix turned off - not "
+                    "mixable.\n");
       return {MixFlags::None};
     }
 
-    return calculateMixability(Check, LType.getLocalUnqualifiedType(),
-                               RType.getLocalUnqualifiedType(), Ctx,
-                               ImplicitMode) |
-           MixFlags::Qualifiers;
+    CompareUnqualifiedTypes = true;
   }
   if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() &&
       LType.getLocalCVRQualifiers() != 0) {
-    LLVM_DEBUG(llvm::dbgs()
-               << "--- calculateMixability. LHS and RHS same CVR.\n");
+    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;
+  }
+
+  if (CompareUnqualifiedTypes) {
+    if (hasNonCVRMixabilityBreakingQualifiers(Ctx, LType, RType)) {
+      LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Additional "
+                                 "non-equal incompatible qualifiers.\n");
+      return {MixFlags::None};
+    }
+
+    MixData UnqualifiedMixability =
+        calculateMixability(Check, LType.getLocalUnqualifiedType(),
+                            RType.getLocalUnqualifiedType(), Ctx, ImplicitMode);
+
+    if (!RequalifyUnqualifiedMixabilityResult)
+      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 calculateMixability(Check, LType.getLocalUnqualifiedType(),
-                               RType.getLocalUnqualifiedType(), Ctx,
-                               ImplicitMode)
-        .qualify(LType.getLocalQualifiers());
+    return UnqualifiedMixability.qualify(LType.getLocalQualifiers(), Ctx);
   }
 
+  // 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,
for (const model::Mix &M : R.Mixes) {
 
       if (hasFlag(M.flags(), MixFlags::TypeAlias) &&
           UniqueTypeAlias(LType, RType, CommonType)) {
         StringRef DiagText;
         bool ExplicitlyPrintCommonType = false;
         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()) {
           DiagText = "after resolving type aliases, the common type of '%0' "
                      "and '%1' is '%2'";
           ExplicitlyPrintCommonType = true;
         }

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

void memberTypedefDependentReference3(
     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
 
 void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void (*Throwing)()) {}
 // NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise.
+
+void attributedParam1(const __attribute__((address_space(256))) int *One,
+                      const __attribute__((address_space(256))) int *Two) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam1' of similar type ('const __attribute__((address_space(256))) int *') are
+// 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.
+
+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

 // CHECK-MESSAGES: :[[@LINE-4]]:37: note: 'CInt &' and 'int' parameters accept and bind the same kind of values
 
 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 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: '__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.