[clang][ASTImporter] Add a 'Message' member to ASTImportError and use it throughout ASTImporter

[Michael137]

Summary

This patch adds a new ASTImportError::Message which is used
to more accurately describe a failure when ASTImportError::log
is called. We then set this error message throughout ASTImporter.

The motivation for this is that in upcoming patches LLDB will
check ASTImportErrors and log them on failures. At the moment
these logs wouldn't be helpful since we wouldn't be able to
differentiate between the different conflicts that occur
during a single import process.

[Michael137]

• Remove redundant header additions

[Michael137]

Didn't add tests for this since checking the contents of the error message didn't seem worth testing.
But don't mind adding tests if reviewers feel more comfortable

[Michael137]

• Fix commit message

[balazske]

The goal is to have a message always in ASTImportError? Then probably the constructor without message can be removed, at least to check if really the message is added at all places. I found that it is missing in VisitObjCImplementationDecl.
I do not know what happens with messages passed to FromDiag or ToDiag and if these are available somehow to LLDB. Otherwise it would be even better to remove all FromDiag and ToDiag messages from ASTImporter and put these messages into ASTImportError and use later in the way that is needed by the use case. This would require to pass a message to HandleNameConflict but then we have always the detailed message. There are places where diagnostic is generated but there is no error, we should check if this is correct and if we can remove the diagnostic or make import error.

[Michael137]

In D154709#4487776, @balazske wrote:

The goal is to have a message always in ASTImportError? Then probably the constructor without message can be removed, at least to check if really the message is added at all places. I found that it is missing in VisitObjCImplementationDecl.

Agree, will do that

I do not know what happens with messages passed to FromDiag or ToDiag and if these are available somehow to LLDB.

Ideally yes, but I found they're not very helpful in LLDB's use-case because:

1. the diagnostic doesn't get issued at all call-sites
2. LLDB switches off any odr related warnings because AFAIU they occur a lot (outside the ASTImporter) and would flood the user with redundant information

Otherwise it would be even better to remove all FromDiag and ToDiag messages from ASTImporter and put these messages into ASTImportError and use later in the way that is needed by the use case. This would require to pass a message to HandleNameConflict but then we have always the detailed message. There are places where diagnostic is generated but there is no error, we should check if this is correct and if we can remove the diagnostic or make import error.

That sounds like a good way forward. The -ast-merge clang option does benefit from these diagnostics. So we would probably need to change it to dump the ASTImportError instead of relying on the diagnostics.

clang/include/clang/AST/ASTImportError.h

enum ErrorKind {
     Unknown               /// Other error.
   };
 
   ErrorKind Error;
 
   static char ID;
 
   ASTImportError() : Error(Unknown) {}
-  ASTImportError(const ASTImportError &Other) : Error(Other.Error) {}
-  ASTImportError &operator=(const ASTImportError &Other) {
-    Error = Other.Error;
-    return *this;
-  }
+
+  ASTImportError(const ASTImportError &) = default;
+  ASTImportError(ASTImportError &&) = default;
+
+  ASTImportError &operator=(const ASTImportError &) = default;
+  ASTImportError &operator=(ASTImportError &&) = default;
+
+  ASTImportError(ErrorKind Error, std::string Message)
+      : Error(Error), Message(std::move(Message)) {}
   ASTImportError(ErrorKind Error) : Error(Error) {}
 
   std::string toString() const;
 
   void log(llvm::raw_ostream &OS) const override;
   std::error_code convertToErrorCode() const override;
+
+private:
+  std::string Message;
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_ASTIMPORTERROR_H

clang/lib/AST/ASTImporter.cpp

namespace clang {
   using ExpectedType = llvm::Expected<QualType>;
   using ExpectedStmt = llvm::Expected<Stmt *>;
   using ExpectedExpr = llvm::Expected<Expr *>;
   using ExpectedDecl = llvm::Expected<Decl *>;
   using ExpectedSLoc = llvm::Expected<SourceLocation>;
   using ExpectedName = llvm::Expected<DeclarationName>;
 
