[ASTImporter] Propagate errors during import of overridden methods.

[balazske]

Summary

If importing overridden methods fails for a method it can be seen
incorrectly as non-virtual. To avoid this inconsistency the method
is marked with import error to avoid later use of it.

[martong]

LGTM, other than a few comments.

clang/lib/AST/ASTImporter.cpp

7809

I think this is the time to change the name of this function. In fact, we import the overridden methods and not the ones which override this function.
So, I suggest this rename: ImportOverrides -> ImportOverriddenMethods

clang/unittests/AST/ASTImporterTest.cpp

5222

Perhaps we should check the error here is UnsupportedConstruct too.

[shafik]

LGTM but I agree w/ Gabor's comments

[balazske]

• Renamed ImportOverrides, additional check in test.

clang/lib/AST/ASTImporter.cpp

Error ImportContainerChecked(
           InContainer.begin(), InContainer.end(), OutContainer.begin());
     }
 
     template<typename InContainerTy, typename OIter>
     Error ImportArrayChecked(const InContainerTy &InContainer, OIter Obegin) {
       return ImportArrayChecked(InContainer.begin(), InContainer.end(), Obegin);
     }
 
-    void ImportOverrides(CXXMethodDecl *ToMethod, CXXMethodDecl *FromMethod);
+    Error ImportOverrides(CXXMethodDecl *ToMethod, CXXMethodDecl *FromMethod);
 
     Expected<FunctionDecl *> FindFunctionTemplateSpecialization(
         FunctionDecl *FromFD);
   };
 
 template <typename InContainerTy>
 Error ASTNodeImporter::ImportTemplateArgumentListInfo(
     SourceLocation FromLAngleLoc, SourceLocation FromRAngleLoc,
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   // FIXME should we handle this generically in VisitFriendDecl?
   // In Other cases when LexicalDC != DC we don't want it to be added,
   // e.g out-of-class definitions like void B::f() {} .
   if (LexicalDC != DC && IsFriend) {
     DC->makeDeclVisibleInContext(ToFunction);
   }
 
   if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
-    ImportOverrides(cast<CXXMethodDecl>(ToFunction), FromCXXMethod);
+    if (Error Err =
+            ImportOverrides(cast<CXXMethodDecl>(ToFunction), FromCXXMethod))
+      return std::move(Err);
 
   // Import the rest of the chain. I.e. import all subsequent declarations.
   for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) {
     ExpectedDecl ToRedeclOrErr = import(*RedeclIt);
     if (!ToRedeclOrErr)
       return ToRedeclOrErr.takeError();
   }
 
ExpectedStmt ASTNodeImporter::VisitCXXTypeidExpr(CXXTypeidExpr *E) {
   ExpectedExpr ToExprOperandOrErr = import(E->getExprOperand());
   if (!ToExprOperandOrErr)
     return ToExprOperandOrErr.takeError();
 
   return new (Importer.getToContext()) CXXTypeidExpr(
       *ToTypeOrErr, *ToExprOperandOrErr, *ToSourceRangeOrErr);
 }
 
-void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
+Error ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
                                        CXXMethodDecl *FromMethod) {
+  Error ImportErrors = Error::success();
   for (auto *FromOverriddenMethod : FromMethod->overridden_methods()) {
     if (auto ImportedOrErr = import(FromOverriddenMethod))
       ToMethod->getCanonicalDecl()->addOverriddenMethod(cast<CXXMethodDecl>(
           (*ImportedOrErr)->getCanonicalDecl()));
     else
-      consumeError(ImportedOrErr.takeError());
+      ImportErrors =
+          joinErrors(std::move(ImportErrors), ImportedOrErr.takeError());
   }
+  return ImportErrors;
 }
 
 ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          ASTContext &FromContext, FileManager &FromFileManager,
                          bool MinimalImport,
                          std::shared_ptr<ASTImporterSharedState> SharedState)
     : SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),

clang/unittests/AST/ASTImporterTest.cpp

// Try to import  X again but from another TU.
     ASTImporter *Importer = findFromTU(FromX)->Importer.get();
     Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
     // And an error is set for this new X in the "from" ctx.
     ASSERT_TRUE(OptErr);
     EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
   }
 }
 
+TEST_P(ErrorHandlingTest, ImportOfOverriddenMethods) {
+  auto MatchFooA =
+      functionDecl(hasName("foo"), hasAncestor(cxxRecordDecl(hasName("A"))));
+  auto MatchFooB =
+      functionDecl(hasName("foo"), hasAncestor(cxxRecordDecl(hasName("B"))));
+  auto MatchFooC =
+      functionDecl(hasName("foo"), hasAncestor(cxxRecordDecl(hasName("C"))));
+
+  // Provoke import of a method that has overridden methods with import error.
+  TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+        struct C;
+        struct A {
+          virtual void foo();
+          void f1(C *);
+        };
+        void A::foo() {
+          )") + ErroneousStmt + R"(
+        }
+        struct B : public A {
+          void foo() override;
+        };
+        struct C : public B {
+          void foo() override;
+        };
+        )",
+                                          Lang_CXX11);
+  auto *FromFooA = FirstDeclMatcher<FunctionDecl>().match(FromTU, MatchFooA);
+  auto *FromFooB = FirstDeclMatcher<FunctionDecl>().match(FromTU, MatchFooB);
+  auto *FromFooC = FirstDeclMatcher<FunctionDecl>().match(FromTU, MatchFooC);
+
+  FunctionDecl *ImportedFooA = Import(FromFooA, Lang_CXX11);
+  EXPECT_FALSE(ImportedFooA);
+  ASTImporter *Importer = findFromTU(FromFooA)->Importer.get();
+  Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromFooA);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  EXPECT_FALSE(Import(FromFooB, Lang_CXX11));
+  EXPECT_FALSE(Import(FromFooC, Lang_CXX11));
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
       R"(
       void f() {
         auto L = [](){};
       }
       )",
       Lang_CXX11, "input0.cc");