[clang-tidy] EndSourceFile() for preprocessor before diagnostic client (#145784)

The comment for `DiagnosticConsumer::BeginSourceFile()` states that
"diagnostics with source range information are required to only be
emitted in between BeginSourceFile() and EndSourceFile().". While
working on some upcoming changes to the static analyzer, we hit some
crashes when diagnostics were reported from the `EndOfMainFile` callback
in the preprocessor. This turned out to be because
`FrontEndAction::EndSourceFile()` notifies the diagnostic clients of the
end of the source file before it notifies the preprocessor. Thus, the
diagnostics from the preprocessor callback are reported when the
diagnostic client is no longer expecting any diagnostics.

The fix is to swap the order of the `EndSourceFile()` calls so that the
preprocessor is notified first.

I've added asserts to the `ClangTidyDiagnosticConsumer` to catch
unexpected diagnostics outside of a source file. Before swapping the
order of the calls as described above, this causes several failures in
the clang-tidy regression tests. With the swap, there are no failures in
`check-all`.

rdar://141230583

Author: dbartol

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -358,8 +358,27 @@ getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix) {
 
 } // namespace clang::tidy
 
+void ClangTidyDiagnosticConsumer::BeginSourceFile(const LangOptions &LangOpts,
+                                                  const Preprocessor *PP) {
+  DiagnosticConsumer::BeginSourceFile(LangOpts, PP);
+
+  assert(!InSourceFile);
+  InSourceFile = true;
+}
+
+void ClangTidyDiagnosticConsumer::EndSourceFile() {
+  assert(InSourceFile);
+  InSourceFile = false;
+
+  DiagnosticConsumer::EndSourceFile();
+}
+
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
+  // A diagnostic should not be reported outside of a
+  // BeginSourceFile()/EndSourceFile() pair if it has a source location.
+  assert(InSourceFile || Info.getLocation().isInvalid());
+
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

@@ -292,6 +292,11 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
+  void BeginSourceFile(const LangOptions &LangOpts,
+                       const Preprocessor *PP = nullptr) override;
+
+  void EndSourceFile() override;
+
   // Retrieve the diagnostics that were captured.
   std::vector<ClangTidyError> take();
 
@@ -326,6 +331,11 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
   bool LastErrorRelatesToUserCode = false;
   bool LastErrorPassesLineFilter = false;
   bool LastErrorWasIgnored = false;
+  /// Tracks whether we're currently inside a
+  /// `BeginSourceFile()/EndSourceFile()` pair. Outside of a source file, we
+  /// should only receive diagnostics that have to source location, such as
+  /// command-line warnings.
+  bool InSourceFile = false;
 };
 
 } // end namespace tidy

clang/lib/Frontend/FrontendAction.cpp

@@ -1243,13 +1243,15 @@ llvm::Error FrontendAction::Execute() {
 void FrontendAction::EndSourceFile() {
   CompilerInstance &CI = getCompilerInstance();
 
-  // Inform the diagnostic client we are done with this source file.
-  CI.getDiagnosticClient().EndSourceFile();
-
   // Inform the preprocessor we are done.
   if (CI.hasPreprocessor())
     CI.getPreprocessor().EndSourceFile();
 
+  // Inform the diagnostic client we are done with this source file.
+  // Do this after notifying the preprocessor, so that end-of-file preprocessor
+  // callbacks can report diagnostics.
+  CI.getDiagnosticClient().EndSourceFile();
+
   // Finalize the action.
   EndSourceFileAction();