Ast importer visitors

[ganenkokb-yandex]

I've rebased commit from Evianaive and compiled it. I hope it will speed up fix for #129393.

[github-actions[bot]]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (ganenkokb-yandex)

Changes

I've rebased commit from Evianaive and compiled it. I hope it will speed up fix for #129393.

---

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

1 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+266-1)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index b481ad5df667e..cff0050208784 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -564,6 +564,9 @@ namespace clang {
     ExpectedDecl VisitVarTemplateDecl(VarTemplateDecl *D);
     ExpectedDecl VisitVarTemplateSpecializationDecl(VarTemplateSpecializationDecl *D);
     ExpectedDecl VisitFunctionTemplateDecl(FunctionTemplateDecl *D);
+    ExpectedDecl VisitConceptDecl(ConceptDecl* D);
+    ExpectedDecl VisitRequiresExprBodyDecl(RequiresExprBodyDecl* E);
+    ExpectedDecl VisitImplicitConceptSpecializationDecl(ImplicitConceptSpecializationDecl* D);
 
     // Importing statements
     ExpectedStmt VisitStmt(Stmt *S);
@@ -680,6 +683,8 @@ namespace clang {
     ExpectedStmt VisitTypeTraitExpr(TypeTraitExpr *E);
     ExpectedStmt VisitCXXTypeidExpr(CXXTypeidExpr *E);
     ExpectedStmt VisitCXXFoldExpr(CXXFoldExpr *E);
+    ExpectedStmt VisitRequiresExpr(RequiresExpr* E);
+    ExpectedStmt VisitConceptSpecializationExpr(ConceptSpecializationExpr* E);
 
     // Helper for chaining together multiple imports. If an error is detected,
     // subsequent imports will return default constructed nodes, so that failure
@@ -735,6 +740,40 @@ namespace clang {
     // that type is declared inside the body of the function.
     // E.g. auto f() { struct X{}; return X(); }
     bool hasReturnTypeDeclaredInside(FunctionDecl *D);
+    
+    Expected<ConstraintSatisfaction> FillConstraintSatisfaction(const ASTConstraintSatisfaction& from) {
+      auto ImportStringRef = [this](const StringRef& FromString) {
+        char* ToDiagMessage = new (Importer.getToContext()) char[FromString.size()];
+        std::copy(FromString.begin(),FromString.end(),ToDiagMessage);
+        return StringRef(ToDiagMessage,FromString.size());
+      };
+      ConstraintSatisfaction Satisfaction;
+      Satisfaction.IsSatisfied = from.IsSatisfied;
+      Satisfaction.ContainsErrors = from.ContainsErrors;        
+      if (!Satisfaction.IsSatisfied) {
+        using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
+        for (auto &Record : from) {
+          if (auto *SubstDiag = Record.dyn_cast<SubstitutionDiagnostic *>()) {
+            Error Err = Error::success();
+    
+            auto ToPairFirst = import(SubstDiag->first);
+            if(!ToPairFirst)
+              return ToPairFirst.takeError();
+            StringRef ToPairSecond = ImportStringRef(SubstDiag->second);
+            Satisfaction.Details.emplace_back(new (Importer.getToContext())
+              ConstraintSatisfaction::SubstitutionDiagnostic{
+                ToPairFirst.get(), ToPairSecond});
+          } else { 
+            const Expr *ConstraintExpr = Record.dyn_cast<Expr *>();
+            Expected<Expr *> ToConstraintExpr = import(ConstraintExpr);
+            if(!ToConstraintExpr)
+              return ToConstraintExpr.takeError();
+            Satisfaction.Details.emplace_back(ToConstraintExpr.get());
+          }
+        }
+      }
+      return Satisfaction;
+    }
   };
 
 template <typename InContainerTy>
@@ -1063,6 +1102,142 @@ Expected<LambdaCapture> ASTNodeImporter::import(const LambdaCapture &From) {
       EllipsisLoc);
 }
 
+template<>
+Expected<concepts::Requirement*> ASTNodeImporter::import(concepts::Requirement* FromRequire) {
+  auto ImportStringRef = [this](const StringRef& FromString) {
+      char* ToDiagMessage = new (Importer.getToContext()) char[FromString.size()];
+      std::copy(FromString.begin(),FromString.end(),ToDiagMessage);
+      return StringRef(ToDiagMessage,FromString.size());
+    };
+    
+  auto ImportSubstitutionDiagnos = [this, &ImportStringRef]
+  (concepts::Requirement::SubstitutionDiagnostic* FromDiagnos, Error& Err)->concepts::Requirement::SubstitutionDiagnostic* {
+    const auto& ToEntity = ImportStringRef(FromDiagnos->SubstitutedEntity);
+    Expected<SourceLocation> ToLoc = import(FromDiagnos->DiagLoc);
+    if(!ToLoc) {
+      Err = ToLoc.takeError();
+      return nullptr;
+    }
+    const auto& ToDiagMessage =  ImportStringRef(FromDiagnos->DiagMessage);
+    return new (Importer.getToContext()) concepts::Requirement::SubstitutionDiagnostic{
+      ToEntity,
+      ToLoc.get(),
+      ToDiagMessage};
+  };
+  switch (FromRequire->getKind()) {
+  case concepts::Requirement::RequirementKind::RK_Type: {
+    auto *From = cast<concepts::TypeRequirement>(FromRequire);
+    if(From->isSubstitutionFailure())
+    {
+      // Should we return Error directly if TypeRequirement isSubstitutionFailure?
+      Error Err = Error::success();
+      auto Diagnos = ImportSubstitutionDiagnos(From->getSubstitutionDiagnostic(),Err);
+      if (Err)
+        return std::move(Err);
+      return new (Importer.getToContext()) concepts::TypeRequirement(Diagnos);
+    }
+    else {
+      Expected<TypeSourceInfo *> ToType = import(From->getType());
+      if(!ToType)
+        return ToType.takeError();
+      return new (Importer.getToContext()) concepts::TypeRequirement(ToType.get());
+    }
+    break;
+  }
+  case concepts::Requirement::RequirementKind::RK_Compound: 
+  case concepts::Requirement::RequirementKind::RK_Simple: {
+    const auto *From = cast<concepts::ExprRequirement>(FromRequire);
+    
+    auto Status = From->getSatisfactionStatus();
+    llvm::PointerUnion<concepts::Requirement::SubstitutionDiagnostic *, Expr *> E;
+    if (Status == concepts::ExprRequirement::SS_ExprSubstitutionFailure) {
+      Error Err = Error::success();
+      E = ImportSubstitutionDiagnos(From->getExprSubstitutionDiagnostic(),Err);
+      if (Err)
+        return std::move(Err);
+    } else {
+      auto ExpectE = import(From->getExpr());
+      if (!ExpectE)
+        return ExpectE.takeError();
+      E = ExpectE.get();
+    }
+
+    std::optional<concepts::ExprRequirement::ReturnTypeRequirement> Req;
+    ConceptSpecializationExpr *SubstitutedConstraintExpr = nullptr;
+    SourceLocation NoexceptLoc;
+    bool IsRKSimple = FromRequire->getKind() == concepts::Requirement::RK_Simple;
+    if (IsRKSimple) {
+      Req.emplace();
+    } else {
+      auto NoexceptLoc = import(From->getNoexceptLoc());
+      if(!NoexceptLoc)
+        return NoexceptLoc.takeError();
+      auto& FromTypeRequirement = From->getReturnTypeRequirement();
+
+      if(FromTypeRequirement.isTypeConstraint()) {
+        auto ParamsOrErr = import(FromTypeRequirement.getTypeConstraintTemplateParameterList());
+        if (!ParamsOrErr)
+          return ParamsOrErr.takeError();
+        if (Status >=
+          concepts::ExprRequirement::SS_ConstraintsNotSatisfied) {
+          auto ExpectSubstitutedConstraintExpr = import(From->getReturnTypeRequirementSubstitutedConstraintExpr());
+          if (!ExpectSubstitutedConstraintExpr)
+            return ExpectSubstitutedConstraintExpr.takeError();
+          SubstitutedConstraintExpr = ExpectSubstitutedConstraintExpr.get();
+        }
+        Req.emplace(ParamsOrErr.get());
+      }      
+      else if(FromTypeRequirement.isSubstitutionFailure()) {
+        Error Err = Error::success();
+        concepts::Requirement::SubstitutionDiagnostic *ToDiagnos =
+            ImportSubstitutionDiagnos(
+                FromTypeRequirement.getSubstitutionDiagnostic(), Err);
+        if (Err)
+          return std::move(Err);
+       Req.emplace(ToDiagnos);
+      }
+      else {
+        Req.emplace();
+      }      
+    }
+    if (Expr *Ex = E.dyn_cast<Expr *>())
+      return new (Importer.getToContext()) concepts::ExprRequirement(
+              Ex, IsRKSimple, NoexceptLoc,
+              std::move(*Req), Status, SubstitutedConstraintExpr);
+    else
+      return new (Importer.getToContext()) concepts::ExprRequirement(
+              E.get<concepts::Requirement::SubstitutionDiagnostic *>(),
+              IsRKSimple, NoexceptLoc,
+              std::move(*Req));
+    break;
+  }
+  case concepts::Requirement::RequirementKind::RK_Nested: {
+    auto *From = cast<concepts::NestedRequirement>(FromRequire);
+    const auto& FromSatisfaction = From->getConstraintSatisfaction();
+    if(From->hasInvalidConstraint()) {
+      const auto& ToConstraintEntity = ImportStringRef(From->getInvalidConstraintEntity());
+      auto ToSatisfaction = ASTConstraintSatisfaction::Rebuild(Importer.getToContext(),FromSatisfaction);
+      return new (Importer.getToContext()) concepts::NestedRequirement(ToConstraintEntity,ToSatisfaction);
+    } else {
+      Expected<Expr *> ToExpr = import(From->getConstraintExpr());
+      if(!ToExpr)
+        return ToExpr.takeError();
+      // FromSatisfaction.IsSatisfied;
+      if(ToExpr.get()->isInstantiationDependent())
+        return new (Importer.getToContext()) concepts::NestedRequirement(ToExpr.get());
+      else {
+        auto expected_satisfaction = FillConstraintSatisfaction(FromSatisfaction);
+        if (!expected_satisfaction) {
+          return expected_satisfaction.takeError();
+        }
+        return new (Importer.getToContext()) concepts::NestedRequirement(Importer.getToContext(),ToExpr.get(), *expected_satisfaction);
+      }      
+    }
+    break;
+  }
+  }
+}
+
 template <typename T>
 bool ASTNodeImporter::hasSameVisibilityContextAndLinkage(T *Found, T *From) {
   if (Found->getLinkageInternal() != From->getLinkageInternal())
@@ -7322,6 +7497,96 @@ ExpectedStmt ASTNodeImporter::VisitExpr(Expr *E) {
   return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
 }
 
+ExpectedStmt ASTNodeImporter::VisitRequiresExpr(RequiresExpr* E) {
+  Error Err = Error::success();
+  // auto ToType = importChecked(Err, E->getType());
+  auto RequiresKWLoc = importChecked(Err,E->getRequiresKWLoc());
+  auto RParenLoc = importChecked(Err,E->getRParenLoc());
+  auto RBraceLoc = importChecked(Err,E->getRBraceLoc());
+
+  auto Body = importChecked(Err,E->getBody());
+  auto LParenLoc = importChecked(Err,E->getLParenLoc());
+  if(Err)
+    return std::move(Err);
+  SmallVector<ParmVarDecl*, 4> LocalParameters;
+  if (Error Err = ImportArrayChecked(E->getLocalParameters(),LocalParameters.begin()))
+    return std::move(Err);
+  SmallVector<concepts::Requirement*, 4> Requirements;
+  if (Error Err = ImportArrayChecked(E->getRequirements(),Requirements.begin()))
+    return std::move(Err);
+  return RequiresExpr::Create(Importer.getToContext(),RequiresKWLoc, Body, LParenLoc,
+                    LocalParameters, RParenLoc, Requirements, RBraceLoc);
+}
+
+ExpectedDecl ASTNodeImporter::VisitRequiresExprBodyDecl(RequiresExprBodyDecl* D) {
+  DeclContext *DC, *LexicalDC;
+  Error Err = Error::success();
+  Err = ImportDeclContext(D, DC, LexicalDC);  
+  auto RequiresLoc = importChecked(Err,D->getLocation());
+  return RequiresExprBodyDecl::Create(Importer.getToContext(),DC,RequiresLoc);
+}
+
+ExpectedStmt ASTNodeImporter::VisitConceptSpecializationExpr(ConceptSpecializationExpr* E) {
+  Error Err = Error::success();
+  
+  auto CL = importChecked(Err,E->getConceptReference());
+  auto CSD = importChecked(Err,E->getSpecializationDecl());
+  // auto Satisfaction = importChecked(Err,E->getSatisfaction());  
+  if (Err)
+    return std::move(Err);
+  // E->getDependence();
+  if(E->isValueDependent()) {
+    return ConceptSpecializationExpr::Create(
+      Importer.getToContext(), CL,
+      const_cast<ImplicitConceptSpecializationDecl *>(CSD), nullptr);
+  }
+  const auto& FromSatisfaction = E->getSatisfaction();
+  auto ImportStringRef = [this](const StringRef& FromString) {
+    char* ToDiagMessage = new (Importer.getToContext()) char[FromString.size()];
+    std::copy(FromString.begin(),FromString.end(),ToDiagMessage);
+    return StringRef(ToDiagMessage,FromString.size());
+  };
+  auto expected_satisfaction = FillConstraintSatisfaction(FromSatisfaction);
+  if (!expected_satisfaction) {
+    return expected_satisfaction.takeError();
+  }
+  return ConceptSpecializationExpr::Create(
+    Importer.getToContext(), CL,
+    const_cast<ImplicitConceptSpecializationDecl *>(CSD), &*expected_satisfaction);
+}
+
+ExpectedDecl ASTNodeImporter::VisitConceptDecl(ConceptDecl* D) {
+  // Import the context of this declaration.
+  DeclContext *DC, *LexicalDC;
+  Error Err = Error::success();
+  Err = ImportDeclContext(D, DC, LexicalDC);  
+  auto BeginLocOrErr = importChecked(Err, D->getBeginLoc());
+  auto LocationOrErr = importChecked(Err, D->getLocation());
+  auto NameDeclOrErr = importChecked(Err,D->getDeclName());
+  auto ToTemplateParameters = importChecked(Err, D->getTemplateParameters());
+  auto ConstraintExpr = importChecked(Err, D->getConstraintExpr());
+  if(Err)
+    return std::move(Err);
+  return ConceptDecl::Create(
+    Importer.getToContext(),DC,
+    LocationOrErr,NameDeclOrErr,
+    ToTemplateParameters,ConstraintExpr);
+}
+
+ExpectedDecl ASTNodeImporter::VisitImplicitConceptSpecializationDecl(ImplicitConceptSpecializationDecl* D) {
+  DeclContext *DC, *LexicalDC;
+  Error Err = Error::success();
+  Err = ImportDeclContext(D, DC, LexicalDC);
+  auto ToSL = importChecked(Err,D->getLocation());
+  if(Err)
+    return std::move(Err);
+  SmallVector<TemplateArgument,2> ToArgs;
+  if(Error Err = ImportTemplateArguments(D->getTemplateArguments(),ToArgs))
+    return std::move(Err);
+
+  return ImplicitConceptSpecializationDecl::Create(Importer.getToContext(),DC,ToSL,ToArgs);
+}
+
 ExpectedStmt ASTNodeImporter::VisitSourceLocExpr(SourceLocExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
@@ -10547,4 +10812,4 @@ bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To,
       getToContext().getLangOpts(), FromContext, ToContext, NonEquivalentDecls,
       getStructuralEquivalenceKind(*this), false, Complain);
   return Ctx.IsEquivalent(From, To);
-}
+}
\ No newline at end of file

