[ASTImporter] Propagate error from ImportDeclContext

[martong]

Summary

During analysis of one project we failed to import one
CXXDestructorDecl. But since we did not propagate the error in
importDeclContext we had a CXXRecordDecl without a destructor. Then the
analyzer engine had a CallEvent where the nonexistent dtor was requested
(crash).

Solution is to propagate the errors we have during importing a
DeclContext.

[jkorous]

I don't have the insight to LGTM the whole change - just a nit about implementation detail.

clang/lib/AST/ASTImporter.cpp

1724

You might consider using just a lambda with llvm::make_scope_exit.

https://llvm.org/doxygen/namespacellvm.html#a4896534f3c6278be56967444daf38e3f

For example:

To->startDefinition();
auto DefinitionCompleter = llvm::make_scope_exit( [To](){To->completeDefinition();} );

[martong]

• Remove formatv b/c it can't handle braces in code

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

4744

Maybe add a similar test for namespace and check that the error is not propagated?

[martong]

• Remove the macro and use std::string.op+

[martong]

clang/lib/AST/ASTImporter.cpp

1724

Thanks! I have changed to make_scope_exit.

clang/unittests/AST/ASTImporterTest.cpp

4744

Ok, I have added that test.

[martong]

• Use make_scope_exit
• Add test ErrorIsNotPropagatedFromMemberToNamespace

[martong]

@a_sidorin Ping

[a_sidorin]

Hi Gabor,
The patch looks good. Thanks!

[martong]

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.

[martong]

Jenkins is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29802/

clang/lib/AST/ASTImporter.cpp

 }
 
 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();
   llvm::SmallVector<Decl *, 8> ImportedDecls;
   for (auto *From : FromDC->decls()) {
     ExpectedDecl ImportedOrErr = import(From);
-    if (!ImportedOrErr)
-      // Ignore the error, continue with next Decl.
-      // FIXME: Handle this case somehow better.
+    if (!ImportedOrErr) {
+      if (AccumulateChildErrors)
+        ChildErrors =
+            joinErrors(std::move(ChildErrors), ImportedOrErr.takeError());
+      else
         consumeError(ImportedOrErr.takeError());
     }
+  }
 
-  return Error::success();
+  return ChildErrors;
 }
 
 Error ASTNodeImporter::ImportDeclContext(
     Decl *FromD, DeclContext *&ToDC, DeclContext *&ToLexicalDC) {
   auto ToDCOrErr = Importer.ImportContext(FromD->getDeclContext());
   if (!ToDCOrErr)
     return ToDCOrErr.takeError();
   ToDC = *ToDCOrErr;
Error ASTNodeImporter::ImportDefinition(
     RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind) {
   if (To->getDefinition() || To->isBeingDefined()) {
     if (Kind == IDK_Everything)
       return ImportDeclContext(From, /*ForceImport=*/true);
 
     return Error::success();
   }
 
-  To->startDefinition();
+  // Complete the definition even if error is returned.
+  // The RecordDecl may be already part of the AST so it is better to
+  // have it in complete state even if something is wrong with it.
+  struct DefinitionCompleter {
+    RecordDecl *To;
+    DefinitionCompleter(RecordDecl *To) : To(To) { To->startDefinition(); }
+    ~DefinitionCompleter() { To->completeDefinition(); }
+  };
+  DefinitionCompleter CompleterRAII(To);
 
   if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
     return Err;
 
   // Add base classes.
   auto *ToCXX = dyn_cast<CXXRecordDecl>(To);
   auto *FromCXX = dyn_cast<CXXRecordDecl>(From);
   if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
     if (!Bases.empty())
       ToCXX->setBases(Bases.data(), Bases.size());
   }
 
   if (shouldForceImportDeclContext(Kind))
     if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
       return Err;
 
-  To->completeDefinition();
   return Error::success();
 }
 
 Error ASTNodeImporter::ImportInitializer(VarDecl *From, VarDecl *To) {
   if (To->getAnyInitializer())
     return Error::success();
 
   Expr *FromInit = From->getInit();

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ErrorHandlingTest, ErrorHappensAfterNodeIsCreatedAndLinked) {
   Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromProto);
   ASSERT_TRUE(OptErr);
   EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
   OptErr = Importer->getImportDeclErrorIfAny(FromDef);
   ASSERT_TRUE(OptErr);
   EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
 }
 
+// An error should be set for a class if we cannot import one member.
+TEST_P(ErrorHandlingTest, ErrorIsPropagatedFromMemberToClass) {
+  std::string Code = formatv(
+      R"(
+      class X {
+        void f() { {0}; } // This member has the error during import.
+        void ok();        // The error should not prevent importing this.
+      };                  // An error will be set for X too.
+      )",
+      ErroneousStmt);
+  TranslationUnitDecl *FromTU = getTuDecl(Code, Lang_CXX);
+
+  auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("X")));
+  CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+  // An error is set for X.
+  EXPECT_FALSE(ImportedX);
+  ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+  Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+  // An error is set for f().
+  auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
+      FromTU, cxxMethodDecl(hasName("f")));
+  OptErr = Importer->getImportDeclErrorIfAny(FromF);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  // And any subsequent import should fail.
+  CXXMethodDecl *ImportedF = Import(FromF, Lang_CXX);
+  EXPECT_FALSE(ImportedF);
+
+  // There is no error set for ok().
+  auto *FromOK = FirstDeclMatcher<CXXMethodDecl>().match(
+      FromTU, cxxMethodDecl(hasName("ok")));
+  OptErr = Importer->getImportDeclErrorIfAny(FromOK);
+  EXPECT_FALSE(OptErr);
+  // And we should be able to import.
+  CXXMethodDecl *ImportedOK = Import(FromOK, Lang_CXX);
+  EXPECT_TRUE(ImportedOK);
+
+  // Unwary clients may access X even if the error is set, so, at least make
+  // sure the class is set to be complete.
+  CXXRecordDecl *ToX = cast<CXXRecordDecl>(ImportedOK->getDeclContext());
+  EXPECT_TRUE(ToX->isCompleteDefinition());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );