clang-tidy-19: modernize-type-traits false positive when modern type traits are already used

[hvdijk]

This is heavily reduced from real code:

#include <utility>
template <class T>
struct S { template <class U, class = std::enable_if_t<!std::is_same_v<S, std::remove_reference_t<U>>>> S(U); };
S<int> s(std::move(1));

Running clang-tidy-19 --checks=modernize-type-traits test.cpp produces:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "test.cpp"
No compilation database found in /home/harald or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
116 warnings generated.
/home/harald/test.cpp:3:99: warning: use c++14 style type templates [modernize-type-traits]
    3 | struct S { template <class U, class = std::enable_if_t<!std::is_same_v<S, std::remove_reference_t<U>>>> S(U); };
      |                                                                                                   ^
      |                                                                                                   _t
Suppressed 114 warnings (114 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

It appears that clang-tidy-19 is getting confused by std::remove_reference_t's definition being in terms of std::remove_reference, ultimately the same issue as #67896. However, that issue should not be hit here because the location of std::remove_reference should be the system headers, and that should cause the warning to be suppressed. Because it is detected at the wrong location, the warning does not get suppressed.

No warning is emitted for this on clang-tidy-18.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Harald van Dijk (hvdijk)

This is heavily reduced from real code: ```c++ #include <utility> template <class T> struct S { template <class U, class = std::enable_if_t<!std::is_same_v<S, std::remove_reference_t<U>>>> S(U); }; S<int> s(std::move(1)); ``` Running `clang-tidy-19 --checks=modernize-type-traits test.cpp` produces: ``` Error while trying to load a compilation database: Could not auto-detect compilation database for file "test.cpp" No compilation database found in /home/harald or any parent directory fixed-compilation-database: Error while opening fixed database: No such file or directory json-compilation-database: Error while opening JSON database: No such file or directory Running without flags. 116 warnings generated. /home/harald/test.cpp:3:99: warning: use c++14 style type templates [modernize-type-traits] 3 | struct S { template <class U, class = std::enable_if_t<!std::is_same_v<S, std::remove_reference_t<U>>>> S(U); }; | ^ | _t Suppressed 114 warnings (114 in non-user code). Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well. ``` It appears that `clang-tidy-19` is getting confused by `std::remove_reference_t`'s definition being in terms of `std::remove_reference`, ultimately the same issue as #67896. However, that issue should not be hit here because the location of `std::remove_reference` should be the system headers, and that should cause the warning to be suppressed. Because it is detected at the wrong location, the warning does not get suppressed.

No warning is emitted for this on clang-tidy-18.

[hvdijk]

This started with 1202837 (#91393), cc @sdkrystian, is this fundamentally a problem that already existed but now starts showing up in some different cases, or is this a limitation of that PR?

[hvdijk]

Standalone test that does not depend on system headers:

#1 "utility" 3
namespace std {
template <bool, typename T = void>
struct enable_if;
template <typename T>
struct enable_if<true, T> { using type = T; };
template <bool B, typename T = void>
using enable_if_t = typename enable_if<B, T>::type;
template <typename T, typename U>
struct is_same { static constexpr bool value = false; };
template <typename T>
struct is_same<T, T> { static constexpr bool value = true; };
template <typename T, typename U>
constexpr bool is_same_v = is_same<T, U>::value;
template <typename T>
struct remove_reference { using type = T; };
template <typename T>
struct remove_reference<T &> { using type = T; };
template <typename T>
struct remove_reference<T &&> { using type = T; };
template <typename T>
using remove_reference_t = typename remove_reference<T>::type;
}
#1 "test.cpp"
template <class T>
struct S { template <class U, class = std::enable_if_t<!std::is_same_v<U, S>>> S(U); };
S<int> s(std::remove_reference<int>::type(0));

This results in:

/home/harald/test.cpp:26:72: warning: use c++14 style type templates [modernize-type-traits]
   26 | struct S { template <class U, class = std::enable_if_t<!std::is_same_v<U, S>>> S(U); };
      |                                                                        ^
      |                                                                        _t
/home/harald/test.cpp:27:10: warning: use c++14 style type templates [modernize-type-traits]
   27 | S<int> s(std::remove_reference<int>::type(0));
      |          ^                         ~~~~~~
      |                               _t

Here, the second warning is correct, but the first is not.

It does make it slightly clearer what is going on: the constructor is instantiated with U = std::remove_reference<int>::type rather than with U = int, which I guess makes sense. It's just that the location is wrong and results in suggestions that cannot work.

[hvdijk]

The actual change in #91393 that results in different behaviour is that for

VarTemplateSpecializationDecl 0xabccdb439fa8 <utility:12:1, line:13:43> col:16 referenced is_same_v 'const bool' implicit_instantiation constexpr cinit
|-value: Int 0
|-TemplateArgument type 'int'
| `-BuiltinType 0xabccdb3068a0 'int'
|-TemplateArgument type 'S<int>'
| `-RecordType 0xabccdb437e70 'S<int>'
|   `-ClassTemplateSpecialization 0xabccdb437d90 'S'
`-ImplicitCastExpr 0xabccdb43af48 <col:28, col:43> 'bool' <LValueToRValue>
  `-DeclRefExpr 0xabccdb43af18 <col:28, col:43> 'const bool' lvalue Var 0xabccdb43ae00 'value' 'const bool' non_odr_use_constant
    `-NestedNameSpecifier TypeSpec 'is_same<int, S<int>>':'std::is_same<int, S<int>>'

we would hit

#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)                    \
  DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {                \
    /* For implicit instantiations ("set<int> x;"), we don't want to           \
       recurse at all, since the instatiated template isn't written in         \
       the source code anywhere.  (Note the instatiated *type* --              \
       set<int> -- is written, and will still get a callback of                \
       TemplateSpecializationType).  For explicit instantiations               \
       ("template set<int>;"), we do need a callback, since this               \
       is the only callback that's made for this instantiation.                \
       We use getTypeAsWritten() to distinguish. */                            \
    if (TypeSourceInfo *TSI = D->getTypeAsWritten())                           \
      TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));                              \

Here, D->getTypeAsWritten() would be nullptr.

This was updated to

#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)                    \
  DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {                \
    /* For implicit instantiations ("set<int> x;"), we don't want to           \
       recurse at all, since the instatiated template isn't written in         \
       the source code anywhere.  (Note the instatiated *type* --              \
       set<int> -- is written, and will still get a callback of                \
       TemplateSpecializationType).  For explicit instantiations               \
       ("template set<int>;"), we do need a callback, since this               \
       is the only callback that's made for this instantiation.                \
       We use getTemplateArgsAsWritten() to distinguish. */                    \
    if (const auto *ArgsWritten = D->getTemplateArgsAsWritten()) {             \
      /* The args that remains unspecialized. */                               \
      TRY_TO(TraverseTemplateArgumentLocsHelper(                               \
          ArgsWritten->getTemplateArgs(), ArgsWritten->NumTemplateArgs));      \
    }                                                                          \

but D->getTemplateArgsAsWritten() does return something, so we now process the template arguments in a context where we previously skipped them.

Is the expectation that for any implicit instantiation, D->getTemplateArgsAsWritten() returns nullptr and that went wrong somewhere, or is this not the right check for implicit instantiations?