   std::string ASTImportError::toString() const {
-    // FIXME: Improve error texts.
     switch (Error) {
     case NameConflict:
       return "NameConflict";
     case UnsupportedConstruct:
       return "UnsupportedConstruct";
     case Unknown:
       return "Unknown error";
     }
     llvm_unreachable("Invalid error code.");
     return "Invalid error code.";
   }
 
-  void ASTImportError::log(raw_ostream &OS) const { OS << toString(); }
+  void ASTImportError::log(raw_ostream &OS) const {
+    OS << toString();
+    if (!Message.empty())
+      OS << ": " << Message;
+  }
 
   std::error_code ASTImportError::convertToErrorCode() const {
     llvm_unreachable("Function not implemented.");
   }
 
   char ASTImportError::ID;
 
   template <class T>

 // Import Types
 //----------------------------------------------------------------------------
 
 using namespace clang;
 
 ExpectedType ASTNodeImporter::VisitType(const Type *T) {
   Importer.FromDiag(SourceLocation(), diag::err_unsupported_ast_node)
     << T->getTypeClassName();
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    T->getTypeClassName());
 }
 
 ExpectedType ASTNodeImporter::VisitAtomicType(const AtomicType *T){
   ExpectedType UnderlyingTypeOrErr = import(T->getValueType());
   if (!UnderlyingTypeOrErr)
     return UnderlyingTypeOrErr.takeError();
 
   return Importer.getToContext().getAtomicType(*UnderlyingTypeOrErr);
if (isa<RecordDecl>(D) && (FunDecl = dyn_cast<FunctionDecl>(OrigDC)) &&
     };
     for (const ParmVarDecl *P : FunDecl->parameters()) {
       const Type *LeafT =
           getLeafPointeeType(P->getType().getCanonicalType().getTypePtr());
       auto *RT = dyn_cast<RecordType>(LeafT);
       if (RT && RT->getDecl() == D) {
         Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
             << D->getDeclKindName();
-        return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+        return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                          D->getDeclKindName());
       }
     }
   }
 
   // Import the context of this declaration.
   if (Error Err = ImportDeclContext(D, DC, LexicalDC))
     return Err;
 
StructuralEquivalenceContext Ctx(
       Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
       false, Complain);
   return Ctx.IsEquivalent(From, To);
 }
 
 ExpectedDecl ASTNodeImporter::VisitDecl(Decl *D) {
   Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
     << D->getDeclKindName();
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    D->getDeclKindName());
 }
 
 ExpectedDecl ASTNodeImporter::VisitImportDecl(ImportDecl *D) {
   Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
       << D->getDeclKindName();
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    D->getDeclKindName());
 }
 
 ExpectedDecl ASTNodeImporter::VisitEmptyDecl(EmptyDecl *D) {
   // Import the context of this declaration.
   DeclContext *DC, *LexicalDC;
   if (Error Err = ImportDeclContext(D, DC, LexicalDC))
     return std::move(Err);
 
if (FieldDecl *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
       }
 
       // FIXME: Why is this case not handled with calling HandleNameConflict?
       Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
         << Name << D->getType() << FoundField->getType();
       Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
         << FoundField->getType();
 
-      return make_error<ASTImportError>(ASTImportError::NameConflict);
+      std::string ErrMessage;
+      llvm::raw_string_ostream OS(ErrMessage);
+      OS << "Field '" << Name.getAsString()
+         << "' declared with incompatible types in different TUs ('"
+         << D->getType() << "' and '" << FoundField->getType() << "')";
+      return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                        std::move(ErrMessage));
     }
   }
 
   Error Err = Error::success();
   auto ToType = importChecked(Err, D->getType());
   auto ToTInfo = importChecked(Err, D->getTypeSourceInfo());
   auto ToBitWidth = importChecked(Err, D->getBitWidth());
   auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
