[clang-tidy] Ignore unevaluated context in bugprone-optional-value-conversion

[PiotrZSL]

Ignore optionals in unevaluated context, like static_assert or decltype.

Closes #89593

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Changes

Ignore optionals in unevaluated context, like static_assert or decltype.

Closes #89593

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp (+3-1)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp (+2)

diff --git a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
index 9ab59e6b0474f0..600eab37552766 100644
--- a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
@@ -71,7 +71,9 @@ void OptionalValueConversionCheck::registerMatchers(MatchFinder *Finder) {
               ofClass(matchers::matchesAnyListedName(OptionalTypes)))),
           hasType(ConstructTypeMatcher),
           hasArgument(0U, ignoringImpCasts(anyOf(OptionalDereferenceMatcher,
-                                                 StdMoveCallMatcher))))
+                                                 StdMoveCallMatcher))),
+          unless(anyOf(hasAncestor(typeLoc()),
+                       hasAncestor(expr(matchers::hasUnevaluatedContext())))))
           .bind("expr"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3038d2b125f20d..062e619e0fe141 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -190,6 +190,10 @@ Changes in existing checks
   eliminating false positives resulting from direct usage of bitwise operators
   within parentheses.
 
+- Improved :doc:`bugprone-optional-value-conversion
+  <clang-tidy/checks/bugprone/optional-value-conversion>` check by eliminating
+  false positives resulting from use of optionals in unevaluated context.
+
 - Improved :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check by replacing the local
   options `HeaderFileExtensions` and `ImplementationFileExtensions` by the
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
index 72ef35c956d2e8..1228d64bb6909e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
@@ -210,4 +210,6 @@ void correct(std::optional<int> param)
   std::optional<long>* p2 = &p;
   takeOptionalValue(p2->value_or(5U));
   takeOptionalRef(p2->value_or(5U));
+
+  using Type = decltype(takeOptionalValue(*param));
 }

[5chmidti]

Do we maybe want to explicitly match decltype, sizeof, static_cast etc. instead of typeLoc?
Because I don't think we would want to ignore https://godbolt.org/z/r3bK961do

#include <optional>

constexpr std::optional<int> foo() { return 42; }

constexpr int select(std::optional<int> val) { return val.value_or(0); }

template <int val>
struct bar {};

void use() { using F = bar<select(foo().value())>; }

(There are other checks with this ignore pattern, depending on the answer to my question we should check those as well)

[PiotrZSL]

@5chmidti
I did some testing with this. And in example that you provided there is no issue.
This is because if optional will be uninitialized, then this code won't compile regardless if .value() or operator * is used.
Compiler will simply complain that expression is not constexpr.

When used normally, you will get undefined behavior or exception.

[5chmidti]

And in example that you provided there is no issue.

Alright, at least not a bugrprone issue. And the argument for a readability issue (which is the direction I initially was going) would probably end in someone saying we use this to diagnose compile errors earlier, not deeper in any instantiations