[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

8848–8850

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

8851–8853

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

8851–8853

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

8851–8853

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

namespace clang {
 
   void updateFlags(const Decl *From, Decl *To) {
     // Check if some flags or attrs are new in 'From' and copy into 'To'.
     // FIXME: Other flags or attrs?
     if (From->isUsed(false) && !To->isUsed(false))
       To->setIsUsed();
   }
 
+  /// How to handle import errors that occur when import of a child declaration
+  /// of a DeclContext fails.
+  class ChildErrorHandlingStrategy {
+    /// This context is imported (in the 'from' domain).
+    /// It is nullptr if a non-DeclContext is imported.
+    const DeclContext *const FromDC;
+    /// Ignore import errors of the children.
+    /// If true, the context can be imported successfully if a child
+    /// of it failed to import. Otherwise the import errors of the child nodes
+    /// are accumulated (joined) into the import error object of the parent.
+    /// (Import of a parent can fail in other ways.)
+    bool const IgnoreChildErrors;
+
+  public:
+    ChildErrorHandlingStrategy(const DeclContext *FromDC)
+        : FromDC(FromDC), IgnoreChildErrors(!isa<TagDecl>(FromDC)) {}
+    ChildErrorHandlingStrategy(const Decl *FromD)
+        : FromDC(dyn_cast<DeclContext>(FromD)),
+          IgnoreChildErrors(!isa<TagDecl>(FromD)) {}
+
+    /// Process the import result of a child (of the current declaration).
+    /// \param ResultErr The import error that can be used as result of
+    /// importing the parent. This may be changed by the function.
+    /// \param ChildErr Result of importing a child. Can be success or error.
+    void handleChildImportResult(Error &ResultErr, Error &&ChildErr) {
+      if (ChildErr && !IgnoreChildErrors)
+        ResultErr = joinErrors(std::move(ResultErr), std::move(ChildErr));
+      else
+        consumeError(std::move(ChildErr));
+    }
+
+    /// Determine if import failure of a child does not cause import failure of
+    /// its parent.
+    bool ignoreChildErrorOnParent(Decl *FromChildD) const {
+      if (!IgnoreChildErrors || !FromDC)
+        return false;
+      return FromDC->containsDecl(FromChildD);
+    }
+  };
+
   class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
                           public DeclVisitor<ASTNodeImporter, ExpectedDecl>,
                           public StmtVisitor<ASTNodeImporter, ExpectedStmt> {
     ASTImporter &Importer;
 
     // Use this instead of Importer.importInto .
     template <typename ImportT>
     LLVM_NODISCARD Error importInto(ImportT &To, const ImportT &From) {
ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // 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);
+  ChildErrorHandlingStrategy HandleChildErrors(FromDC);
 
   Error ChildErrors = Error::success();
   for (auto *From : FromDC->decls()) {
     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
if (ImportedOrErr) {
             ToRecordDecl = RecordTo->getDecl();
           }
         }
 
         if (FromRecordDecl && ToRecordDecl) {
           if (FromRecordDecl->isCompleteDefinition() &&
               !ToRecordDecl->isCompleteDefinition()) {
             Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl);
-
-            if (Err && AccumulateChildErrors)
-              ChildErrors =  joinErrors(std::move(ChildErrors), std::move(Err));
-            else
-              consumeError(std::move(Err));
+            HandleChildErrors.handleChildImportResult(ChildErrors,
+                                                      std::move(Err));
           }
         }
       }
     } else {
-      if (AccumulateChildErrors)
-        ChildErrors =
-            joinErrors(std::move(ChildErrors), ImportedOrErr.takeError());
-      else
-        consumeError(ImportedOrErr.takeError());
+      HandleChildErrors.handleChildImportResult(ChildErrors,
+                                                ImportedOrErr.takeError());
     }
   }
 
   // We reorder declarations in RecordDecls because they may have another order
   // in the "to" context than they have in the "from" context. This may happen
   // e.g when we import a class like this:
   //    struct declToImport {
   //        int a = c + b;
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]) {
+      // The import path contains import-dependency nodes first.
+      // Save the node that was imported as dependency of the current node.
+      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 and all following nodes in
+        // the path if child import errors are ignored.
+        if (ChildErrorHandlingStrategy(FromDi).ignoreChildErrorOnParent(
+                PrevFromDi))
+          break;
+        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");