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 differently qualified versions 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 @@ -62,6 +62,9 @@ /// 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; + using namespace clang::ast_matchers; namespace clang { @@ -80,16 +83,24 @@ enum class MixFlags : unsigned char { Invalid = 0, //< Sentinel bit pattern. DO NOT USE! - None = 1, //< Mix between the two parameters is not possible. - Trivial = 2, //< The two mix trivially, and are the exact same type. - Canonical = 4, //< The two mix because the types refer to the same + //< 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. + WorkaroundDisableCanonicalEquivalence = 1, + + None = 2, //< Mix between the two parameters is not possible. + Trivial = 4, //< The two mix trivially, and are the exact same type. + Canonical = 8, //< The two mix because the types refer to the same // CanonicalType, but we do not elaborate as to how. - TypeAlias = 8, //< The path from one type to the other involves + TypeAlias = 16, //< The path from one type to the other involves // desugaring type aliases. - ReferenceBind = 16, //< The mix involves the binding power of "const &". - Qualifiers = 32, //< The mix involves change in the qualifiers. + ReferenceBind = 32, //< The mix involves the binding power of "const &". + Qualifiers = 64, //< The mix involves change in the qualifiers. + ImplicitConversion = 128, //< The mixing of the parameters is possible + // through implicit conversions between the types. - LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/Qualifiers) + LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/ImplicitConversion) }; LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); @@ -114,7 +125,7 @@ if (F == MixFlags::Invalid) return "#Inv!"; - SmallString<8> Str{"------"}; + SmallString<8> Str{"-------"}; if (hasFlag(F, MixFlags::None)) // Shows the None bit explicitly, as it can be applied in the recursion @@ -130,6 +141,11 @@ Str[4] = '&'; if (hasFlag(F, MixFlags::Qualifiers)) Str[5] = 'Q'; + if (hasFlag(F, MixFlags::ImplicitConversion)) + Str[6] = 'i'; + + if (hasFlag(F, MixFlags::WorkaroundDisableCanonicalEquivalence)) + Str.append("(~C)"); return Str.str().str(); } @@ -140,23 +156,254 @@ #endif // NDEBUG +/// The results of the steps of an Implicit Conversion Sequence is saved in +/// an instance of this record. +/// +/// A ConversionSequence maps the steps of the conversion with a member for +/// each type involved in the conversion. Imagine going from a hypothetical +/// Complex class to projecting it to the real part as a const double. +/// +/// I.e., given: +/// +/// struct Complex { +/// operator double() const; +/// }; +/// +/// void functionBeingAnalysed(Complex C, const double R); +/// +/// we will get the following sequence: +/// +/// (Begin=) Complex +/// +/// The first standard conversion is a qualification adjustment. +/// (AfterFirstStandard=) const Complex +/// +/// Then the user-defined conversion is executed. +/// (UDConvOp.ConversionOperatorResultType=) double +/// +/// Then this 'double' is qualifier-adjusted to 'const double'. +/// (AfterSecondStandard=) double +/// +/// The conversion's result has now been calculated, so it ends here. +/// (End=) double. +/// +/// Explicit storing of Begin and End in this record is needed, because +/// getting to what Begin and End here are needs further resolution of types, +/// e.g. in the case of typedefs: +/// +/// using Comp = Complex; +/// using CD = const double; +/// void functionBeingAnalysed2(Comp C, CD R); +/// +/// In this case, the user will be diagnosed with a potential conversion +/// between the two typedefs as written in the code, but to elaborate the +/// reasoning behind this conversion, we also need to show what the typedefs +/// mean. See FormattedConversionSequence towards the bottom of this file! +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 AfterFirstStandard; + + /// 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 AfterSecondStandard; + + /// 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 !AfterFirstStandard.isNull() || UDConvKind != UDCK_None || + !AfterSecondStandard.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; + auto EmplaceIfDifferent = [&Ret](QualType QT) { + if (QT.isNull()) + return; + if (Ret.empty()) + Ret.emplace_back(QT); + else if (Ret.back() != QT) + Ret.emplace_back(QT); + }; + + EmplaceIfDifferent(AfterFirstStandard); + 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(AfterSecondStandard); + + 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 &update(const ConversionSequence &RHS) { + if (!RHS.AfterFirstStandard.isNull()) + AfterFirstStandard = RHS.AfterFirstStandard; + 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.AfterSecondStandard.isNull()) + AfterSecondStandard = RHS.AfterSecondStandard; + + return *this; + } + + /// Sets the user-defined conversion to the given constructor. + void setConversion(const UserDefinedConvertingConstructor &UDCC) { + UDConvKind = UDCK_Ctor; + UDConvCtor = UDCC; + } + + /// Sets the user-defined conversion to the given operator. + void setConversion(const UserDefinedConversionOperator &UDCO) { + UDConvKind = UDCK_Oper; + UDConvOp = UDCO; + } + + /// 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: + return {}; + } + llvm_unreachable("Invalid UDConv kind."); + } + + const CXXMethodDecl *getUserDefinedConversionFunction() const { + switch (UDConvKind) { + case UDCK_Ctor: + return UDConvCtor.Fun; + case UDCK_Oper: + return UDConvOp.Fun; + case UDCK_None: + return {}; + } + llvm_unreachable("Invalid UDConv kind."); + } + + /// Returns the SourceRange in the text that corresponds to the interesting + /// part of the user-defined conversion. This is either the parameter type + /// in a converting constructor, or the conversion result type in a conversion + /// operator. + SourceRange getUserDefinedConversionHighlight() const { + switch (UDConvKind) { + case UDCK_Ctor: + return UDConvCtor.Fun->getParamDecl(0)->getSourceRange(); + case UDCK_Oper: + // getReturnTypeSourceRange() does not work for CXXConversionDecls as the + // returned type is physically behind the declaration's name ("operator"). + if (const FunctionTypeLoc FTL = UDConvOp.Fun->getFunctionTypeLoc()) + if (const TypeLoc RetLoc = FTL.getReturnLoc()) + return RetLoc.getSourceRange(); + return {}; + case UDCK_None: + return {}; + } + llvm_unreachable("Invalid UDConv kind."); + } +}; + /// 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 = MixFlags::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 != MixFlags::Invalid && "sanitize() called on invalid bitvec"); + MixFlags CanonicalAndWorkaround = + MixFlags::Canonical | MixFlags::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 = MixFlags::None; + return; + } + if (hasFlag(Flags, MixFlags::None)) { // If anywhere down the recursion a potential mix "path" is deemed // impossible, throw away all the other bits because the mix is not @@ -173,11 +420,34 @@ // recursion other bit(s) were set, remove the trivial bit, as it is not // trivial. Flags &= ~MixFlags::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 |= MixFlags::ImplicitConversion; + else + Flags &= ~MixFlags::ImplicitConversion; } + bool isValid() const { return Flags >= MixFlags::None; } + + bool indicatesMixability() const { return Flags > MixFlags::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. @@ -190,8 +460,14 @@ MixData qualify(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}; } }; @@ -206,7 +482,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; + } }; // NOLINTNEXTLINE(misc-redundant-expression): Seems to be a bogus warning. @@ -243,10 +527,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 +}; + +static MixData +isLRefEquallyBindingToType(const TheCheck &Check, + const LValueReferenceType *LRef, QualType Ty, + const ASTContext &Ctx, bool IsRefRHS, + ImplicitConversionModellingMode ImplicitMode); + +static MixData +approximateImplicitConversion(const TheCheck &Check, QualType LType, + QualType RType, const ASTContext &Ctx, + ImplicitConversionModellingMode ImplicitMode); + +static inline bool isUselessSugar(const Type *T) { + return isa(T); +} /// 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 @@ -257,13 +565,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, 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}; @@ -272,15 +586,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 (isUselessSugar(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 (isUselessSugar(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 @@ -288,12 +604,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) | MixFlags::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) | MixFlags::ReferenceBind; } @@ -301,13 +619,14 @@ if (LType->getAs()) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n"); return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx), - RType, Ctx) | + RType, Ctx, ImplicitMode) | MixFlags::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) | MixFlags::TypeAlias; } @@ -326,7 +645,8 @@ } return calculateMixability(Check, LType.getLocalUnqualifiedType(), - RType.getLocalUnqualifiedType(), Ctx) | + RType.getLocalUnqualifiedType(), Ctx, + ImplicitMode) | MixFlags::Qualifiers; } if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() && @@ -336,38 +656,113 @@ // 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) .qualify(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 (hasFlag(MixOfPointee.Flags, + MixFlags::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 (hasFlag(MixLTR.Flags, MixFlags::ImplicitConversion)) llvm::dbgs() + << "--- calculateMixability. Implicit Left -> Right found.\n";); + + if (ImplicitMode == ICMM_OneWaySingleStandardOnly && MixLTR.Conversion && + !MixLTR.Conversion.AfterFirstStandard.isNull() && + MixLTR.Conversion.UDConvKind == ConversionSequence::UDCK_None && + MixLTR.Conversion.AfterSecondStandard.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 {MixFlags::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 (hasFlag(MixRTL.Flags, MixFlags::ImplicitConversion)) llvm::dbgs() + << "--- calculateMixability. Implicit Right -> Left found.\n";); + + if (MixLTR.Conversion && MixRTL.Conversion) { + LLVM_DEBUG( + llvm::dbgs() + << "<<< calculateMixability. Implicit conversion, bidirectional.\n"); + return {MixFlags::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{MixFlags::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 {MixFlags::Canonical, LType.getCanonicalType()}; + MixToReturn = {MixFlags::Canonical, LCanonical}; } - LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. No match found.\n"); - return {MixFlags::None}; + if (RecursiveReturnDiscardingCanonicalType) + MixToReturn |= MixFlags::WorkaroundDisableCanonicalEquivalence; + + LLVM_DEBUG(if (MixToReturn.Flags == MixFlags::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';); @@ -414,8 +809,464 @@ 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, QualType From, + 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 (hasFlag(QualConv.Flags, MixFlags::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: " + << formatMixFlags(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() << "--- selectUserDefinedConv. Candidate flags: " + << formatMixFlags(Prepared.Flags) << '\n'); + if (!BestConversion) { + BestConversion = Prepared; + ++HowManyGoodConversions; + continue; + } + + bool BestConversionHasImplicit = + hasFlag(BestConversion->Flags, MixFlags::ImplicitConversion); + bool ThisConversionHasImplicit = + hasFlag(Prepared.Flags, MixFlags::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: " + << formatMixFlags(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, + 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.AfterSecondStandard = SelectedConversion->Seq.AfterFirstStandard; + + ConversionSequence::UserDefinedConversionOperator ConvOp; + ConvOp.Fun = cast(SelectedConversion->ConversionFun); + ConvOp.UserDefinedType = RecordType; + ConvOp.ConversionOperatorResultType = ConvOp.Fun->getConversionType(); + Result.setConversion(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, 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.AfterFirstStandard = SelectedConversion->Seq.AfterFirstStandard; + + ConversionSequence::UserDefinedConvertingConstructor Ctor; + Ctor.Fun = cast(SelectedConversion->ConversionFun); + Ctor.ConstructorParameterType = Ctor.Fun->getParamDecl(0)->getType(); + Ctor.UserDefinedType = RecordType; + Result.setConversion(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, QualType LType, + 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 {MixFlags::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.AfterFirstStandard = AfterFirstStdConv.getValue(); + WorkType = ImplicitSeq.AfterFirstStandard; + } + + if (ImplicitMode == ICMM_OneWaySingleStandardOnly) + // If the caller only requested modelling of a standard conversion, bail. + return {ImplicitSeq.AfterFirstStandard.isNull() + ? MixFlags::None + : MixFlags::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.update(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.update(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 {MixFlags::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.AfterSecondStandard = + SecondStdConv.Conversion.AfterFirstStandard; + WorkType = ImplicitSeq.AfterSecondStandard; + } + + if (ImplicitSeq) { + LLVM_DEBUG(llvm::dbgs() + << "<<< approximateImplicitConversion. Found a conversion.\n"); + return {MixFlags::ImplicitConversion, ImplicitSeq}; + } + + LLVM_DEBUG( + llvm::dbgs() << "<<< approximateImplicitConversion. No match found.\n"); + return {MixFlags::None}; } static MixableParameterRange modelMixingRange(const TheCheck &Check, @@ -447,16 +1298,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) : " << formatMixFlags(M.flags()) << '\n'); M.sanitize(); LLVM_DEBUG(llvm::dbgs() << "Mix flags (after sanitize): " << formatMixFlags(M.flags()) << '\n'); - assert(M.flags() != MixFlags::Invalid && "All flags decayed!"); + assert(M.flagsValid() && "All flags decayed!"); - if (M.flags() != MixFlags::None) + if (M.mixable()) MixesOfIth.emplace_back(std::move(M)); } @@ -595,8 +1448,80 @@ MixFlags::Qualifiers)); } +/// Returns whether a particular Mix between the two parameters should have +/// implicit conversions elaborated. +static inline bool needsToElaborateImplicitConversion(const model::Mix &M) { + return hasFlag(M.flags(), model::MixFlags::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 { @@ -677,7 +1602,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) { @@ -687,6 +1614,7 @@ Options.store(Opts, "IgnoredParameterTypeSuffixes", optutils::serializeStringList(IgnoredParameterTypeSuffixes)); Options.store(Opts, "QualifiersMix", QualifiersMix); + Options.store(Opts, "ModelImplicitConversions", ModelImplicitConversions); } void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) { @@ -740,12 +1668,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 @@ -780,55 +1713,94 @@ // too verbose. UniqueTypeAliasDiagnosticHelper UniqueTypeAlias; InsertOnce UniqueBindPower; + InsertOnce UniqueImplicitConversion; + + for (const model::Mix &M : R.Mixes) { + 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. + const ParmVarDecl *LVar = M.First; + const ParmVarDecl *RVar = M.Second; + QualType LType = LVar->getType(); + QualType RType = RVar->getType(); + QualType CommonType = M.commonUnderlyingType(); + std::string LTypeStr = LType.getAsString(PP); + std::string RTypeStr = RType.getAsString(PP); + std::string CommonTypeStr = CommonType.getAsString(PP); + + if (hasFlag(M.flags(), MixFlags::TypeAlias) && + UniqueTypeAlias(LType, RType, CommonType)) { + StringRef DiagText; + bool ExplicitlyPrintCommonType = false; + if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) + if (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; + } + + auto Diag = + diag(LVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note) + << LTypeStr << RTypeStr; + if (ExplicitlyPrintCommonType) + Diag << CommonTypeStr; + } + + if ((hasFlag(M.flags(), MixFlags::ReferenceBind) || + hasFlag(M.flags(), MixFlags::Qualifiers)) && + UniqueBindPower({LType, RType})) { + StringRef DiagText = "'%0' and '%1' parameters accept and bind the " + "same kind of values"; + diag(RVar->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}; - for (const Mix &M : R.Mixes) { - assert(M.flags() >= MixFlags::Trivial && - "Sentinel or false mix in result."); - - if (needsToPrintTypeInDiagnostic(M)) { - // Typedefs might result in the type of the variable needing to be - // emitted to a note diagnostic, so prepare it. - const ParmVarDecl *LVar = M.First; - const ParmVarDecl *RVar = M.Second; - QualType LType = LVar->getType(); - QualType RType = RVar->getType(); - QualType CommonType = M.commonUnderlyingType(); - std::string LTypeStr = LType.getAsString(PP); - std::string RTypeStr = RType.getAsString(PP); - std::string CommonTypeStr = CommonType.getAsString(PP); - - if (hasFlag(M.flags(), MixFlags::TypeAlias) && - UniqueTypeAlias(LType, RType, CommonType)) { - StringRef DiagText; - bool ExplicitlyPrintCommonType = false; - if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) - if (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 { - DiagText = "after resolving type aliases, the common type of '%0' " - "and '%1' is '%2'"; - ExplicitlyPrintCommonType = true; - } + 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(LVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note) + diag(RVar->getOuterLocStart(), DiagText, DiagnosticIDs::Note) << LTypeStr << RTypeStr; - if (ExplicitlyPrintCommonType) - Diag << CommonTypeStr; - } - if ((hasFlag(M.flags(), MixFlags::ReferenceBind) || - hasFlag(M.flags(), MixFlags::Qualifiers)) && - UniqueBindPower({LType, RType})) { - StringRef DiagText = "'%0' and '%1' parameters accept and bind the " - "same kind of values"; - diag(RVar->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) + << LTR.getUserDefinedConversionHighlight(); + if (const FunctionDecl *RFD = RTL.getUserDefinedConversionFunction()) + diag(RFD->getLocation(), ConversionFunctionDiagText, + DiagnosticIDs::Note) + << static_cast(RTL.UDConvKind) + << RTL.getUserDefinedConversionHighlight(); } } } 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: 'int' and 'const double' parameters accept and bind the same kind of values +// 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: ]}' -- namespace std { @@ -340,3 +341,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: 'typename Vector::element_type' and 'const typename Vector::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. 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