Add -Wctad-maybe-unsupported to diagnose CTAD on types with no user defined deduction guides.

[EricWF]

Summary

Some style guides want to allow using CTAD only on types that "opt-in"; i.e. on types that are designed to support it and not just types that *happen* to work with it.

This patch implements the -Wctad-maybe-unsupported warning, which is off by default, which warns when CTAD is used on a type that does not define any deduction guides.

The following pattern can be used to suppress the warning in cases where the type intentionally doesn't define any deduction guides:

struct allow_ctad_t;

template <class T>
struct TestSuppression {
  TestSuppression(T) {}
};
TestSuppression(allow_ctad_t)->TestSuppression<void>; // guides with incomplete parameter types are never considered.

[lebedev.ri]

include/clang/Basic/DiagnosticGroups.td

1054

Should this be in some group?
Alternatively, would it make sense to add it to new -Wctad group?

include/clang/Basic/DiagnosticSemaKinds.td

2129

class template argument deduction <was used? happened?> for %0 that has no user
the sentence looks incomplete to me.

lib/Sema/SemaInit.cpp

9443

on a .... ?

[• Quuxplusone]

include/clang/Basic/DiagnosticSemaKinds.td

2129

s/user defined/user-defined/, and perhaps s/that has/with/ for brevity.

lib/Sema/SemaInit.cpp

9287

Nitpick: I don't know if this is LLVM style, but I wish this were written as

if (!GD->isImplicit())
  HasUserDeclaredDeductionGuideCandidate = true;

for clarity. Also, is it "user-defined" (per the error message) or "user-declared" (per the name of this variable)?

test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

426

I suggest that additional (more realistic) motivating examples can be found in STL's CppCon 2018 talk. E.g.

template<class T, class U>
struct Pair {
    T first; U second;
    explicit Pair(const T& t, const U& u) : first(t), second(u) {}
};
Pair p(42, "hello world");  // constructs a Pair<int, char[12]>

template<class T, class U>
struct Pair2 {
    T first; U second;
    explicit Pair2(T t, U u) : first(t), second(u) {}
};
Pair2 p(42, "hello world");  // constructs a Pair2<int, const char*>

I would expect (and, with your patch, receive) diagnostics for both of these.

But for something like Vector("a", "b") your patch gives no diagnostic: https://godbolt.org/z/_9zhav
And for something like [std::optional o(x); in generic context](https://quuxplusone.github.io/blog/2018/12/11/dont-inherit-from-std-types/) your patch gives no diagnostic.

I do have a patch out for a general-purpose -Wctad that would give warnings on those latter cases as well: D54565
I think -Wimplicit-ctad is a good start, but it doesn't solve all the cases I'd like to see solved.

[EricWF]

include/clang/Basic/DiagnosticSemaKinds.td

2129

How about using class template argument deduction for %0 that has no user-defined deduction guides?

test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

426

I'll add the tests you described. And indeed, this solution is incomplete.

Can you expand on you std::optional example? I don't understand it.

[EricWF]

Address inline comments.

[gromer]

include/clang/Basic/DiagnosticSemaKinds.td

2129

I'd prefer a phrasing more along the lines of "using class template argument deduction for %0 that might not intentionally support it". That gives us more room to do things like add an attribute later if necessary, and it helps the user understand why this warning indicates a potential problem.

[• Quuxplusone]

test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

426

Can you expand on you std::optional example? I don't understand it.

https://quuxplusone.github.io/blog/2018/12/09/wctad/
https://akrzemi1.wordpress.com/2018/12/09/deducing-your-intentions/

Essentially, CTAD tries to guess your intention, and it's easy for CTAD to guess wrong in a generic context. The vector v("a", "b") example is about being in a concrete context and always guessing wrong; the more pernicious auto o = optional(x) example is about being in a generic context and sometimes guessing right and sometimes guessing wrong, depending on the type of x (and maybe not finding out until after you've shipped your header-only library).

[rsmith]

I like @Quuxplusone's suggestions for more heuristics here, but I think they should probably live under a different warning flag (collectively grouped under -Wctad as @lebedev.ri suggested). Concretely, I think we could warn if, during template instantiation from a type-dependent initializer, we select the copy deduction candidate and there was another viable candidate (the optional o(x) case, the tuple t{args...}case, etc), maybe under a new -Wctad-unwrap or similar.

