[clang-tidy] Allow bugprone-unchecked-optional-access to handle calls…

… to `std::forward` The check now understands that calling `std::forward` will not modify the underlying optional value. This fixes #59705 Reviewed By: PiotrZSL Differential Revision: https://reviews.llvm.org/D147383
llvm · Apr 4, 2023 · 25956d5 · 25956d5
1 parent 3afe3db
commit 25956d5
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 4 deletions.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -175,6 +175,10 @@ Changes in existing checks
   <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/checks/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.

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
@@ -110,3 +110,50 @@ class C4 {
   }
   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();
+}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -96,7 +96,6 @@ auto hasAnyOptionalType() {
       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 @@ auto isStdSwapCall() {
                   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 @@ void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
   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 +715,6 @@ auto buildTransferMatchSwitch() {
       .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
                                        transferValueOrConversionConstructor)
 
-
       // optional::operator=
       .CaseOfCFGStmt<CXXOperatorCallExpr>(
           isOptionalValueOrConversionAssignment(),
@@ -745,6 +773,9 @@ auto buildTransferMatchSwitch() {
       // std::swap
       .CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
 
+      // std::forward
+      .CaseOfCFGStmt<CallExpr>(isStdForwardCall(), transferStdForwardCall)
+
       // opt.value_or("").empty()
       .CaseOfCFGStmt<Expr>(isValueOrStringEmptyCall(),
                            transferValueOrStringEmptyCall)
@@ -845,10 +876,12 @@ ComparisonResult UncheckedOptionalAccessModel::compare(
     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