[PATCH] D148377: [clang][dataflow] Drop optional model's dependency on libc++ internals.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG09b462ef8539: [clang][dataflow] Drop optional models 
dependency on libc++ internals. (authored by ymandel).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148377/new/

https://reviews.llvm.org/D148377

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
@@ -1784,8 +1784,7 @@
   )");
 }
 
-TEST_P(UncheckedOptionalAccessTest, ValueOrComparison) {
-  // Pointers.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonPointers) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1798,8 +1797,9 @@
   }
 }
   )code");
+}
 
-  // Integers.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonIntegers) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1812,8 +1812,9 @@
   }
 }
   )code");
+}
 
-  // Strings.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonStrings) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1839,9 +1840,9 @@
   }
 }
   )code");
+}
 
-  // Pointer-to-optional.
-  //
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonPointerToOptional) {
   // FIXME: make `opt` a parameter directly, once we ensure that all `optional`
   // values have a `has_value` property.
   ExpectDiagnosticsFor(
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Stmt.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
@@ -36,16 +37,44 @@
 
 namespace clang {
 namespace dataflow {
+
+static bool isTopLevelNamespaceWithName(const NamespaceDecl ,
+llvm::StringRef Name) {
+  return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
+ NS.getParent() != nullptr && NS.getParent()->isTranslationUnit();
+}
+
+static bool hasOptionalClassName(const CXXRecordDecl ) {
+  if (!RD.getDeclName().isIdentifier())
+return false;
+
+  if (RD.getName() == "optional") {
+if (const auto *N = dyn_cast_or_null(RD.getDeclContext()))
+  return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
+return false;
+  }
+
+  if (RD.getName() == "Optional") {
+// Check whether namespace is "::base".
+const auto *N = dyn_cast_or_null(RD.getDeclContext());
+return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+  }
+
+  return false;
+}
+
 namespace {
 
 using namespace ::clang::ast_matchers;
 using LatticeTransferState = TransferState;
 
+AST_MATCHER(CXXRecordDecl, hasOptionalClassNameMatcher) {
+  return hasOptionalClassName(Node);
+}
+
 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
-  hasAnyName("::std::optional", "::std::__optional_storage_base",
- "::std::__optional_destruct_base", "::absl::optional",
- "::base::Optional"),
+  hasOptionalClassNameMatcher(),
   hasTemplateArgument(0, refersToType(type().bind("T";
 }
 
@@ -63,8 +92,10 @@
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
 : cxxThisExpr());
   return cxxMemberCallExpr(
-  on(expr(Exception)),
-  callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass();
+  on(expr(Exception,
+  anyOf(hasOptionalType(),
+hasType(pointerType(pointee(optionalOrAliasType())),
+  callee(cxxMethodDecl(hasName(MemberName;
 }
 
 auto isOptionalOperatorCallWithName(
@@ -251,34 +282,12 @@
   return Type->isReferenceType() ? Type->getPointeeType() : Type;
 }
 
-bool isTopLevelNamespaceWithName(const NamespaceDecl ,
- 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 

[PATCH] D148377: [clang][dataflow] Drop optional model's dependency on libc++ internals.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added a comment.

Nice deduplication and compatibility fixes!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148377/new/

https://reviews.llvm.org/D148377

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148377: [clang][dataflow] Drop optional model's dependency on libc++ internals.

[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.

Adjusts the matchers in the optional model to avoid dependency on internal
implementation details of libc++'s `std::optional`. In the process, factors out
the code to check the name of these types so that it's shared throughout.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148377

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
@@ -1784,8 +1784,7 @@
   )");
 }
 
-TEST_P(UncheckedOptionalAccessTest, ValueOrComparison) {
-  // Pointers.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonPointers) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1798,8 +1797,9 @@
   }
 }
   )code");
+}
 
-  // Integers.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonIntegers) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1812,8 +1812,9 @@
   }
 }
   )code");
+}
 
-  // Strings.
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonStrings) {
   ExpectDiagnosticsFor(
   R"code(
 #include "unchecked_optional_access_test.h"
@@ -1839,9 +1840,9 @@
   }
 }
   )code");
+}
 
-  // Pointer-to-optional.
-  //
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparisonPointerToOptional) {
   // FIXME: make `opt` a parameter directly, once we ensure that all `optional`
   // values have a `has_value` property.
   ExpectDiagnosticsFor(
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Stmt.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
@@ -36,16 +37,44 @@
 
 namespace clang {
 namespace dataflow {
+
+static bool isTopLevelNamespaceWithName(const NamespaceDecl ,
+llvm::StringRef Name) {
+  return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
+ NS.getParent() != nullptr && NS.getParent()->isTranslationUnit();
+}
+
+static bool hasOptionalClassName(const CXXRecordDecl ) {
+  if (!RD.getDeclName().isIdentifier())
+return false;
+
+  if (RD.getName() == "optional") {
+if (const auto *N = dyn_cast_or_null(RD.getDeclContext()))
+  return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
+return false;
+  }
+
+  if (RD.getName() == "Optional") {
+// Check whether namespace is "::base".
+const auto *N = dyn_cast_or_null(RD.getDeclContext());
+return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+  }
+
+  return false;
+}
+
 namespace {
 
 using namespace ::clang::ast_matchers;
 using LatticeTransferState = TransferState;
 
+AST_MATCHER(CXXRecordDecl, hasOptionalClassNameMatcher) {
+  return hasOptionalClassName(Node);
+}
+
 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
-  hasAnyName("::std::optional", "::std::__optional_storage_base",
- "::std::__optional_destruct_base", "::absl::optional",
- "::base::Optional"),
+  hasOptionalClassNameMatcher(),
   hasTemplateArgument(0, refersToType(type().bind("T";
 }
 
@@ -63,8 +92,10 @@
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
 : cxxThisExpr());
   return cxxMemberCallExpr(
-  on(expr(Exception)),
-  callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass();
+  on(expr(Exception,
+  anyOf(hasOptionalType(),
+hasType(pointerType(pointee(optionalOrAliasType())),
+  callee(cxxMethodDecl(hasName(MemberName;
 }
 
 auto isOptionalOperatorCallWithName(
@@ -251,34 +282,12 @@
   return Type->isReferenceType() ? Type->getPointeeType() : Type;
 }
 
-bool isTopLevelNamespaceWithName(const NamespaceDecl ,
- llvm::StringRef Name) {
-  return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
- NS.getParent()