if (auto *FoundField = dyn_cast<IndirectFieldDecl>(FoundDecls[I])) {
         continue;
 
       // FIXME: Why is this case not handled with calling HandleNameConflict?
       Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
         << Name << D->getType() << FoundField->getType();
       Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
         << FoundField->getType();
 
-      return make_error<ASTImportError>(ASTImportError::NameConflict);
+      std::string ErrMessage;
+      llvm::raw_string_ostream OS(ErrMessage);
+      OS << "IndirectField '" << Name.getAsString()
+         << "' declared with incompatible types in different TUs ('"
+         << D->getType() << "' and '" << FoundField->getType() << "')";
+      return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                        std::move(ErrMessage));
     }
   }
 
   // Import the type.
   auto TypeOrErr = import(D->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 
if (ObjCIvarDecl *FoundIvar = dyn_cast<ObjCIvarDecl>(FoundDecl)) {
         return FoundIvar;
       }
 
       Importer.ToDiag(Loc, diag::warn_odr_ivar_type_inconsistent)
         << Name << D->getType() << FoundIvar->getType();
       Importer.ToDiag(FoundIvar->getLocation(), diag::note_odr_value_here)
         << FoundIvar->getType();
 
-      return make_error<ASTImportError>(ASTImportError::NameConflict);
+      std::string ErrMessage;
+      llvm::raw_string_ostream OS(ErrMessage);
+      OS << "ObjCIvarDecl '" << Name.getAsString()
+         << "' declared with incompatible types in different TUs ('"
+         << D->getType() << "' and '" << FoundIvar->getType() << "')";
+      return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                        std::move(ErrMessage));
     }
   }
 
   Error Err = Error::success();
   auto ToType = importChecked(Err, D->getType());
   auto ToTypeSourceInfo = importChecked(Err, D->getTypeSourceInfo());
   auto ToBitWidth = importChecked(Err, D->getBitWidth());
   auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
if (auto *FoundMethod = dyn_cast<ObjCMethodDecl>(FoundDecl)) {
                                              FoundMethod->getReturnType())) {
         Importer.ToDiag(Loc, diag::warn_odr_objc_method_result_type_inconsistent)
             << D->isInstanceMethod() << Name << D->getReturnType()
             << FoundMethod->getReturnType();
         Importer.ToDiag(FoundMethod->getLocation(),
                         diag::note_odr_objc_method_here)
           << D->isInstanceMethod() << Name;
 
-        return make_error<ASTImportError>(ASTImportError::NameConflict);
+        std::string ErrMessage;
+        llvm::raw_string_ostream OS(ErrMessage);
+        OS << "ObjCMethodDecl '" << Name.getAsString()
+           << "' has incompatible result types in different TUs ('"
+           << D->getReturnType() << "' and '" << FoundMethod->getReturnType()
+           << "')";
+        return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                          std::move(ErrMessage));
       }
 
       // Check the number of parameters.
       if (D->param_size() != FoundMethod->param_size()) {
         Importer.ToDiag(Loc, diag::warn_odr_objc_method_num_params_inconsistent)
           << D->isInstanceMethod() << Name
           << D->param_size() << FoundMethod->param_size();
         Importer.ToDiag(FoundMethod->getLocation(),
                         diag::note_odr_objc_method_here)
           << D->isInstanceMethod() << Name;
 
-        return make_error<ASTImportError>(ASTImportError::NameConflict);
+        std::string ErrMessage;
+        llvm::raw_string_ostream OS(ErrMessage);
+        OS << "ObjCMethodDecl '" << Name.getAsString()
+           << "' has a different number of parameters in different TUs ('"
+           << D->param_size() << "' and '" << FoundMethod->param_size() << "')";
+        return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                          std::move(ErrMessage));
       }
 
       // Check parameter types.
       for (ObjCMethodDecl::param_iterator P = D->param_begin(),
              PEnd = D->param_end(), FoundP = FoundMethod->param_begin();
            P != PEnd; ++P, ++FoundP) {
         if (!Importer.IsStructurallyEquivalent((*P)->getType(),
                                                (*FoundP)->getType())) {
           Importer.FromDiag((*P)->getLocation(),
                             diag::warn_odr_objc_method_param_type_inconsistent)
             << D->isInstanceMethod() << Name
             << (*P)->getType() << (*FoundP)->getType();
           Importer.ToDiag((*FoundP)->getLocation(), diag::note_odr_value_here)
             << (*FoundP)->getType();
 
-          return make_error<ASTImportError>(ASTImportError::NameConflict);
+          std::string ErrMessage;
+          llvm::raw_string_ostream OS(ErrMessage);
+          OS << "ObjCMethodDecl '" << Name.getAsString()
+             << "' has a parameter with different types in different TUs ('"
+             << (*P)->getType() << "' and '" << (*FoundP)->getType() << "')";
+          return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                            std::move(ErrMessage));
         }
       }
 
       // Check variadic/non-variadic.
       // Check the number of parameters.
       if (D->isVariadic() != FoundMethod->isVariadic()) {
         Importer.ToDiag(Loc, diag::warn_odr_objc_method_variadic_inconsistent)
           << D->isInstanceMethod() << Name;
         Importer.ToDiag(FoundMethod->getLocation(),
                         diag::note_odr_objc_method_here)
           << D->isInstanceMethod() << Name;
 
-        return make_error<ASTImportError>(ASTImportError::NameConflict);
+        std::string ErrMessage;
+        llvm::raw_string_ostream OS(ErrMessage);
+        OS << "ObjCMethodDecl '" << Name.getAsString()
+           << "' is variadic in one TU but not the other";
+        return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                          std::move(ErrMessage));
       }
 
       // FIXME: Any other bits we need to merge?
       return Importer.MapImported(D, FoundMethod);
     }
   }
 
   Error Err = Error::success();
