[clang-tidy] NOLINT not working in macros in clang-tidy-14 (regression)

[paulaltin]

Clang-tidy version 13 generates no warnings for this code:

#define CREATE_STRUCT(name)    \
struct name {	/* NOLINT */   \
    int a = 0;                 \
    int b;                     \
}; 				

CREATE_STRUCT(X)
CREATE_STRUCT(Y)
CREATE_STRUCT(Z)

when invoked as:

clang-tidy-13 test.cpp -checks=cppcoreguidelines-pro-type-member-init

Clang-tidy version 14 generates warnings on lines 7-9 when invoked the same way.

[paulaltin]

It appears from the summary of this patch that the intention is for "clang-tidy [to] look at the line of macro invocation and every line of macro definition for a NOLINT comment."

https://reviews.llvm.org/D24845

[paulaltin]

If anyone knows of a workaround for this (short of marking every use of the macro with NOLINT), I would be very grateful. Adding NOLINT to every line that uses the macro isn't possible in my case, but I can modify the macro definition.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[firewave]

I encountered a similar issue with NOLINTNEXTLINE quite a while ago - see #46511.

[paulaltin]

@firewave – interesting, thanks for the info! Did you ever find a solution/workaround?

It looks like this issue is also specific to this check, and doesn't actually have anything to do with the macro having multiple lines – if I put the entire macro on one line the problem still occurs:

// NOLINTNEXTLINE
#define CREATE_STRUCT_ONELINE(name) struct name { int a = 0; int b; }; // NOLINT

Either of the NOLINTNEXTLINE and NOLINT comments were enough to suppress the warning in version 13, but neither works in version 14.

Other warnings can be suppressed as expected, even in multi-line macros, e.g. the NOLINT comment here works to suppress the readability-use-anyofallof warning in both versions:

#include <vector>

#define CREATE_FUNCTION(name)           \
bool name() {                           \
    std::vector<int> v;                 \
    for (auto el : v)  /* NOLINT */     \
        if (el != 0)                    \
            return false;               \
    return true;                        \
}

CREATE_FUNCTION(F)

[Skylion007]

I am encountering similar issues with google-explicit-constructor when trying to update clang-tidy to 14 for pybind11: https://github.com/pybind/pybind11/blob/2e331308d38a521c087e7fc0cfee227cd29f3f71/include/pybind11/pytypes.h#L1029 . NOLINTS are ignored. Interestingly, the 'bugprone-macro-*' NOLINT comments seem to work fine. Even NOLINTBEGIN NOLINTEND do not work when used in the multiline macro.

[Skylion007]

@paulaltin This implies the issue is not specific to this check, feel free to update the title.

[paulaltin]

Checks known to have regressed in version 14:

• cppcoreguidelines-pro-type-member-init
• google-explicit-constructor

Checks known to have regressed in version 13:

• google-explicit-constructor

Checks that were not working before version 13/14:

• modernize-avoid-c-arrays

[Skylion007]

@paulaltin I have confirmed that the 'google-explicit ctor' seems to have actually regressed in version 13 as well (at least the latest minor release of it).

[paulaltin]

Thanks @Skylion007 – I've updated the comment above.

[Skylion007]

I don't have that much experience in the clang-tidy code base, but trying to look around the codebase, I am bit suspicious of this commit: b9d9968, but I don't see anything obvious that changed in these checks that could have broken them between 12-13 and 13-14.

I also noticed a lot of the other checks that are working have preprocessor callbacks.

I am really not familiar with the codebase though so I am bit unsure where the regression is coming. Any thoughts @EugeneZelenko ?

[paulaltin]

Looks like that commit was authored by @HighCommander4, perhaps they can help?

[HighCommander4]

Stepping through the code suggests that this loop iterates twice, and checks the line CHECK_STRUCT(X) for a NOLINT comment both times, when the intention is clearly to check the CHECK_STRUCT(X) line in one iteration, and the struct Name { /* NOLINT */ line on the other.

[HighCommander4]

Looks to be a regression from 5da7c04, cc @salman-javed-nz

[HighCommander4]

The regression is caused by a change made in that patch to how the location to be checked is updated from one iteration of the loop to the next.

Before the patch:

Loc = SM.getImmediateExpansionRange(Loc).getBegin();

After the patch:

DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);

These two functions have slightly different behaviour, specifically in the case where getImmediateMacroCallerLoc() takes this early exit, which happens in this code example.

I'm not sure why the patch made this change; it seems unrelated to the purpose of the patch.

[salman-javed-nz]

Thanks for the discovery. I'll see what I can do to fix this.

From a brief initial look, it looks like @HighCommander4 is right on the money with the line of code he has isolated this issue to.

There's a number of lit test cases regarding macro expansion so it's interesting that they didn't pick this up. Will have to update the tests to cover this regression. Please continue sharing any other code samples that exhibit this issue. They will help me make sure that the fix works for everyone.

[paulaltin]

Thanks @HighCommander4 and @salman-javed-nz, really appreciate your quick responses and attention to this!

[Skylion007]

@paulaltin @salman-javed-nz Did we miss the opportunity for this to be backported in to the 14.x branch?