[cxx-interop] Use LookupQualifiedName in derived conformances

This is the first patch of a series of patches intended to shift lookup
toward using Clang lookup, to avoid Swift lookup's dependency on eager
Clang member importation.

The lookupCxxTypeMember() helper function replaces the previous helper
function, lookupNestedClangTypeDecl(). Functionally, the main difference
is that lookupCxxTypeMember() uses clang::Sema::LookupQualifiedName(),
which takes cares of issues like inheritance, ambiguity, etc. Meanwhile,
lookupNestedClangTypeDecl() only used clang::DeclContext::lookup(),
which is limited to the lexical context, and does not work with
inheritance.

The isIterator() function is renamed to hasIteratorConcept() to better
reflect what it is checking, and still uses clang::DeclContext::lookup()
to preserve the original behavior and to avoid requiring its callers to
supply a clang::Sema instance.

Author: j-hui

lib/ClangImporter/ClangDerivedConformances.cpp

@@ -18,7 +18,9 @@
 #include "swift/AST/ProtocolConformance.h"
 #include "swift/Basic/Assertions.h"
 #include "swift/ClangImporter/ClangImporterRequests.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/Sema/DelayedDiagnostic.h"
+#include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 
 using namespace swift;
@@ -58,6 +60,29 @@ static CxxStdType identifyCxxStdTypeByName(StringRef name) {
 #undef CxxStdCase
 }
 
+static const clang::TypeDecl *
+lookupCxxTypeMember(clang::Sema &Sema, const clang::CXXRecordDecl *Rec,
+                    StringRef name) {
+  auto R = clang::LookupResult(Sema, &Sema.PP.getIdentifierTable().get(name),
+                               clang::SourceLocation(),
+                               clang::Sema::LookupMemberName);
+  R.suppressDiagnostics();
+
+  auto *Ctx = static_cast<const clang::DeclContext *>(Rec);
+  Sema.LookupQualifiedName(R, const_cast<clang::DeclContext *>(Ctx));
+
+  if (R.isSingleResult()) {
+    if (auto *paths = R.getBasePaths();
+        paths && R.getBasePaths()->front().Access != clang::AS_public)
+      return nullptr;
+
+    for (auto *nd : R)
+      if (auto *td = dyn_cast<clang::TypeDecl>(nd))
+        return td;
+  }
+  return nullptr;
+}
+
 /// Alternative to `NominalTypeDecl::lookupDirect`.
 /// This function does not attempt to load extensions of the nominal decl.
 static TinyPtrVector<ValueDecl *>
@@ -129,32 +154,6 @@ static FuncDecl *getInsertFunc(NominalTypeDecl *decl,
   return insert;
 }
 
