[clang-tidy]: `bugprone-unchecked-optional-access` false-positive if inside a loop we use checked optional value (might be a regression of Dataflow Analysis)

[BaLiKfromUA]

Way to reproduce

Smallest example which me and my colleague managed to make:

#include <optional>
#include <vector>

std::vector<std::optional<int>> getOptionals();

void test() {

  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
        int x = opt.value(); // NO ERROR
      }
  }

  std::optional<int> opt; 
  if (opt.has_value()) {
    for (int i = 0; i < 10; ++i) {
        int x = opt.value(); // NO ERROR
    }
  }  

  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
          for (int i = 0; i < 10; ++i) {
            int x = opt.value(); // ERROR
          }
      }
  }

  // Also happens when the loop is unbounded
  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
          for (;;) {
            int x = opt.value(); // ERROR
          }
      }
  }
}

Compiler Explorer Link

Initial problem / User Impact

After fixing #168863 I found new false-positive like:

bsl::vector<bsl::optional<bsl::string>> getOptionals();

void test_with_ball_log_and_loop() {
    BALL_LOG_SET_CATEGORY(__func__);
  
    for (const auto& opt : getOptionals()) {
        if (opt.has_value()) {
            BALL_LOG_INFO << opt.value(); // ERROR?!
        }
    }
}

BALL_LOG is BDE tooling used for logging. Link

After some experiments, I found out that I can reproduce the same issue with std::optional<int> so it was somehow connected with BALL_LOG.

Compiler Explorer Link.

Removing/expanding macros and includes resulted in example from the beginning :)

---

So even though the initial example might be niche, the original issue is a pretty common pattern.

JFYI, I cannot reproduce it with clang-18 and clang-19 releases but I can reproduce it with clang-20 and clang-21. So maybe it's a regression??

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Valentyn Yukhymenko (BaLiKfromUA)

Details

## Way to reproduce

Smallest example which me and my colleague managed to make:

#include <optional>
#include <vector>

std::vector<std::optional<int>> getOptionals();

void test() {

  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
        int x = opt.value(); // NO ERROR
      }
  }

  std::optional<int> opt; 
  if (opt.has_value()) {
    for (int i = 0; i < 10; ++i) {
        int x = opt.value(); // NO ERROR
    }
  }  

  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
          for (int i = 0; i < 10; ++i) {
            int x = opt.value(); // ERROR
          }
      }
  }

  // Also happens when the loop is unbounded
  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
          for (;;) {
            int x = opt.value(); // ERROR
          }
      }
  }
}

Compiler Explorer Link

Initial problem / User Impact

After fixing #168863 I found new false-positive like:

bsl::vector<bsl::optional<bsl::string>> getOptionals();

void test_with_ball_log_and_loop() {
    BALL_LOG_SET_CATEGORY(__func__);
  
    for (const auto& opt : getOptionals()) {
        if (opt.has_value()) {
            BALL_LOG_INFO << opt.value(); // ERROR?!
        }
    }
}

BALL_LOG is BDE tooling used for logging. Link

After some experiments, I found out that I can reproduce the same issue with std::optional<int> so it was somehow connected with BALL_LOG.

Compiler Explorer Link.

Removing/expanding macros and includes resulted in example from the beginning :)

---

So even though the initial example might be niche, the original issue is pretty common pattern reported by users.

JFYI, I cannot reproduce it with clang-18 and clang-19 releases but I can reproduce it with clang-20 and clang-21. So maybe it's a regression??

[BaLiKfromUA]

I am happy to contribute the fix and will start looking for a root cause soon.

My suspicion is that it is connected to Dataflow Framework, not the Model itself.

Since my familiarity with that part of code is almost zero, I would appreciate initial guidance and help :)

Thanks!

[BaLiKfromUA]

Just as another experiment, false-positive doesn't happen for code like this:

    for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
          int x = opt.value(); // NO ERROR
          {
            int x = opt.value(); // NO ERROR
          }
      }
  }

So it might be related with looping inside loop, not adding new scope block

[zeyi2]

I would appreciate initial guidance and help :)

(The following writeup was improved by Gemini for clarity, as I'm not a native English speaker)

Hi, I did some research into the issue and I think that in commit 6761b24, if I commented out the generic const accessor handlers:

llvm-project/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines 1120 to 1124 in ad2c2b2

	// const accessor calls
	.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
	transferConstMemberCall)
	.CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
	transferConstMemberOperatorCall)

the false positive disappears.

IMO there could be a flaw in the valueCall matcher (maybe related to 58fe7f9 or 2f0630f). In range-based for loops with reference types like const auto &opt fails to capture the call. This causes opt.value() to fall through to the new isZeroParamConstMemberCall generic matcher.

A dirty "hack" would be explicitly excluding the core std::optional methods from the generic matchers.

This ensures that even if the specialized matchers fail, we don't accidentally pollute the lattice and trigger destructive widening.

@@ -338,13 +338,15 @@ auto isValueOrNotEqX() {
 }
 
 auto isZeroParamConstMemberCall() {
-  return cxxMemberCallExpr(
-      callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+  return cxxMemberCallExpr(callee(cxxMethodDecl(
+      parameterCountIs(0), isConst(),
+      unless(hasAnyName("value", "has_value", "hasValue", "operator bool")))));
 }
 
 auto isZeroParamConstMemberOperatorCall() {
-  return cxxOperatorCallExpr(
-      callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+  return cxxOperatorCallExpr(callee(cxxMethodDecl(
+      parameterCountIs(0), isConst(),
+      unless(hasAnyName("operator*", "operator->")))));
 }
 
 auto isNonConstMemberCall() {

The ideal fix might be to repair the valueCall matcher itself, but unfortunately I'm currently unable to find a simple way to do this. But the above information should be a good starting point to continue :)

[BaLiKfromUA]

@zeyi2 Thank you a lot! These are very useful findings!

I am more familiar with Model codebase so I might be able to find a fix.

I am now travelling so will try to look on Sunday or next Monday for a fix.

IMO there could be a flaw in the valueCall matcher (maybe related to 58fe7f9 or 2f0630f). In range-based for loops with reference types like const auto &opt fails to capture the call.

58fe7f9 looks for me more related then 6761b24

How 2f0630f is related I don't understand since it just extends list of checked types, no?

[zeyi2]

How 2f0630f I don't understand yet since it just extends list of checked types, no?

Ah thanks for pointing that out, I just ran git blame and didn't do a more careful check.

[BaLiKfromUA]

In range-based for loops with reference types like const auto &opt fails to capture the call.

Indeed looks like something is failing in range-based loop

This code doesn't produce false positive:

  auto list = getOptionals();
  for (int ind = 0; ind < list.size(); ++ind) {
      const auto& opt = list[ind]; 
      if (opt.has_value()) {
          for (int i = 0; i < 10; ++i) {
            int x = opt.value();  // NO ERROR
          }
      }
  }

However I don't understand why this code works then, if only range-based loop is a trigger:

  for (const auto &opt : getOptionals()) {
      if (opt.has_value()) {
        int x = opt.value(); // NO ERROR
      }
  }

Anyway, might be a one step forward :)