[libclang] Allow skipping warnings from all included files

[nik]

Summary

Depending on the included files and the used warning flags, e.g. -
Weverything, a huge number of warnings can be reported for included
files. As processing that many diagnostics comes with a performance
impact and not all clients are interested in those diagnostics, add a
flag to skip them.

[yvvan]

Cool! That's actually quite a small change with big outcome!

include/clang/Basic/Diagnostic.h

216

Probably mention that it also suppresses everything with lower priority?

lib/Basic/DiagnosticIDs.cpp

484

{ } around return

[nik]

Addressed comments.

[yvvan]

@ilya-biryukov what do you think?

[ilya-biryukov]

Sorry for the late response, was on vacation.

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

I wonder if you want to handle notes and remarks in a special manner? They can be seen as part of the original diagnostic, rather than the separate diagnostic. E.g. showing a note in the main file, but not showing the diagnostic from the headers file that this note comes from, might be confusing to the users.
Maybe also add tests for diagnostics in the main file with notes/remarks in the header files and vice versa?

[yvvan]

The idea was to ignore everything including notes and remarks so that only errors from system headers are still collected.

Changing consumer might be a possible way to go as well but it requires changing (or wrapping) all consumers that we need to be affected. The advantage of this change that it also covers consumers that come from plugins that we use (tidy and clazy).

[yvvan]

