[clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[PiotrZSL]

Summary

Fix issue preventing suppression of compiler warnings with
-Wno-<warning> under C++20 and above. Add call to
ProcessWarningOptions and propagate DiagnosticOpts more properly.

Fixes: #56709, #61969

[carlosgalvezp]

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

82

A bit unclear to me why we should add this line here, grepping for this function in the repo I only find hits in the clang folder. How come it's not needed in other places?

[PiotrZSL]

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

82

We create here new Preprocessor (line 96) and new DiagEngine (line 74), when C++20/Modules are enabled this class is register as an second Preprocessor and both are (+-) executed.
Unfortunately when we pass -Wno-macro-redefined it's pass only to original DiagEngine, and we run into situation when warning is suppressed by first DiagEngine, but not by second that is used by second Preprocessor.

Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as it's does not apply settings, only calling this function apply them. (somehow).
This is gray area for me.

More about problem here: https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628

[carlosgalvezp]

A thought came to mind - since we are doing workarounds anyway, would it be easier to ask people to simply add -clang-diagnostic* to the Checks in their config file? It's fair to assume they will get those warnings when compiling the code. I feel the more workarounds we add in the code the harder it will be to clean it up later :)

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

75

When downloading your patch, this seems to not be needed to make the tests pass, should it be removed?

82

Thanks for the explanation! I'm not sure what the best way forward is. Would it make sense to add some TODO or FIXME comment to further investigate in the future if we want that line of code ?

[PiotrZSL]

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

75

No idea, it seem reasonable.

82

Yes, FIXME could be added, but ExpandModularHeadersPPCallbacks got also other issues, for example with TargetTriple propagation and __has_builtin, looks like all those got same source, simply we shoudn't create separate Preprocessor.

[carlosgalvezp]

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

75

Do you mean it seems reasonable to keep it, or reasonable to remove it?

82

Agreed, the whole thing looks fishy.

[PiotrZSL]

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

75

reasonable to keep it, we want both DiagEngines to have same settings

[carlosgalvezp]

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

75

Reason I ask is that it seems the majority of DiagnosticOptions are initialized with default ctor:

$ git grep -E " DiagnosticOptions\(\w" | wc -l
3
$ git grep -E " DiagnosticOptions\(\)" | wc -l
74

we want both DiagEngines to have same settings

Do you know where "the other" DiagEngine is initialized? The one I can find is initialized without the compiler opts.

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544

Since the added code does not have any impact on the result of the test, I'd argue that either the test is insufficient or the added code is dead code that should be removed.

[PiotrZSL]

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

75

sure, maybe ProcessWarningOptions will override it anyway, I didnt check more deeply.

[carlosgalvezp]

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

75

In that case, could you please revert the change to this line, since it's not needed?

[PiotrZSL]

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

75

Yes, but later today.

[carlosgalvezp]

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

75

Ok! I will approve the patch now so you don't need to wait for me to land it before branching tomorrow

[PiotrZSL]

Review comments fixes

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

 ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
     CompilerInstance *CI,
     IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS)
     : Recorder(std::make_unique<FileRecorder>()), Compiler(*CI),
       InMemoryFs(new llvm::vfs::InMemoryFileSystem),
       Sources(Compiler.getSourceManager()),
       // Forward the new diagnostics to the original DiagnosticConsumer.
       Diags(new DiagnosticIDs, new DiagnosticOptions,
             new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
       LangOpts(Compiler.getLangOpts()) {
   // Add a FileSystem containing the extra files needed in place of modular
   // headers.
   OverlayFS->pushOverlay(InMemoryFs);
 
   Diags.setSourceManager(&Sources);
+  // FIXME: Investigate whatever is there better way to initialize DiagEngine
+  // or whatever DiagEngine can be shared by multiple preprocessors
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
 
   LangOpts.Modules = false;
 
   auto HSO = std::make_shared<HeaderSearchOptions>();
   *HSO = Compiler.getHeaderSearchOpts();
 
   HeaderInfo = std::make_unique<HeaderSearch>(HSO, Sources, Diags, LangOpts,
                                                &Compiler.getTarget());

clang-tools-extra/docs/ReleaseNotes.rst

 - Support specifying `SystemHeaders` in the `.clang-tidy` configuration file,
   with the same functionality as the command-line option `--system-headers`.
 
 - `WarningsAsErrors` (`--warnings-as-errors=`) no longer promotes unlisted
   warnings to errors. Only the warnings listed in `Checks` (`--checks=`) will
   be promoted to errors. For custom error promotion, use `-Werror=<warning>`
   on the compiler command-line, irrespective of `Checks` (`--checks=`) settings.
 
+- Fixed an issue where compiler warnings couldn't be suppressed using
+  `-Wno-<warning>` under C++20 and above.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-empty-catch
   <clang-tidy/checks/bugprone/empty-catch>` check.
 
   Detects and suggests addressing issues with empty catch statements.
 

clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp

 // RUN: echo '[{"directory": "%/T/diagnostics/","command": "clang++ -fan-option-from-compilation-database -c %/T/diagnostics/input.cpp", "file": "%/T/diagnostics/input.cpp"}]' > %T/diagnostics/compile_commands.json
 // RUN: cat %s > %T/diagnostics/input.cpp
 // RUN: not clang-tidy -checks='-*,modernize-use-override' %T/diagnostics/nonexistent.cpp -- 2>&1 | FileCheck -check-prefix=CHECK1 -implicit-check-not='{{warning:|error:}}' %s
 // RUN: not clang-tidy -checks='-*,clang-diagnostic-*,google-explicit-constructor' %T/diagnostics/input.cpp -- -fan-unknown-option 2>&1 | FileCheck -check-prefix=CHECK2 -implicit-check-not='{{warning:|error:}}' %s
 // RUN: not clang-tidy -checks='-*,google-explicit-constructor,clang-diagnostic-literal-conversion' %T/diagnostics/input.cpp -- -fan-unknown-option 2>&1 | FileCheck -check-prefix=CHECK3 -implicit-check-not='{{warning:|error:}}' %s
 // RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck -check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
 // RUN: not clang-tidy -checks='-*,clang-diagnostic-*,google-explicit-constructor' %T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5 -implicit-check-not='{{warning:|error:}}' %s
 // RUN: not clang-tidy -checks='-*,modernize-use-override' %T/diagnostics/input.cpp -- -DCOMPILATION_ERROR 2>&1 | FileCheck -check-prefix=CHECK6 -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %s -- -DMACRO_FROM_COMMAND_LINE -std=c++20 | FileCheck -check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-macro-redefined,clang-diagnostic-literal-conversion' %s -- -DMACRO_FROM_COMMAND_LINE -std=c++20 -Wno-macro-redefined | FileCheck --check-prefix=CHECK7 -implicit-check-not='{{warning:|error:}}' %s
 
 // CHECK1: error: no input files [clang-diagnostic-error]
 // CHECK1: error: no such file or directory: '{{.*}}nonexistent.cpp' [clang-diagnostic-error]
 // CHECK1: error: unable to handle compilation{{.*}} [clang-diagnostic-error]
 // CHECK2: error: unknown argument: '-fan-unknown-option' [clang-diagnostic-error]
 // CHECK3: error: unknown argument: '-fan-unknown-option' [clang-diagnostic-error]
 // CHECK5: error: unknown argument: '-fan-option-from-compilation-database' [clang-diagnostic-error]
 
+// CHECK7: :[[@LINE+4]]:9: warning: implicit conversion from 'double' to 'int' changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
 // CHECK2: :[[@LINE+3]]:9: warning: implicit conversion from 'double' to 'int' changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
 // CHECK3: :[[@LINE+2]]:9: warning: implicit conversion from 'double' to 'int' changes value
 // CHECK5: :[[@LINE+1]]:9: warning: implicit conversion from 'double' to 'int' changes value
 int a = 1.5;
 
 // CHECK2: :[[@LINE+3]]:11: warning: single-argument constructors must be marked explicit
 // CHECK3: :[[@LINE+2]]:11: warning: single-argument constructors must be marked explicit
 // CHECK5: :[[@LINE+1]]:11: warning: single-argument constructors must be marked explicit
 class A { A(int) {} };
 
 #define MACRO_FROM_COMMAND_LINE
 // CHECK4: :[[@LINE-1]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined
+// CHECK7-NOT: :[[@LINE-2]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined
 
 #ifdef COMPILATION_ERROR
 void f(int a) {
   &(a + 1);
   // CHECK6: :[[@LINE-1]]:3: error: cannot take the address of an rvalue of type 'int' [clang-diagnostic-error]
 }
 #endif