llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Valentyn Yukhymenko (BaLiKfromUA) <details> <summary>Changes</summary> Initially was found in https://github.com/llvm/llvm-project/pull/168863#discussion_r2620106435 but I have reports from internal users as well. 🔎 [**Compiler explorer link** to illustrate false-positives in the main branch.](https://compiler-explorer.com/z/1GxzzK4xj) 🤖 **AI usage:** I used LLM to generate mocks by pointing to BDE source code locally. I also used LLM for code review and documentation. Manual test with real headers: ```bash balik@<!-- -->BaLiKfromUA:~/Desktop/llvm-project$ build/bin/clang-tidy --checks='-*,bugprone-unchecked-optional-access' \ bde-optional-repro.cpp -- -std=c++17 \ $(find ~/Desktop/bde/groups ~/Desktop/bde/standalones \ -mindepth 2 -maxdepth 2 -type d -printf '-I%p ') 3 warnings generated. /home/balik/Desktop/llvm-project/bde-optional-repro.cpp:41:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] 41 | opt1.value(); // <-- WARNING EXPECTED (true positive) | ^~~~ /home/balik/Desktop/llvm-project/bde-optional-repro.cpp:51:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] 51 | opt1.value(); // <-- WARNING EXPECTED (true positive) | ^~~~ /home/balik/Desktop/llvm-project/bde-optional-repro.cpp:54:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] 54 | opt2.value(); // <-- WARNING EXPECTED (true positive) | ^~~~ ``` --- Full diff: https://github.com/llvm/llvm-project/pull/224969.diff 5 Files Affected: - (modified) clang-tools-extra/docs/ReleaseNotes.md (+5) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h (+17) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h (+115-4) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp (+66) - (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+47-9) ``````````diff diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 0447458c147ad..b3063cf09d44a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -189,6 +189,11 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/bugprone/std-namespace-modification>` when checking lambda closure types used as template arguments. +- Improved {doc}`bugprone-unchecked-optional-access + <clang-tidy/checks/bugprone/unchecked-optional-access>` by fixing false + positives on `bsl::optional` and `bdlb::NullableValue` constructed from a + value or returned by `bsl::make_optional`. + - Improved {doc}`cppcoreguidelines-missing-std-forward <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by diagnosing unforwarded `auto&&` parameters in C++20 abbreviated function templates. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h index 0812677111995..729d83268fc2c 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h @@ -13,6 +13,23 @@ class NullableValue : public bsl::optional<T> { constexpr NullableValue(bsl::nullopt_t) noexcept; + /// Mock of `bdlb::NullableValue::EnableType`. + struct EnableType {}; + + template <typename OTHER> + using IfConstructsFrom = typename bsl::enable_if< + BloombergLP::bslstl::Optional_ConstructsFromType<T, OTHER>::value && + BloombergLP::bslstl::Optional_IsNotDerivedFromOptional<T, + OTHER>::value, + EnableType>::type; + + template <typename OTHER> + NullableValue(OTHER &&value, IfConstructsFrom<OTHER> = EnableType()); + + template <typename OTHER> + NullableValue(OTHER &&value, const bsl::allocator &allocator, + IfConstructsFrom<OTHER> = EnableType()); + NullableValue(const NullableValue &) = default; NullableValue(NullableValue &&) = default; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h index a12572351a41c..364e557706b7a 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h @@ -5,6 +5,32 @@ namespace bsl { class string {}; + + template <typename T> class optional; + + struct nullopt_t { + constexpr explicit nullopt_t() {} + }; + + constexpr nullopt_t nullopt; + + struct in_place_t { + constexpr explicit in_place_t() {} + }; + + constexpr in_place_t in_place; + + struct allocator_arg_t { + constexpr explicit allocator_arg_t() {} + }; + + constexpr allocator_arg_t allocator_arg; + + /// Mock of the allocator type taken by the allocator-extended constructors. + class allocator {}; + + template <bool B, class T> struct enable_if {}; + template <class T> struct enable_if<true, T> { using type = T; }; } /// Mock of `BloombergLP::bslstl::Optional_Base` @@ -20,6 +46,57 @@ constexpr bool isAllocatorAware<bsl::string>() { return true; } +/// Mock of `BloombergLP::bslstl::Optional_OptNoSuchType` +struct Optional_OptNoSuchType { + explicit Optional_OptNoSuchType(int) noexcept {} +}; + +template <class T> struct Optional_RemoveCVRef { using type = T; }; +template <class T> struct Optional_RemoveCVRef<T &> : Optional_RemoveCVRef<T> {}; +template <class T> struct Optional_RemoveCVRef<T &&> : Optional_RemoveCVRef<T> {}; +template <class T> struct Optional_RemoveCVRef<const T> : Optional_RemoveCVRef<T> {}; + +template <class T> struct Optional_IsTagType { + static constexpr bool value = false; +}; +template <> struct Optional_IsTagType<bsl::nullopt_t> { + static constexpr bool value = true; +}; +template <> struct Optional_IsTagType<bsl::in_place_t> { + static constexpr bool value = true; +}; +template <> struct Optional_IsTagType<bsl::allocator_arg_t> { + static constexpr bool value = true; +}; +template <> struct Optional_IsTagType<bsl::allocator> { + static constexpr bool value = true; +}; + +template <class T> struct Optional_IsStdOptional { + static constexpr bool value = false; +}; +template <class T> struct Optional_IsStdOptional<std::optional<T>> { + static constexpr bool value = true; +}; + +/// Mock of `BloombergLP::bslstl::Optional_ConstructsFromType`. +template <class TYPE, class ANY_TYPE> +struct Optional_ConstructsFromType { +private: + using Any = typename Optional_RemoveCVRef<ANY_TYPE>::type; + +public: + static constexpr bool value = + !Optional_IsTagType<Any>::value && !Optional_IsStdOptional<Any>::value; +}; + +/// Mock of the trait behind `BSLSTL_OPTIONAL_DEFINE_IF_NOT_DERIVED_FROM_OPTIONAL`. +template <class TYPE, class ANY_TYPE> +struct Optional_IsNotDerivedFromOptional { + static constexpr bool value = !__is_base_of( + bsl::optional<TYPE>, typename Optional_RemoveCVRef<ANY_TYPE>::type); +}; + // Note: real `Optional_Base` uses `BloombergLP::bslma::UsesBslmaAllocator` // to check if type is allocator-aware. // This is simplified mock to illustrate similar behaviour. @@ -67,11 +144,19 @@ class Optional_Base<T, false> : public std::optional<T> { /// Mock of `bsl::optional`. namespace bsl { -struct nullopt_t { - constexpr explicit nullopt_t() {} -}; +/// Mocks of the `BSLSTL_OPTIONAL_DECLARE_IF_*` macros. +template <class TYPE, class ANY_TYPE> +using Optional_IfConstructsFrom = typename enable_if< + BloombergLP::bslstl::Optional_ConstructsFromType<TYPE, ANY_TYPE>::value, + BloombergLP::bslstl::Optional_OptNoSuchType>::type; + +template <class TYPE, class ANY_TYPE> +using Optional_IfNotDerivedFromOptional = typename enable_if< + BloombergLP::bslstl::Optional_IsNotDerivedFromOptional<TYPE, + ANY_TYPE>::value, + BloombergLP::bslstl::Optional_OptNoSuchType>::type; -constexpr nullopt_t nullopt; +using Optional_NoSuchType = BloombergLP::bslstl::Optional_OptNoSuchType; template <typename T> class optional : public BloombergLP::bslstl::Optional_Base<T> { @@ -80,11 +165,37 @@ class optional : public BloombergLP::bslstl::Optional_Base<T> { constexpr optional(nullopt_t) noexcept; + template <typename ANY_TYPE = T> + optional(ANY_TYPE &&v, + Optional_IfConstructsFrom<T, ANY_TYPE> = Optional_NoSuchType(0), + Optional_IfNotDerivedFromOptional<T, ANY_TYPE> = Optional_NoSuchType(0)); + + template <typename ANY_TYPE> + optional(const std::optional<ANY_TYPE> &v, + Optional_IfConstructsFrom<T, ANY_TYPE> = Optional_NoSuchType(0), + Optional_IfNotDerivedFromOptional<T, ANY_TYPE> = Optional_NoSuchType(0)); + + template <typename... ARGS> + explicit optional(in_place_t, ARGS &&...args); + + optional(allocator_arg_t, allocator); + + template <typename ANY_TYPE = T> + optional(allocator_arg_t, allocator, ANY_TYPE &&v, + Optional_IfConstructsFrom<T, ANY_TYPE> = Optional_NoSuchType(0), + Optional_IfNotDerivedFromOptional<T, ANY_TYPE> = Optional_NoSuchType(0)); + + template <typename... ARGS> + explicit optional(allocator_arg_t, allocator, in_place_t, ARGS &&...args); + optional(const optional &) = default; optional(optional &&) = default; }; +template <typename T, typename... ARGS> +optional<T> make_optional(ARGS &&...args); + } // namespace bsl #endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp index 337474bdf7535..8bff66670f0da 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp @@ -231,6 +231,72 @@ void nullable_value_make_value(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo opt2.value(); } + +void bsl_optional_value_constructor(int v, bsl::string s) { + bsl::optional<int> opt1 = v; + opt1.value(); + + bsl::optional<int> opt2(v); + opt2.value(); + + bsl::optional<bsl::string> opt3 = s; + opt3.value(); + + bsl::optional<bsl::string> opt4(s); + opt4.value(); +} + +void bsl_optional_allocator_extended_value_constructor(bsl::string s) { + bsl::optional<bsl::string> opt(bsl::allocator_arg, bsl::allocator{}, s); + opt.value(); +} + +void bsl_optional_in_place_constructor(bsl::string s) { + bsl::optional<bsl::string> opt1(bsl::in_place, s); + opt1.value(); + + bsl::optional<bsl::string> opt2(bsl::allocator_arg, bsl::allocator{}, + bsl::in_place, s); + opt2.value(); +} + +void bsl_optional_make_optional() { + bsl::optional<int> opt = bsl::make_optional<int>(1); + opt.value(); +} + +void bsl_optional_converting_constructor(std::optional<int> src) { + bsl::optional<int> opt1 = src; + opt1.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + if (src.has_value()) { + bsl::optional<int> opt2 = src; + opt2.value(); + } +} + +void bsl_optional_empty_constructors() { + bsl::optional<bsl::string> opt1(bsl::allocator_arg, bsl::allocator{}); + opt1.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + bsl::optional<int> opt2 = bsl::nullopt; + opt2.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] +} + +void nullable_value_value_constructor(int v, bsl::string s) { + BloombergLP::bdlb::NullableValue<int> opt1 = v; + opt1.value(); + + BloombergLP::bdlb::NullableValue<bsl::string> opt2 = s; + opt2.value(); + + BloombergLP::bdlb::NullableValue<bsl::string> opt3(s, bsl::allocator{}); + opt3.value(); +} + void assertion_handler() __attribute__((analyzer_noreturn)); void function_calling_analyzer_noreturn(const bsl::optional<int>& opt) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 568564fb361f4..8bc7204a540ad 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -247,7 +247,7 @@ auto isMakeOptionalCall() { callee(functionDecl(hasAnyName( "std::make_optional", "base::make_optional", "absl::make_optional", "folly::make_optional", "bsl::make_optional"))), - hasOptionalType()); + hasOptionalOrDerivedType()); } auto nulloptTypeDecl() { @@ -264,6 +264,12 @@ auto inPlaceClass() { "bsl::in_place_t")); } +auto allocatorArgClass() { + return namedDecl(hasAnyName("std::allocator_arg_t", "bsl::allocator_arg_t")); +} + +auto hasAllocatorArgType() { return hasType(allocatorArgClass()); } + auto isOptionalNulloptConstructor() { return cxxConstructExpr( hasDeclaration(cxxConstructorDecl(parameterCountIs(1), @@ -272,15 +278,29 @@ auto isOptionalNulloptConstructor() { } auto isOptionalInPlaceConstructor() { - return cxxConstructExpr(hasArgument(0, hasType(inPlaceClass())), + return cxxConstructExpr(hasAnyArgument(hasType(inPlaceClass())), hasOptionalOrDerivedType()); } +// `optional(value, ...)`. Arguments after the value are ignored. Tag types are +// excluded because they denote other constructions. auto isOptionalValueOrConversionConstructor() { return cxxConstructExpr( unless(hasDeclaration( cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor())))), - argumentCountIs(1), hasArgument(0, unless(hasNulloptType())), + argumentCountAtLeast(1), + hasArgument(0, unless(anyOf(hasNulloptType(), hasType(inPlaceClass()), + hasAllocatorArgType()))), + hasOptionalOrDerivedType()); +} + +// `optional(allocator_arg_t, allocator, value, ...)`. +auto isOptionalAllocatorExtendedValueOrConversionConstructor() { + return cxxConstructExpr( + unless(hasDeclaration( + cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor())))), + hasArgument(0, hasAllocatorArgType()), argumentCountAtLeast(3), + hasArgument(2, unless(anyOf(hasNulloptType(), hasType(inPlaceClass())))), hasOptionalOrDerivedType()); } @@ -757,16 +777,30 @@ BoolValue &valueOrConversionHasValue(QualType DestType, const Expr &E, return State.Env.makeAtomicBoolValue(); } -void transferValueOrConversionConstructor( - const CXXConstructExpr *E, const MatchFinder::MatchResult &MatchRes, - LatticeTransferState &State) { - assert(E->getNumArgs() > 0); +void transferValueOrConversionConstructorImpl( + const CXXConstructExpr *E, unsigned ValueArgIdx, + const MatchFinder::MatchResult &MatchRes, LatticeTransferState &State) { + assert(E->getNumArgs() > ValueArgIdx); constructOptionalValue( *E, State.Env, valueOrConversionHasValue( - E->getConstructor()->getThisType()->getPointeeType(), *E->getArg(0), - MatchRes, State)); + E->getConstructor()->getThisType()->getPointeeType(), + *E->getArg(ValueArgIdx), MatchRes, State)); +} + +void transferValueOrConversionConstructor( + const CXXConstructExpr *E, const MatchFinder::MatchResult &MatchRes, + LatticeTransferState &State) { + transferValueOrConversionConstructorImpl(E, /*ValueArgIdx=*/0, MatchRes, + State); +} + +void transferAllocatorExtendedValueOrConversionConstructor( + const CXXConstructExpr *E, const MatchFinder::MatchResult &MatchRes, + LatticeTransferState &State) { + transferValueOrConversionConstructorImpl(E, /*ValueArgIdx=*/2, MatchRes, + State); } void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal, @@ -1015,6 +1049,10 @@ auto buildTransferMatchSwitch() { // optional::optional (value/conversion) .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(), transferValueOrConversionConstructor) + // optional::optional (allocator-extended value/conversion) + .CaseOfCFGStmt<CXXConstructExpr>( + isOptionalAllocatorExtendedValueOrConversionConstructor(), + transferAllocatorExtendedValueOrConversionConstructor) // optional::operator= .CaseOfCFGStmt<CXXOperatorCallExpr>( `````````` </details> https://github.com/llvm/llvm-project/pull/224969 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
