[clang] Enable the special enable_if_t diagnostics for libc++'s __enable_if_t as well.

[• Quuxplusone]

Summary

This comes from lengthy discussion between @Quuxplusone and @ldionne over on D108216.
Right now, libc++ uses a "SCARY metaprogramming" version of _EnableIf that bypasses all of Clang's clever diagnostic stuff and produces bad diagnostics. My recent benchmarks ( https://quuxplusone.github.io/blog/2021/09/04/enable-if-benchmark/ ) have determined that the SCARYness is not buying us any speedup; therefore we are happy to drop it and go back to using the standard std::enable_if for all our SFINAE needs. However, we don't want to type out typename std::enable_if<X>::type all over the library; we want to use an alias template. And we can't use std::enable_if_t because we need a solution that works in C++11, and we do not provide std::enable_if_t in C++11.

Therefore, D109435 switches us from SCARY _EnableIf to a normal __enable_if_t (at least in C++11 mode, and possibly everywhere for consistency). Simultaneously, this Clang patch enables the good diagnostics for __enable_if_t. We don't need to enable good diagnostics for _EnableIf because the name _EnableIf has only ever been used for the SCARY version where the good diagnostics don't trigger anyway.

(Btw, this existing code is all sorts of broken, theoretically speaking. I filed https://bugs.llvm.org/show_bug.cgi?id=51696 about it last week. So if someone wants to use this PR as an excuse to go down the rabbit hole and fix it for real, that would be cool too.)

[dblaikie]

Would at. least need a test case, but otherwise sounds OK to make things a bit better.

[ldionne]

This shouldn't be necessary anymore since we won't be using _EnableIf in libc++ going forward.

[ldionne]

Actually, scratch that, we could tweak this patch so it accepts __enable_if_t as well (that will be the new spelling for enable_if_t for pre-C++14 places going forward.

[• Quuxplusone]

Okay, I think I've got it all sorted out in my head. This path forward seems reasonable to me:

D109435 switches us from SCARY _EnableIf to a normal __enable_if_t (at least in C++11 mode, and possibly everywhere for consistency). Simultaneously, this Clang patch enables the good diagnostics for __enable_if_t. We don't need to enable good diagnostics for _EnableIf because the name _EnableIf has only ever been used for the SCARY version where the good diagnostics don't trigger anyway.

Added a test case.

[ldionne]

This seems reasonable to me.

clang/lib/Sema/SemaTemplate.cpp

3514

Nit: libc++ >= 14

[dblaikie]

Looks like most of the testing for the enable_if custom dialog was added in clang/test/SemaTemplate/overload-candidates.cpp in rG6f8d2c6c9c3451effdf075a7034bbe77045bfeba. (improved in rG00fa10b43f25d98e88cc81b653dfe8d49327722d - though no substantially novel test coverage added, just updating the improved diagnostic text) - maybe pull that test coverage over into this new dedicated test file? But no big deal.

[• Quuxplusone]

In D109411#2990494, @dblaikie wrote:

Looks like most of the testing for the enable_if custom dialog was added in clang/test/SemaTemplate/overload-candidates.cpp in rG6f8d2c6c9c3451effdf075a7034bbe77045bfeba. (improved in rG00fa10b43f25d98e88cc81b653dfe8d49327722d - though no substantially novel test coverage added, just updating the improved diagnostic text) - maybe pull that test coverage over into this new dedicated test file? But no big deal.

Yeah, I noticed those changed-but-not-really-added tests over there. I did deliberately make a new file because I wanted it to explicitly test alias templates like enable_if_t, whereas overload-candidates.cpp runs in C++98 and thus cannot contain alias templates. I don't know if the existing tests are adding super much value, but I don't think it would add any more value to pull them over into this new file (in fact we'd have to stop running them in C++98 mode), and I'm not confident enough that they add zero value to go entirely remove them. So, status quo is I just left them alone.

Anyone particularly want to jump in here? because otherwise I think I'll land this tomorrow-ish.

clang/lib/Sema/SemaTemplate.cpp

case BTK__type_pack_element:
     // We simply return the type at index `Index`.
     auto Nth = std::next(Ts.pack_begin(), Index.getExtValue());
     return Nth->getAsType();
   }
   llvm_unreachable("unexpected BuiltinTemplateDecl!");
 }
 
 /// Determine whether this alias template is "enable_if_t".
+/// libc++ >=14 uses "__enable_if_t" in C++11 mode.
 static bool isEnableIfAliasTemplate(TypeAliasTemplateDecl *AliasTemplate) {
-  return AliasTemplate->getName().equals("enable_if_t");
+  return AliasTemplate->getName().equals("enable_if_t") ||
+         AliasTemplate->getName().equals("__enable_if_t");
 }
 
 /// Collect all of the separable terms in the given condition, which
 /// might be a conjunction.
 ///
 /// FIXME: The right answer is to convert the logical expression into
 /// disjunctive normal form, so we can find the first failed term
 /// within each possible clause.

clang/test/SemaTemplate/diagnose-enable-if-t.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++14 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+
+namespace std {
+  inline namespace __1 {
+    template<bool, class = void> struct enable_if {};
+    template<class T> struct enable_if<true, T> { using type = T; };
+    template<bool B, class T = void> using enable_if_t = typename enable_if<B, T>::type;
+  }
+}
+
+namespace similar_to_user_code {
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T, class = std::enable_if_t<sizeof(T) != 1>>
+  void f(T, short);
+
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T, std::enable_if_t<sizeof(T) != 1>* = nullptr>
+  void f(T, int);
+
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T>
+  std::enable_if_t<sizeof(T) != 1, void> f(T, long);
+
+  void test() {
+    f('x', 0); // expected-error{{no matching function}}
+  }
+}
+
+namespace similar_to_libcxx_version_14 {
+  template<bool, class = void> struct enable_if {};
+  template<class T> struct enable_if<true, T> { using type = T; };
+  template<bool B, class T = void> using __enable_if_t = typename enable_if<B, T>::type;
+
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T, class = __enable_if_t<sizeof(T) != 1>>
+  void f(T, short);
+
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T, __enable_if_t<sizeof(T) != 1>* = nullptr>
+  void f(T, int);
+
+  // expected-note@+2 {{candidate template ignored: requirement 'sizeof(char) != 1' was not satisfied [with T = char]}}
+  template<class T>
+  __enable_if_t<sizeof(T) != 1, void> f(T, long);
+
+  void test() {
+    f('x', 0); // expected-error{{no matching function}}
+  }
+}
+
+namespace similar_to_libcxx_version_13 {
+  template<bool> struct _MetaBase {};
+  template<> struct _MetaBase<true> { template<class R> using _EnableIfImpl = R; };
+  template<bool B, class T = void> using _EnableIf = typename _MetaBase<B>::template _EnableIfImpl<T>;
+
+  // expected-note@+2 {{no member named '_EnableIfImpl'}}
+  template<class T, class = _EnableIf<sizeof(T) != 1>>
+  void f(T, short);
+
+  // expected-note@+2 {{no member named '_EnableIfImpl'}}
+  template<class T, _EnableIf<sizeof(T) != 1>* = nullptr>
+  void f(T, int);
+
+  // expected-note@+2 {{no member named '_EnableIfImpl'}}
+  template<class T>
+  _EnableIf<sizeof(T) != 1, void> f(T, long);
+
+  void test() {
+    f('x', 0); // expected-error{{no matching function}}
+  }
+}
+
+namespace not_all_names_are_magic {
+  template<bool, class = void> struct enable_if {};
+  template<class T> struct enable_if<true, T> { using type = T; };
+  template<bool B, class T = void> using a_pony = typename enable_if<B, T>::type;
+
+  // expected-note@-2 {{candidate template ignored: disabled by 'enable_if' [with T = char]}}
+  template<class T, class = a_pony<sizeof(T) != 1>>
+  void f(T, short);
+
+  // expected-note@-6 {{candidate template ignored: disabled by 'enable_if' [with T = char]}}
+  template<class T, a_pony<sizeof(T) != 1>* = nullptr>
+  void f(T, int);
+
+  // expected-note@-10 {{candidate template ignored: disabled by 'enable_if' [with T = char]}}
+  template<class T>
+  a_pony<sizeof(T) != 1, void> f(T, long);
+
+  void test() {
+    f('x', 0); // expected-error{{no matching function}}
+  }
+}