[ASTImporter] Corrected diagnostic client handling in tests.

[balazske]

Summary

ASTImporter tests may produce source file related warnings, the diagnostic
client should be in correct state to handle it. Added 'beginSourceFile' to set
the client state.

[martong]

Adding Argyrios and Daniel as a reviewer for ASTUnit related changes.

[martong]

LGTM! But let's wait the review of those people who have history in ASTUnit.cpp.

[a.sidorin]

Looks good to me, but the approval from AST code owners is required, I think.

[akyrtzi]

It's not clear to me what kind of issue you are addressing, for example is the unit test hitting an assertion hit without your changes ? Or is there something else ?
Also it would be good to add some documentation comments to clarify what's the use case and when ASTUnit::beginSourceFile() should be getting called.

[balazske]

If BeginSourceFile is not called on the diagnostic client object, it is not possible to have compiler warnings or errors that come from the "To" context while importing something (there is assertion in TextDiagnosticPrinter::HandleDiagnostic). In the currently existing tests there is no such case but I have not yet committed tests that produce such warnings (for example ODR violations during import or encounter of unsupported constructs).

[balazske]

The new ASTUnit::beginSourceFile is only there to simplify the code. It is possible to get Ctx, PP and getDiagnostic() from outside of ASTUnit and call the same thing, but requires more code to write. Probably a more smart place to call BeginSourceFile can be found if we look deep into ASTUnit and buildAST functions that generate the AST from code that has associated source file but this would be a change that affects more code.

[akyrtzi]

Thanks for the explanation. Please do add documentation comments for the new method so people using ASTUnit in their own code have an idea when and why they would need to call this. Something like "if you intend to emit additional diagnostics after the ASTUnit is created [...]".
Also consider making the naming more clear to match the intended purpose, like enableEmittingAdditionalDiagnostics() or something similar.
Otherwise LGTM.

[balazske]

• Added comment, renamed beginSourceFile, removed check for PP.

Check for PP is removed because it is allowed to be nullptr.

[balazske]

From API point of view if there is a enableSourceFileDiagnostics there should be a disableSourceFileDiagnostics too (that calls the EndSourceFile). But I am not sure how and if to use it at all. In the unit tests it is not needed, the ASTUnit contains a single entity for the "To" context.

[akyrtzi]

We could leave disableSourceFileDiagnostics off until someone finds a use case for it.

[balazske]

I want this change to be committed by somebody. I have no commit right.

include/clang/Frontend/ASTUnit.h

public:
   std::shared_ptr<Preprocessor> getPreprocessorPtr() const { return PP; }
 
   const ASTContext &getASTContext() const { return *Ctx; }
   ASTContext &getASTContext() { return *Ctx; }
 
   void setASTContext(ASTContext *ctx) { Ctx = ctx; }
   void setPreprocessor(std::shared_ptr<Preprocessor> pp);
 
+  void beginSourceFile();
+
   bool hasSema() const { return (bool)TheSema; }
 
   Sema &getSema() const { 
     assert(TheSema && "ASTUnit does not have a Sema object!");
     return *TheSema;
   }
 
   const LangOptions &getLangOpts() const {

lib/Frontend/ASTUnit.cpp

ASTUnit::~ASTUnit() {
   if (getenv("LIBCLANG_OBJTRACKING"))
     fprintf(stderr, "--- %u translation units\n", --ActiveASTUnitObjects);
 }
 
 void ASTUnit::setPreprocessor(std::shared_ptr<Preprocessor> PP) {
   this->PP = std::move(PP);
 }
 
+void ASTUnit::beginSourceFile() {
+  assert(getDiagnostics().getClient() && PP && Ctx &&
+      "Bad context for source file");
+  getDiagnostics().getClient()->BeginSourceFile(Ctx->getLangOpts(), PP.get());
+}
+
 /// Determine the set of code-completion contexts in which this
 /// declaration should be shown.
 static unsigned getDeclShowContexts(const NamedDecl *ND,
                                     const LangOptions &LangOpts,
                                     bool &IsNestedNameSpecifier) {
   IsNestedNameSpecifier = false;
 
   if (isa<UsingShadowDecl>(ND))

unittests/AST/ASTImporterTest.cpp

testImport(const std::string &FromCode, const ArgVector &FromArgs,
   std::unique_ptr<ASTUnit>
       FromAST = tooling::buildASTFromCodeWithArgs(
         FromCode, FromArgs, InputFileName),
       ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
 
   ASTContext &FromCtx = FromAST->getASTContext(),
       &ToCtx = ToAST->getASTContext();
 
+  FromAST->beginSourceFile();
+  ToAST->beginSourceFile();
+
   ASTImporter Importer(ToCtx, ToAST->getFileManager(),
                        FromCtx, FromAST->getFileManager(), false);
 
   auto FoundNodes = match(SearchMatcher, FromCtx);
   if (FoundNodes.size() != 1)
     return testing::AssertionFailure()
            << "Multiple potential nodes were found!";
 
struct TU {
     std::string Code;
     std::string FileName;
     std::unique_ptr<ASTUnit> Unit;
     TranslationUnitDecl *TUDecl = nullptr;
     TU(StringRef Code, StringRef FileName, ArgVector Args)
         : Code(Code), FileName(FileName),
           Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
                                                  this->FileName)),
-          TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {}
+          TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
+      Unit->beginSourceFile();
+    }
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
   // moving of the stored objects because that would mean breaking the
   // references in the AST. By using a vector a move could happen when the
ArgVector FromArgs = getArgVectorForLanguage(FromLang),
               ToArgs = getArgVectorForLanguage(ToLang);
 
     FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
     TU &FromTU = FromTUs.back();
 
     ToCode = ToSrcCode;
     assert(!ToAST);
     ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+    ToAST->beginSourceFile();
 
     ASTContext &FromCtx = FromTU.Unit->getASTContext(),
                &ToCtx = ToAST->getASTContext();
 
     createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
 
     ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
                          FromTU.Unit->getFileManager(), false);
public:
   }
 
   // Creates the To context with the given source code and returns the TU decl.
   TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
     ArgVector ToArgs = getArgVectorForLanguage(ToLang);
     ToCode = ToSrcCode;
     assert(!ToAST);
     ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+    ToAST->beginSourceFile();
 
     return ToAST->getASTContext().getTranslationUnitDecl();
   }
 
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
   Decl *Import(Decl *From, Language ToLang) {
     if (!ToAST) {
       ArgVector ToArgs = getArgVectorForLanguage(ToLang);
       // Build the AST from an empty file.
       ToAST =
           tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+      ToAST->beginSourceFile();
     }
 
     // Create a virtual file in the To Ctx which corresponds to the file from
     // which we want to import the `From` Decl. Without this source locations
     // will be invalid in the ToCtx.
     auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
       return E.TUDecl == From->getTranslationUnitDecl();
     });