diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -175,6 +175,10 @@ ` check. Global options of the same name should be used instead. +- Improved :doc:`bugprone-unchecked-optional-access + ` check to properly handle calls + to ``std::forward``. + - Improved :doc:`bugprone-use-after-move ` check to also cover constructor initializers. 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 --- 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 @@ -110,3 +110,50 @@ } int foo_; }; + +// llvm#59705 +namespace std +{ + template + constexpr T&& forward(T& type) noexcept { + return static_cast(type); + } + + template + constexpr T&& forward(T&& type) noexcept { + return static_cast(type); + } +} + +void std_forward_copy(absl::optional opt) { + std::forward>(opt).value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional +} + +void std_forward_copy_safe(absl::optional opt) { + if (!opt) return; + + std::forward>(opt).value(); +} + +void std_forward_copy(absl::optional& opt) { + std::forward>(opt).value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional +} + +void std_forward_lvalue_ref_safe(absl::optional& opt) { + if (!opt) return; + + std::forward>(opt).value(); +} + +void std_forward_copy(absl::optional&& opt) { + std::forward>(opt).value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional +} + +void std_forward_rvalue_ref_safe(absl::optional&& opt) { + if (!opt) return; + + std::forward>(opt).value(); +} diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -96,7 +96,6 @@ recordType(hasDeclaration(anyOf(nulloptTypeDecl(), optionalClass()))))); } - auto inPlaceClass() { return recordDecl( hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t")); @@ -149,6 +148,11 @@ hasArgument(1, hasOptionalType())); } +auto isStdForwardCall() { + return callExpr(callee(functionDecl(hasName("std::forward"))), + argumentCountIs(1), hasArgument(0, hasOptionalType())); +} + constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall"; auto isValueOrStringEmptyCall() { @@ -571,6 +575,31 @@ transferSwap(*E->getArg(0), SkipPast::Reference, *E->getArg(1), State.Env); } +void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(E->getNumArgs() == 1); + + StorageLocation *LocRet = State.Env.getStorageLocation(*E, SkipPast::None); + if (LocRet != nullptr) + return; + + StorageLocation *LocArg = + State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference); + + if (LocArg == nullptr) + return; + + Value *ValArg = State.Env.getValue(*LocArg); + if (ValArg == nullptr) + ValArg = &createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()); + + // Create a new storage location + LocRet = &State.Env.createStorageLocation(*E); + State.Env.setStorageLocation(*E, *LocRet); + + State.Env.setValue(*LocRet, *ValArg); +} + BoolValue &evaluateEquality(Environment &Env, BoolValue &EqVal, BoolValue &LHS, BoolValue &RHS) { // Logically, an optional object is composed of two values - a `has_value` @@ -686,7 +715,6 @@ .CaseOfCFGStmt(isOptionalValueOrConversionConstructor(), transferValueOrConversionConstructor) - // optional::operator= .CaseOfCFGStmt( isOptionalValueOrConversionAssignment(), @@ -745,6 +773,9 @@ // std::swap .CaseOfCFGStmt(isStdSwapCall(), transferStdSwapCall) + // std::forward + .CaseOfCFGStmt(isStdForwardCall(), transferStdForwardCall) + // opt.value_or("").empty() .CaseOfCFGStmt(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall) @@ -845,10 +876,12 @@ return ComparisonResult::Unknown; bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1); bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2); - if (MustNonEmpty1 && MustNonEmpty2) return ComparisonResult::Same; + if (MustNonEmpty1 && MustNonEmpty2) + return ComparisonResult::Same; // If exactly one is true, then they're different, no reason to check whether // they're definitely empty. - if (MustNonEmpty1 || MustNonEmpty2) return ComparisonResult::Different; + if (MustNonEmpty1 || MustNonEmpty2) + return ComparisonResult::Different; // Check if they're both definitely empty. return (isEmptyOptional(Val1, Env1) && isEmptyOptional(Val2, Env2)) ? ComparisonResult::Same