[clang-tidy] Implement bugprone-incorrect-enable-if

[ccotter]

Summary

Detects incorrect usages of std::enable_if that don't name the
nested 'type' type.

[ccotter]

• Edited wrong file

[PiotrZSL]

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp

58

Result got source manager....
https://clang.llvm.org/doxygen/structclang_1_1ast__matchers_1_1MatchFinder_1_1MatchResult.html

62–63

do we still need typename in C++20 in this context ?
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0634r3.html

68

since C++14 we should recommend using enable_if_t

clang-tools-extra/docs/ReleaseNotes.rst

129

clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst

22

shoudnlt be enable_if ?

35

this is valid usage, uses enable_if_t

38

this may not compile, anyway, what if "type" got "type" ?:P

45

should be suffucient

48

maybe "Consider"

[ccotter]

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp

68

I didn't add replacement logic for C++14 or C++20 since there are separate tools for those, and though it'd be cleaner to have independent set of tools (e.g., first run bugprone-incorrect-enable-if, then run modernize-type-traits).

[carlosgalvezp]

Do you have plans to also detect the bugprone scenario described in the Notes here?

https://en.cppreference.com/w/cpp/types/enable_if

No need to have it in this patch, but would be good to keep it in mind if we want to add it in the future (preferably) or create a separate check.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst

7

Please synchronize with statement in Release Notes after Piotr's comment addressed.

[ccotter]

• Fix docs, handle C++20 simplification

[ccotter]

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp

62–63

Oh, I did not know that. Thanks!

[ccotter]

> Do you have plans to also detect the bugprone scenario described in the Notes here?

I didn't have plans in this review, or in the immediate future after. I did name this check broadly as "bugprone-incorrect-enable-if," so I imagine this other scenario could go into this new check.

[PiotrZSL]

LGTM (+-)

[PiotrZSL]

I'm pushing this, if you want to change anything else, just please open new review.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "EmptyCatchCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncDecInConditionsCheck.h"
+#include "IncorrectEnableIfCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
         "bugprone-implicit-widening-of-multiplication-result");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
+    CheckFactories.registerCheck<IncorrectEnableIfCheck>(
+        "bugprone-incorrect-enable-if");
     CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(
         "bugprone-switch-missing-default-case");
     CheckFactories.registerCheck<IncDecInConditionsCheck>(
         "bugprone-inc-dec-in-conditions");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");
     CheckFactories.registerCheck<IntegerDivisionCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   EasilySwappableParametersCheck.cpp
   EmptyCatchCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
+  IncorrectEnableIfCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
   IncDecInConditionsCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.h