[balazske]

We should ensure that the code builds with -DBUILD_SHARED_LIBS=ON cmake option. I think this is why I can not build it. I get this error:

/usr/bin/ld: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o: in function `std::conditional<is_base_of_v<clang::Type, clang::concepts::Requirement>, llvm::Expected<clang::concepts::Requirement const*>, llvm::Expected<clang::concepts::Requirement*> >::type clang::ASTNodeImporter::import<clang::concepts::Requirement>(clang::concepts::Requirement*)':
llvm-project/clang/lib/AST/ASTImporter.cpp:1144: undefined reference to `clang::concepts::TypeRequirement::TypeRequirement(clang::TypeSourceInfo*)'
/usr/bin/ld: llvm-project/clang/lib/AST/ASTImporter.cpp:1207: undefined reference to `clang::concepts::ExprRequirement::ExprRequirement(clang::Expr*, bool, clang::SourceLocation, clang::concepts::ExprRequirement::ReturnTypeRequirement, clang::concepts::ExprRequirement::SatisfactionStatus, clang::ConceptSpecializationExpr*)'
/usr/bin/ld: llvm-project/clang/lib/AST/ASTImporter.cpp:1212: undefined reference to `clang::concepts::ExprRequirement::ExprRequirement(clang::concepts::Requirement::SubstitutionDiagnostic*, bool, clang::SourceLocation, clang::concepts::ExprRequirement::ReturnTypeRequirement)'
/usr/bin/ld: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o: in function `void std::_Optional_base_impl<clang::concepts::ExprRequirement::ReturnTypeRequirement, std::_Optional_base<clang::concepts::ExprRequirement::ReturnTypeRequirement, true, true> >::_M_construct<clang::TemplateParameterList*&>(clang::TemplateParameterList*&)':
/usr/include/c++/9/optional:416: undefined reference to `clang::concepts::ExprRequirement::ReturnTypeRequirement::ReturnTypeRequirement(clang::TemplateParameterList*)'
collect2: error: ld returned 1 exit status

