[clang-tidy] bugprone-unchecked-optional-access: handle BloombergLP::bdlb:NullableValue::makeValue to prevent false-positives

[BaLiKfromUA]

#101450 added support for BloombergLP::bdlb::NullableValue.

However, NullableValue::makeValue and NullableValue::makeValueInplace have been missed which impacts code like this:

  if (opt.isNull()) {
    opt.makeValue(42);
  }

  opt.value(); // triggers false positive warning from `bugprone-unchecked-optional-access`

My patch addresses this issue.

Docs that I used for methods mocks

[llvmbot]

@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Valentyn Yukhymenko (BaLiKfromUA)

Changes

#101450 added support for BloombergLP::bdlb::NullableValue.

However, NullableValue::makeValue and NullableValue::makeValueInplace have been missed which impacts code like this:

  if (opt.isNull()) {
    opt.makeValue(42);
  }

  opt.value(); // triggers false positive warning from `bugprone-unchecked-optional-access`

My patch addresses this issue.

---

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

3 Files Affected:

• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h (+8)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp (+14)
• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+13)

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 4411bcfd60a74..0812677111995 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
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional<T> {
   const T &value() const &;
   T &value() &;
 
+  constexpr T &makeValue();
+
+  template <typename U>
+  constexpr T &makeValue(U&& v);
+
+  template <typename... ARGS>
+  constexpr T &makeValueInplace(ARGS &&... args);
+
   // 'operator bool' is inherited from bsl::optional
 
   constexpr bool isNull() const noexcept;
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 3167b85f0e024..b910db20b3c2e 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
@@ -141,6 +141,20 @@ void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo
   }
 }
 
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue<int> &opt1, BloombergLP::bdlb::NullableValue<int> &opt2) {
+  if (opt1.isNull()) {
+    opt1.makeValue(42);
+  }
+
+  opt1.value();
+
+  if (opt2.isNull()) {
+    opt2.makeValueInplace(42);
+  }
+
+  opt2.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 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
           isOptionalMemberCallWithNameMatcher(hasName("isNull")),
           transferOptionalIsNullCall)
 
+      // NullableValue::makeValue, NullableValue::makeValueInplace
+      // Only NullableValue has these methods, but this
+      // will also pass for other types  
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue", "makeValueInplace")),
+          [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+             LatticeTransferState &State) {
+            if (RecordStorageLocation *Loc =
+                    getImplicitObjectLocation(*E, State.Env)) {
+              setHasValue(*Loc, State.Env.getBoolLiteralValue(true), State.Env);
+            }
+          })
+
       // optional::emplace
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithNameMatcher(hasName("emplace")),

[BaLiKfromUA]

At the moment of the previous PR, there was a concern about configurability of check -- #101450 (review)

I am happy to address it separately if it's still a concern and if someone can guide me for first steps :)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vbvictor]

At the moment of the previous PR, there was a concern about configurability of check

I think I was about adding an option to configure list of optional classes. Probably OptionalClasses or similar.
I haven't seen issues about it, so it may not be a high priority.

[HerrCai0907]

I think configurable option is needed instead of hard code function name for non-std libarary.

[BaLiKfromUA]

@HerrCai0907 I am happy to do so! Are there any examples how it looks for other clang-tidy checks?

Also, do you want to do it in scope of this PR or as a separate one?
Since non-std function names (e.g. hasValue) are also processed for folly::Optional in code above my change.

Thanks for review!