+//===--- IncorrectEnableIfCheck.h - clang-tidy ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCORRECTENABLEIFCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCORRECTENABLEIFCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects incorrect usages of ``std::enable_if`` that don't name the nested
+/// ``type`` type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/incorrect-enable-if.html
+class IncorrectEnableIfCheck : public ClangTidyCheck {
+public:
+  IncorrectEnableIfCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCORRECTENABLEIFCHECK_H

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp

+//===--- IncorrectEnableIfCheck.cpp - clang-tidy --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "IncorrectEnableIfCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER_P(TemplateTypeParmDecl, hasUnnamedDefaultArgument,
+              ast_matchers::internal::Matcher<TypeLoc>, InnerMatcher) {
+  if (Node.getIdentifier() != nullptr || !Node.hasDefaultArgument() ||
+      Node.getDefaultArgumentInfo() == nullptr)
+    return false;
+
+  TypeLoc DefaultArgTypeLoc = Node.getDefaultArgumentInfo()->getTypeLoc();
+  return InnerMatcher.matches(DefaultArgTypeLoc, Finder, Builder);
+}
+
+} // namespace
+
+void IncorrectEnableIfCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      templateTypeParmDecl(
+          hasUnnamedDefaultArgument(
+              elaboratedTypeLoc(
+                  hasNamedTypeLoc(templateSpecializationTypeLoc(
+                                      loc(qualType(hasDeclaration(namedDecl(
+                                          hasName("::std::enable_if"))))))
+                                      .bind("enable_if_specialization")))
+                  .bind("elaborated")))
+          .bind("enable_if"),
+      this);
+}
+
+void IncorrectEnableIfCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *EnableIf =
+      Result.Nodes.getNodeAs<TemplateTypeParmDecl>("enable_if");
+  const auto *ElaboratedLoc =
+      Result.Nodes.getNodeAs<ElaboratedTypeLoc>("elaborated");
+  const auto *EnableIfSpecializationLoc =
+      Result.Nodes.getNodeAs<TemplateSpecializationTypeLoc>(
+          "enable_if_specialization");
+
+  if (!EnableIf || !ElaboratedLoc || !EnableIfSpecializationLoc)
+    return;
+
+  const SourceManager &SM = *Result.SourceManager;
+  SourceLocation RAngleLoc =
+      SM.getExpansionLoc(EnableIfSpecializationLoc->getRAngleLoc());
+
+  auto Diag = diag(EnableIf->getBeginLoc(),
+                   "incorrect std::enable_if usage detected; use "
+                   "'typename std::enable_if<...>::type'");
+  if (!getLangOpts().CPlusPlus20) {
+    Diag << FixItHint::CreateInsertion(ElaboratedLoc->getBeginLoc(),
+                                       "typename ");
+  }
+  Diag << FixItHint::CreateInsertion(RAngleLoc.getLocWithOffset(1), "::type");
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New :doc:`bugprone-inc-dec-in-conditions
   <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
 
   Detects when a variable is both incremented/decremented and referenced inside
   a complex condition and suggests moving them outside to avoid ambiguity in
   the variable's value.
 
+- New :doc:`bugprone-incorrect-enable-if
+  <clang-tidy/checks/bugprone/incorrect-enable-if>` check.
+
+  Detects incorrect usages of ``std::enable_if`` that don't name the nested 
+  ``type`` type.
+
 - New :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check.
 
   Detects implicit conversions between pointers of different levels of
   indirection.
 
 - New :doc:`bugprone-optional-value-conversion
   <clang-tidy/checks/bugprone/optional-value-conversion>` check.

clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst

+.. title:: clang-tidy - bugprone-incorrect-enable-if
+
+bugprone-incorrect-enable-if
+============================
+
+Detects incorrect usages of ``std::enable_if`` that don't name the nested
+``type`` type.
+
+In C++11 introduced ``std::enable_if`` as a convenient way to leverage SFINAE.
+One form of using ``std::enable_if`` is to declare an unnamed template type
+parameter with a default type equal to
+``typename std::enable_if<condition>::type``. If the author forgets to name
+the nested type ``type``, then the code will always consider the candidate
+template even if the condition is not met.
+
+Below are some examples of code using ``std::enable_if`` correctly and
+incorrect examples that this check flags.
+
+.. code-block:: c++
+
+  template <typename T, typename = typename std::enable_if<T::some_trait>::type>
+  void valid_usage() { ... }
+
+  template <typename T, typename = std::enable_if_t<T::some_trait>>
+  void valid_usage_with_trait_helpers() { ... }
+
+  // The below code is not a correct application of SFINAE. Even if
+  // T::some_trait is not true, the function will still be considered in the
+  // set of function candidates. It can either incorrectly select the function
+  // when it should not be a candidates, and/or lead to hard compile errors
+  // if the body of the template does not compile if the condition is not
+  // satisfied.
+  template <typename T, typename = std::enable_if<T::some_trait>>
+  void invalid_usage() { ... }
+
+  // The tool suggests the following replacement for 'invalid_usage':
+  template <typename T, typename = typename std::enable_if<T::some_trait>::type>
+  void fixed_invalid_usage() { ... }
+
+C++14 introduced the trait helper ``std::enable_if_t`` which reduces the
+likelihood of this error. C++20 introduces constraints, which generally
+supersede the use of ``std::enable_if``. See
+:doc:`modernize-type-traits <../modernize/type-traits>` for another tool
+that will replace ``std::enable_if`` with
+``std::enable_if_t``, and see
+:doc:`modernize-use-constraints <../modernize/use-constraints>` for another
+tool that replaces ``std::enable_if`` with C++20 constraints. Consider these
+newer mechanisms where possible.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    :doc:`bugprone-empty-catch <bugprone/empty-catch>`,
    :doc:`bugprone-exception-escape <bugprone/exception-escape>`,
    :doc:`bugprone-fold-init-type <bugprone/fold-init-type>`,
    :doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`,
    :doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`,
    :doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes"
    :doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
    :doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
+   :doc:`bugprone-incorrect-enable-if <bugprone/incorrect-enable-if>`, "Yes"
    :doc:`bugprone-incorrect-roundings <bugprone/incorrect-roundings>`,
    :doc:`bugprone-infinite-loop <bugprone/infinite-loop>`,
    :doc:`bugprone-integer-division <bugprone/integer-division>`,
    :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`,
    :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes"
    :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`,
    :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes"
    :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone/incorrect-enable-if.cpp

+// RUN: %check_clang_tidy -std=c++11 %s bugprone-incorrect-enable-if %t
+// RUN: %check_clang_tidy -check-suffix=CXX20 -std=c++20 %s bugprone-incorrect-enable-if %t
+
+// NOLINTBEGIN
+namespace std {
+template <bool B, class T = void> struct enable_if { };
+
+template <class T> struct enable_if<true, T> { typedef T type; };
+
+template <bool B, class T = void>
+using enable_if_t = typename enable_if<B, T>::type;
+
+} // namespace std
+// NOLINTEND
+
+template <typename T, typename = typename std::enable_if<T::some_value>::type>
+void valid_function1() {}
+
+template <typename T, typename std::enable_if<T::some_value>::type = nullptr>
+void valid_function2() {}
+
+template <typename T, typename std::enable_if<T::some_value>::type = nullptr>
+struct ValidClass1 {};
+
+template <typename T, typename = std::enable_if<T::some_value>>
+void invalid() {}
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename = typename std::enable_if<T::some_value>::type>
+// CHECK-FIXES-CXX20: template <typename T, typename = std::enable_if<T::some_value>::type>
+
+template <typename T, typename = std::enable_if<T::some_value> >
+void invalid_extra_whitespace() {}
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename = typename std::enable_if<T::some_value>::type >
+// CHECK-FIXES-CXX20: template <typename T, typename = std::enable_if<T::some_value>::type >
+
+template <typename T, typename=std::enable_if<T::some_value>>
+void invalid_extra_no_whitespace() {}
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename=typename std::enable_if<T::some_value>::type>
+// CHECK-FIXES-CXX20: template <typename T, typename=std::enable_if<T::some_value>::type>
+
+template <typename T, typename /*comment1*/ = /*comment2*/std::enable_if<T::some_value>/*comment3*/>
+void invalid_extra_comment() {}
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename /*comment1*/ = /*comment2*/typename std::enable_if<T::some_value>::type/*comment3*/>
+
+template <typename T, typename = std::enable_if<T::some_value>, typename = std::enable_if<T::other_value>>
+void invalid_multiple() {}
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-MESSAGES: [[@LINE-3]]:65: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename = typename std::enable_if<T::some_value>::type, typename = typename std::enable_if<T::other_value>::type>
+// CHECK-FIXES-CXX20: template <typename T, typename = std::enable_if<T::some_value>::type, typename = std::enable_if<T::other_value>::type>
+
+template <typename T, typename = std::enable_if<T::some_value>>
+struct InvalidClass {};
+// CHECK-MESSAGES: [[@LINE-2]]:23: warning: incorrect std::enable_if usage detected; use 'typename std::enable_if<...>::type' [bugprone-incorrect-enable-if]
+// CHECK-FIXES: template <typename T, typename = typename std::enable_if<T::some_value>::type>
+// CHECK-FIXES-CXX20: template <typename T, typename = std::enable_if<T::some_value>::type>