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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added reviewers: xazax.hun, gribozavr2.
Herald added subscribers: martong, rnkovacs.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148344

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1353,6 +1353,25 @@
       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() {
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -43,9 +43,10 @@
 
 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
-      anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
-            hasName("__optional_destruct_base"), hasName("absl::optional"),
-            hasName("base::Optional")),
+      anyOf(hasName("::std::optional"),
+            hasName("::std::__optional_storage_base"),
+            hasName("__optional_destruct_base"), hasName("::absl::optional"),
+            hasName("::base::Optional")),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }
 
@@ -251,14 +252,34 @@
   return Type->isReferenceType() ? Type->getPointeeType() : Type;
 }
 
+static bool isTopLevelNamespaceWithName(const NamespaceDecl &NS,
+                                        std::string_view 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`.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D148344.513596.patch
Type: text/x-patch
Size: 3224 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230414/8f6feccf/attachment.bin>