[ASTImporter] Add ability to convert CXXRecordDecls to RecordDecls when importing to C language TUs

[teemperor]

Summary

Right now when using the ASTImporter to bring struct declarations from a C++ AST to a C AST, the
resulting imported Decl will just be a CXXRecordDecl even though in a C AST we only have RecordDecls.
When the 'to' AST already has a RecordDecl with the same name we currently also fail to merge the
CXXRecordDecl from the 'from' AST into it and instead just create a duplicated CXXRecordDecl.

Our use case for this is actually for Swift where the ASTImporter is used in a lot of rather adventurous
way to move things around clang::ASTContext instances. In LLDB we are (at the moment) always in
an AST that supports C++ so this won't have any effect on LLDB itself.

This patch introduces basic conversion functionality that converts a CXXRecordDecl that is just expressing
a valid C struct into a RecordDecl. If the target AST already has a equivalent RecordDecl this will now
also merge the 'from' record into that one.

There are a bunch of things that this patch doesn't try to fix:

• Unsupported C++ nodes such as user-defined constructors are still imported when explicitly requested by the user.
• C to C++ conversion for structs (i.e., adding the missing pieces to a RecordDecl and turn it into a CXXRecordDecl).

I'll try to address those in follow up patches. I also have some plans to at least implement the error handling for
importing from Objective-C to non-Objective-C contexts. The same goes for importing nodes that are exclusive
to one C/C++ standard to an AST with (usually earlier) standard where the node can't exist.

[teemperor]

This patch is also intended to get early feedback on how we should approach this. We could also discuss *if* we want to really support this whole cross-language
translation in the ASTImporter, but so far it hasn't been explicitly forbidden so people already started using it this way.

I think supporting the most common scenario of translating Record/CXXRecordDecl and making the rest an importer error doesn't seem so complicated
and that seems like reasonable behaviour for the ASTImporter.

(Also I plan to test all this x-language importing in its own test file, hence why the tests are in ASTImporterCrossLanguageTest.cpp).

[teemperor]

clang/unittests/AST/ASTImporterCrossLanguageTest.cpp

118

Do we actually have a test utility for checking *what* error actually happened? I could just implement the usual llvm::Error handling here otherwise.

[balazske]

I think it would be better to have a fully new module for converting AST between language versions. It would do only conversion, not AST merge. The ASTImporter is already too large and complicated (should be split into files) code, with this conversion logic embedded into other import functions and with multiple language versions (in the future) it would be really hard to debug (number of special cases at import increases and is already big).

[teemperor]

So something like an ASTConverter that just sits on top of the ASTImporter? In theory that class could implement the ImportImpl callback and do the custom imports it needs to do on its own, so I think that would work. I'm putting this in "planned changes" for now until I get around to make an ASTConverter patch..

[balazske]

I did not mean to reuse ASTImporter, that is a possibility too. Probably it can be more easy to handle AST conversion only without importing, specially if during the conversion no kind of error can occur, so no error handling or ODR search are needed. The conversion can happen before doing the AST import (with converted AST).

I do not know how this feature will be used, is it not possible to simply set C++ mode for every used AST?

[martong]

Hi Raphael,

Thanks for addressing this problem! In CTU, currently we simply do not allow to import anything from another TU if the two languages differ. What's more, we do this even if the languages are the same but the dialects are different (e.g. C++98 vs C++11). We might change this by the help of this new functionality and it could widen the usability of CTU analysis as well! So, I am excited to see this new feature evolving. On the other hand, I agree with @balazske that we should separate this functionality into it's own class without further complicating the existing ASTImporter, if possible.

[martong]

So something like an ASTConverter that just sits on top of the ASTImporter? In theory that class could implement the ImportImpl callback and do the custom imports it needs to do on its own

I am seeing some difficulties with this approach b/c this would require to replace an existing node in the AST with a new one in some cases. And that might not be always possible without a full retraverse of the AST. Perhaps we could come up with some simplified visitation or could reuse some data from ASTImporter::ImportedDecls.

I do not know how this feature will be used, is it not possible to simply set C++ mode for every used AST?

