diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h @@ -39,8 +39,14 @@ /// ignored. const std::vector IgnoredParameterTypeSuffixes; - /// Whether to consider an unqualified and a qualified type mixable. + /// Whether to consider an unqualified and a qualified version of the + /// same type mixable. const bool QualifiersMix; + + /// Whether to model implicit conversions "in full" (conditions apply) + /// during analysis and consider types that are implicitly convertible to + /// one another mixable. + const bool ModelImplicitConversions; }; } // namespace bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp @@ -57,11 +57,14 @@ /// The default value for the QualifiersMix check option. static constexpr bool DefaultQualifiersMix = false; +/// The default value for the ModelImplicitConversions check option. +static constexpr bool DefaultModelImplicitConversions = true; + #ifndef NDEBUG template static inline std::string formatBitsImpl(std::size_t Num) { - constexpr std::size_t WidthWithPadding = Width + (Width / 4); + static constexpr std::size_t WidthWithPadding = Width + (Width / 4); char S[WidthWithPadding]; for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) { if (CurPos % 5 == 0) { @@ -78,7 +81,7 @@ /// Formats the bit representation of a numeric type as a string in groups of 4. template static inline std::string formatBits(T &&V) { - return formatBitsImpl(V); + return formatBitsImpl(V); } #else @@ -103,42 +106,240 @@ /// The language features involved in allowing the mix between two parameters. enum MixFlags : unsigned char { - MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE! + //< Sentinel bit pattern for null results. Should not appear as a real result + // after modelling. + MIX_Invalid = 0, #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) - FB(None, 1), //< Mix between the two parameters is not possible. - FB(Trivial, 2), //< The two mix trivially, and are the exact same type. - FB(Canonical, 3), //< The two mix because the types refer to the same - // CanonicalType, but we do not elaborate as to how. - FB(TypeAlias, 4), //< The path from one type to the other involves - // desugaring type aliases. - FB(ReferenceBind, 5), //< The mix involves the binding power of "const &". - FB(Qualifiers, 6), //< The mix involves change in the qualifiers. + //< Certain constructs (such as pointers to noexcept/non-noexcept functions) + // have the same CanonicalType, which would result in false positives. + // During the recursive modelling call, this flag is set if a later diagnosed + // canonical type equivalence should be thrown away. + FB(WorkaroundDisableCanonicalEquivalence, 1), + + //< Mix between the two parameters is not possible. + FB(None, 2), + + //< The two mix trivially, and are the exact same type. + FB(Trivial, 3), + + //< The two mix because the types involved refer to the same CanonicalType, + // but we do not elabore how. Trust Clang that ->getCanonicalType() made the + // right decision. + FB(Canonical, 4), + + //< The path from one type to the other involves desugaring type aliases. + FB(TypeAlias, 5), + + //< The mixing of the two parameters involves the binding power of "const &". + FB(ReferenceBind, 6), + + //< The mixing of the two parameters involves going between differently + // qualified types. + FB(Qualifiers, 7), + + //< The mixing of the parameters is possible through implicit conversions + // between the types. + FB(ImplicitConversion, 8), #undef FB - LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/MIX_Qualifiers) + LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/MIX_ImplicitConversion) }; LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); +/// The results of the steps of an Implicit Conversion Sequence is saved in +/// an instance of this record. +struct ConversionSequence { + enum UserDefinedConversionKind { UDCK_None, UDCK_Ctor, UDCK_Oper }; + + struct UserDefinedConvertingConstructor { + const CXXConstructorDecl *Fun; + QualType ConstructorParameterType; + QualType UserDefinedType; + }; + + struct UserDefinedConversionOperator { + const CXXConversionDecl *Fun; + QualType UserDefinedType; + QualType ConversionOperatorResultType; + }; + + /// The type the conversion stared from. + QualType Begin; + + /// The intermediate type after the first Standard Conversion Sequence. + QualType AfterPreStd; + + /// The details of the user-defined conversion involved, as a tagged union. + union { + char None; + UserDefinedConvertingConstructor UDConvCtor; + UserDefinedConversionOperator UDConvOp; + }; + UserDefinedConversionKind UDConvKind; + + /// The intermediate type after performing the second Standard Conversion + /// Sequence. + QualType AfterPostStd; + + /// The result type the conversion targeted. + QualType End; + + ConversionSequence() : None(0), UDConvKind(UDCK_None) {} + ConversionSequence(QualType From, QualType To) + : Begin(From), None(0), UDConvKind(UDCK_None), End(To) {} + + explicit operator bool() const { + return !AfterPreStd.isNull() || UDConvKind != UDCK_None || + !AfterPostStd.isNull(); + } + + /// Returns all the "steps" (non-unique and non-similar) types involved in + /// the conversion sequence. This method does **NOT** return Begin and End. + SmallVector getInvolvedTypesInSequence() const { + SmallVector Ret; + const auto &EmplaceIfDifferent = [&Ret](QualType QT) { + if (QT.isNull()) + return; + + LLVM_DEBUG(QT.dump()); + + if (Ret.empty()) + Ret.emplace_back(QT); + else if (Ret.back() != QT) + Ret.emplace_back(QT); + }; + + EmplaceIfDifferent(AfterPreStd); + switch (UDConvKind) { + case UDCK_Ctor: + EmplaceIfDifferent(UDConvCtor.ConstructorParameterType); + EmplaceIfDifferent(UDConvCtor.UserDefinedType); + break; + case UDCK_Oper: + EmplaceIfDifferent(UDConvOp.UserDefinedType); + EmplaceIfDifferent(UDConvOp.ConversionOperatorResultType); + break; + case UDCK_None: + break; + } + EmplaceIfDifferent(AfterPostStd); + + return Ret; + } + + /// Updates the steps of the conversion sequence with the steps from the + /// other instance. + /// + /// \note This method does not check if the resulting conversion sequence is + /// sensible! + ConversionSequence &operator<<=(const ConversionSequence &RHS) { + if (!RHS.AfterPreStd.isNull()) + AfterPreStd = RHS.AfterPreStd; + switch (RHS.UDConvKind) { + case UDCK_Ctor: + UDConvKind = UDCK_Ctor; + UDConvCtor = RHS.UDConvCtor; + break; + case UDCK_Oper: + UDConvKind = UDCK_Oper; + UDConvOp = RHS.UDConvOp; + break; + case UDCK_None: + break; + } + if (!RHS.AfterPostStd.isNull()) + AfterPostStd = RHS.AfterPostStd; + + return *this; + } + + /// Sets the user-defined conversion to the given constructor. + ConversionSequence &operator%=(const UserDefinedConvertingConstructor &RHS) { + UDConvKind = UDCK_Ctor; + UDConvCtor = RHS; + return *this; + } + + /// Sets the user-defined conversion to the given operator. + ConversionSequence &operator%=(const UserDefinedConversionOperator &RHS) { + UDConvKind = UDCK_Oper; + UDConvOp = RHS; + return *this; + } + + /// Returns the type in the conversion that's formally "in our hands" once + /// the user-defined conversion is executed. + QualType getTypeAfterUserDefinedConversion() const { + switch (UDConvKind) { + case UDCK_Ctor: + return UDConvCtor.UserDefinedType; + case UDCK_Oper: + return UDConvOp.ConversionOperatorResultType; + case UDCK_None: + break; + } + return {}; + } + + const CXXMethodDecl *getUserDefinedConversionFunction() const { + switch (UDConvKind) { + case UDCK_Ctor: + return UDConvCtor.Fun; + case UDCK_Oper: + return UDConvOp.Fun; + case UDCK_None: + break; + } + return nullptr; + } +}; + /// 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; + MixFlags Flags = MIX_Invalid; /// A potentially calculated common underlying type after desugaring, that /// both sides of the mix can originate from. QualType CommonType; + /// The steps an implicit conversion performs to get from one type to the + /// other. + ConversionSequence Conversion, ConversionRTL; + + /// True if the MixData was specifically created with only a one-way + /// conversion modelled. + bool CreatedFromOneWayConversion = false; + MixData(MixFlags Flags) : Flags(Flags) {} MixData(MixFlags Flags, QualType CommonType) : Flags(Flags), CommonType(CommonType) {} + MixData(MixFlags Flags, ConversionSequence Conv) + : Flags(Flags), Conversion(Conv), CreatedFromOneWayConversion(true) {} + MixData(MixFlags Flags, ConversionSequence LTR, ConversionSequence RTL) + : Flags(Flags), Conversion(LTR), ConversionRTL(RTL) {} + MixData(MixFlags Flags, QualType CommonType, ConversionSequence LTR, + ConversionSequence RTL) + : Flags(Flags), CommonType(CommonType), Conversion(LTR), + ConversionRTL(RTL) {} void sanitize() { assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec"); + MixFlags CanonicalAndWorkaround = + MIX_Canonical | MIX_WorkaroundDisableCanonicalEquivalence; + if ((Flags & CanonicalAndWorkaround) == CanonicalAndWorkaround) { + // A workaround for too eagerly equivalent canonical types was requested, + // and a canonical equivalence was proven. Fulfill the request and throw + // this result away. + Flags = MIX_None; + return; + } + if (Flags & MIX_None) { // If anywhere down the recursion a potential mix "path" is deemed // impossible, throw away all the other bits because the mix is not @@ -155,11 +356,34 @@ // recursion other bit(s) were set, remove the trivial bit, as it is not // trivial. Flags &= ~MIX_Trivial; + + bool ShouldHaveImplicitConvFlag = false; + if (CreatedFromOneWayConversion && Conversion) + ShouldHaveImplicitConvFlag = true; + else if (!CreatedFromOneWayConversion && Conversion && ConversionRTL) + // Only say that we have implicit conversion mix possibility if it is + // bidirectional. Otherwise, the compiler would report an *actual* swap + // at a call site... + ShouldHaveImplicitConvFlag = true; + + if (ShouldHaveImplicitConvFlag) + Flags |= MIX_ImplicitConversion; + else + Flags &= ~MIX_ImplicitConversion; } + bool isValid() const { return Flags >= MIX_None; } + + bool indicatesMixability() const { return Flags > MIX_None; } + /// Add the specified flag bits to the flags. MixData operator|(MixFlags EnableFlags) const { - return {Flags | EnableFlags, CommonType}; + if (CreatedFromOneWayConversion) { + MixData M{Flags | EnableFlags, Conversion}; + M.CommonType = CommonType; + return M; + } + return {Flags | EnableFlags, CommonType, Conversion, ConversionRTL}; } /// Add the specified flag bits to the flags. @@ -172,8 +396,14 @@ MixData operator<<(Qualifiers Quals) const { SplitQualType Split = CommonType.split(); Split.Quals.addQualifiers(Quals); + QualType CommonType{Split.Ty, Split.Quals.getAsOpaqueValue()}; - return {Flags, QualType(Split.Ty, Split.Quals.getAsOpaqueValue())}; + if (CreatedFromOneWayConversion) { + MixData M{Flags, Conversion}; + M.CommonType = CommonType; + return M; + } + return {Flags, CommonType, Conversion, ConversionRTL}; } }; @@ -188,7 +418,15 @@ void sanitize() { Data.sanitize(); } MixFlags flags() const { return Data.Flags; } + bool flagsValid() const { return Data.isValid(); } + bool mixable() const { return Data.indicatesMixability(); } QualType commonUnderlyingType() const { return Data.CommonType; } + const ConversionSequence &leftToRightConversionSequence() const { + return Data.Conversion; + } + const ConversionSequence &rightToLeftConversionSequence() const { + return Data.ConversionRTL; + } }; static_assert(std::is_trivially_copyable::value && @@ -224,10 +462,34 @@ } }; -static MixData isLRefEquallyBindingToType(const TheCheck &Check, - const LValueReferenceType *LRef, - QualType Ty, const ASTContext &Ctx, - bool IsRefRHS); +/// Helper enum for the recursive calls in the modelling that toggle what kinds +/// of implicit conversions are to be modelled. +enum ImplicitConversionModellingMode : unsigned char { + //< No implicit conversions are modelled. + ICMM_None, + + //< The full implicit conversion sequence is modelled. + ICMM_All, + + //< Only model a unidirectional implicit conversion and within it only one + // standard conversion sequence. + ICMM_OneWaySingleStandardOnly, + + //< Only standard conversion sequence is modelled, so this skips + // user-defined conversions. + // ICMM_StandardSeqOnly +}; + +static MixData +isLRefEquallyBindingToType(const TheCheck &Check, + const LValueReferenceType *LRef, QualType Ty, + const ASTContext &Ctx, bool IsRefRHS, + ImplicitConversionModellingMode ImplicitMode); + +static MixData +approximateImplicitConversion(const TheCheck &Check, const QualType LType, + const QualType RType, const ASTContext &Ctx, + ImplicitConversionModellingMode ImplicitMode); /// 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 @@ -238,13 +500,19 @@ /// 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) { +static MixData +calculateMixability(const TheCheck &Check, const QualType LType, + const 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 {MIX_Trivial, LType}; @@ -253,15 +521,17 @@ // 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(LType.getTypePtr())) { - LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is ParenType.\n"); + if (isa(LType.getTypePtr())) { + LLVM_DEBUG(llvm::dbgs() + << "--- calculateMixability. LHS is useless sugar.\n"); return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx), - RType, Ctx); + RType, Ctx, ImplicitMode); } - if (isa(RType.getTypePtr())) { - LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is ParenType.\n"); - return calculateMixability(Check, LType, - RType.getSingleStepDesugaredType(Ctx), Ctx); + if (isa(RType.getTypePtr())) { + LLVM_DEBUG(llvm::dbgs() + << "--- calculateMixability. RHS is useless sugar.\n"); + return calculateMixability( + Check, LType, RType.getSingleStepDesugaredType(Ctx), Ctx, ImplicitMode); } // At a particular call site, what could be passed to a 'T' or 'const T' might @@ -269,12 +539,14 @@ // side effect on the passed expressions. if (const auto *LRef = LType->getAs()) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is &.\n"); - return isLRefEquallyBindingToType(Check, LRef, RType, Ctx, false) | + return isLRefEquallyBindingToType(Check, LRef, RType, Ctx, false, + ImplicitMode) | MIX_ReferenceBind; } if (const auto *RRef = RType->getAs()) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is &.\n"); - return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true) | + return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true, + ImplicitMode) | MIX_ReferenceBind; } @@ -282,13 +554,14 @@ if (LType->getAs()) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n"); return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx), - RType, Ctx) | + RType, Ctx, ImplicitMode) | MIX_TypeAlias; } if (RType->getAs()) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is typedef.\n"); return calculateMixability(Check, LType, - RType.getSingleStepDesugaredType(Ctx), Ctx) | + RType.getSingleStepDesugaredType(Ctx), Ctx, + ImplicitMode) | MIX_TypeAlias; } @@ -309,7 +582,8 @@ } return calculateMixability(Check, LType.getLocalUnqualifiedType(), - RType.getLocalUnqualifiedType(), Ctx) | + RType.getLocalUnqualifiedType(), Ctx, + ImplicitMode) | MIX_Qualifiers; } if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() && @@ -320,38 +594,111 @@ // 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) + RType.getLocalUnqualifiedType(), Ctx, + ImplicitMode) << LType.getLocalQualifiers(); } 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. + // 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"); - return calculateMixability(Check, LType->getPointeeType(), - RType->getPointeeType(), Ctx); + MixData MixOfPointee = + calculateMixability(Check, LType->getPointeeType(), + RType->getPointeeType(), Ctx, ICMM_None); + if (MixOfPointee.Flags & MIX_WorkaroundDisableCanonicalEquivalence) + RecursiveReturnDiscardingCanonicalType = true; + + MixOfPointee.sanitize(); + if (MixOfPointee.indicatesMixability()) { + LLVM_DEBUG(llvm::dbgs() + << "<<< calculateMixability. Pointees are mixable.\n"); + return MixOfPointee; + } + } + + if (ImplicitMode > ICMM_None) { + LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. Start implicit...\n"); + MixData MixLTR = + approximateImplicitConversion(Check, LType, RType, Ctx, ImplicitMode); + LLVM_DEBUG( + if (MixLTR.Flags & MIX_ImplicitConversion) llvm::dbgs() + << "--- calculateMixability. Implicit Left -> Right found.\n";); + + if (ImplicitMode == ICMM_OneWaySingleStandardOnly && MixLTR.Conversion && + !MixLTR.Conversion.AfterPreStd.isNull() && + MixLTR.Conversion.UDConvKind == ConversionSequence::UDCK_None && + MixLTR.Conversion.AfterPostStd.isNull()) { + // The invoker of the method requested only modelling a single standard + // conversion, in only the forward direction, and they got just that. + LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Implicit " + "conversion, one-way, standard-only.\n"); + return {MIX_ImplicitConversion, MixLTR.Conversion}; + } + + // Otherwise if the invoker requested a full modelling, do the other + // direction as well. + MixData MixRTL = + approximateImplicitConversion(Check, RType, LType, Ctx, ImplicitMode); + LLVM_DEBUG( + if (MixRTL.Flags & MIX_ImplicitConversion) llvm::dbgs() + << "--- calculateMixability. Implicit Right -> Left found.\n";); + + if (MixLTR.Conversion && MixRTL.Conversion) { + LLVM_DEBUG( + llvm::dbgs() + << "<<< calculateMixability. Implicit conversion, bidirectional.\n"); + return {MIX_ImplicitConversion, MixLTR.Conversion, MixRTL.Conversion}; + } + } + + if (RecursiveReturnDiscardingCanonicalType) + LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. Before CanonicalType, " + "Discard was enabled.\n"); + + // Certain kinds unfortunately need to be side-stepped for canonical type + // matching. + if (LType->getAs() || RType->getAs()) { + // Unfortunately, the canonical type of a function pointer becomes the + // same even if exactly one is "noexcept" and the other isn't, making us + // give a false positive report irrespective of implicit conversions. + LLVM_DEBUG(llvm::dbgs() + << "--- calculateMixability. Discarding potential canonical " + "equivalence on FunctionProtoTypes.\n"); + RecursiveReturnDiscardingCanonicalType = true; } + MixData MixToReturn{MIX_None}; + // 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()) { + QualType LCanonical = LType.getCanonicalType(); + if (LCanonical == RType.getCanonicalType()) { LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Same CanonicalType.\n"); - return {MIX_Canonical, LType.getCanonicalType()}; + MixToReturn = {MIX_Canonical, LCanonical}; } - LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. No match found.\n"); - return {MIX_None}; + if (RecursiveReturnDiscardingCanonicalType) + MixToReturn |= MIX_WorkaroundDisableCanonicalEquivalence; + + LLVM_DEBUG(if (MixToReturn.Flags == MIX_None) llvm::dbgs() + << "<<< calculateMixability. No match found.\n"); + return MixToReturn; } /// 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) { +/// +/// \param ImplicitMode is forwarded in the possible recursive call to +/// calculateMixability. +static MixData +isLRefEquallyBindingToType(const TheCheck &Check, + const LValueReferenceType *LRef, QualType Ty, + const ASTContext &Ctx, bool IsRefRHS, + ImplicitConversionModellingMode ImplicitMode) { 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';); @@ -398,8 +745,462 @@ LLVM_DEBUG( llvm::dbgs() << "--- isLRefEquallyBindingToType. Checking mix for underlying type.\n"); - return IsRefRHS ? calculateMixability(Check, Ty, NonConstReferredType, Ctx) - : calculateMixability(Check, NonConstReferredType, Ty, Ctx); + return IsRefRHS ? calculateMixability(Check, Ty, NonConstReferredType, Ctx, + ImplicitMode) + : calculateMixability(Check, NonConstReferredType, Ty, Ctx, + ImplicitMode); +} + +static inline bool isDerivedToBase(const CXXRecordDecl *Derived, + const CXXRecordDecl *Base) { + return Derived && Base && Derived->isCompleteDefinition() && + Base->isCompleteDefinition() && Derived->isDerivedFrom(Base); +} + +static Optional +approximateStandardConversionSequence(const TheCheck &Check, + const QualType From, const QualType To, + const ASTContext &Ctx) { + LLVM_DEBUG(llvm::dbgs() << ">>> approximateStdConv for LType:\n"; + From.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n"; + To.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';); + + // A standard conversion sequence consists of the following, in order: + // * Maybe either LValue->RValue conv., Array->Ptr conv., Function->Ptr conv. + // * Maybe Numeric promotion or conversion. + // * Maybe function pointer conversion. + // * Maybe qualifier adjustments. + QualType WorkType = From; + // Get out the qualifiers of the original type. This will always be + // re-applied to the WorkType to ensure it is the same qualification as the + // original From was. + auto QualifiersToApply = From.split().Quals.getAsOpaqueValue(); + + // LValue->RValue is irrelevant for the check, because it is a thing to be + // done at a call site, and will be performed if need be performed. + + // Array->Ptr decay. + if (const auto *ArrayT = dyn_cast(From)) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. Array->Ptr decayed.\n"); + WorkType = ArrayT->getPointeeType(); + } + + // Function->Pointer conversions are also irrelevant, because a + // "FunctionType" cannot be the type of a parameter variable, so this + // conversion is only meaningful at call sites. + + // Numeric promotions and conversions. + const auto *FromBuiltin = WorkType->getAs(); + const auto *ToBuiltin = To->getAs(); + bool FromNumeric = FromBuiltin && (FromBuiltin->isIntegerType() || + FromBuiltin->isFloatingType()); + bool ToNumeric = + ToBuiltin && (ToBuiltin->isIntegerType() || ToBuiltin->isFloatingType()); + if (FromNumeric && ToNumeric) { + // If both are integral types, the numeric conversion is performed. + // Reapply the qualifiers of the original type, however, so + // "const int -> double" in this case moves over to + // "const double -> double". + LLVM_DEBUG(llvm::dbgs() + << "--- approximateStdConv. Conversion between numerics.\n"); + WorkType = QualType{ToBuiltin, QualifiersToApply}; + } + + const auto *FromEnum = WorkType->getAs(); + const auto *ToEnum = To->getAs(); + if (FromEnum && ToNumeric && FromEnum->isUnscopedEnumerationType()) { + // Unscoped enumerations (or enumerations in C) convert to numerics. + LLVM_DEBUG(llvm::dbgs() + << "--- approximateStdConv. Unscoped enum to numeric.\n"); + WorkType = QualType{ToBuiltin, QualifiersToApply}; + } else if (FromNumeric && ToEnum && ToEnum->isUnscopedEnumerationType()) { + // Numeric types convert to enumerations only in C. + if (Ctx.getLangOpts().CPlusPlus) { + LLVM_DEBUG(llvm::dbgs() << "<<< approximateStdConv. Numeric to unscoped " + "enum, not possible in C++!\n"); + return {}; + } + + LLVM_DEBUG(llvm::dbgs() + << "--- approximateStdConv. Numeric to unscoped enum.\n"); + WorkType = QualType{ToEnum, QualifiersToApply}; + } + + // Check for pointer conversions. + const auto *FromPtr = WorkType->getAs(); + const auto *ToPtr = To->getAs(); + if (FromPtr && ToPtr) { + if (ToPtr->isVoidPointerType()) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. To void pointer.\n"); + WorkType = QualType{ToPtr, QualifiersToApply}; + } + + const auto *FromRecordPtr = FromPtr->getPointeeCXXRecordDecl(); + const auto *ToRecordPtr = ToPtr->getPointeeCXXRecordDecl(); + if (isDerivedToBase(FromRecordPtr, ToRecordPtr)) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. Derived* to Base*\n"); + WorkType = QualType{ToPtr, QualifiersToApply}; + } + } + + // Model the slicing Derived-to-Base too, as "BaseT temporary = derived;" + // can also be compiled. + const auto *FromRecord = WorkType->getAsCXXRecordDecl(); + const auto *ToRecord = To->getAsCXXRecordDecl(); + if (isDerivedToBase(FromRecord, ToRecord)) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. Derived To Base.\n"); + WorkType = QualType{ToRecord->getTypeForDecl(), QualifiersToApply}; + } + + if (Ctx.getLangOpts().CPlusPlus17 && FromPtr && ToPtr) { + // Function pointer conversion: A noexcept function pointer can be passed + // to a non-noexcept one. + const auto *FromFunctionPtr = + FromPtr->getPointeeType()->getAs(); + const auto *ToFunctionPtr = + ToPtr->getPointeeType()->getAs(); + if (FromFunctionPtr && ToFunctionPtr && + FromFunctionPtr->hasNoexceptExceptionSpec() && + !ToFunctionPtr->hasNoexceptExceptionSpec()) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. noexcept function " + "pointer to non-noexcept.\n"); + WorkType = QualType{ToPtr, QualifiersToApply}; + } + } + + // Qualifier adjustments are modelled according to the user's request in + // the QualifiersMix check config. + LLVM_DEBUG(llvm::dbgs() + << "--- approximateStdConv. Trying qualifier adjustment...\n"); + MixData QualConv = calculateMixability(Check, WorkType, To, Ctx, ICMM_None); + QualConv.sanitize(); + if (QualConv.Flags & MIX_Qualifiers) { + LLVM_DEBUG(llvm::dbgs() + << "<<< approximateStdConv. Qualifiers adjusted.\n"); + WorkType = To; + } + + if (WorkType == To) { + LLVM_DEBUG(llvm::dbgs() << "<<< approximateStdConv. Reached 'To' type.\n"); + return {WorkType}; + } + + LLVM_DEBUG(llvm::dbgs() << "<<< approximateStdConv. Did not reach 'To'.\n"); + return {}; +} + +namespace { + +/// Helper class for storing possible user-defined conversion calls that +/// *could* take place in an implicit conversion, and selecting the one that +/// most likely *does*, if any. +class UserDefinedConversionSelector { +public: + /// The conversion assocaited with a conversion function, together with the + /// mixability flags of the conversion function's parameter or return type + /// to the rest of the sequence the selector is used in, and the sequence + /// that applied through the conversion itself. + struct PreparedConversion { + const CXXMethodDecl *ConversionFun; + MixFlags Flags; + ConversionSequence Seq; + + PreparedConversion(const CXXMethodDecl *CMD, MixFlags F, + ConversionSequence S) + : ConversionFun(CMD), Flags(F), Seq(S) {} + }; + + UserDefinedConversionSelector(const TheCheck &Check) : Check(Check) {} + + /// Adds the conversion between the two types for the given function into + /// the possible implicit conversion set. FromType and ToType is either: + /// * the result of a standard sequence and a converting ctor parameter + /// * the return type of a conversion operator and the expected target of + /// an implicit conversion. + void addConversion(const CXXMethodDecl *ConvFun, QualType FromType, + QualType ToType) { + // Try to go from the FromType to the ToType wiht only a single implicit + // conversion, to see if the conversion function is applicable. + MixData Mix = + calculateMixability(Check, FromType, ToType, ConvFun->getASTContext(), + ICMM_OneWaySingleStandardOnly); + Mix.sanitize(); + if (!Mix.indicatesMixability()) + return; + + LLVM_DEBUG(llvm::dbgs() << "--- tryConversion. Found viable with flags: " + << formatBits(Mix.Flags) << '\n'); + FlaggedConversions.emplace_back(ConvFun, Mix.Flags, Mix.Conversion); + } + + /// Selects the best conversion function that is applicable from the + /// prepared set of potential conversion functions taken. + Optional operator()() const { + if (FlaggedConversions.empty()) { + LLVM_DEBUG(llvm::dbgs() << "--- selectUserDefinedConv. Empty.\n"); + return {}; + } + if (FlaggedConversions.size() == 1) { + LLVM_DEBUG(llvm::dbgs() << "--- selectUserDefinedConv. Single.\n"); + return FlaggedConversions.front(); + } + + Optional BestConversion; + unsigned short HowManyGoodConversions = 0; + for (const auto &Prepared : FlaggedConversions) { + LLVM_DEBUG(llvm::dbgs() << formatBits(Prepared.Flags) << '\n'); + if (!BestConversion) { + LLVM_DEBUG(llvm::dbgs() << "First match.\n"); + BestConversion = Prepared; + ++HowManyGoodConversions; + continue; + } + + bool BestConversionHasImplicit = + BestConversion->Flags & MIX_ImplicitConversion; + bool ThisConversionHasImplicit = Prepared.Flags & MIX_ImplicitConversion; + if (!BestConversionHasImplicit && ThisConversionHasImplicit) + // This is a worse conversion, because a better one was found earlier. + continue; + + if (BestConversionHasImplicit && !ThisConversionHasImplicit) { + // If the so far best selected conversion needs a previous implicit + // conversion to match the user-defined converting function, but this + // conversion does not, this is a better conversion, and we can throw + // away the previously selected conversion(s). + BestConversion = Prepared; + HowManyGoodConversions = 1; + continue; + } + + if (BestConversionHasImplicit == ThisConversionHasImplicit) + // The current conversion is the same in term of goodness than the + // already selected one. + ++HowManyGoodConversions; + } + + if (HowManyGoodConversions == 1) { + LLVM_DEBUG(llvm::dbgs() + << "--- selectUserDefinedConv. Unique result. Flags: " + << formatBits(BestConversion->Flags) << '\n'); + return BestConversion; + } + + LLVM_DEBUG(llvm::dbgs() + << "--- selectUserDefinedConv. No, or ambiguous.\n"); + return {}; + } + +private: + llvm::SmallVector FlaggedConversions; + const TheCheck &Check; +}; + +} // namespace + +static Optional +tryConversionOperators(const TheCheck &Check, const CXXRecordDecl *RD, + const QualType ToType) { + if (!RD || !RD->isCompleteDefinition()) + return {}; + RD = RD->getDefinition(); + + LLVM_DEBUG(llvm::dbgs() << ">>> tryConversionOperators: " << RD->getName() + << " to:\n"; + ToType.dump(llvm::dbgs(), RD->getASTContext()); + llvm::dbgs() << '\n';); + + UserDefinedConversionSelector ConversionSet{Check}; + + for (const NamedDecl *Method : RD->getVisibleConversionFunctions()) { + const auto *Con = dyn_cast(Method); + if (!Con || Con->isExplicit()) + continue; + LLVM_DEBUG(llvm::dbgs() << "--- tryConversionOperators. Trying:\n"; + Con->dump(llvm::dbgs()); llvm::dbgs() << '\n';); + + // Try to go from the result of conversion operator to the expected type, + // without calculating another user-defined conversion. + ConversionSet.addConversion(Con, Con->getConversionType(), ToType); + } + + if (Optional + SelectedConversion = ConversionSet()) { + QualType RecordType{RD->getTypeForDecl(), 0}; + + ConversionSequence Result{RecordType, ToType}; + // The conversion from the operator call's return type to ToType was + // modelled as a "pre-conversion" in the operator call, but it is the + // "post-conversion" from the point of view of the original conversion + // we are modelling. + Result.AfterPostStd = SelectedConversion->Seq.AfterPreStd; + + ConversionSequence::UserDefinedConversionOperator ConvOp; + ConvOp.Fun = cast(SelectedConversion->ConversionFun); + ConvOp.UserDefinedType = RecordType; + ConvOp.ConversionOperatorResultType = ConvOp.Fun->getConversionType(); + Result %= ConvOp; + + LLVM_DEBUG(llvm::dbgs() << "<<< tryConversionOperators. Found result.\n"); + return Result; + } + + LLVM_DEBUG(llvm::dbgs() << "<<< tryConversionOperators. No conversion.\n"); + return {}; +} + +static Optional +tryConvertingConstructors(const TheCheck &Check, const QualType FromType, + const CXXRecordDecl *RD) { + if (!RD || !RD->isCompleteDefinition()) + return {}; + RD = RD->getDefinition(); + + LLVM_DEBUG(llvm::dbgs() << ">>> tryConveringConstructors: " << RD->getName() + << " from:\n"; + FromType.dump(llvm::dbgs(), RD->getASTContext()); + llvm::dbgs() << '\n';); + + UserDefinedConversionSelector ConversionSet{Check}; + + for (const CXXConstructorDecl *Con : RD->ctors()) { + if (Con->isCopyOrMoveConstructor() || + !Con->isConvertingConstructor(/* AllowExplicit =*/false)) + continue; + LLVM_DEBUG(llvm::dbgs() << "--- tryConvertingConstructors. Trying:\n"; + Con->dump(llvm::dbgs()); llvm::dbgs() << '\n';); + + // Try to go from the original FromType to the converting constructor's + // parameter type without another user-defined conversion. + ConversionSet.addConversion(Con, FromType, Con->getParamDecl(0)->getType()); + } + + if (Optional + SelectedConversion = ConversionSet()) { + QualType RecordType{RD->getTypeForDecl(), 0}; + + ConversionSequence Result{FromType, RecordType}; + Result.AfterPreStd = SelectedConversion->Seq.AfterPreStd; + + ConversionSequence::UserDefinedConvertingConstructor Ctor; + Ctor.Fun = cast(SelectedConversion->ConversionFun); + Ctor.ConstructorParameterType = Ctor.Fun->getParamDecl(0)->getType(); + Ctor.UserDefinedType = RecordType; + Result %= Ctor; + + LLVM_DEBUG(llvm::dbgs() + << "<<< tryConvertingConstructors. Found result.\n"); + return Result; + } + + LLVM_DEBUG(llvm::dbgs() << "<<< tryConvertingConstructors. No conversion.\n"); + return {}; +} + +/// Returns whether an expression of LType can be used in an RType context, as +/// per the implicit conversion rules. +/// +/// Note: the result of this operation, unlike that of calculateMixability, is +/// **NOT** symmetric. +static MixData +approximateImplicitConversion(const TheCheck &Check, const QualType LType, + const QualType RType, const ASTContext &Ctx, + ImplicitConversionModellingMode ImplicitMode) { + LLVM_DEBUG(llvm::dbgs() << ">>> approximateImplicitConversion for LType:\n"; + LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n"; + RType.dump(llvm::dbgs(), Ctx); + llvm::dbgs() << "\nimplicit mode: " << ImplicitMode << '\n';); + if (LType == RType) + return {MIX_Trivial, LType}; + + // An implicit conversion sequence consists of the following, in order: + // * Maybe standard conversion sequence. + // * Maybe user-defined conversion. + // * Maybe standard conversion sequence. + ConversionSequence ImplicitSeq{LType, RType}; + QualType WorkType = LType; + + Optional AfterFirstStdConv = + approximateStandardConversionSequence(Check, LType, RType, Ctx); + if (AfterFirstStdConv) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateImplicitConversion. Standard " + "Pre-Conversion found!\n"); + ImplicitSeq.AfterPreStd = AfterFirstStdConv.getValue(); + WorkType = ImplicitSeq.AfterPreStd; + } + + if (ImplicitMode == ICMM_OneWaySingleStandardOnly) + // If the caller only requested modelling of a standard conversion, bail. + return {ImplicitSeq.AfterPreStd.isNull() ? MIX_None + : MIX_ImplicitConversion, + ImplicitSeq}; + + if (Ctx.getLangOpts().CPlusPlus) { + bool FoundConversionOperator = false, FoundConvertingCtor = false; + + if (const auto *LRD = WorkType->getAsCXXRecordDecl()) { + Optional ConversionOperatorResult = + tryConversionOperators(Check, LRD, RType); + if (ConversionOperatorResult) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateImplicitConversion. Found " + "conversion operator.\n"); + ImplicitSeq <<= ConversionOperatorResult.getValue(); + WorkType = ImplicitSeq.getTypeAfterUserDefinedConversion(); + FoundConversionOperator = true; + } + } + + if (const auto *RRD = RType->getAsCXXRecordDecl()) { + // Use the original "LType" here, and not WorkType, because the + // conversion to the converting constructors' parameters will be + // modelled in the recursive call. + Optional ConvCtorResult = + tryConvertingConstructors(Check, LType, RRD); + if (ConvCtorResult) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateImplicitConversion. Found " + "converting constructor.\n"); + ImplicitSeq <<= ConvCtorResult.getValue(); + WorkType = ImplicitSeq.getTypeAfterUserDefinedConversion(); + FoundConvertingCtor = true; + } + } + + if (FoundConversionOperator && FoundConvertingCtor) { + // If both an operator and a ctor matches, the sequence is ambiguous. + LLVM_DEBUG(llvm::dbgs() + << "<<< approximateImplicitConversion. Found both " + "user-defined conversion kinds in the same sequence!\n"); + return {MIX_None}; + } + } + + // After the potential user-defined conversion, another standard conversion + // sequence might exist. + LLVM_DEBUG( + llvm::dbgs() + << "--- approximateImplicitConversion. Try to find post-conversion.\n"); + MixData SecondStdConv = approximateImplicitConversion( + Check, WorkType, RType, Ctx, ICMM_OneWaySingleStandardOnly); + if (SecondStdConv.indicatesMixability()) { + LLVM_DEBUG(llvm::dbgs() << "--- approximateImplicitConversion. Standard " + "Post-Conversion found!\n"); + + // The single-step modelling puts the modelled conversion into the "PreStd" + // variable in the recursive call, but from the PoV of this function, it is + // the post-conversion. + ImplicitSeq.AfterPostStd = SecondStdConv.Conversion.AfterPreStd; + WorkType = ImplicitSeq.AfterPostStd; + } + + if (ImplicitSeq) { + LLVM_DEBUG(llvm::dbgs() + << "<<< approximateImplicitConversion. Found a conversion.\n"); + return {MIX_ImplicitConversion, ImplicitSeq}; + } + + LLVM_DEBUG( + llvm::dbgs() << "<<< approximateImplicitConversion. No match found.\n"); + return {MIX_None}; } static MixableParameterRange modelMixingRange(const TheCheck &Check, @@ -431,16 +1232,18 @@ << "Check mix of #" << J << " against #" << I << "...\n"); Mix M{Jth, Ith, - calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx)}; + calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx, + Check.ModelImplicitConversions ? ICMM_All + : ICMM_None)}; 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'); - assert(M.flags() != MIX_Invalid && "All flags decayed!"); + assert(M.flagsValid() && "All flags decayed!"); - if (M.flags() != MIX_None) + if (M.mixable()) MixesOfIth.emplace_back(std::move(M)); } @@ -576,8 +1379,80 @@ model::MIX_Qualifiers); } +/// Returns whether a particular Mix between the two parameters should have +/// implicit conversions elaborated. +static inline bool needsToElaborateImplicitConversion(const model::Mix &M) { + return M.flags() & model::MIX_ImplicitConversion; +} + namespace { +/// This class formats a conversion sequence into a "Ty1 -> Ty2 -> Ty3" line +/// that can be used in diagnostics. +struct FormattedConversionSequence { + std::string DiagnosticText; + + /// The formatted sequence is trivial if it is "Ty1 -> Ty2", but Ty1 and + /// Ty2 are the types that are shown in the code. A trivial diagnostic + /// does not need to be printed. + bool Trivial; + + FormattedConversionSequence(const PrintingPolicy &PP, + StringRef StartTypeAsDiagnosed, + const model::ConversionSequence &Conv, + StringRef DestinationTypeAsDiagnosed) { + Trivial = true; + llvm::raw_string_ostream OS{DiagnosticText}; + + // Print the type name as it is printed in other places in the diagnostic. + OS << '\'' << StartTypeAsDiagnosed << '\''; + std::string LastAddedType = StartTypeAsDiagnosed.str(); + std::size_t NumElementsAdded = 1; + + // However, the parameter's defined type might not be what the implicit + // conversion started with, e.g. if a typedef is found to convert. + std::string SeqBeginTypeStr = Conv.Begin.getAsString(PP); + std::string SeqEndTypeStr = Conv.End.getAsString(PP); + if (StartTypeAsDiagnosed != SeqBeginTypeStr) { + OS << " (as '" << SeqBeginTypeStr << "')"; + LastAddedType = SeqBeginTypeStr; + Trivial = false; + } + + const auto &AddType = [&](StringRef ToAdd) { + if (LastAddedType != ToAdd && ToAdd != SeqEndTypeStr) { + OS << " -> '" << ToAdd << '\''; + LastAddedType = ToAdd.str(); + ++NumElementsAdded; + } + }; + for (QualType InvolvedType : Conv.getInvolvedTypesInSequence()) + // Print every type that's unique in the sequence into the diagnosis. + AddType(InvolvedType.getAsString(PP)); + + if (LastAddedType != DestinationTypeAsDiagnosed) { + OS << " -> '" << DestinationTypeAsDiagnosed << '\''; + LastAddedType = DestinationTypeAsDiagnosed.str(); + ++NumElementsAdded; + } + + // Same reasoning as with the Begin, e.g. if the converted-to type is a + // typedef, it will not be the same inside the conversion sequence (where + // the model already tore off typedefs) as in the code. + if (DestinationTypeAsDiagnosed != SeqEndTypeStr) { + OS << " (as '" << SeqEndTypeStr << "')"; + LastAddedType = SeqEndTypeStr; + Trivial = false; + } + + if (Trivial && NumElementsAdded > 2) + // If the thing is still marked trivial but we have more than the + // from and to types added, it should not be trivial, and elaborated + // when printing the diagnostic. + Trivial = false; + } +}; + /// Retains the elements called with and returns whether the call is done with /// a new element. template class InsertOnce { @@ -658,7 +1533,9 @@ IgnoredParameterTypeSuffixes(optutils::parseStringList( Options.get("IgnoredParameterTypeSuffixes", DefaultIgnoredParameterTypeSuffixes))), - QualifiersMix(Options.get("QualifiersMix", DefaultQualifiersMix)) {} + QualifiersMix(Options.get("QualifiersMix", DefaultQualifiersMix)), + ModelImplicitConversions(Options.get("ModelImplicitConversions", + DefaultModelImplicitConversions)) {} void EasilySwappableParametersCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { @@ -668,6 +1545,7 @@ Options.store(Opts, "IgnoredParameterTypeSuffixes", optutils::serializeStringList(IgnoredParameterTypeSuffixes)); Options.store(Opts, "QualifiersMix", QualifiersMix); + Options.store(Opts, "ModelImplicitConversions", ModelImplicitConversions); } void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) { @@ -719,12 +1597,17 @@ } bool NeedsAnyTypeNote = llvm::any_of(R.Mixes, needsToPrintTypeInDiagnostic); + bool HasAnyImplicits = + llvm::any_of(R.Mixes, needsToElaborateImplicitConversion); const ParmVarDecl *First = R.getFirstParam(), *Last = R.getLastParam(); std::string FirstParamTypeAsWritten = First->getType().getAsString(PP); { StringRef DiagText; - if (NeedsAnyTypeNote) + if (HasAnyImplicits) + DiagText = "%0 adjacent parameters of %1 of convertible types are " + "easily swapped by mistake"; + else if (NeedsAnyTypeNote) DiagText = "%0 adjacent parameters of %1 of similar type are easily " "swapped by mistake"; else @@ -750,52 +1633,91 @@ // too verbose. UniqueTypeAliasDiagnosticHelper UniqueTypeAlias; InsertOnce UniqueBindPower; + InsertOnce UniqueImplicitConversion; for (const model::Mix &M : R.Mixes) { - assert(M.flags() >= model::MIX_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. - QualType LType = M.First->getType(); - QualType RType = M.Second->getType(); - QualType CommonType = M.commonUnderlyingType(); - std::string LTypeStr = LType.getAsString(PP); - std::string RTypeStr = RType.getAsString(PP); - std::string CommonTypeStr = CommonType.getAsString(PP); - - if (M.flags() & model::MIX_TypeAlias && - UniqueTypeAlias(LType, RType, CommonType)) { - StringRef DiagText; - bool ExplicitlyPrintCommonType = false; - if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) - if (M.flags() & model::MIX_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 { - DiagText = "after resolving type aliases, the common type of '%0' " - "and '%1' is '%2'"; - ExplicitlyPrintCommonType = true; - } + assert(M.mixable() && "Sentinel or false mix in result."); + if (!needsToPrintTypeInDiagnostic(M) && + !needsToElaborateImplicitConversion(M)) + continue; + + // Typedefs might result in the type of the variable needing to be + // emitted to a note diagnostic, so prepare it. + QualType LType = M.First->getType(); + QualType RType = M.Second->getType(); + QualType CommonType = M.commonUnderlyingType(); + std::string LTypeStr = LType.getAsString(PP); + std::string RTypeStr = RType.getAsString(PP); + std::string CommonTypeStr = CommonType.getAsString(PP); + + if (M.flags() & model::MIX_TypeAlias && + UniqueTypeAlias(LType, RType, CommonType)) { + StringRef DiagText; + bool ExplicitlyPrintCommonType = false; + if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) + if (M.flags() & model::MIX_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; + } + auto Diag = + diag(M.First->getOuterLocStart(), DiagText, DiagnosticIDs::Note) + << LTypeStr << RTypeStr; + if (ExplicitlyPrintCommonType) + Diag << CommonTypeStr; + } + + if (M.flags() & (model::MIX_ReferenceBind | model::MIX_Qualifiers) && + UniqueBindPower({LType, RType})) { + StringRef DiagText = "a call site binds an expression to '%0' and " + "'%1' with the same force"; + diag(M.Second->getOuterLocStart(), DiagText, DiagnosticIDs::Note) + << LTypeStr << RTypeStr; + } + + if (needsToElaborateImplicitConversion(M) && + UniqueImplicitConversion({LType, RType})) { + const model::ConversionSequence <R = + M.leftToRightConversionSequence(); + const model::ConversionSequence &RTL = + M.rightToLeftConversionSequence(); + FormattedConversionSequence LTRFmt{PP, LTypeStr, LTR, RTypeStr}; + FormattedConversionSequence RTLFmt{PP, RTypeStr, RTL, LTypeStr}; + LLVM_DEBUG(llvm::dbgs() << "Seq trivial LTR " << LTRFmt.Trivial + << ", RTL " << RTLFmt.Trivial << '\n'); + + StringRef DiagText = "'%0' and '%1' may be implicitly converted"; + if (!LTRFmt.Trivial || !RTLFmt.Trivial) + DiagText = "'%0' and '%1' may be implicitly converted: %2, %3"; + + { auto Diag = - diag(M.First->getOuterLocStart(), DiagText, DiagnosticIDs::Note) + diag(M.Second->getOuterLocStart(), DiagText, DiagnosticIDs::Note) << LTypeStr << RTypeStr; - if (ExplicitlyPrintCommonType) - Diag << CommonTypeStr; - } - if (M.flags() & (model::MIX_ReferenceBind | model::MIX_Qualifiers) && - UniqueBindPower({LType, RType})) { - StringRef DiagText = "a call site binds an expression to '%0' and " - "'%1' with the same force"; - diag(M.Second->getOuterLocStart(), DiagText, DiagnosticIDs::Note) - << LTypeStr << RTypeStr; + if (!LTRFmt.Trivial || !RTLFmt.Trivial) + Diag << LTRFmt.DiagnosticText << RTLFmt.DiagnosticText; } + + StringRef ConversionFunctionDiagText = + "the implicit conversion involves the " + "%select{|converting constructor|conversion operator}0 " + "declared here"; + if (const FunctionDecl *LFD = LTR.getUserDefinedConversionFunction()) + diag(LFD->getLocation(), ConversionFunctionDiagText, + DiagnosticIDs::Note) + << static_cast(LTR.UDConvKind); + if (const FunctionDecl *RFD = RTL.getUserDefinedConversionFunction()) + diag(RFD->getLocation(), ConversionFunctionDiagText, + DiagnosticIDs::Note) + << static_cast(RTL.UDConvKind); } } } diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst @@ -57,8 +57,40 @@ .. code-block:: c++ - void *memcpy(const void *Destination, void *Source, std::size_t N) {} + void *memcpy(const void *Destination, void *Source, std::size_t N) { /* ... */ } +.. option:: ModelImplicitConversions + + Whether to consider parameters of type ``T`` and ``U`` mixable if there + exists an implicit conversion from ``T`` to ``U`` and ``U`` to ``T``. + If `false`, the check will not consider implicitly convertible types for + mixability. + `True` turns warnings for implicit conversions on. + Defaults to `true`. + + The following examples produce a diagnostic only if + `ModelImplicitConversions` is enabled: + + .. code-block:: c++ + + void fun(int Int, double Double) { /* ... */ } + void compare(const char *CharBuf, std::string String) { /* ... */ } + + .. note:: + + Changing the qualifiers of an expression's type (e.g. from ``int`` to + ``const int``) is defined as an *implicit conversion* in the C++ + Standard. + However, the check separates this decision-making on the mixability of + differently qualified types based on whether `QualifiersMix` was + enabled. + + For example, the following code snippet will only produce a diagnostic + if **both** `QualifiersMix` and `ModelImplicitConversions` are enabled: + + .. code-block:: c++ + + void fun2(int Int, const double Double) { /* ... */ } Filtering options ^^^^^^^^^^^^^^^^^ @@ -133,7 +165,7 @@ template int add(T X, U Y) { return X + Y }; - void TheseAreNotWarnedAbout() { + void theseAreNotWarnedAbout() { printf("%d %d\n", 1, 2); // Two ints passed, they could be swapped. someOldCFunction(1, 2, 3); // Similarly, multiple ints passed. @@ -153,14 +185,43 @@ // Diagnosed: Explicit instantiation was done by the user, we can prove it // is the same type. - void Explicit(int A, Vector::element_type B) { /* ... */ } + void instantiated(int A, Vector::element_type B) { /* ... */ } // Diagnosed: The two parameter types are exactly the same. template - void Exact(typename Vector::element_type A, + void exact(typename Vector::element_type A, typename Vector::element_type B) { /* ... */ } // Skipped: The two parameters are both 'T' but we can not prove this // without actually instantiating. template - void FalseNegative(T A, typename Vector::element_type B) { /* ... */ } + void falseNegative(T A, typename Vector::element_type B) { /* ... */ } + +In the context of *implicit conversions* (when `ModelImplicitConversions` is +enabled), the modelling performed by the check +warns if the parameters are swappable and the swapped order matches implicit +conversions. +It does not model whether there exists an unrelated third type from which +*both* parameters can be given in a function call. +This means that in the following example, even while ``strs()`` clearly carries +the possibility to be called with swapped arguments (as long as the arguments +are string literals), will not be warned about. + +.. code-block:: c++ + + struct String { + String(const char *Buf); + }; + + struct StringView { + StringView(const char *Buf); + operator const char *() const; + }; + + // Skipped: Directly swapping expressions of the two type cannot mix. + // (Note: StringView -> const char * -> String would be **two** + // user-defined conversions, which is disallowed by the language.) + void strs(String Str, StringView SV) { /* ... */ } + + // Diagnosed: StringView implicitly converts to and from a buffer. + void cStr(StringView SV, const char *Buf() { /* ... */} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp @@ -3,7 +3,8 @@ // 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.QualifiersMix, value: 0} \ +// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \ // RUN: ]}' -- void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp @@ -0,0 +1,15 @@ +// 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.QualifiersMix, value: 1}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \ +// RUN: ]}' -- + +void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'numericAndQualifierConversion' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:56: note: the last parameter in the range is 'CD' +// CHECK-MESSAGES: :[[@LINE-4]]:43: note: a call site binds an expression to 'int' and 'const double' with the same force +// CHECK-MESSAGES: :[[@LINE-5]]:43: note: 'int' and 'const double' may be implicitly converted: 'int' -> 'const double' (as 'double'), 'const double' (as 'double') -> 'int' diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c @@ -0,0 +1,75 @@ +// 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.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \ +// RUN: ]}' -- + +void implicitDoesntBreakOtherStuff(int A, int B) {} +// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'implicitDoesntBreakOtherStuff' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'A' +// CHECK-MESSAGES: :[[@LINE-3]]:47: note: the last parameter in the range is 'B' + +void arrayAndPtr1(int *IP, int IA[]) { arrayAndPtr1(IA, IP); } +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr1' of similar type ('int *') +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP' +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA' + +void arrayAndPtr2(int *IP, int IA[8]) { arrayAndPtr2(IA, IP); } +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr2' of similar type ('int *') +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP' +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA' + +void arrayAndElement(int I, int IA[]) {} // NO-WARN. + +void numericConversion1(int I, double D) { numericConversion1(D, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion1' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters] +// 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 'D' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'double' may be implicitly converted{{$}} + +void numericConversion2(int I, short S) { numericConversion2(S, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion2' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in the range is 'S' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'short' may be implicitly converted{{$}} + +void numericConversion3(float F, unsigned long UL) { numericConversion3(UL, F); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion3' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:31: note: the first parameter in the range is 'F' +// CHECK-MESSAGES: :[[@LINE-3]]:48: note: the last parameter in the range is 'UL' +// CHECK-MESSAGES: :[[@LINE-4]]:34: note: 'float' and 'unsigned long' may be implicitly converted{{$}} + +enum Unscoped { U_A, + U_B }; +enum UnscopedFixed : char { UF_A, + UF_B }; + +void numericConversion4(int I, enum Unscoped U) { numericConversion4(U, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion4' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:46: note: the last parameter in the range is 'U' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'enum Unscoped' may be implicitly converted{{$}} + +void numericConversion5(int I, enum UnscopedFixed UF) { numericConversion5(UF, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion5' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:51: note: the last parameter in the range is 'UF' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'enum UnscopedFixed' may be implicitly converted{{$}} + +void numericConversion7(double D, enum Unscoped U) { numericConversion7(U, D); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion7' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-3]]:49: note: the last parameter in the range is 'U' +// CHECK-MESSAGES: :[[@LINE-4]]:35: note: 'double' and 'enum Unscoped' may be implicitly converted{{$}} + +void numericConversion8(double D, enum UnscopedFixed UF) { numericConversion8(UF, D); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion8' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-3]]:54: note: the last parameter in the range is 'UF' +// CHECK-MESSAGES: :[[@LINE-4]]:35: note: 'double' and 'enum UnscopedFixed' may be implicitly converted{{$}} + +void pointeeConverison(int *IP, double *DP) { pointeeConversion(DP, IP); } +// NO-WARN: Even though this is possible in C, a swap is diagnosed by the compiler. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp @@ -0,0 +1,303 @@ +// RUN: %check_clang_tidy -std=c++17 %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.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \ +// RUN: ]}' -- + +void implicitDoesntBreakOtherStuff(int A, int B) {} +// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'implicitDoesntBreakOtherStuff' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'A' +// CHECK-MESSAGES: :[[@LINE-3]]:47: note: the last parameter in the range is 'B' + +void arrayAndPtr1(int *IP, int IA[]) { arrayAndPtr1(IA, IP); } +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr1' of similar type ('int *') +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP' +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA' + +void arrayAndPtr2(int *IP, int IA[8]) { arrayAndPtr2(IA, IP); } +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr2' of similar type ('int *') +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP' +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA' + +void arrayAndElement(int I, int IA[]) {} // NO-WARN. + +void numericConversion1(int I, double D) { numericConversion1(D, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion1' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters] +// 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 'D' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'double' may be implicitly converted{{$}} + +void numericConversion2(int I, short S) { numericConversion2(S, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion2' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in the range is 'S' +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'short' may be implicitly converted{{$}} + +void numericConversion3(float F, unsigned long long ULL) { numericConversion3(ULL, F); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion3' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:31: note: the first parameter in the range is 'F' +// CHECK-MESSAGES: :[[@LINE-3]]:53: note: the last parameter in the range is 'ULL' +// CHECK-MESSAGES: :[[@LINE-4]]:34: note: 'float' and 'unsigned long long' may be implicitly converted{{$}} + +enum Unscoped { U_A, + U_B }; +enum UnscopedFixed : char { UF_A, + UF_B }; +enum struct Scoped { A, + B }; + +void numericConversion4(int I, Unscoped U) {} // NO-WARN. + +void numericConversion5(int I, UnscopedFixed UF) {} // NO-WARN. + +void numericConversion6(int I, Scoped S) {} // NO-WARN. + +void numericConversion7(double D, Unscoped U) {} // NO-WARN. + +void numericConversion8(double D, UnscopedFixed UF) {} // NO-WARN. + +void numericConversion9(double D, Scoped S) {} // NO-WARN. + +void numericConversionMultiUnique(int I, double D1, double D2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 3 adjacent parameters of 'numericConversionMultiUnique' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'D2' +// CHECK-MESSAGES: :[[@LINE-4]]:42: note: 'int' and 'double' may be implicitly converted{{$}} +// (Note: int<->double conversion for I<->D2 not diagnosed again.) + +typedef int MyInt; +using MyDouble = double; + +void numericConversion10(MyInt MI, MyDouble MD) { numericConversion10(MD, MI); } +// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 2 adjacent parameters of 'numericConversion10' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'MI' +// CHECK-MESSAGES: :[[@LINE-3]]:45: note: the last parameter in the range is 'MD' +// CHECK-MESSAGES: :[[@LINE-4]]:36: note: 'MyInt' and 'MyDouble' may be implicitly converted: 'MyInt' (as 'int') -> 'MyDouble' (as 'double'), 'MyDouble' (as 'double') -> 'MyInt' (as 'int') + +void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); } +// NO-WARN: Qualifier mixing is handled by a different check option. + +struct FromInt { + FromInt(int); +}; + +void oneWayConversion1(int I, FromInt FI) {} // NO-WARN: One-way. + +struct AmbiguousConvCtor { + AmbiguousConvCtor(int); + AmbiguousConvCtor(double); +}; + +void ambiguous1(long L, AmbiguousConvCtor ACC) {} // NO-WARN: Ambiguous, one-way. + +struct ToInt { + operator int() const; +}; + +void oneWayConversion2(ToInt TI, int I) {} // NO-WARN: One-way. + +struct AmbiguousConvOp { + operator int() const; + operator double() const; +}; + +void ambiguous2(AmbiguousConvOp ACO, long L) {} // NO-WARN: Ambiguous, one-way. + +struct AmbiguousEverything1; +struct AmbiguousEverything2; +struct AmbiguousEverything1 { + AmbiguousEverything1(); + AmbiguousEverything1(AmbiguousEverything2); + operator AmbiguousEverything2() const; +}; +struct AmbiguousEverything2 { + AmbiguousEverything2(); + AmbiguousEverything2(AmbiguousEverything1); + operator AmbiguousEverything1() const; +}; + +void ambiguous3(AmbiguousEverything1 AE1, AmbiguousEverything2 AE2) {} // NO-WARN: Ambiguous. + +struct Integer { + Integer(int); + operator int() const; +}; + +void userDefinedConversion1(int I1, Integer I2) { userDefinedConversion1(I2, I1); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion1' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:33: note: the first parameter in the range is 'I1' +// CHECK-MESSAGES: :[[@LINE-3]]:45: note: the last parameter in the range is 'I2' +// CHECK-MESSAGES: :[[@LINE-4]]:37: note: 'int' and 'Integer' may be implicitly converted{{$}} +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the conversion operator declared here + +struct Ambiguous { + Ambiguous(int); + Ambiguous(double); + operator long() const; + operator float() const; +}; + +void ambiguous3(char C, Ambiguous A) {} // NO-WARN: Ambiguous. + +struct CDouble { + CDouble(const double &); + operator const double &() const; +}; + +void userDefinedConversion2(double D, CDouble CD) { userDefinedConversion2(CD, D); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion2' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-3]]:47: note: the last parameter in the range is 'CD' +// CHECK-MESSAGES: :[[@LINE-4]]:39: note: 'double' and 'CDouble' may be implicitly converted: 'double' -> 'const double &' -> 'CDouble', 'CDouble' -> 'const double &' -> 'double' +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the conversion operator declared here + +void userDefinedConversion3(int I, CDouble CD) { userDefinedConversion3(CD, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion3' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:33: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'CD' +// CHECK-MESSAGES: :[[@LINE-4]]:36: note: 'int' and 'CDouble' may be implicitly converted: 'int' -> 'double' -> 'const double &' -> 'CDouble', 'CDouble' -> 'const double &' -> 'int' +// CHECK-MESSAGES: :[[@LINE-17]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-17]]:3: note: the implicit conversion involves the conversion operator declared here + +struct TDInt { + TDInt(const MyInt &); + operator MyInt() const; +}; + +void userDefinedConversion4(int I, TDInt TDI) { userDefinedConversion4(TDI, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion4' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:33: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:42: note: the last parameter in the range is 'TDI' +// CHECK-MESSAGES: :[[@LINE-4]]:36: note: 'int' and 'TDInt' may be implicitly converted: 'int' -> 'const MyInt &' -> 'TDInt', 'TDInt' -> 'MyInt' -> 'int' +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the conversion operator declared here + +struct TDIntDouble { + TDIntDouble(const MyInt &); + TDIntDouble(const MyDouble &); + operator MyInt() const; + operator MyDouble() const; +}; + +void userDefinedConversion5(int I, TDIntDouble TDID) { userDefinedConversion5(TDID, I); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion5' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:33: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:48: note: the last parameter in the range is 'TDID' +// CHECK-MESSAGES: :[[@LINE-4]]:36: note: 'int' and 'TDIntDouble' may be implicitly converted: 'int' -> 'const MyInt &' -> 'TDIntDouble', 'TDIntDouble' -> 'MyInt' -> 'int' +// CHECK-MESSAGES: :[[@LINE-11]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-10]]:3: note: the implicit conversion involves the conversion operator declared here + +void userDefinedConversion6(double D, TDIntDouble TDID) { userDefinedConversion6(TDID, D); } +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent parameters of 'userDefinedConversion6' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-3]]:51: note: the last parameter in the range is 'TDID' +// CHECK-MESSAGES: :[[@LINE-4]]:39: note: 'double' and 'TDIntDouble' may be implicitly converted: 'double' -> 'const MyDouble &' -> 'TDIntDouble', 'TDIntDouble' -> 'MyDouble' -> 'double' +// CHECK-MESSAGES: :[[@LINE-18]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-17]]:3: note: the implicit conversion involves the conversion operator declared here + +void userDefinedConversion7(char C, TDIntDouble TDID) {} // NO-WARN: Ambiguous. + +struct Forward1; +struct Forward2; + +void incomplete(Forward1 *F1, Forward2 *F2) {} // NO-WARN: Do not compare incomplete types. + +void pointeeConverison(int *IP, double *DP) {} // NO-WARN. + +void pointerConversion1(void *VP, int *IP) {} // NO-WARN: One-way. + +struct PointerBox { + PointerBox(void *); + operator int *() const; +}; + +void pointerConversion2(PointerBox PB, int *IP) { pointerConversion2(IP, PB); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'pointerConversion2' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'PB' +// CHECK-MESSAGES: :[[@LINE-3]]:45: note: the last parameter in the range is 'IP' +// CHECK-MESSAGES: :[[@LINE-4]]:40: note: 'PointerBox' and 'int *' may be implicitly converted: 'PointerBox' -> 'int *', 'int *' -> 'void *' -> 'PointerBox' +// CHECK-MESSAGES: :[[@LINE-8]]:3: note: the implicit conversion involves the conversion operator declared here +// CHECK-MESSAGES: :[[@LINE-10]]:3: note: the implicit conversion involves the converting constructor declared here + +void pointerConversion3(PointerBox PB, double *DP) {} // NO-WARN: Not convertible. + +struct Base {}; +struct Derived : Base {}; + +void pointerConversion4(Base *BP, Derived *DP) {} // NO-WARN: One-way. + +struct BaseAndDerivedInverter { + BaseAndDerivedInverter(Base); // Takes a Base + operator Derived() const; // and becomes a Derived. +}; + +void pointerConversion5(BaseAndDerivedInverter BADI, Derived D) { pointerConversion5(D, BADI); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'pointerConversion5' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:48: note: the first parameter in the range is 'BADI' +// CHECK-MESSAGES: :[[@LINE-3]]:62: note: the last parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-4]]:54: note: 'BaseAndDerivedInverter' and 'Derived' may be implicitly converted: 'BaseAndDerivedInverter' -> 'Derived', 'Derived' -> 'Base' -> 'BaseAndDerivedInverter' +// CHECK-MESSAGES: :[[@LINE-8]]:3: note: the implicit conversion involves the conversion operator declared here +// CHECK-MESSAGES: :[[@LINE-10]]:3: note: the implicit conversion involves the converting constructor declared here + +void pointerConversion6(void (*NTF)() noexcept, void (*TF)()) {} +// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise. + +using NonThrowingFunction = void (*)() noexcept; + +struct NoexceptMaker { + NoexceptMaker(void (*ThrowingFunction)()); + // Need to use a typedef here because + // "conversion function cannot convert to a function type". + // operator (void (*)() noexcept) () const; + operator NonThrowingFunction() const; +}; + +void pointerConversion7(void (*NTF)() noexcept, NoexceptMaker NM) { pointerConversion7(NM, NTF); } +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'pointerConversion7' of convertible types +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'NTF' +// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the last parameter in the range is 'NM' +// CHECK-MESSAGES: :[[@LINE-4]]:49: note: 'void (*)() noexcept' and 'NoexceptMaker' may be implicitly converted: 'void (*)() noexcept' -> 'void (*)()' -> 'NoexceptMaker', 'NoexceptMaker' -> 'NonThrowingFunction' -> 'void (*)() noexcept' +// CHECK-MESSAGES: :[[@LINE-12]]:3: note: the implicit conversion involves the converting constructor declared here +// CHECK-MESSAGES: :[[@LINE-9]]:3: note: the implicit conversion involves the conversion operator declared here + +struct ToType; +struct MiddleStep1 { + operator ToType() const; +}; +struct FromType { + operator MiddleStep1() const; +}; +struct MiddleStep2 { + operator FromType() const; +}; +struct ToType { + operator MiddleStep2() const; +}; + +void f(FromType F, ToType T) { // NO-WARN: The path takes two steps. + MiddleStep2 MS2 = T; + FromType F2 = MS2; + + MiddleStep1 MS1 = F; + ToType T2 = MS1; + + f(F2, T2); +} + +// Synthesised example from OpenCV. +template +struct TemplateConversion { + template + operator TemplateConversion() const; +}; +using IntConverter = TemplateConversion; +using FloatConverter = TemplateConversion; + +void templateConversion(IntConverter IC, FloatConverter FC) { templateConversion(FC, IC); } +// Note: even though this swap is possible, we do not model things when it comes to "template magic". +// But at least the check should not crash! diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp @@ -3,7 +3,8 @@ // 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.QualifiersMix, value: 0} \ +// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \ // RUN: ]}' -- #define assert(X) ((void)(X)) @@ -331,3 +332,6 @@ // 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: a call site binds an expression to 'typename Vector::element_type' and 'const typename Vector::element_type &' + +void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void (*Throwing)()) {} +// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp @@ -3,7 +3,8 @@ // 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.QualifiersMix, value: 0} \ +// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \ // RUN: ]}' -- int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp @@ -3,7 +3,8 @@ // 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.QualifiersMix, value: 1} \ +// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \ // RUN: ]}' -- typedef int MyInt1; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c @@ -3,7 +3,8 @@ // 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: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"}, \ -// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \ +// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \ +// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \ // RUN: ]}' -- -x c #define bool _Bool