But this one misses a way to set this flag for everything except libclang. We can probably change that (Nikolai is in vacation till the end of summer so it's probably my part now) and add some tests outside of Index for it (probably Frontend)

[nik]

In D48116#1144732, @ilya-biryukov wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Will check.

I wonder if you want to handle notes and remarks in a special manner? They can be seen as part of the original diagnostic, rather than the separate diagnostic. E.g. showing a note in the main file, but not showing the diagnostic from the headers file that this note comes from, might be confusing to the users.

Looks like this case is handled fine.

WIthout the new option, a note is printed as expected/always (shortened output):

$ bin/c-index-test -test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
ignore-warnings-from-headers.h:1:12: warning: unused parameter 'unusedInHeader' [-Wunused-parameter]
ignore-warnings-from-headers.cpp:1:10: note: in file included from ignore-warnings-from-headers.cpp:1:
ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 'unusedInMainFile' [-Wunused-parameter]

With the new option, note is not printed and thus no confusion should arise:

$ CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 bin/c-index-test -test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 'unusedInMainFile' [-Wunused-parameter]

I will add "// CHECK-NOT: note: in file included from" to the test.

Maybe also add tests for diagnostics in the main file with notes/remarks in the header files and vice versa?

"Vice versa" is covered as shown above. If the diagnostic is in main file and a note of that one refers to the header, then the note should be shown/included. This case seems also fine - I've added a test for this.

In D48116#1144838, @yvvan wrote:

But this one misses a way to set this flag for everything except libclang. We can probably change that (Nikolai is in vacation till the end of summer so it's probably my part now) and add some tests outside of Index for it (probably Frontend)

What's the use case of this?

[nik]

In D48116#1144732, @ilya-biryukov wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.

If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

$ git grep "bool CaptureDiagnostics"
include/clang/Frontend/ASTUnit.h:  bool CaptureDiagnostics = false;
include/clang/Frontend/ASTUnit.h:                             ASTUnit &AST, bool CaptureDiagnostics,
include/clang/Frontend/ASTUnit.h:         IntrusiveRefCntPtr<DiagnosticsEngine> Diags, bool CaptureDiagnostics,
include/clang/Frontend/ASTUnit.h:      bool CaptureDiagnostics = false, bool AllowPCHWithCompilerErrors = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
lib/Frontend/ASTUnit.cpp:                             ASTUnit &AST, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool CaptureDiagnostics, bool AllowPCHWithCompilerErrors,
lib/Frontend/ASTUnit.cpp:                bool CaptureDiagnostics, bool UserFilesAreVolatile) {
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
tools/libclang/Indexing.cpp:  bool CaptureDiagnostics = !Logger::isLoggingEnabled();

[ilya-biryukov]

In D48116#1322878, @nik wrote:

In D48116#1144732, @ilya-biryukov wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

I

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.
If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

Yeah, LG. The changes in the ASTUnit look strictly better than changes in clang - the latter seems to already provide enough to do the filtering.
If you avoid changing the StoredDiagnosticConsumer (or writing a filtering wrapper for DiagnosticConsumer), you'll end up having some diagnostics inside headers generated after preamble was built, right?
That might be either ok or not, depending on the use-case.

[nik]

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.
If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

Yeah, LG. The changes in the ASTUnit look strictly better than changes in clang - the latter seems to already provide enough to do the filtering.
If you avoid changing the StoredDiagnosticConsumer (or writing a filtering wrapper for DiagnosticConsumer), you'll end up having some diagnostics inside headers generated after preamble was built, right?

If there is some #include after the first declaration, possibly. Why is that relevant?

[nik]

OK, filtering happens now in FilterAndStoreDiagnosticConsumer, the former
StoredDiagnosticConsumer.

[ilya-biryukov]

This LGTM wrt to layering, i.e. it's nice we don't need to change the code of diagnostics.
It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

Added a few nits too.

lib/Frontend/ASTUnit.cpp

682 ↗

(On Diff #187318)

isWrittenInMainFile might be a better fit: it does not look at presumed locations. That would be the expected behavior in the most common case, i.e. showing errors in an IDE or a text editor.

694 ↗

(On Diff #187318)

Why do we need this extra check? We checked for StoredDiags and Standalone diags in the body of the statement later again.

[nik]

Ping.

[nik]

lib/Frontend/ASTUnit.cpp

682 ↗

(On Diff #187318)

Oh, good catch! Thanks! :)

694 ↗

(On Diff #187318)

Ops, looks like a left-over from a previous version, removed.

[nik]

Addressed inline comments.

[nik]

Sorry for the pointless ping, haven't seen the inline comments. They are addressed now.

I've also increased CINDEX_VERSION_MINOR so clients can detect availability of this new flag.

It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

Benjamin? Argyrios? Would you be so kind? :)

[nik]

Jan?

[nik]

Ping.

[nik]

Ping.

[ilya-biryukov]

@jkorous, could you take a look or suggest someone who can review the libclang changes?
As mentioned before, I've reviewed the clang bits (which are now gone), but don't have much experience in libclang parts.

[yvvan]

libclang part is quite small here and looks ok. I would just accept it

include/clang-c/Index.h

enum CXTranslationUnit_Flags {
   CXTranslationUnit_SingleFileParse = 0x400,
 
   /**
    * \brief Used in combination with CXTranslationUnit_SkipFunctionBodies to
    * constrain the skipping of function bodies to the preamble.
    *
    * The function bodies of the main file are not skipped.
    */
-  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800
+  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800,
+
+  /**
+   * Used to indicate that non-errors from included files should be ignored.
+   *
+   * If set, clang_getDiagnosticSetFromTU() will not report e.g. warnings from
+   * included files anymore. This speeds up clang_getDiagnosticSetFromTU() for
+   * the case where these warnings are not of interest, as for an IDE for
+   * example, which typically shows only the diagnostics in the main file.
+   */
+  CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles = 0x1000
 };
 
 /**
  * Returns the set of flags that is suitable for parsing a translation
  * unit that is being edited.
  *
  * The set of flags returned provide options for \c clang_parseTranslationUnit()
  * to indicate that the translation unit is likely to be reparsed many times,

include/clang/Basic/Diagnostic.h

private:
   unsigned char AllExtensionsSilenced = 0;
 
   // Suppress diagnostics after a fatal error?
   bool SuppressAfterFatalError = true;
 
   // Suppress all diagnostics.
   bool SuppressAllDiagnostics = false;
 
+  // Suppress non-errors from all included files.
+  bool SuppressNonErrorsFromIncludedFiles = false;
+
   // Elide common types of templates.
   bool ElideType = true;
 
   // Print a tree when comparing templates.
   bool PrintTemplateTree = false;
 
   // Color printing is enabled.
   bool ShowColors = false;
public:
   /// Suppress all diagnostics, to silence the front end when we 
   /// know that we don't want any more diagnostics to be passed along to the
   /// client
   void setSuppressAllDiagnostics(bool Val = true) { 
     SuppressAllDiagnostics = Val; 
   }
   bool getSuppressAllDiagnostics() const { return SuppressAllDiagnostics; }
 
+  void setSuppressNonErrorsFromIncludedFiles(bool Val = true) {
+    SuppressNonErrorsFromIncludedFiles = Val;
+  }
+
   /// Set type eliding, to skip outputting same types occurring in
   /// template types.
   void setElideType(bool Val = true) { ElideType = Val; }
   bool getElideType() { return ElideType; }
  
   /// Set tree printing, to outputting the template difference in a
   /// tree format.
   void setPrintTemplateTree(bool Val = false) { PrintTemplateTree = Val; }

lib/Basic/DiagnosticIDs.cpp

DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
 
   // If -Wfatal-errors is enabled, map errors to fatal unless explicitly
   // disabled.
   if (Result == diag::Severity::Error) {
     if (State->ErrorsAsFatal && !Mapping.hasNoErrorAsFatal())
       Result = diag::Severity::Fatal;
   }
 
+  // If requested, ignore non-errors from all included files.
+  if (Diag.SuppressNonErrorsFromIncludedFiles &&
+      Result <= diag::Severity::Warning && Loc.isValid() &&
+      !Diag.getSourceManager().isInMainFile(
+          Diag.getSourceManager().getExpansionLoc(Loc))) {
+    return diag::Severity::Ignored;
+  }
+
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
 
   // If we are in a system header, we ignore it. We look at the diagnostic class
   // because we also want to ignore extensions and warnings in -Werror and
   // -pedantic-errors modes, which *map* warnings/extensions to errors.
   if (State->SuppressSystemWarnings && !ShowInSystemHeader && Loc.isValid() &&

test/Index/ignore-warnings-from-headers.h

+void f(int unusedInHeader) {}

test/Index/ignore-warnings-from-headers.cpp

+#include "ignore-warnings-from-headers.h"
+
+void g(int unusedInMainFile) {}
+
+// RUN: env CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 c-index-test -test-load-source function %s -Wunused-parameter 2>&1 | FileCheck %s
+// CHECK-NOT: warning: unused parameter 'unusedInHeader'
+// CHECK: warning: unused parameter 'unusedInMainFile'

tools/c-index-test/c-index-test.c

static unsigned getDefaultParsingOptions() {
   if (getenv("CINDEXTEST_COMPLETION_BRIEF_COMMENTS"))
     options |= CXTranslationUnit_IncludeBriefCommentsInCodeCompletion;
   if (getenv("CINDEXTEST_CREATE_PREAMBLE_ON_FIRST_PARSE"))
     options |= CXTranslationUnit_CreatePreambleOnFirstParse;
   if (getenv("CINDEXTEST_KEEP_GOING"))
     options |= CXTranslationUnit_KeepGoing;
   if (getenv("CINDEXTEST_LIMIT_SKIP_FUNCTION_BODIES_TO_PREAMBLE"))
     options |= CXTranslationUnit_LimitSkipFunctionBodiesToPreamble;
+  if (getenv("CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES"))
+    options |= CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles;
 
   return options;
 }
 
 static void ModifyPrintingPolicyAccordingToEnv(CXPrintingPolicy Policy) {
   struct Mapping {
     const char *name;
     enum CXPrintingPolicyProperty property;

tools/libclang/CIndex.cpp

clang_parseTranslationUnit_Impl(CXIndex CIdx, const char *source_filename,
 
   // Configure the diagnostics.
   IntrusiveRefCntPtr<DiagnosticsEngine>
     Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
 
   if (options & CXTranslationUnit_KeepGoing)
     Diags->setSuppressAfterFatalError(false);
 
+  if (options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+      Diags->setSuppressNonErrorsFromIncludedFiles(true);
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar<DiagnosticsEngine,
     llvm::CrashRecoveryContextReleaseRefCleanup<DiagnosticsEngine> >
     DiagCleanup(Diags.get());
 
   std::unique_ptr<std::vector<ASTUnit::RemappedFile>> RemappedFiles(
       new std::vector<ASTUnit::RemappedFile>());