[clang][ASTImporter] Fix an import error handling related bug.

[balazske]

Summary

This bug can cause that more import errors are generated than necessary
and many objects fail to import. Chance of an invalid AST after these
imports increases.

[martong]

Nice catch!

clang/lib/AST/ASTImporter.cpp

8789–8791

I think, this comment would be better to describe the declaration of PrevFromDi.

8792–8794

We should elevate this logic and the one in ImportDeclContext that uses consumeError into a common abstraction. I.e. making them closer to each other in a class.
Something like (draft, did not test it, might not compile):

class ErrorHandlingStrategy {
  static bool accumulateChildErrors(DeclContext*);
  static bool accumulateChildErrors(Decl *FromDi, Decl* Parent); // Use this one when traversing through the import path!
public:
  static Error handleDeclContextError(Error Err, DeclContext* FromDC) {
    Error ChildErrors = Error::success();
            if (Err && accumulateChildErrors(FromDC))
              ChildErrors =  joinErrors(std::move(ChildErrors), std::move(Err));
            else
              consumeError(std::move(Err));
     return ChildErrors
  }
  
};

[martong]

Related: https://reviews.llvm.org/D112132

[balazske]

clang/lib/AST/ASTImporter.cpp

8792–8794

The AccumulateChildErrors value looks the only common thing. It can mean: Accumulate errors (or not) for declarations that are in child-relationship with the parent. The value is only dependent on the parent, true if it is a TagDecl. This value can be extracted into a function that is used in importDeclContext and at Import(Decl *). At importDeclContext we should accumulate or discard errors for every child node. But not for possible other nodes that are imported but are not a child node (probably no such exists now but theoretically it is possible, and when using the error handling strategy for non-namespace-like nodes).

[martong]

clang/lib/AST/ASTImporter.cpp

8792–8794

My point is to try to encapsulate the error handling logic as much as possible. It is not just about AccumulateChildErrors. The current change scatters the error handling logic into seemingly distant code parts. The ErrorHandlingStrategy class could serve this encapsulation, because that's what classes do.

[balazske]

Created a common place for the DeclContext child error handling code.

[martong]

Very good! Thanks for the update!

clang/lib/AST/ASTImporter.cpp

handleAllErrors(ToDOrErr.takeError(),
                     [&ErrOut](const ImportError &E) { ErrOut = E; });
     setImportDeclError(FromD, ErrOut);
     // Set the error for the mapped to Decl, which is in the "to" context.
     if (Pos != ImportedDecls.end())
       SharedState->setImportDeclError(Pos->second, ErrOut);
 
     // Set the error for all nodes which have been created before we
     // recognized the error.
-    for (const auto &Path : SavedImportPaths[FromD])
+    for (const auto &Path : SavedImportPaths[FromD]) {
+      Decl *PrevFromDi = FromD;
       for (Decl *FromDi : Path) {
+        // Begin and end of the path equals 'FromD', skip it.
+        if (FromDi == FromD)
+          continue;
+        // We should not set import error on a node that "consumes" the error
+        // during import of its children (failing import of the child does not
+        // cause failure of the parent). See ImportDeclContext and uses of
+        // 'consumeError'.
+        // The import path contains child nodes first.
+        // (Parent-child relationship is used here in sense of import
+        // dependency.)
+        if (!isa<TagDecl>(FromDi))
+          if (auto *FromDiDC = dyn_cast<DeclContext>(FromDi))
+            if (FromDiDC->containsDecl(PrevFromDi))
+              break; // Skip every following node in the path.
+        PrevFromDi = FromDi;
         setImportDeclError(FromDi, ErrOut);
         //FIXME Should we remove these Decls from ImportedDecls?
         // Set the error for the mapped to Decl, which is in the "to" context.
         auto Ii = ImportedDecls.find(FromDi);
         if (Ii != ImportedDecls.end())
           SharedState->setImportDeclError(Ii->second, ErrOut);
           // FIXME Should we remove these Decls from the LookupTable,
           // and from ImportedFromDecls?
       }
+    }
     SavedImportPaths.erase(FromD);
 
     // Do not return ToDOrErr, error was taken out of it.
     return make_error<ImportError>(ErrOut);
   }
 
   ToD = *ToDOrErr;
 

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ErrorHandlingTest, ODRViolationWithinParmVarDecls) {
   auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
       FromTU, functionDecl(hasName("f")));
   ASSERT_TRUE(FromF);
 
   auto *ImportedF = Import(FromF, Lang_CXX11);
   EXPECT_FALSE(ImportedF);
 }
 
+TEST_P(ErrorHandlingTest, DoNotInheritErrorFromNonDependentChild) {
+  // Declarations should not inherit an import error from a child object
+  // if the declaration has no direct dependence to such a child.
+  // For example a namespace should not get import error if one of the
+  // declarations inside it fails to import.
+  // There was a special case in error handling (when "import path circles" are
+  // encountered) when this property was not held. This case is provoked by the
+  // following code.
+  constexpr auto ToTUCode = R"(
+      namespace ns {
+        struct Err {
+          char A;
+        };
+      }
+      )";
+  constexpr auto FromTUCode = R"(
+      namespace ns {
+        struct A {
+          using U = struct Err;
+        };
+      }
+      namespace ns {
+        struct Err {}; // ODR violation
+        void f(A) {}
+      }
+      )";
+
+  Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+  static_cast<void>(ToTU);
+  Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("A"), hasDefinition()));
+  ASSERT_TRUE(FromA);
+  auto *ImportedA = Import(FromA, Lang_CXX11);
+  // 'A' can not be imported: ODR error at 'Err'
+  EXPECT_FALSE(ImportedA);
+  // When import of 'A' failed there was a "saved import path circle" that
+  // contained namespace 'ns' (A - U - Err - ns - f - A). This should not mean
+  // that every object in this path fails to import.
+
+  Decl *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+      FromTU, namespaceDecl(hasName("ns")));
+  EXPECT_TRUE(FromNS);
+  auto *ImportedNS = Import(FromNS, Lang_CXX11);
+  EXPECT_TRUE(ImportedNS);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
       R"(
       void f() {
         auto L = [](){};
       }
       )",
       Lang_CXX11, "input0.cc");