if (auto *FoundProp = dyn_cast<ObjCPropertyDecl>(FoundDecl)) {
       // Check property types.
       if (!Importer.IsStructurallyEquivalent(D->getType(),
                                              FoundProp->getType())) {
         Importer.ToDiag(Loc, diag::warn_odr_objc_property_type_inconsistent)
           << Name << D->getType() << FoundProp->getType();
         Importer.ToDiag(FoundProp->getLocation(), diag::note_odr_value_here)
           << FoundProp->getType();
 
-        return make_error<ASTImportError>(ASTImportError::NameConflict);
+        std::string ErrMessage;
+        llvm::raw_string_ostream OS(ErrMessage);
+        OS << "ObjCPropertyDecl '" << Name.getAsString()
+           << "' declared with incompatible types in different TUs ('"
+           << D->getType() << "' and '" << FoundProp->getType() << "')";
+        return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                          std::move(ErrMessage));
       }
 
       // FIXME: Check property attributes, getters, setters, etc.?
 
       // Consider these properties to be equivalent.
       Importer.MapImported(D, FoundProp);
       return FoundProp;
     }
if (IsStructuralMatch(D, PrevDecl)) {
         // TODO Import instantiated exception specifications.
         //
         // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
         // what else could be fused during an AST merge.
         return PrevDefinition;
       }
     } else { // ODR violation.
       // FIXME HandleNameConflict
-      return make_error<ASTImportError>(ASTImportError::NameConflict);
+      std::string ErrMessage;
+      llvm::raw_string_ostream OS(ErrMessage);
+      OS << "ClassTemplateSpecializationDecl '" << D->getName()
+         << "' has incompatible definitions in different TUs.";
+      return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                        std::move(ErrMessage));
     }
   }
 
   // Import the location of this declaration.
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
   ExpectedSLoc IdLocOrErr = import(D->getLocation());

 
 //----------------------------------------------------------------------------
 // Import Statements
 //----------------------------------------------------------------------------
 
 ExpectedStmt ASTNodeImporter::VisitStmt(Stmt *S) {
   Importer.FromDiag(S->getBeginLoc(), diag::err_unsupported_ast_node)
       << S->getStmtClassName();
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    S->getStmtClassName());
 }
 
 
 ExpectedStmt ASTNodeImporter::VisitGCCAsmStmt(GCCAsmStmt *S) {
   if (Importer.returnWithErrorInTest())
-    return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+    return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                      S->getStmtClassName());
   SmallVector<IdentifierInfo *, 4> Names;
   for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++) {
     IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(I));
     // ToII is nullptr when no symbolic name is given for output operand
     // see ParseStmtAsm::ParseAsmOperandsOpt
     Names.push_back(ToII);
   }
 

 }
 
 //----------------------------------------------------------------------------
 // Import Expressions
 //----------------------------------------------------------------------------
 ExpectedStmt ASTNodeImporter::VisitExpr(Expr *E) {
   Importer.FromDiag(E->getBeginLoc(), diag::err_unsupported_ast_node)
       << E->getStmtClassName();
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    E->getStmtClassName());
 }
 
 ExpectedStmt ASTNodeImporter::VisitSourceLocExpr(SourceLocExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
   auto BLoc = importChecked(Err, E->getBeginLoc());
   auto RParenLoc = importChecked(Err, E->getEndLoc());
   if (Err)
case Stmt::ObjCBridgedCastExprClass: {
     if (!ToBridgeKeywordLocOrErr)
       return ToBridgeKeywordLocOrErr.takeError();
     return new (Importer.getToContext()) ObjCBridgedCastExpr(
         *ToLParenLocOrErr, OCE->getBridgeKind(), E->getCastKind(),
         *ToBridgeKeywordLocOrErr, ToTypeInfoAsWritten, ToSubExpr);
   }
   default:
     llvm_unreachable("Cast expression of unsupported type!");
-    return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+    return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                      E->getCastKindName());
   }
 }
 
 ExpectedStmt ASTNodeImporter::VisitOffsetOfExpr(OffsetOfExpr *E) {
   SmallVector<OffsetOfNode, 4> ToNodes;
   for (int I = 0, N = E->getNumComponents(); I < N; ++I) {
     const OffsetOfNode &FromNode = E->getComponent(I);
 
return CXXReinterpretCastExpr::Create(
         Importer.getToContext(), ToType, VK, CK, ToSubExpr, &(*ToBasePathOrErr),
         ToTypeInfoAsWritten, ToOperatorLoc, ToRParenLoc, ToAngleBrackets);
   } else if (isa<CXXConstCastExpr>(E)) {
     return CXXConstCastExpr::Create(
         Importer.getToContext(), ToType, VK, ToSubExpr, ToTypeInfoAsWritten,
         ToOperatorLoc, ToRParenLoc, ToAngleBrackets);
   } else {
     llvm_unreachable("Unknown cast type");
-    return make_error<ASTImportError>();
+    return make_error<ASTImportError>(ASTImportError::Unknown,
+                                      E->getCastName());
   }
 }
 
 ExpectedStmt ASTNodeImporter::VisitSubstNonTypeTemplateParmExpr(
     SubstNonTypeTemplateParmExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
   auto ToExprLoc = importChecked(Err, E->getExprLoc());

 ASTImporter::Import(ExprWithCleanups::CleanupObject From) {
   if (auto *CLE = From.dyn_cast<CompoundLiteralExpr *>()) {
     if (Expected<Expr *> R = Import(CLE))
       return ExprWithCleanups::CleanupObject(cast<CompoundLiteralExpr>(*R));
   }
 
   // FIXME: Handle BlockDecl when we implement importing BlockExpr in
   //        ASTNodeImporter.
-  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct);
+  return make_error<ASTImportError>(ASTImportError::UnsupportedConstruct,
+                                    "BlockDecl");
 }
 
 ExpectedTypePtr ASTImporter::Import(const Type *FromT) {
   if (!FromT)
     return FromT;
 
   // Check whether we've already imported this type.
   llvm::DenseMap<const Type *, const Type *>::iterator Pos =

 
 Expected<DeclarationName> ASTImporter::HandleNameConflict(DeclarationName Name,
                                                           DeclContext *DC,
                                                           unsigned IDNS,
                                                           NamedDecl **Decls,
                                                           unsigned NumDecls) {
   if (ODRHandling == ODRHandlingType::Conservative)
     // Report error at any name conflict.
-    return make_error<ASTImportError>(ASTImportError::NameConflict);
+    return make_error<ASTImportError>(ASTImportError::NameConflict,
+                                      Name.getAsString());
   else
     // Allow to create the new Decl with the same name.
     return Name;
 }
 
 DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) {
   if (LastDiagFromFrom)
     ToContext.getDiagnostics().notePriorDiagnosticFrom(