[balazske]

I think that ASTImporter must be moved into a new component (directory in "clang/lib") to fix this problem. AST is already dependency of Sema so Sema can not be added to AST as dependency (but could be added to ASTImporter if it would be a new component and Sema would not need ASTImporter). Or the code of some concept related classes must be moved but this looks more difficult.

[balazske]

I checked the code and it looks difficult to split the ASTImporter to a separate component. The problem is that ExternalASTSource is used for AST related things and uses ASTImporter too. Another way to fix the problem is to move code of classes like concepts::TypeRequirement from Sema to AST. These seem to belong logically more to AST than Sema. I did not check how difficult would be to make these changes but it should be possible.

[balazske]

I have now a working patch for "splitting" ASTImporter from AST. If this change will be accepted it is possible to add the new visit functions. The "split" change may require discussion on discord because it affects clang code layout.

[ganenkokb-yandex]

@balazske I've put some more fixes to patch to make it work. And now it is on top of main, and compiles with -DBUILD_SHARED_LIBS=ON

Full cmake command is: cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld" -DLLVM_TARGETS_TO_BUILD="ARM" -DLLVM_DEFAULT_TARGET_TRIPLE="arm64-apple-darwin24.3.0" -DLLVM_BUILD_TESTS=ON -DCLANG_BUILD_UNIT_TESTS=ON -DBUILD_SHARED_LIBS=ON ../llvm