I don't see a good general way to catch and warn on bugs like vector<string> v("foo", "bar") (this problem isn't really specific to CTAD), but we could certainly add some special-case heuristics to detect cases where a pair of string literals is passed to a constructor of a container-like type (for example, we could check whether the constructor looks like it can be called with arbitrary iterators as arguments, the class also has an initializer_list<T> constructor where a string literal can be implicitly converted to T, and the class has begin and end member functions).

include/clang/Basic/DiagnosticGroups.td

1054

Let's not add a -Wctad until we have more than one thing to put in it. I expect that to happen relatively soon :)

include/clang/Basic/DiagnosticSemaKinds.td

2129

I like that approach; something like "using class template argument deduction for %0 that might not intend to support it" -- or perhaps more directly "%0 might not intend to support class template argument deduction" -- along with a note describing how to syntactically suppress the warning (w"add a deduction guide to suppress this warning" or "use the [[clang::ctad]] attribute to suppress this warning" or whatever we decide is best).

lib/Sema/SemaInit.cpp

9268

Nit: LLVM / Clang style wants a period at the end of a comment.

9287

Formally, it's actually just "any deduction guides". Constructors aren't transformed into deduction guides; rather, deduction guides and constructors both form candidates for CTAD. So HasAnyDeductionGuides would work. (I also think we should omit the "candidates", because we don't care whether any deduction guides were candidates for this particular overload resolution, only whether there are any at all -- if we're excluding explicit deduction guides and the only deduction guides are explicit, we still want to (and do!) suppress the warning.

[• Quuxplusone]

Concretely, I think we could warn if, during template instantiation from a type-dependent initializer, we select the copy deduction candidate and there was another viable candidate (the optional o(x) case, the tuple t{args...} case, etc), maybe under a new -Wctad-unwrap or similar.

The trick is that you have to give the warning even when the copy deduction candidate is not selected — e.g. tuple t{args...} when args... is not a 1-parameter pack right now but if it might turn out to be a 1-parameter pack at "production time" (after the library has reached the customer).

I don't see a good general way to catch and warn on bugs like vector<string> v("foo", "bar")

You mean vector v("foo", "bar"). Without CTAD — vector<string> v("foo", "bar") — there's no bug; the line of code simply doesn't compile, that's all. What CTAD adds is the possibility for the line to quietly compile into the wrong thing.

(this problem isn't really specific to CTAD)

Maybe not, but it's certainly correlated, right?

but we could certainly add some special-case heuristics to detect cases where a pair of string literals is passed to a constructor of a container-like type (for example, we could check whether the constructor looks like it can be called with arbitrary iterators as arguments, the class also has an initializer_list<T> constructor where a string literal can be implicitly converted to T, and the class has begin and end member functions).

For my own use-cases, I will continue to want a 100% comprehensive -Wctad. All these "heuristics" you're proposing seem very ad-hoc, and make a lot of work for the compiler vendor, and seem complicated enough that I would still worry about bugs slipping through the cracks. Whereas, if the user can simply 100% outlaw CTAD, then they don't ever have to worry.

[jdennett]

In D56731#1358907, @Quuxplusone wrote:

For my own use-cases, I will continue to want a 100% comprehensive -Wctad. All these "heuristics" you're proposing seem very ad-hoc, and make a lot of work for the compiler vendor, and seem complicated enough that I would still worry about bugs slipping through the cracks. Whereas, if the user can simply 100% outlaw CTAD, then they don't ever have to worry.

That's fair; I don't think anyone here is speaking against such a diagnostic (though maybe the name will be a bikeshed). It's just that this patch is a solution for a different problem: allowing the sufficiently safe uses of CTAD without allowing too many bugs.

[gromer]

In D56731#1358926, @jdennett wrote:

In D56731#1358907, @Quuxplusone wrote:

For my own use-cases, I will continue to want a 100% comprehensive -Wctad. All these "heuristics" you're proposing seem very ad-hoc, and make a lot of work for the compiler vendor, and seem complicated enough that I would still worry about bugs slipping through the cracks. Whereas, if the user can simply 100% outlaw CTAD, then they don't ever have to worry.

That's fair; I don't think anyone here is speaking against such a diagnostic (though maybe the name will be a bikeshed).

Agreed.

It's just that this patch is a solution for a different problem: allowing the sufficiently safe uses of CTAD without allowing too many bugs.

That makes it sound like a difference of degree, but I think it's more fundamental: those other warnings/heuristics/whatever are intended to protect library users against buggy uses of CTAD, but this one is intended to protect library maintainers from being forced to support CTAD usages that are correct (from the user's point of view), but that the maintainer didn't intend to support. So the only way that anything can "slip through the cracks" of this warning is if there's some way to use CTAD with a class template whose maintainers never intended to support CTAD, without triggering this warning (I'm reluctantly willing to exclude the case where user code opens up the library's namespace and adds a deduction guide, because I expect most maintainers will be only too happy to break such code).

[rsmith]

[This discussion should probably move to cfe-dev@, since it's not directly relevant for this patch.]

In D56731#1358907, @Quuxplusone wrote:

Concretely, I think we could warn if, during template instantiation from a type-dependent initializer, we select the copy deduction candidate and there was another viable candidate (the optional o(x) case, the tuple t{args...} case, etc), maybe under a new -Wctad-unwrap or similar.

The trick is that you have to give the warning even when the copy deduction candidate is not selected — e.g. tuple t{args...} when args... is not a 1-parameter pack right now but if it might turn out to be a 1-parameter pack at "production time" (after the library has reached the customer).

There's a balance here between practical usability and warning as early as possible. If the template is never actually instantiated with only one argument, then warning on it is noise, and noise that'd be hard to turn off without losing the value of the warning. So we need to decide whether we want a (hopefully!) low-false-positive warning that helps library vendors that think to test this corner case themselves, or a (presumably) higher-false-positive warning that also helps the incautious library vendors that insufficiently tested their library, and we need to keep in mind while making that decision that a warning that is either off-by-default or turned off by nearly everyone delivers much less value than a warning that can be made on by default and that people don't turn off. Are there open-source projects making use of CTAD on which we could perform an analysis?

I don't see a good general way to catch and warn on bugs like vector<string> v("foo", "bar")

You mean vector v("foo", "bar").

Hmm, I think I was actually thinking of cases more like:

vector<string> s = {{"foo", "bar"}};

... which I have seen come up quite a lot.

[EricWF]

include/clang/Basic/DiagnosticSemaKinds.td

2129

This sounds like a reasonable change to me. Done.

I'm not sure an attribute is needed at this point; AFAIK there is no case where a user defined guide can't be added. Even if it's just a dummy guide to suppress the warning. For example:

struct allow_ctad_t {
  allow_ctad_t() = delete;
};

template <class T>
struct TestSuppression {
  TestSuppression(int, T) {}
};
TestSuppression(allow_ctad_t) -> TestSuppression<void>;

Also, before we add an attribute, we want to be sure that it's not harmful to allowing users to suppress the warning without actually addressing the root problem (tha implicit CTAD results are often surprising). I would like to see real world examples of types that don't need user-defined guides to work.

lib/Sema/SemaInit.cpp

9287

I changed the name to HasAnyDeductionGuides.

Otherwise, I think I've address all of the cases in your comment.

[EricWF]

Address review comments.

• Use "%0 maybe not intend to support ctad" like warning text.
• Add note about suppression.
• Rename -Wimplicit-ctad -> -Wctad-maybe-unsupported (Perhaps it should be -Wctad-maybe-unintended?)

Add more tests!

[• Quuxplusone]

In D56731#1359190, @rsmith wrote:

we need to keep in mind while making that decision that a warning that is either off-by-default or turned off by nearly everyone delivers much less value

Agreed. I would expect the D54565 version of -Wctad to be off-by-default, but turned on by nearly everyone, so it'd be a middle road. We'd have to try it and see. :)

Are there open-source projects making use of CTAD on which we could perform an analysis?

I think that's the same question as "Are there open-source projects making use of C++17?" If C++17 projects exist, then either they're making use of CTAD on purpose, or they're making use of it accidentally (in which case -Wctad would be able to count the number of accidents). I just went and did an analysis of yomm2, Yato, and nytl: https://quuxplusone.github.io/blog/2019/01/16/counting-ctad/
yomm2 and Yato do not use CTAD. nytl uses CTAD heavily and intentionally, and 5 of its 30 instances are on a single class which has no deduction guides. (I think there's no bug. If it is a subtle bug, then that would be amazingly strong evidence in favor of Eric's heuristic!)

---

Hmm, I think I was actually thinking of cases more like:

vector<string> s = {{"foo", "bar"}};

... which I have seen come up quite a lot.

Yikes! That's a new one to me. But not CTAD-related, I agree. :)

[gromer]

include/clang/Basic/DiagnosticSemaKinds.td

2129

I'm not sure an attribute is needed at this point

I agree; I just want to keep the option open in case we decide we need one later.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

2129

TestSuppression(allow_ctad_t) -> TestSuppression<void>; doesn't always work:

struct X { X(); };
template<typename T = X> struct TestSuppression {
  TestSuppression(T);
};
TestSuppression(allow_ctad_t) -> TestSuppression<void>;
TestSuppression x({}); // ok without deduction guide, ill-formed with

I think this should work reliably (note that the incomplete parameter type makes the deduction guide always non-viable):

TestSuppression(struct AllowCTAD) -> TestSuppression<void>;

... and maybe we should suggest that in our note, perhaps with a fix-it hint pointing immediately after the class template definition?

2132

Drop "user-defined" here? (There's no other kind of deduction guide.)

[EricWF]

address comments.

include/clang/Basic/DiagnosticSemaKinds.td

2129

I'm a bit skeptical of a diagnostic pointing at white space at the end of classes. If we could produce a actionable fixit, then maybe, but what do we put on the right hand side of ->?

2132

lol. Are you saying asking the user to define a user-defined entity is redundant?

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

2129

Yeah, the "what goes on the right-hand side" problem is troublesome. What I'm worried about is that people will read the note and add a deduction guide that breaks their deduction semantics, or get annoyed with us because it's not obvious how to add a deduction guide without breaking their deduction semantics or duplicating deduction rules from the constructors.

OK, let's go with this for now and see how users react.

include/clang/Basic/DiagnosticGroups.td

 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
 
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
 def CrossTU : DiagGroup<"ctu">;
+
+def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;

include/clang/Basic/DiagnosticSemaKinds.td

 def err_deduction_guide_wrong_access : Error<
   "deduction guide has different access from the corresponding "
   "member template">;
 def note_deduction_guide_template_access : Note<
   "member template declared %0 here">;
 def note_deduction_guide_access : Note<
   "deduction guide declared %0 by intervening access specifier">;
 def warn_cxx14_compat_class_template_argument_deduction : Warning<
   "class template argument deduction is incompatible with C++ standards "
   "before C++17%select{|; for compatibility, use explicit type name %1}0">,
   InGroup<CXXPre17Compat>, DefaultIgnore;
+def warn_ctad_maybe_unsupported : Warning<
+  "%0 may not intend to support class template argument deduction">,
+  InGroup<CTADMaybeUnsupported>, DefaultIgnore;
+def note_suppress_ctad_maybe_unsupported : Note<
+  "add a deduction guide to suppress this warning">;
+
 
 // C++14 deduced return types
 def err_auto_fn_deduction_failure : Error<
   "cannot deduce return type %0 from returned value of type %1">;
 def err_auto_fn_different_deductions : Error<
   "'%select{auto|decltype(auto)}0' in return type deduced as %1 here but "
   "deduced as %2 in earlier return statement">;
 def err_auto_fn_used_before_defined : Error<

lib/Sema/SemaInit.cpp

QualType Sema::DeduceTemplateSpecializationFromInitializer(
   //   of a hypothetical class type, where the selected functions and function
   //   templates are considered to be the constructors of that class type
   //
   // Since we know we're initializing a class type of a type unrelated to that
   // of the initializer, this reduces to something fairly reasonable.
   OverloadCandidateSet Candidates(Kind.getLocation(),
                                   OverloadCandidateSet::CSK_Normal);
   OverloadCandidateSet::iterator Best;
+
+  bool HasAnyDeductionGuide = false;
+
   auto tryToResolveOverload =
       [&](bool OnlyListConstructors) -> OverloadingResult {
     Candidates.clear(OverloadCandidateSet::CSK_Normal);
+    HasAnyDeductionGuide = false;
+
     for (auto I = Guides.begin(), E = Guides.end(); I != E; ++I) {
       NamedDecl *D = (*I)->getUnderlyingDecl();
       if (D->isInvalidDecl())
         continue;
 
       auto *TD = dyn_cast<FunctionTemplateDecl>(D);
       auto *GD = dyn_cast_or_null<CXXDeductionGuideDecl>(
           TD ? TD->getTemplatedDecl() : dyn_cast<FunctionDecl>(D));
       if (!GD)
         continue;
 
+      if (!GD->isImplicit())
+        HasAnyDeductionGuide = true;
+
       // C++ [over.match.ctor]p1: (non-list copy-initialization from non-class)
       //   For copy-initialization, the candidate functions are all the
       //   converting constructors (12.3.1) of that class.
       // C++ [over.match.copy]p1: (non-list copy-initialization from class)
       //   The converting constructors of T are candidate functions.
       if (Kind.isCopyInit() && !ListInit) {
         // Only consider converting constructors.
         if (GD->isExplicit())
QualType Sema::DeduceTemplateSpecializationFromInitializer(
   // C++ [dcl.type.class.deduct]p1:
   //  The placeholder is replaced by the return type of the function selected
   //  by overload resolution for class template deduction.
   QualType DeducedType =
       SubstAutoType(TSInfo->getType(), Best->Function->getReturnType());
   Diag(TSInfo->getTypeLoc().getBeginLoc(),
        diag::warn_cxx14_compat_class_template_argument_deduction)
       << TSInfo->getTypeLoc().getSourceRange() << 1 << DeducedType;
+
+  // Warn if CTAD was used on a type that does not have any user-defined
+  // deduction guides.
+  if (!HasAnyDeductionGuide) {
+    Diag(TSInfo->getTypeLoc().getBeginLoc(),
+         diag::warn_ctad_maybe_unsupported)
+        << TemplateName;
+    Diag(Template->getLocation(), diag::note_suppress_ctad_maybe_unsupported);
+  }
+
   return DeducedType;
 }

test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

 template<typename U> struct B {
   B(U u, X<sizeof(u)>);
 };
 B b(0, {});
 }
 
 }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wctad-maybe-unsupported"
+namespace test_implicit_ctad_warning {
+
+template <class T>
+struct Tag {};
+
+template <class T>
+struct NoExplicit { // expected-note {{add a deduction guide to suppress this warning}}
+  NoExplicit(T) {}
+  NoExplicit(T, int) {}
+};
+
+// expected-warning@+1 {{'NoExplicit' may not intend to support class template argument deduction}}
+NoExplicit ne(42);
+
+template <class U>
+struct HasExplicit {
+  HasExplicit(U) {}
+  HasExplicit(U, int) {}
+};
+template <class U> HasExplicit(U, int) -> HasExplicit<Tag<U>>;
+
+HasExplicit he(42);
+
+// Motivating examples from (taken from Stephan Lavavej's 2018 Cppcon talk)
+template <class T, class U>
+struct AmateurPair { // expected-note {{add a deduction guide to suppress this warning}}
+  T first;
+  U second;
+  explicit AmateurPair(const T &t, const U &u) {}
+};
+// expected-warning@+1 {{'AmateurPair' may not intend to support class template argument deduction}}
+AmateurPair p1(42, "hello world"); // deduces to Pair<int, char[12]>
+
+template <class T, class U>
+struct AmateurPair2 { // expected-note {{add a deduction guide to suppress this warning}}
+  T first;
+  U second;
+  explicit AmateurPair2(T t, U u) {}
+};
+// expected-warning@+1 {{'AmateurPair2' may not intend to support class template argument deduction}}
+AmateurPair2 p2(42, "hello world"); // deduces to Pair2<int, const char*>
+
+template <class T, class U>
+struct ProPair {
+  T first; U second;
+    explicit ProPair(T const& t, U  const& u)  {}
+};
+template<class T1, class T2>
+ProPair(T1, T2) -> ProPair<T1, T2>;
+ProPair p3(42, "hello world"); // deduces to ProPair<int, const char*>
+static_assert(__is_same(decltype(p3), ProPair<int, const char*>));
+
+// Test that user-defined explicit guides suppress the warning even if they
+// aren't used as candidates.
+template <class T>
+struct TestExplicitCtor {
+  TestExplicitCtor(T) {}
+};
+template <class T>
+explicit TestExplicitCtor(TestExplicitCtor<T> const&) -> TestExplicitCtor<void>;
+TestExplicitCtor<int> ce1{42};
+TestExplicitCtor ce2 = ce1;
+static_assert(__is_same(decltype(ce2), TestExplicitCtor<int>), "");
+
+struct allow_ctad_t {
+  allow_ctad_t() = delete;
+};
+
+template <class T>
+struct TestSuppression {
+  TestSuppression(T) {}
+};
+TestSuppression(allow_ctad_t)->TestSuppression<void>;
+TestSuppression ta("abc");
+static_assert(__is_same(decltype(ta), TestSuppression<const char *>), "");
+}
+#pragma clang diagnostic pop
+
 #else
 
 // expected-no-diagnostics
 namespace undefined_warnings {
   // Make sure we don't get an "undefined but used internal symbol" warning for the deduction guide here.
   namespace {
     template <typename T>
     struct TemplDObj {