[clang-tidy] Add support for bsl::optional #101450

ccotter · 2024-08-01T03:51:15Z

No description provided.

llvmbot · 2024-08-01T03:51:46Z

@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Chris Cotter (ccotter)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/101450.diff

5 Files Affected:

(modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
(added) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h (+38)
(added) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h (+75)
(modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp (+91)
(modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+49-13)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 642ad39cc0c1c..cc999b142561f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`bugprone-unchecked-optional-access
+  <clang-tidy/checks/bugprone/unchecked-optional-access>` to support
+  `bsl::optional` and `bdlb::NullableValue` from
+  <https://github.com/bloomberg/bde>_.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
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
new file mode 100644
index 0000000000000..53efebba1bb9f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -0,0 +1,38 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_
+
+#include "bsl_optional.h"
+
+/// Mock of `bdbl::NullableValue`.
+namespace BloombergLP::bdlb {
+
+template <typename T>
+class NullableValue : public bsl::optional<T> {
+public:
+  constexpr NullableValue() noexcept;
+
+  constexpr NullableValue(bsl::nullopt_t) noexcept;
+
+  NullableValue(const NullableValue &) = default;
+
+  NullableValue(NullableValue &&) = default;
+
+  const T &value() const &;
+  T &value() &;
+
+  // 'operator bool' is inherited from bsl::optional
+
+  constexpr bool isNull() const noexcept;
+
+  template <typename U>
+  constexpr T valueOr(U &&v) const &;
+
+  // 'reset' is inherited from bsl::optional
+
+  template <typename U> NullableValue &operator=(const U &u);
+};
+
+
+} // namespace BloombergLP::bdlb
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_
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
new file mode 100644
index 0000000000000..7e1a129e04a55
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h
@@ -0,0 +1,75 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_
+
+/// Mock of `bsl::optional`.
+namespace bsl {
+
+// clang-format off
+template <typename T> struct remove_reference      { using type = T; };
+template <typename T> struct remove_reference<T&>  { using type = T; };
+template <typename T> struct remove_reference<T&&> { using type = T; };
+// clang-format on
+
+template <typename T>
+using remove_reference_t = typename remove_reference<T>::type;
+
+template <typename T>
+constexpr T &&forward(remove_reference_t<T> &t) noexcept;
+
+template <typename T>
+constexpr T &&forward(remove_reference_t<T> &&t) noexcept;
+
+template <typename T>
+constexpr remove_reference_t<T> &&move(T &&x);
+
+struct nullopt_t {
+  constexpr explicit nullopt_t() {}
+};
+
+constexpr nullopt_t nullopt;
+
+template <typename T>
+class optional {
+public:
+  constexpr optional() noexcept;
+
+  constexpr optional(nullopt_t) noexcept;
+
+  optional(const optional &) = default;
+
+  optional(optional &&) = default;
+
+  const T &operator*() const &;
+  T &operator*() &;
+  const T &&operator*() const &&;
+  T &&operator*() &&;
+
+  const T *operator->() const;
+  T *operator->();
+
+  const T &value() const &;
+  T &value() &;
+  const T &&value() const &&;
+  T &&value() &&;
+
+  constexpr explicit operator bool() const noexcept;
+  constexpr bool has_value() const noexcept;
+
+  template <typename U>
+  constexpr T value_or(U &&v) const &;
+  template <typename U>
+  T value_or(U &&v) &&;
+
+  template <typename... Args>
+  T &emplace(Args &&...args);
+
+  void reset() noexcept;
+
+  void swap(optional &rhs) noexcept;
+
+  template <typename U> optional &operator=(const U &u);
+};
+
+} // 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 13a3ff52f3ebc..3167b85f0e024 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
@@ -2,6 +2,8 @@
 
 #include "absl/types/optional.h"
 #include "folly/types/Optional.h"
+#include "bde/types/bsl_optional.h"
+#include "bde/types/bdlb_nullablevalue.h"
 
 void unchecked_value_access(const absl::optional<int> &opt) {
   opt.value();
@@ -50,6 +52,95 @@ void folly_checked_access(const folly::Optional<int> &opt) {
   }
 }
 
+void bsl_optional_unchecked_value_access(const bsl::optional<int> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  int x = *opt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  if (!opt) {
+    return;
+  }
+
+  opt.value();
+  x = *opt;
+}
+
+void bsl_optional_checked_access(const bsl::optional<int> &opt) {
+  if (opt.has_value()) {
+    opt.value();
+  }
+  if (opt) {
+    opt.value();
+  }
+}
+
+void bsl_optional_value_after_swap(bsl::optional<int> &opt1, bsl::optional<int> &opt2) {
+  if (opt1) {
+    opt1.swap(opt2);
+    opt1.value();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+  }
+}
+
+void nullable_value_unchecked_value_access(const BloombergLP::bdlb::NullableValue<int> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  int x = *opt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  if (opt.isNull()) {
+    opt.value();
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  if (!opt) {
+    opt.value();
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  if (!opt) {
+    return;
+  }
+
+  opt.value();
+  x = *opt;
+}
+
+void nullable_value_optional_checked_access(const BloombergLP::bdlb::NullableValue<int> &opt) {
+  if (opt.has_value()) {
+    opt.value();
+  }
+  if (opt) {
+    opt.value();
+  }
+  if (!opt.isNull()) {
+    opt.value();
+  }
+}
+
+void nullable_value_emplaced(BloombergLP::bdlb::NullableValue<int> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+
+  opt.emplace(1);
+  opt.value();
+
+  opt.reset();
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+}
+
+void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, BloombergLP::bdlb::NullableValue<int> &opt2) {
+  if (opt1) {
+    opt1.swap(opt2);
+    opt1.value();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+  }
+}
+
 template <typename T>
 void function_template_without_user(const absl::optional<T> &opt) {
   opt.value(); // no-warning
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 0707aa662e4cc..9d88754639fa3 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -38,10 +38,22 @@
 namespace clang {
 namespace dataflow {
 
-static bool isTopLevelNamespaceWithName(const NamespaceDecl &NS,
-                                        llvm::StringRef Name) {
-  return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
-         NS.getParent() != nullptr && NS.getParent()->isTranslationUnit();
+template <class... NameTypes>
+static bool hasNestedNamespace(const NamespaceDecl &NS, llvm::StringRef Name,
+                               NameTypes... Names) {
+  if (!(NS.getDeclName().isIdentifier() && NS.getName() == Name &&
+        NS.getParent() != nullptr))
+    return false;
+
+  if constexpr (sizeof...(NameTypes) > 0) {
+    if (NS.getParent()->isTranslationUnit())
+      return false;
+    if (const auto *NextNS = dyn_cast_or_null<NamespaceDecl>(NS.getParent()))
+      return hasNestedNamespace(*NextNS, Names...);
+    return false;
+  } else {
+    return NS.getParent()->isTranslationUnit();
+  }
 }
 
 static bool hasOptionalClassName(const CXXRecordDecl &RD) {
@@ -50,15 +62,21 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) {
 
   if (RD.getName() == "optional") {
     if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()))
-      return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
+      return N->isStdNamespace() || hasNestedNamespace(*N, "absl") ||
+             hasNestedNamespace(*N, "bsl");
     return false;
   }
 
   if (RD.getName() == "Optional") {
     // Check whether namespace is "::base" or "::folly".
     const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
-    return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
-                            isTopLevelNamespaceWithName(*N, "folly"));
+    return N != nullptr &&
+           (hasNestedNamespace(*N, "base") || hasNestedNamespace(*N, "folly"));
+  }
+
+  if (RD.getName() == "NullableValue") {
+    const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
+    return N != nullptr && hasNestedNamespace(*N, "bdlb", "BloombergLP");
   }
 
   return false;
@@ -195,22 +213,25 @@ auto isOptionalOperatorCallWithName(
 }
 
 auto isMakeOptionalCall() {
-  return callExpr(callee(functionDecl(hasAnyName(
-                      "std::make_optional", "base::make_optional",
-                      "absl::make_optional", "folly::make_optional"))),
-                  hasOptionalType());
+  return callExpr(
+      callee(functionDecl(hasAnyName(
+          "std::make_optional", "base::make_optional", "absl::make_optional",
+          "folly::make_optional", "bsl::make_optional"))),
+      hasOptionalType());
 }
 
 auto nulloptTypeDecl() {
   return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
-                              "base::nullopt_t", "folly::None"));
+                              "base::nullopt_t", "folly::None",
+                              "bsl::nullopt_t"));
 }
 
 auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
 
 auto inPlaceClass() {
   return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
-                               "base::in_place_t", "folly::in_place_t"));
+                               "base::in_place_t", "folly::in_place_t",
+                               "bsl::in_place_t"));
 }
 
 auto isOptionalNulloptConstructor() {
@@ -415,6 +436,15 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
   }
 }
 