Can you make it a chance once more?

[balazske]

After change #141104 it is possible to compile and link the code without problems. There are some things to fix in this patch (with naming and formatting rules and code correctness), I would like better if the code would look like in PR #138845. Additionally tests are needed for all of the new Decl and Expr nodes.

[ganenkokb-yandex]

I would like better if the code would look like in PR #138845.

I've applied your changes merged with some fixes from my PR. Thanks.

Additionally tests are needed for all of the new Decl and Expr nodes.

On my way!

[ganenkokb-yandex]

@balazske tests are ready

[ganenkokb-yandex]

Code looks almost acceptable. I could not verify if the tests are sufficient (I am not familiar with this new syntax). Can you test this on large projects that use these types of expressions?

It is my approach. All fixes aside ASTImporter are came from real cases. I covered with tests all code paths for new decls and expressions for concepts, except

ImportExprRequirement
...
if (FromTypeRequirement.isSubstitutionFailure())
 --- how to get here? - it is compile time error?

Newly added VisitSubstNonTypeTemplateParmPackExpr, VisitPseudoObjectExpr, VisitCXXParenListInitExpr are also without tests. I've faced them on my project, and looks like it is not because of newly implemented concepts imports. But I'm not sure. Should I move 'em to separate pull request?

[balazske]

Newly added VisitSubstNonTypeTemplateParmPackExpr, VisitPseudoObjectExpr, VisitCXXParenListInitExpr are also without tests. I've faced them on my project, and looks like it is not because of newly implemented concepts imports. But I'm not sure. Should I move 'em to separate pull request?

It would be better to have not strictly related changes in a different pull request to make debugging more easy (removal of a commit should not remove unrelated code).

[balazske]

If FromTypeRequirement.isSubstitutionFailure() can be true at compile error it can be still possible to call testImport or getTuDecl with the code.

[ganenkokb-yandex]

I think we can put in this change. I tested it on some projects, the number of crashes got less after this change (it is possible that new ones appeared but the total count was less).

Thank you for your attention. What additional steps are expected of me to take the pull request further?

[SweetVishnya]

@balazske, @Michael137, could you, please, merge this PR?

[ganenkokb-yandex]

@balazske I don't have rights for merging PR into main. Could you please help?

[SweetVishnya]

At least we need to somehow approve CI run

[github-actions[bot]]

@ganenkokb-yandex Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[irishrover]

@balazske I don't have rights for merging PR into main. Could you please help?

Done!