[Clang][ASTImporter] Fix cycle in importing template specialization on auto type with typename

[ganenkokb-yandex]

ASTImporter on importing template specialization with auto return type faces cycle when return type is not nested one, but typename from template arguments and other template.
There is code, that prevents cycle to auto return types when nested type declared. Solved case differs somehow from nested types, but have same solution with UsedDifferentProtoType - with delayed return type determining.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (ganenkokb-yandex)

Changes

On importing template specialization with auto return type cycle occurs when return type is not nested one, but typename from template arguments and other template.
There is code, that prevents cycle to auto return types when nested type declared. Solved case differs somehow from nested types, but have same solution with UsedDifferentProtoType - with delayed return type determining.

---

Full diff: https://github.com/llvm/llvm-project/pull/162514.diff

3 Files Affected:

• (modified) clang/include/clang/AST/ASTImporter.h (+1)
• (modified) clang/lib/AST/ASTImporter.cpp (+8-1)
• (modified) clang/unittests/AST/ASTImporterTest.cpp (+50)

diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h
index 4a0ca45b785a9..eea4ccccb1600 100644
--- a/clang/include/clang/AST/ASTImporter.h
+++ b/clang/include/clang/AST/ASTImporter.h
@@ -254,6 +254,7 @@ class TypeSourceInfo;
     /// Declaration (from, to) pairs that are known not to be equivalent
     /// (which we have already complained about).
     NonEquivalentDeclSet NonEquivalentDecls;
+    llvm::DenseSet<const Decl *> DeclTypeCycles;
 
     using FoundDeclsTy = SmallVector<NamedDecl *, 2>;
     FoundDeclsTy findDeclsInToCtx(DeclContext *DC, DeclarationName Name);
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index f43fa8c90ad3b..0dc2e1c3b4f8b 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -4035,7 +4035,8 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
     // E.g.: auto foo() { struct X{}; return X(); }
     // To avoid an infinite recursion when importing, create the FunctionDecl
     // with a simplified return type.
-    if (hasReturnTypeDeclaredInside(D)) {
+    if (hasReturnTypeDeclaredInside(D) ||
+      Importer.DeclTypeCycles.find(D) != Importer.DeclTypeCycles.end()) {
       FromReturnTy = Importer.getFromContext().VoidTy;
       UsedDifferentProtoType = true;
     }
@@ -4058,7 +4059,13 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   }
 
   Error Err = Error::success();
+  if (!UsedDifferentProtoType) {
+    Importer.DeclTypeCycles.insert(D);
+  }
   auto T = importChecked(Err, FromTy);
+  if (!UsedDifferentProtoType) {
+    Importer.DeclTypeCycles.erase(D);
+  }
   auto TInfo = importChecked(Err, FromTSI);
   auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
   auto ToEndLoc = importChecked(Err, D->getEndLoc());
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index e7160bcf2e0c2..5f7fcdf817ea0 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -3204,6 +3204,56 @@ TEST_P(ImportExpr, UnresolvedMemberExpr) {
                  compoundStmt(has(callExpr(has(unresolvedMemberExpr())))))))));
 }
 
+TEST_P(ImportExpr, CycleInAutoTemplateSpec) {
+  MatchVerifier<Decl> Verifier;
+  const char *Code = R"(
+  template <class _CharT>
+  struct basic_string {
+    using value_type = _CharT;
+  };
+
+  template<typename T>
+  struct basic_string_view {
+    using value_type = T;
+  };
+
+  using string_view = basic_string_view<char>;
+  using string = basic_string<char>;
+
+  template<typename T>
+  struct span {
+  };
+
+  template <typename StringT>
+  auto StrCatT(span<const StringT> pieces) {
+    basic_string<typename StringT::value_type> result;
+    return result;
+  }
+
+  string StrCat(span<const string_view> pieces) {
+      return StrCatT(pieces);
+  }
+
+  string StrCat(span<const string> pieces) {
+      return StrCatT(pieces);
+  }
+
+  template <typename T>
+  auto declToImport(T pieces) {
+    return StrCat(pieces);
+  }
+
+  void test() {
+    span<const string> pieces;
+    auto result = declToImport(pieces);
+  }
+)";
+  // This test reproduces the StrCatT recursion pattern with concepts and span
+  // that may cause infinite recursion during AST import due to circular dependencies
+  testImport(Code, Lang_CXX20, "", Lang_CXX20, Verifier,
+             functionTemplateDecl(hasName("declToImport")));
+}
+
 TEST_P(ImportExpr, ConceptNoRequirement) {
   MatchVerifier<Decl> Verifier;
   const char *Code = R"(

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shafik]

Your summary should mention ASTImporter and your subject should have [Clang][ASTImporter]

[balazske]

This approach can be used to detect any import cycle when a FunctionDecl is imported recursively. It does not matter if the recursion comes from the return type or something other that is imported before the new function is created. The call to function hasReturnTypeDeclaredInside can be removed and it should still work (I tried it out and got no test failures). The recursive import of return type is (and was) often cause of new crashes (because new "unknown" code constructs) so I would like if the function hasReturnTypeDeclaredInside (and related things) is removed. This would make the code more future-proof. But this change can be added in a later commit.

[balazske]

Looks mostly OK, I have only naming changes.

[shafik]

@balazske please give your approval if your happy, I was just doing a drive by review. If there a better person to tag on ASTImporter PRs let me know (hopefully I won't forget).

[balazske]

I wanted to have a different pull request for removal of hasReturnTypeDeclaredInside. This can make debugging possible problems later more easy. Additionally the function has dependent code (IsTypeDeclaredInsideVisitor) that should also be removed.

[ganenkokb-yandex]

@shafik @ojhunt Could I have your review please?

[ganenkokb-yandex]

@ojhunt should I implement your suggested approach, or should I wait for @shafik opinion?

[ojhunt]

@ojhunt should I implement your suggested approach, or should I wait for @shafik opinion?

I think it's reasonable to use the make_scope_exit rather than adding another new RAII object

[balazske]

To me the code looks good after the name changes.

[ganenkokb-yandex]

@ojhunt can you have a look please?

[github-actions]

🐧 Linux x64 Test Results

• 111355 tests passed
• 4426 tests skipped