[clang][dataflow] Refine matching of optional types to anchor at top level.

[ymandel]

Summary

This patch refines the matching of the relevant optional types to anchor on the
global namespace. Previously, we could match anything with the right name
(e.g. base::Optional) even if nested within other namespaces. This over
matching resulted in an assertion violation when _different_ base::Optional
was encountered nested inside another namespace.

Fixes issue #57036.

[xazax.hun]

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

261

Why do we need two places to define what is an optional type? Would it be possible to somehow reuse the Declaration matcher on the RecordDecel here? Or would that be bad for performance?

[ymandel]

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

261

Good point. I dismissed reusing the matcher here because of performance concerns -- setting up the match is expensive (in the constant-time sense but still a lot of overhead). But, we're already using matching quite a lot so I suspect it doesn't matter. Still, what about the reverse -- use this predicate in the matcher, which is faster while still ensuring consistent results?

That said, what I really want is for the matcher logic to be made available outside of the matcher framework -- I think NamedDecl should have the core logic (parallel to getQualifiedNameAsString) and that could be used elsewhere, including the matcher.

[ymandel]

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

261

regardless, the matcher is wrong (if we keep it): it should be hasAnyName rather than anyOf and lots of hasName. not sure how I never caught that earlier...

[ymandel]

Improves the matcher and adds (missing) std qualifier to one of the names.

[ymandel]

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

261

I've updated the matcher but otherwise left the code alone. I will send a followup patch that shares the code now that I understand how to avoid the mention of the libc++ internal names.

[ymandel]

tweaks

[xazax.hun]

LG! Thanks!

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

 namespace dataflow {
 namespace {
 
 using namespace ::clang::ast_matchers;
 using LatticeTransferState = TransferState<NoopLattice>;
 
 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
-      anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
-            hasName("__optional_destruct_base"), hasName("absl::optional"),
-            hasName("base::Optional")),
+      hasAnyName("::std::optional", "::std::__optional_storage_base",
+                 "::std::__optional_destruct_base", "::absl::optional",
+                 "::base::Optional"),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }
 
 auto optionalOrAliasType() {
   return hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(optionalClass())));
 }
 

 }
 
 /// If `Type` is a reference type, returns the type of its pointee. Otherwise,
 /// returns `Type` itself.
 QualType stripReference(QualType Type) {
   return Type->isReferenceType() ? Type->getPointeeType() : Type;
 }
 
+bool isTopLevelNamespaceWithName(const NamespaceDecl &NS,
+                                 llvm::StringRef Name) {
+  return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
+         NS.getParent() != nullptr && NS.getParent()->isTranslationUnit();
+}
+
 /// Returns true if and only if `Type` is an optional type.
 bool isOptionalType(QualType Type) {
   if (!Type->isRecordType())
     return false;
-  // FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
-  auto TypeName = Type->getAsCXXRecordDecl()->getQualifiedNameAsString();
-  return TypeName == "std::optional" || TypeName == "absl::optional" ||
-         TypeName == "base::Optional";
+  const CXXRecordDecl *D = Type->getAsCXXRecordDecl();
+  if (D == nullptr || !D->getDeclName().isIdentifier())
+    return false;
+  if (D->getName() == "optional") {
+    if (const auto *N =
+        dyn_cast_or_null<NamespaceDecl>(D->getDeclContext()))
+      return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
+    return false;
+  }
+
+  if (D->getName() == "Optional") {
+    // Check whether namespace is "::base".
+    const auto *N =
+        dyn_cast_or_null<NamespaceDecl>(D->getDeclContext());
+    return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+  }
+
+  return false;
 }
 
 /// Returns the number of optional wrappers in `Type`.
 ///
 /// For example, if `Type` is `optional<optional<int>>`, the result of this
 /// function will be 2.
 int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
   if (!isOptionalType(Type))

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

INSTANTIATE_TEST_SUITE_P(
     UncheckedOptionalUseTestInst, UncheckedOptionalAccessTest,
     ::testing::Values(OptionalTypeIdentifier{"std", "optional"},
                       OptionalTypeIdentifier{"absl", "optional"},
                       OptionalTypeIdentifier{"base", "Optional"}),
     [](const ::testing::TestParamInfo<OptionalTypeIdentifier> &Info) {
       return Info.param.NamespaceName;
     });
 
+// Verifies that similarly-named types are ignored.
+TEST_P(UncheckedOptionalAccessTest, NonTrackedOptionalType) {
+  ExpectDiagnosticsFor(
+      R"(
+    namespace other {
+    namespace $ns {
+    template <typename T>
+    struct $optional {
+      T value();
+    };
+    }
+
+    void target($ns::$optional<int> opt) {
+      opt.value();
+    }
+    }
+  )");
+}
+
 TEST_P(UncheckedOptionalAccessTest, EmptyFunctionBody) {
   ExpectDiagnosticsFor(R"(
     void target() {
       (void)0;
     }
   )");
 }