Page MenuHomePhabricator

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

Authored by EricWF on Jan 15 2019, 10:20 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

EricWF created this revision.Jan 15 2019, 10:20 AM
lebedev.ri added inline comments.
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 added inline comments.
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 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 marked 6 inline comments as done.Jan 15 2019, 11:13 AM
EricWF added inline comments.
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 updated this revision to Diff 181829.Jan 15 2019, 11:13 AM
EricWF marked an inline comment as done.

Address inline comments.

gromer added inline comments.Jan 15 2019, 11:53 AM
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 added inline comments.Jan 15 2019, 2:24 PM
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).

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.

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.

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.

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).

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

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 marked 10 inline comments as done.Jan 15 2019, 9:21 PM
EricWF added inline comments.
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 marked 2 inline comments as done.Jan 15 2019, 9:28 PM
EricWF updated this revision to Diff 181972.Jan 15 2019, 9:42 PM

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!

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 marked an inline comment as done.Jan 16 2019, 9:06 AM
gromer added inline comments.
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 accepted this revision.Jan 16 2019, 3:53 PM
rsmith added inline comments.
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.)

This revision is now accepted and ready to land.Jan 16 2019, 3:53 PM
EricWF updated this revision to Diff 182206.Jan 16 2019, 6:56 PM
EricWF marked 3 inline comments as done.

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 accepted this revision.Jan 17 2019, 11:24 AM
rsmith added inline comments.
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.

EricWF retitled this revision from Add -Wimplicit-ctad warning to diagnose CTAD on types with no user defined deduction guides. to Add -Wctad-maybe-unsupported to diagnose CTAD on types with no user defined deduction guides..Jan 17 2019, 1:37 PM
EricWF edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.