[clang-tidy] have bugprone-unchecked-optional-access check boost::optional usage

[gribozavr2]

Could you change the tests to cover the new case? They are here: llvm-project/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

[muggenhor]

Sure. Looking at the test I'm unsure though, are the optional implementations in there stripped copies of the real implementations? Or just a minimal implementation that fits the basic optional interface in the given namespace?

[gribozavr2]

The mock optional types in the unit test are just declarations of the API - they don't need any implementations (function or method bodies should be omitted). But the declarations of classes, methods, and functions should mirror the production header closely. There are many seemingly trivial choices one can make when designing an API for a complex type like optional - for example, the specific choice of constructor overload set members, choosing to implement an overload set as a set of concrete functions vs. one function template, SFINAE on any functions, additional defaulted template parameters that the user isn't supposed to set etc. These differences would be hardly noticeable for the majority of "boring" C++ code that uses an optional, but they can be detrimental to tooling's ability to identify calls to the relevant APIs.

[PiotrZSL]

• Add some simple test (positive and negative), just to prove that it's detected
• Add release note entry
• Add entry in check documentation, that boost::optional may be partially supported
• boost::optional got other methods like get, get_value_or, get_ptr, is_initialized that won't be detected and therefor false-positive issue can be produced, add support for them or mention this limitation in documentation.

[muggenhor]

FYI: I've added a set of tests. But in the process discovered there's at least one assumption that doesn't hold for boost::optional. So I'll have to adjust the implementation for that. That's a bit more involved change, so will take some time.

(Specifically the assumption is that converting constructors/assignment operators are templates themselves: assert(F.getTemplateSpecializationArgs() != nullptr); in valueOrConversionHasValue).

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

 }
 
 static bool hasOptionalClassName(const CXXRecordDecl &RD) {
   if (!RD.getDeclName().isIdentifier())
     return false;
 
   if (RD.getName() == "optional") {
     if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()))
-      return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
+      return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl") ||
+             isTopLevelNamespaceWithName(*N, "boost");
     return false;
   }
 
   if (RD.getName() == "Optional") {
     // Check whether namespace is "::base".
     const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
     return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
   }