+void transferOptionalIsNullCall(const CXXMemberCallExpr *CallExpr,
+                                const MatchFinder::MatchResult &,
+                                LatticeTransferState &State) {
+  if (auto *HasValueVal = getHasValue(
+          State.Env, getImplicitObjectLocation(*CallExpr, State.Env))) {
+    State.Env.setValue(*CallExpr, State.Env.makeNot(*HasValueVal));
+  }
+}
+
 /// `ModelPred` builds a logical formula relating the predicate in
 /// `ValueOrPredExpr` to the optional's `has_value` property.
 void transferValueOrImpl(
@@ -784,6 +814,12 @@ auto buildTransferMatchSwitch() {
           isOptionalMemberCallWithNameMatcher(hasName("operator bool")),
           transferOptionalHasValueCall)
 
+      // NullableValue::isNull
+      // Only NullableValue has isNull
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithNameMatcher(hasAnyName("isNull")),
+          transferOptionalIsNullCall)
+
       // optional::emplace
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithNameMatcher(hasName("emplace")),

PiotrZSL

I'm fine with change, but at the same time this check should be made configurable.
Also please update check documentation before committing.

ccotter · 2024-08-01T18:12:03Z

Agreed it should be configurable.

I updated the docs, would you mind merging if you are OK as I don't have perms?

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