-static clang::TypeDecl *
-lookupNestedClangTypeDecl(const clang::CXXRecordDecl *clangDecl,
-                          StringRef name) {
-  clang::IdentifierInfo *nestedDeclName =
-      &clangDecl->getASTContext().Idents.get(name);
-  auto nestedDecls = clangDecl->lookup(nestedDeclName);
-  // If this is a templated typedef, Clang might have instantiated several
-  // equivalent typedef decls. If they aren't equivalent, Clang has already
-  // complained about this. Let's assume that they are equivalent. (see
-  // filterNonConflictingPreviousTypedefDecls in clang/Sema/SemaDecl.cpp)
-  if (nestedDecls.empty())
-    return nullptr;
-  auto nestedDecl = nestedDecls.front();
-  return dyn_cast_or_null<clang::TypeDecl>(nestedDecl);
-}
-
-static clang::TypeDecl *
-getIteratorCategoryDecl(const clang::CXXRecordDecl *clangDecl) {
-  return lookupNestedClangTypeDecl(clangDecl, "iterator_category");
-}
-
-static clang::TypeDecl *
-getIteratorConceptDecl(const clang::CXXRecordDecl *clangDecl) {
-  return lookupNestedClangTypeDecl(clangDecl, "iterator_concept");
-}
-
 static ValueDecl *lookupOperator(NominalTypeDecl *decl, Identifier id,
                                  function_ref<bool(ValueDecl *)> isValid) {
   // First look for operator declared as a member.
@@ -420,8 +419,13 @@ static bool synthesizeCXXOperator(ClangImporter::Implementation &impl,
   return true;
 }
 
-bool swift::isIterator(const clang::CXXRecordDecl *clangDecl) {
-  return getIteratorCategoryDecl(clangDecl);
+bool swift::hasIteratorCategory(const clang::CXXRecordDecl *clangDecl) {
+  clang::IdentifierInfo *name =
+      &clangDecl->getASTContext().Idents.get("iterator_category");
+  auto members = clangDecl->lookup(name);
+  if (members.empty() || !members.isSingleResult())
+    return false;
+  return isa<clang::TypeDecl>(members.front());
 }
 
 ValueDecl *
@@ -453,20 +457,27 @@ conformToCxxIteratorIfNeeded(ClangImporter::Implementation &impl,
   PrettyStackTraceDecl trace("trying to conform to UnsafeCxxInputIterator", decl);
   ASTContext &ctx = decl->getASTContext();
   clang::ASTContext &clangCtx = clangDecl->getASTContext();
+  clang::Sema &clangSema = impl.getClangSema();
 
   if (!ctx.getProtocol(KnownProtocolKind::UnsafeCxxInputIterator))
     return;
 
   // We consider a type to be an input iterator if it defines an
   // `iterator_category` that inherits from `std::input_iterator_tag`, e.g.
   // `using iterator_category = std::input_iterator_tag`.
-  auto iteratorCategory = getIteratorCategoryDecl(clangDecl);
-  if (!iteratorCategory)
+  //
+  // FIXME: The second hasIteratorCategory() is more conservative than it should
+  // be  because it doesn't consider things like inheritance, but checking this
+  // here maintains existing behavior and ensures consistency across
+  // ClangImporter, where clang::Sema isn't always readily available.
+  const auto *iteratorCategory =
+      lookupCxxTypeMember(clangSema, clangDecl, "iterator_category");
+  if (!iteratorCategory || !hasIteratorCategory(clangDecl))
     return;
 
   auto unwrapUnderlyingTypeDecl =
-      [](clang::TypeDecl *typeDecl) -> clang::CXXRecordDecl * {
-    clang::CXXRecordDecl *underlyingDecl = nullptr;
+      [](const clang::TypeDecl *typeDecl) -> const clang::CXXRecordDecl * {
+    const clang::CXXRecordDecl *underlyingDecl = nullptr;
     if (auto typedefDecl = dyn_cast<clang::TypedefNameDecl>(typeDecl)) {
       auto type = typedefDecl->getUnderlyingType();
       underlyingDecl = type->getAsCXXRecordDecl();
@@ -525,7 +536,8 @@ conformToCxxIteratorIfNeeded(ClangImporter::Implementation &impl,
   // `iterator_concept`. It is not possible to detect a contiguous iterator
   // based on its `iterator_category`. The type might not have an
   // `iterator_concept` defined.
-  if (auto iteratorConcept = getIteratorConceptDecl(clangDecl)) {
+  if (const auto *iteratorConcept =
+          lookupCxxTypeMember(clangSema, clangDecl, "iterator_concept")) {
     if (auto underlyingConceptDecl =
             unwrapUnderlyingTypeDecl(iteratorConcept)) {
       isContiguousIterator = isContiguousIteratorDecl(underlyingConceptDecl);
@@ -726,7 +738,7 @@ static void conformToCxxOptional(ClangImporter::Implementation &impl,
   // it isn't directly usable from Swift. Let's explicitly instantiate a
   // constructor with the wrapped value type, and then import it into Swift.
 
-  auto valueTypeDecl = lookupNestedClangTypeDecl(clangDecl, "value_type");
+  auto valueTypeDecl = lookupCxxTypeMember(clangSema, clangDecl, "value_type");
   if (!valueTypeDecl)
     // `std::optional` without a value_type?!
     return;
@@ -1174,8 +1186,8 @@ static void conformToCxxSpan(ClangImporter::Implementation &impl,
   if (!elementType || !sizeType)
     return;
 
-  auto pointerTypeDecl = lookupNestedClangTypeDecl(clangDecl, "pointer");
-  auto countTypeDecl = lookupNestedClangTypeDecl(clangDecl, "size_type");
+  auto pointerTypeDecl = lookupCxxTypeMember(clangSema, clangDecl, "pointer");
+  auto countTypeDecl = lookupCxxTypeMember(clangSema, clangDecl, "size_type");
 
   if (!pointerTypeDecl || !countTypeDecl)
     return;

lib/ClangImporter/ClangDerivedConformances.h

@@ -18,7 +18,14 @@
 
 namespace swift {
 
-bool isIterator(const clang::CXXRecordDecl *clangDecl);
+/// Whether a C++ record decl contains a type member named "iterator_category",
+/// a heuristic we use to determine whether that record type is an iterator.
+///
+/// This function returns true if there is exactly one public type member named
+/// "iterator_category", but does not look for inherited members. Note that, as
+/// a result of these limitations, it may return false even if that record type
+/// is usable as an iterator.
+bool hasIteratorCategory(const clang::CXXRecordDecl *clangDecl);
 
 bool isUnsafeStdMethod(const clang::CXXMethodDecl *methodDecl);
 

lib/ClangImporter/ClangImporter.cpp

@@ -8376,7 +8376,7 @@ static bool hasPointerInSubobjects(const clang::CXXRecordDecl *decl) {
             hasUnsafeAPIAttr(cxxRecord))
           return false;
 
-        if (hasIteratorAPIAttr(cxxRecord) || isIterator(cxxRecord))
+        if (hasIteratorAPIAttr(cxxRecord) || hasIteratorCategory(cxxRecord))
           return true;
 
         if (hasPointerInSubobjects(cxxRecord))
@@ -8497,7 +8497,7 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
   if (isSwiftClassType(cxxDecl))
     return CxxRecordSemanticsKind::SwiftClassType;
 
-  if (hasIteratorAPIAttr(cxxDecl) || isIterator(cxxDecl)) {
+  if (hasIteratorAPIAttr(cxxDecl) || hasIteratorCategory(cxxDecl)) {
     return CxxRecordSemanticsKind::Iterator;
   }
 
@@ -8760,7 +8760,7 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
           return true;
 
         if (hasIteratorAPIAttr(cxxRecordReturnType) ||
-            isIterator(cxxRecordReturnType))
+            hasIteratorCategory(cxxRecordReturnType))
           return false;
 
         // Mark this as safe to help our diganostics down the road.