[clang-tidy] Limit modernize-use-constraints to standard enable_if #155237
Conversation
This commit ensures that the modernize-use-constraints check ignores templates that happen to be named `enable_if` or `enable_if_t` if they are not declared in the namespace `std`. This patch motivated by a crash observed during the analysis of the open source library https://github.com/Neargye/magic_enum/ which declares a template `detail::enable_if_t` with semantics that significantly differ from the standard one. (I was unable to reproduce that crash with the standard `enable_if_t`.) However, there are other projects that use non-standard `enable_if`: even `boost` declares a `boost::enable_if` which excepts different parameters than `std::enable_if`.
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Donát Nagy (NagyDonat) ChangesThis commit ensures that the modernize-use-constraints check ignores templates that happen to be named This patch motivated by a crash observed during the analysis of the open source library https://github.com/Neargye/magic_enum/ which declares a template However, there are other projects that use non-standard Full diff: https://github.com/llvm/llvm-project/pull/155237.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index 07274d0376207..1818e30971c21 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -77,7 +77,7 @@ matchEnableIfSpecializationImplTypename(TypeLoc TheType) {
const TemplateDecl *TD =
Specialization->getTemplateName().getAsTemplateDecl();
- if (!TD || TD->getName() != "enable_if")
+ if (!TD || TD->getName() != "enable_if" || !TD->isInStdNamespace())
return std::nullopt;
int NumArgs = SpecializationLoc.getNumArgs();
@@ -101,7 +101,7 @@ matchEnableIfSpecializationImplTrait(TypeLoc TheType) {
const TemplateDecl *TD =
Specialization->getTemplateName().getAsTemplateDecl();
- if (!TD || TD->getName() != "enable_if_t")
+ if (!TD || TD->getName() != "enable_if_t" || !TD->isInStdNamespace())
return std::nullopt;
if (!Specialization->isTypeAlias())
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
index 3bcd5cd74024e..8289efe338e3f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
@@ -756,3 +756,33 @@ abs(const number<T, ExpressionTemplates> &v) {
}
}
+
+// NOLINTBEGIN
+namespace custom {
+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 custom
+// NOLINTEND
+
+namespace use_custom {
+// We cannot assume anything about the behavior of templates that happen to be
+// named `enable_if` or `enable_if_t` if they are not declared in the namespace
+// `std`. (E.g. the first template parameter of `boost::enable_if` is a class
+// and not a boolean and `boost::enable_if<Cond, T>` is equivalent to
+// `std::enable_if<Cond::value, T>`.)
+
+template <typename T>
+typename custom::enable_if<T::some_value, Obj>::type custom_basic() {
+ return Obj{};
+}
+
+template <typename T>
+custom::enable_if_t<T::some_value, Obj> custom_basic_t() {
+ return Obj{};
+}
+}
|
| // NOLINTBEGIN | ||
| namespace custom { | ||
| 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 custom | ||
| // NOLINTEND |
There was a problem hiding this comment.
Except for the name of the namespace, this block is identical to the namespace std block at the beginning of this test file. I don't understand the purpose of the NOLINTBEGIN/NOLINTEND markers, so I just copied them blindly. Feel free to suggest removing them if you know that they are superfluous (in that case I'll probably remove them from the beginning of the file as well).
|
Should this be already fixed by #152938? Initially the check crushed because it expected to have first parameter as bool, now we explicitly check for it. There are also tests with |
|
Oh, I see, that commit was merged on Saturday, while I started to develop this commit (IIRC) on Thursday. Compared to that change, the only difference provided by my trivial patch is that it silences fixits for What do you think about this? Should I just close this ticket or is there any merit in restricting this check to only checking the standard |
|
AFAIK, the check now will not crash if
So we essentially eliminated all "bad" cases from the check. So I'm a little in favor of keeping as is, but it's not strong opinion since it's a very niche case (considering the bug is already fixed) Could you confirm that bug in https://github.com/Neargye/magic_enum/ is fixed in trunk? |
Ok, then I'm closing this ticket. I agree that this is a very niche question, and I don't see any remaining chances for crashes.
The crash was caused by an |
This commit ensures that the modernize-use-constraints check ignores templates that happen to be named
enable_iforenable_if_tif they are not declared in the namespacestd.This patch motivated by a crash observed during the analysis of the open source library https://github.com/Neargye/magic_enum/ which declares a template
detail::enable_if_twith semantics that significantly differ from the standard one. (I was unable to reproduce that crash with the standardenable_if_t.)However, there are other projects that use non-standard
enable_if: evenboostdeclares aboost::enable_ifwhich excepts different parameters thanstd::enable_if.