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

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

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D157239

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

@@ -28,6 +28,7 @@
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncDecInConditionsCheck.h"
+#include "IncorrectEnableIfCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
@@ -122,6 +123,8 @@ public:
         "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>(

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

@@ -22,6 +22,7 @@ add_clang_library(clangTidyBugproneModule
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
+  IncorrectEnableIfCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
   IncDecInConditionsCheck.cpp
   IncorrectRoundingsCheck.cpp

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

@@ -0,0 +1,71 @@
+//===--- 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/clang-tidy/bugprone/IncorrectEnableIfCheck.h

@@ -0,0 +1,34 @@
+//===--- 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/docs/ReleaseNotes.rst

@@ -123,6 +123,12 @@ New checks
   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.
 

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

@@ -0,0 +1,48 @@
+.. 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

@@ -94,6 +94,7 @@ Clang-Tidy Checks
    :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>`,

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

@@ -0,0 +1,59 @@
+// 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>