ccotter · 2024-08-12T19:50:44Z

@ymand could you please re-review?

ccotter · 2024-08-19T02:41:23Z

bump @ymand @PiotrZSL?

dayshah

huge for bloomberg 🔥

ccotter · 2024-09-18T13:01:49Z

bump @ymand @PiotrZSL?

nicovank

LGTM. Rebase to fix release notes conflict and I'll merge 👍

...clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h

…hecked-optional-access/bde/types/bdlb_nullablevalue.h Co-authored-by: Nicolas van Kempen <[email protected]>

[clang-tidy] Add support for bsl::optional

f7e7681

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang-tidy clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Aug 1, 2024

PiotrZSL approved these changes Aug 1, 2024

View reviewed changes

Update docs

5ed4ef3

ymand requested changes Aug 2, 2024

View reviewed changes

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp Outdated Show resolved Hide resolved

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp Outdated Show resolved Hide resolved

ccotter added 2 commits August 9, 2024 14:05

Review feedback

bd2aa7d

Merge remote-tracking branch 'upstream/main' into bsl

be9b79e

Merge branch 'main' into bsl

884a44a

ccotter requested a review from ymand August 22, 2024 21:07

dayshah approved these changes Aug 26, 2024

View reviewed changes

nicovank approved these changes Sep 25, 2024

View reviewed changes

...clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h Outdated Show resolved Hide resolved

ccotter and others added 2 commits September 24, 2024 21:42

Merge branch 'main' of https://github.com/llvm/llvm-project into bsl

a064dd9

Update clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unc…

28e9ec4

…hecked-optional-access/bde/types/bdlb_nullablevalue.h Co-authored-by: Nicolas van Kempen <[email protected]>

nicovank merged commit 11c423f into llvm:main Sep 25, 2024
9 checks passed

ccotter deleted the bsl branch February 6, 2025 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] Add support for bsl::optional #101450

[clang-tidy] Add support for bsl::optional #101450

Uh oh!

ccotter commented Aug 1, 2024

Uh oh!

llvmbot commented Aug 1, 2024 •

edited

Loading

Uh oh!

PiotrZSL left a comment

Uh oh!

ccotter commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

ccotter commented Aug 12, 2024

Uh oh!

ccotter commented Aug 19, 2024

Uh oh!

dayshah left a comment

Uh oh!

ccotter commented Sep 18, 2024

Uh oh!

nicovank left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[clang-tidy] Add support for bsl::optional #101450

[clang-tidy] Add support for bsl::optional #101450

Uh oh!

Conversation

ccotter commented Aug 1, 2024

Uh oh!

llvmbot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

ccotter commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

ccotter commented Aug 12, 2024

Uh oh!

ccotter commented Aug 19, 2024

Uh oh!

dayshah left a comment

Choose a reason for hiding this comment

Uh oh!

ccotter commented Sep 18, 2024

Uh oh!

nicovank left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Aug 1, 2024 •

edited

Loading