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`.
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`.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
