AMS21 created this revision.
AMS21 added reviewers: PiotrZSL, njames93, carlosgalvezp.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
AMS21 requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.
The check now understands that calling `std::forward`
will not modify the underlying optional value.
This fixes llvm#59705
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D147383
Files:
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ 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,32 @@
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<T> object is composed of two values - a `has_value`
@@ -686,7 +716,6 @@
.CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
transferValueOrConversionConstructor)
-
// optional::operator=
.CaseOfCFGStmt<CXXOperatorCallExpr>(
isOptionalValueOrConversionAssignment(),
@@ -745,6 +774,9 @@
// std::swap
.CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+ // std::forward
+ .CaseOfCFGStmt<CallExpr>(isStdForwardCall(), transferStdForwardCall)
+
// opt.value_or("").empty()
.CaseOfCFGStmt<Expr>(isValueOrStringEmptyCall(),
transferValueOrStringEmptyCall)
@@ -845,10 +877,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
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -110,3 +110,50 @@
}
int foo_;
};
+
+// llvm#59705
+namespace std
+{
+ template <typename T>
+ constexpr T&& forward(T& type) noexcept {
+ return static_cast<T&&>(type);
+ }
+
+ template <typename T>
+ constexpr T&& forward(T&& type) noexcept {
+ return static_cast<T&&>(type);
+ }
+}
+
+void std_forward_copy(absl::optional<int> opt) {
+ std::forward<absl::optional<int>>(opt).value();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional
+}
+
+void std_forward_copy_safe(absl::optional<int> opt) {
+ if (!opt) return;
+
+ std::forward<absl::optional<int>>(opt).value();
+}
+
+void std_forward_copy(absl::optional<int>& opt) {
+ std::forward<absl::optional<int>>(opt).value();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional
+}
+
+void std_forward_lvalue_ref_safe(absl::optional<int>& opt) {
+ if (!opt) return;
+
+ std::forward<absl::optional<int>>(opt).value();
+}
+
+void std_forward_copy(absl::optional<int>&& opt) {
+ std::forward<absl::optional<int>>(opt).value();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional
+}
+
+void std_forward_rvalue_ref_safe(absl::optional<int>&& opt) {
+ if (!opt) return;
+
+ std::forward<absl::optional<int>>(opt).value();
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -175,6 +175,10 @@
<clang-tidy/checks/bugprone/suspicious-include>` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-unchecked-optional-access
+ <clang-tidy/decks/bugprone/unchecked-optional-access>` check to properly handle calls
+ to ``std::forward``.
+
- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
initializers.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
