[clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[salman-javed-nz]

Summary

5da7c04 introduced a regression in the NOLINT macro checking loop, replacing the
call to getImmediateExpansionRange().getBegin() with
getImmediateMacroCallerLoc(), which has similar but subtly different
behaviour.

The consequence is that NOLINTs cannot suppress diagnostics when they are
attached to a token that came from a macro argument, rather than elsewhere
in the macro expansion.

Revert to pre-patch behaviour and add test cases to cover this issue.

https://github.com/llvm/llvm-project/issues/55134

[salman-javed-nz]

This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's behaviour before and after this fix.

[paulaltin]

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It turned out that this issue wasn't actually specific to multi-line macros (see this comment), so if the test case on line 96 was passing then it must not be fully/correctly testing NOLINT for single-line macros. I guess the only way to do this would be to add more checks to the nolint.cpp file, but I realise that's not a trivial change.

[nridge]

In D126138#3529820, @paulaltin wrote:

It turned out that this issue wasn't actually specific to multi-line macros (see this comment),

Indeed, single-line vs. multi-line was a red herring. Rather, the issue was specific to cases where the diagnostic being suppressed was attached to a token that came from a macro argument, rather than elsewhere in the macro expansion.

[salman-javed-nz]

Renamed test case to better explain what it is that it's testing.

[salman-javed-nz]

The pre-merge checks are failing on the clang-format check. Not sure why it's complaining about the formatting of the lit test files - those files have never complied with the project style and have not been checked for style for as long as I remember. Has this policy changed recently?

[salman-javed-nz]

Add another test, to test the specific code sample provided in the GitHub issue (check=cppcoreguidelines-pro-type-member-init).

[salman-javed-nz]

In D126138#3529820, @paulaltin wrote:

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It turned out that this issue wasn't actually specific to multi-line macros (see this comment), so if the test case on line 96 was passing then it must not be fully/correctly testing NOLINT for single-line macros. I guess the only way to do this would be to add more checks to the nolint.cpp file, but I realise that's not a trivial change.

I added a second test case to this patch, to cover the specific check mentioned in the GitHub ticket.
This fix is check-agnostic, so I don't think we need to add even more tests than the two proposed here.

I think I've addressed what everyone has discussed in this review up to this point. Let me know what other updates I can make to this patch.

[paulaltin]

In D126138#3530172, @salman-javed-nz wrote:

This fix is check-agnostic, so I don't think we need to add even more tests than the two proposed here.

Yes, you're right; the issue itself is check-agnostic too, I just didn't realise that when I wrote the above comment (thanks to @nridge for explaining what's really going on).

[nridge]

I'm not a clang-tidy maintainer so I don't know if I have the authority to green-light this patch, but I'm fairly confident the fix is correct (it's restoring the previous usage of the SourceManager API in this loop prior to this refactor), and the tests capture the changed behaviour.

[aaron.ballman]

LGTM, though this should have a release note for the fix.

[salman-javed-nz]

Added release note.

(Also taking the opportunity to run the patch through the pre-merge build bot one last time.)

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

if (DiagLoc.isInvalid())
     return false;
   const SourceManager &SrcMgr = Diag.getSourceManager();
   while (true) {
     if (diagHasNoLint(DiagName, DiagLoc, SrcMgr, NoLintErrors, AllowIO,
                       EnableNoLintBlocks))
       return true;
     if (!DiagLoc.isMacroID())
       return false;
-    DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+    DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }
 
 // Look behind and ahead for '\n' characters. These mark the start and end of
 // this line.
 static std::pair<size_t, size_t> getLineStartAndEnd(StringRef Buffer,
                                                     size_t From) {

clang-tools-extra/docs/ReleaseNotes.rst

- Added trace code to help narrow down any checks and the relevant source code
   that result in crashes.
 
 - Clang-tidy now consideres newlines as separators of single elements in the `Checks` section in
   `.clang-tidy` configuration files. Where previously a comma had to be used to distinguish elements in
   this list from each other, newline characters now also work as separators in the parsed YAML. That
   means it is advised to use YAML's block style initiated by the pipe character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
+- Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
+  from suppressing diagnostics associated with macro arguments. This fixes
+  `Issue 55134 <https://github.com/llvm/llvm-project/issues/55134>`_.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
 
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 - New :doc:`bugprone-unchecked-optional-access

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

 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
   int In;
   A1(In, Out);
 }
 // CHECK-MESSAGES-NOT: trigger_warning.h:{{.*}} warning
 // CHECK-MESSAGES-NOT: :[[@LINE-4]]:{{.*}} note

 MACRO(E) // NOLINT
 
 #define MACRO_NOARG class F { F(int i); };
 MACRO_NOARG // NOLINT
 
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)                \
+  class X {                                             \
+    X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)                          \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+    int a = 0;                                                    \
+    int b;                                                        \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
 class D1 { D1(int x); }; // NOLINT(google*)
 class D2 { D2(int x); }; // NOLINT(*explicit-constructor)
 class D3 { D3(int x); }; // NOLINT(*explicit*)
 class D4 { D4(int x); }; // NOLINT(-explicit-constructor)
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 class D5 { D5(int x); }; // NOLINT(google*,-google*)
 class D6 { D6(int x); }; // NOLINT(*,-google*)
 
 int array1[10];
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays]
 
 int array2[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays)
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
 
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)