AFAIK, the parser creates the AST node according to the set language/dialect. So, it is not possible to convert a RecordDecl into a CXXRecordDecl so simply.

[teemperor]

In D99348#2655175, @balazske wrote:

I did not mean to reuse ASTImporter, that is a possibility too. Probably it can be more easy to handle AST conversion only without importing, specially if during the conversion no kind of error can occur, so no error handling or ODR search are needed. The conversion can happen before doing the AST import (with converted AST).
I do not know how this feature will be used, is it not possible to simply set C++ mode for every used AST?

Some downstream projects are using the ASTImporter to move types between ASTS (this is outside the normal LLDB architecture we have on top of the ASTImporter. It's more of a Clang + a bit of LLDB mix). The expectation there seems to be that the ASTImporter just handles the language conversion and that basic POD types can be copied from/to every AST. I believe the AST in Question has a 'To' AST that is Objective-C and the 'From' AST is Objective-C++. Both ASTs contain some basic shared struct definitions that need to be merged/imported. I don't think we can just parse everything as (Objective-)C++ as C++ introduces new keywords that might already be used as a normal identifier in the Objective-C code.

Also just to be clear: We are moving nodes from one AST to another AST with different LangOpts. So I don't think there is a way around using the ASTImporter (or reimplementing equivalent logic). Converting nodes in the same AST doesn't seem very useful, so the use case is similar to what the ASTImporter is doing + some extra work for converting the few Clang nodes that can be converted.

AFAIK, the parser creates the AST node according to the set language/dialect. So, it is not possible to convert a RecordDecl into a CXXRecordDecl so simply.

I assume @balazske meant setting the *parser* to C++ (at least that's what I assume in the answer above).

[balazske]

My idea was that if we have an already existing C AST then the mode of it could be changed to C++, without changing other nodes. Then it is possible to import C++ nodes.
For ASTConverter, I think about something similar to ASTImporter, but the To AST is always empty at start and one correct From AST can be imported into it. This "import" performs the conversion and build of AST nodes. This converted AST can then be processed with normal ASTImporter.

clang/include/clang/AST/ASTImporter.h

 class TagDecl;
 class TranslationUnitDecl;
 class TypeSourceInfo;
 
   class ImportError : public llvm::ErrorInfo<ImportError> {
   public:
     /// \brief Kind of error when importing an AST component.
     enum ErrorKind {
         NameConflict, /// Naming ambiguity (likely ODR violation).
         UnsupportedConstruct, /// Not supported node or case.
+        IncompatibleConstruct, /// Node not valid in target context.
         Unknown /// Other error.
     };
 
     ErrorKind Error;
 
     static char ID;
 
     ImportError() : Error(Unknown) {}

clang/lib/AST/ASTImporter.cpp

namespace clang {
 
   std::string ImportError::toString() const {
     // FIXME: Improve error texts.
     switch (Error) {
     case NameConflict:
       return "NameConflict";
     case UnsupportedConstruct:
       return "UnsupportedConstruct";
+    case IncompatibleConstruct:
+      return "Language construct not compatible with target AST.";
     case Unknown:
       return "Unknown error";
     }
     llvm_unreachable("Invalid error code.");
     return "Invalid error code.";
   }
 
   void ImportError::log(raw_ostream &OS) const {
case DeclarationName::CXXConversionFunctionName: {
     else
       return ToTInfoOrErr.takeError();
     return Error::success();
   }
   }
   llvm_unreachable("Unknown name kind.");
 }
 
+/// Returns true if the ASTImporter should skip this Decl when importing the
+/// Decls inside a DeclContext.
+static bool shouldSkipDeclInContext(ASTContext &ToCtx, Decl *D) {
+  // The To AST doesn't support C++ so filter out implicit C++ nodes that are
+  // generated when parsing C code as C++.
+  if (!ToCtx.getLangOpts().CPlusPlus) {
+    switch (D->getKind()) {
+    // Skip constructors/destructors which are generated by Clang.
+    case Decl::Kind::CXXConstructor:
+    case Decl::Kind::CXXDestructor:
+      return D->isImplicit();
+    case Decl::Kind::CXXRecord:
+      // Injected class names are only used in C++.
+      return cast<CXXRecordDecl>(D)->isInjectedClassName();
+    default:
+      break;
+    }
+  }
+  return false;
+}
+
 Error
 ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   if (Importer.isMinimalImport() && !ForceImport) {
     auto ToDCOrErr = Importer.ImportContext(FromDC);
     return ToDCOrErr.takeError();
   }
 
   // We use strict error handling in case of records and enums, but not
   // with e.g. namespaces.
   //
   // FIXME Clients of the ASTImporter should be able to choose an
   // appropriate error handling strategy for their needs.  For instance,
   // they may not want to mark an entire namespace as erroneous merely
   // because there is an ODR error with two typedefs.  As another example,
   // the client may allow EnumConstantDecls with same names but with
   // different values in two distinct translation units.
   bool AccumulateChildErrors = isa<TagDecl>(FromDC);
 
   Error ChildErrors = Error::success();
   for (auto *From : FromDC->decls()) {
+    if (shouldSkipDeclInContext(Importer.getToContext(), From))
+      continue;
     ExpectedDecl ImportedOrErr = import(From);
 
     // If we are in the process of ImportDefinition(...) for a RecordDecl we
     // want to make sure that we are also completing each FieldDecl. There
     // are currently cases where this does not happen and this is correctness
     // fix since operations such as code generation will expect this to be so.
     if (ImportedOrErr) {
       FieldDecl *FieldFrom = dyn_cast_or_null<FieldDecl>(From);
ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
   if (!SearchName && D->getTypedefNameForAnonDecl()) {
     if (Error Err = importInto(
         SearchName, D->getTypedefNameForAnonDecl()->getDeclName()))
       return std::move(Err);
     IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
     IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  /// Whether the to AST expects a RecordDecl (ant not to a CXXRecordDecl).
+  const bool ConvertToC = !Importer.getToContext().getLangOpts().CPlusPlus;
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod() && !D->isLambda()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     auto FoundDecls =
         Importer.findDeclsInToCtx(DC, SearchName);
     if (!FoundDecls.empty()) {
       // We're going to have to compare D against potentially conflicting Decls,
for (auto *FoundDecl : FoundDecls) {
 
         if (IsStructuralMatch(D, FoundRecord)) {
           RecordDecl *FoundDef = FoundRecord->getDefinition();
           if (D->isThisDeclarationADefinition() && FoundDef) {
             // FIXME: Structural equivalence check should check for same
             // user-defined methods.
             Importer.MapImported(D, FoundDef);
             if (const auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
+              // Only import any implicit method when the target AST supports
+              // C++.
+              if (!Importer.isMinimalImport() && !ConvertToC) {
                 auto *FoundCXX = dyn_cast<CXXRecordDecl>(FoundDef);
                 assert(FoundCXX && "Record type mismatch");
-
-              if (!Importer.isMinimalImport())
                 // FoundDef may not have every implicit method that D has
                 // because implicit methods are created only if they are used.
                 if (Error Err = ImportImplicitMethods(DCXX, FoundCXX))
                   return std::move(Err);
               }
             }
+          }
           PrevDecl = FoundRecord->getMostRecentDecl();
           break;
         }
         ConflictingDecls.push_back(FoundDecl);
       } // kind is RecordDecl
     } // for
 
     if (!ConflictingDecls.empty() && SearchName) {
ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   // Create the record declaration.
   RecordDecl *D2 = nullptr;
   CXXRecordDecl *D2CXX = nullptr;
-  if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
+  auto *DCXX = dyn_cast<CXXRecordDecl>(D);
+  // If the target context doesn't support C++ but From is using C++-exclusive
+  // features then the Decl can't be imported.
+  if (ConvertToC && DCXX && !DCXX->isCLike())
+    return make_error<ImportError>(ImportError::IncompatibleConstruct);
+
+  if (DCXX && !ConvertToC) {
+    // If the target context doesn't support C++ but From is using C++-exclusive
+    // features then there is nothing that can be done.
+    if (ConvertToC && !DCXX->isCLike())
+      return make_error<ImportError>(ImportError::IncompatibleConstruct);
+
     if (DCXX->isLambda()) {
       auto TInfoOrErr = import(DCXX->getLambdaTypeInfo());
       if (!TInfoOrErr)
         return TInfoOrErr.takeError();
       if (GetImportedOrCreateSpecialDecl(
               D2CXX, CXXRecordDecl::CreateLambda, D, Importer.getToContext(),
               DC, *TInfoOrErr, Loc, DCXX->isDependentLambda(),
               DCXX->isGenericLambda(), DCXX->getLambdaCaptureDefault()))

clang/lib/AST/ASTStructuralEquivalence.cpp

bool StructuralEquivalenceContext::CheckCommonEquivalence(Decl *D1, Decl *D2) {
   if (Template1 && !IsStructurallyEquivalent(*this, Template1, Template2))
     return false;
 
   // FIXME: Move check for identifier names into this function.
 
   return true;
 }
 
+/// Returns the Decl::Kind that should be used when checking if Decls have a
+/// similar enough kind to require a more in-depth comparison.
+/// This usually returns the Kind that was passed in except in cases where
+/// the same language construct is expressed by different Decl::Kinds in
+/// different languages (CXXRecord vs Record).
+static Decl::Kind getKindForComparison(Decl::Kind k) {
+  // For CXXRecords fall back to the parent class Record as Records and
+  // CXXRecords might be equivalent. The comparison code for Records handles
+  // both comparing CXXRecord and Records instances.
+  if (k == Decl::Kind::CXXRecord)
+    return Decl::Kind::Record;
+  return k;
+}
+
 bool StructuralEquivalenceContext::CheckKindSpecificEquivalence(
     Decl *D1, Decl *D2) {
 
+  const Decl::Kind Kind1 = getKindForComparison(D1->getKind());
+  const Decl::Kind Kind2 = getKindForComparison(D2->getKind());
+
   // Kind mismatch.
-  if (D1->getKind() != D2->getKind())
+  if (Kind1 != Kind2)
     return false;
 
   // Cast the Decls to their actual subclass so that the right overload of
   // IsStructurallyEquivalent is called.
-  switch (D1->getKind()) {
+  switch (Kind1) {
 #define ABSTRACT_DECL(DECL)
 #define DECL(DERIVED, BASE)                                                    \
   case Decl::Kind::DERIVED:                                                    \
-    return ::IsStructurallyEquivalent(*this, static_cast<DERIVED##Decl *>(D1), \
-                                      static_cast<DERIVED##Decl *>(D2));
+    return ::IsStructurallyEquivalent(*this, llvm::cast<DERIVED##Decl>(D1),    \
+                                      llvm::cast<DERIVED##Decl>(D2));
 #include "clang/AST/DeclNodes.inc"
   }
   return true;
 }
 
 bool StructuralEquivalenceContext::Finish() {
   while (!DeclsToCheck.empty()) {
     // Check the next declaration.

clang/unittests/AST/ASTImporterCrossLanguageTest.cpp

+//===- unittest/AST/ASTImporterCrossLanguageTest.cpp -===================--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Tests for the correct import of AST nodes between translation units of
+// different languages and language standards.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/DeclContextInternals.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+#include "ASTImporterFixtures.h"
+
+using namespace clang::ast_matchers;
+using namespace clang;
+
+namespace {
+/// Tests that import from C++ to C.
+struct CXXToCImport : ASTImporterOptionSpecificTestBase {};
+} // namespace
+
+TEST_P(CXXToCImport, ConvertStruct) {
+  // Import a C struct parsed as C++ to a C AST.
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct B {};
+      )",
+      Lang_CXX11, "input0.cc");
+  CXXRecordDecl *From =
+      FirstDeclMatcher<CXXRecordDecl>().match(FromTU, recordDecl(hasName("B")));
+  RecordDecl *To = Import<RecordDecl>(From, Lang_C99);
+  EXPECT_FALSE(isa<CXXRecordDecl>(To));
+  // The imported struct should contain none of the usual nested C++ Decls.
+  EXPECT_TRUE(To->decls_empty());
+}
+
+TEST_P(CXXToCImport, ConvertStructWithImplicitCtorDotr) {
+  // Import a C struct where Clang generated implicit dtors/ctors.
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct B {};
+      B b = B();
+      )",
+      Lang_CXX11, "input0.cc");
+  CXXRecordDecl *From =
+      FirstDeclMatcher<CXXRecordDecl>().match(FromTU, recordDecl(hasName("B")));
+
+  // Sanity check that the imported AST contains dtors/ctors.
+  FirstDeclMatcher<Decl>().match(FromTU, cxxConstructorDecl(hasName("B")));
+  FirstDeclMatcher<Decl>().match(FromTU, cxxDestructorDecl(hasName("~B")));
+
+  RecordDecl *To = Import<RecordDecl>(From, Lang_C99);
+  EXPECT_FALSE(isa<CXXRecordDecl>(To));
+  for (Decl *D : To->decls()) {
+    // Check that dtors/ctors were not imported.
+    EXPECT_FALSE(isa<CXXConstructorDecl>(D));
+    EXPECT_FALSE(isa<CXXDestructorDecl>(D));
+    // Check that the RecordDecl for the injected class name was not imported.
+    EXPECT_FALSE(isa<RecordDecl>(D));
+  }
+}
+
+TEST_P(CXXToCImport, MergeStruct) {
+  // Merge a C struct parsed as C into a C AST with the same struct.
+  std::string Source = "struct B{};";
+  Decl *ToTU = getToTuDecl(Source, Lang_C99);
+  Decl *FromTU = getTuDecl(Source, Lang_CXX11, "input0.cc");
+
+  // Find the struct in both TUs.
+  RecordDecl *To =
+      FirstDeclMatcher<RecordDecl>().match(ToTU, recordDecl(hasName("B")));
+  CXXRecordDecl *From =
+      FirstDeclMatcher<CXXRecordDecl>().match(FromTU, recordDecl(hasName("B")));
+  // Import it and check that it got merged into the existing struct.
+  RecordDecl *Imported = Import<RecordDecl>(From, Lang_C99);
+  EXPECT_EQ(To, Imported);
+  // The C struct should contain none of the usual nested C++ Decls.
+  EXPECT_TRUE(To->decls_empty());
+}
+
+TEST_P(CXXToCImport, InvalidStruct) {
+  // Try to import different kinds of records that are all only valid in C++.
+  std::vector<std::string> InvalidInputs = {
+      "class B {};",
+      "struct B { public: };",
+      "struct B { private: };",
+      "struct B { using T = int; };",
+      "struct B { static_assert(true, \"\"); };",
+      "struct B { B() {} };",
+      "struct B { B() = default };",
+      "struct B { ~B() {} };",
+      "struct B { ~B() = default };",
+      "struct B { int i = 0; };",
+      "struct B { void func(); };",
+      "struct B { void func() {} };",
+      "struct F {}; struct B { friend class F; };",
+      "struct B { int &i; B(); };",
+      "template<typename T> struct B {};",
+  };
+
+  unsigned InputIndex = 0;
+  for (const std::string &Input : InvalidInputs) {
+    SCOPED_TRACE("Input: " + Input);
+    Decl *FromTU = getTuDecl(Input, Lang_CXX11,
+                             "input" + std::to_string(InputIndex++) + ".cc");
+
+    CXXRecordDecl *From = FirstDeclMatcher<CXXRecordDecl>().match(
+        FromTU, recordDecl(hasName("B")));
+    // Try import the C++ record which has to fail.
+    llvm::Expected<clang::Decl *> Err = importOrError(From, Lang_C99);
+    ASSERT_THAT_EXPECTED(Err, llvm::Failed());
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, CXXToCImport,
+                        ::testing::Values(std::vector<std::string>()), );

clang/unittests/AST/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
   ASTImporterFixtures.cpp
+  ASTImporterCrossLanguageTest.cpp
   ASTImporterTest.cpp
   ASTImporterObjCTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterODRStrategiesTest.cpp
   ASTImporterVisibilityTest.cpp
   ASTTraverserTest.cpp
   ASTTypeTraitsTest.cpp
   ASTVectorTest.cpp