[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.

[vbvictor]

Analysis reviewers, could you please take a look at this #144313 (comment). Your opinion is highly appreciated!

[BaLiKfromUA]

Friendly ping to not lose the visibility of conversation.

Waiting for follow up on #144313 (comment)

Thank you for review!

[BaLiKfromUA]

Still interested in addressing this issue!

Is it possible to move the configuration decision to a separate issue or PR? Or is it a blocker?

I'm happy to work on the configuration, just don't want to delay the fix to false-positive while we're waiting for a decision on the configuration approach.

Thanks!

[haoNoQ]

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.

All warnings/checks focused on misuse of specific standard C++ classes have to deal with that same problem at some point. If only there was a universal, reusable solution in Clang! Like, it'd be nice if there was some kind of fancy attribute to annotate classes as "similar" to standard classes:

[[clang::this_class_is_basically_a_custom("std::optional")]]
class NullableValue {

public:
    [[clang::this_method_basically_works_like("std::optional::emplace")]]
    template <typename... T> T &makeValueInplace(T &&... args);
};

An attribute-based solution would be particularly useful when you're shipping a library and you get to add the attributes to your headers. This way you don't have to ask your users to include stuff in their respective clang-tidy configs whenever they include your library. It'd simply work "out of the box" for all users.

But, of course, that's a matter of a much broader discussion. I don't think your work should be blocked on implementing any of this. I'm just saying that you're not alone in this struggle 😅

[vbvictor]

LGTM, just Release Notes formatting need to be fixed

[BaLiKfromUA]

@vbvictor thanks for approval! I addressed your remark about the release note.

Could you merge this PR, please, when you are available? I don't have merge permissions

[BaLiKfromUA]

@haoNoQ sorry for late reply, want to respond to this idea!

I agree that attribute-based solution gives much more flexibility and could benefit not only bugprone-unchecked-optional-access check but also other existing and future clang-tidy checks.

But, of course, that's a matter of a much broader discussion. I don't think your work should be blocked on implementing any of this. I'm just saying that you're not alone in this struggle 😅

If I were interested in starting such a discussion, what would be the right process to follow?

From what I understand, it might begin with an RFC on Discourse — but since this is related to introducing new attributes, it seems like it could also involve the broader Clang community, not just clang-tidy.

Will be interested to hear your opinion on this process, so I can access my capacity and the scope of work!

Thanks!

cc @vbvictor

[haoNoQ]

Hi! Yes, the usual RFC process can probably handle that, it just needs to go to the Clang Frontend section instead of the clang-tidy section. Just plan ahead a little bit, think about the motivation and the prior art, pros and cons and potential caveats, think how far you're willing to go on your own in this direction and how committed you'd be to maintaining this facility in the future, and lay it out in the RFC.

It may be a good idea to have a tiny proof-of-concept patch (implement the attribute and teach the clang-tidy check to consume it) (it's very easy to implement an attribute in clang - just add it to the list) but don't write too much code before you're able to confirm that clang maintainers are interested.

When it comes to motivation, yeah, every time you find yourself hardcoding a somewhat-standard function or class name in the checker code, it gets better when you have ways to annotate custom classes or replacements. Also the library headers thing is really useful.

Off the top of my head, here's a few places that could benefit:

• Clang Static Analyzer people such as myself, @Xazax-hun, @steakhal. (If you're mostly a clang-tidy person you know us as clang-analyzer-*.) Over the years it's been clear to us that many standard classes require some sort of manual modeling or summary-based modeling in order to handle them correctly. Every time we do that, we might as well support non-standard classes that behave the same way. Two particular pain points for us are:
  • Smart pointers that perform reference counting, such as std::shared_ptr or llvm::IntrusiveRefCntPtr. We produce weird use-after-free false positives when we misunderstand the initial reference count inside the smart pointer.
  • Collections and iterators. Collections such as std::map way are too complex for the analyzer to model manually by reading their source code. Custom collection classes are incredibly common too.
• The -Wunsafe-buffer-usage fixit machine: @jkorous-apple, @dtarditi. I'm not actively following their progress anymore so you gotta ask them, don't quote me as a confirmation that they actually need it. But I remember them expressing interest in recognizing custom replacements for std::span and other buffer containers, mostly for the purposes of suggesting replacements that are appropriate for the codebase that doesn't have access to the standard library. Identifying the appropriate replacement via attributes is one way to do that. If they still need that, an attribute-based solution would be particularly useful for them because hardcoding custom class names in the compiler proper is frowned upon. Like, it'd be super weird if compiler-proper worked differently depending on how you name your classes. That's only acceptable in static analysis tools.
• Not exactly a precise hit but: Any kind of use-after-move analysis (bugprone-use-after-move, clang-analyzer-cplusplus.Move) benefits from recognizing methods that "reset" the object to a valid state, making it legal to use after move. This usually includes assignments to the object and methods such as .reset() or .clear(). If custom classes can be annotated to tell the compiler about such methods the checker can be made significantly more powerful. Note that in this case the nature of the class itself isn't important, only the vague "role" of the method. So this is a slightly different problem but you can probably see how it's closely related and may potentially be addressed by a slight variation of your solution.

When it comes to caveats, it's somewhat important to recognize that many custom replacement classes slightly diverge from their standard counterparts.

• For example, is it really true that llvm::IntrusiveRefCntPtr "is basically a" std::shared_ptr? They may be "close enough" for some purposes (eg. they're both reference-counted smart pointers) but not for other purposes (eg. shared_ptr cannot be unwrapped into a raw pointer and later reconstructed from that raw pointer alone, say after tunneling through some plain C API). Maybe the attribute needs an extra "...for the purposes of" clause? The use-after-move example is kinda the opposite of this problem: it can be seen as "this method is basically a .reset() for an extremely vague purpose of movable class analysis regardless of all other properties of that class".
• Additionally, some custom classes would have methods that have absolutely no counterparts in the original class. For example, they could perform multiple standard operations in one step. Or they may perform an operation to which your checker is sensitive in a very weird way. Depending on how your checker works and what practical tradeoffs you're making in your analysis, it may or may not be important to annotate these methods as "weird" to let the checker know that the checker needs to actively forget the information it otherwise thinks it knows. (Or the checker may do that by default with every unannotated method; in this case a "weird" annotation isn't necessary.)