Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

[• Quuxplusone]

Summary

Since I am interested in diagnosing this specific C++17ism (but not necessarily other C++17isms) in our codebase going forward.
The previous version of this patch named the option -Wc++14-compat-ctad, but I changed it to -Wctad per @rsmith's comments. Personally I see this as similar in spirit to -Wvla so the name -Wctad makes sense to me. My goal is just to have a fine-grained diagnostic by whatever name.

[• Quuxplusone]

Fix naming. (Uploaded an earlier diff by accident.)

[• Quuxplusone]

Third time's the charm!

[lebedev.ri]

Looks about right, but needs tests.

[• Quuxplusone]

Added a test case (partly copied from test/SemaCXX/cxx14-compat.cpp).

On the last warning in the file, I omitted the sketchy part of the warning, which on Godbolt looks like this:
https://godbolt.org/z/CB4z99

<source>:7:5: warning: class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'V<(lambda at <source>:6:16)>' [-Wc++98-c++11-c++14-compat]
    V v(lam);
    ^
1 warning generated.

This seems like a bug in the diagnostic, but fixing it is definitely not part of this patch, and probably above my pay grade at the moment anyway. :)

[• Quuxplusone]

@lebedev.ri ping?

[rsmith]

In the past, we've been resistant to adding more fine-grained compat warnings, because we don't want to encourage subsetting the language (which sounds like exactly what you're trying to do here). We generally don't think it's Clang's business to enforce coding style conventions (such as "don't use CTAD because it makes your code less readable"), but we do consider it to be in-scope for Clang to warn on constructs that are error-prone or that have a negative impact on portability or compatibility, and so on. On that basis, I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.

However, if we consider the goal to be to warn only on using CTAD in places where the class template author didn't design for it, then:

• this should be a separate warning, not a subgroup of -Wc++14-compat
• the warning should be suppressed by any explicitly-declared deduction guides for the class
• there should be some fairly obvious way to suppress the warning in the case where the deduction guides implied by the constructors already do the right thing (maybe an attribute?)

What do you think? Would that cover the use case you're trying to address here, or are you really trying to enforce a style rule of "don't use CTAD ever"?

[• Quuxplusone]

In D54565#1304740, @rsmith wrote:

In the past, we've been resistant to adding more fine-grained compat warnings, because we don't want to encourage subsetting the language (which sounds like exactly what you're trying to do here). We generally don't think it's Clang's business to enforce coding style conventions (such as "don't use CTAD because it makes your code less readable"), but we do consider it to be in-scope for Clang to warn on constructs that are error-prone or that have a negative impact on portability or compatibility, and so on. On that basis, I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.

Yes, that's a good way to put it.
Personally I would go stronger: using CTAD on class templates that were designed for it (1) is dangerous and (2) may create source compatibility problems for future changes to that library! The Standard Library itself deals with (2) by simply promising not to make future changes to the library... and deals with (1) by making changes to the library. For example, CTAD has in the "past/present" had problems distinguishing std::vector{"hello", "world"} (vector<const char*>, or UB?) from std::vector{set.begin(), set.end()} (vector<int>, or vector<set<int>::iterator>?). The guides for std::pair, and the guides around uses-allocator construction, seem particularly in flux, to my outside-observer eyes.

However, if we consider the goal to be to warn only on using CTAD in places where the class template author didn't design for it, then:

Unfortunately, I don't think the diagnostic will ever be able to detect "design"; the best it could do would be to detect "coding", which I don't think is good enough, in this case. ;)

• this should be a separate warning, not a subgroup of -Wc++14-compat
• the warning should be suppressed by any explicitly-declared deduction guides for the class
• there should be some fairly obvious way to suppress the warning in the case where the deduction guides implied by the constructors already do the right thing (maybe an attribute?)

What do you think? Would that cover the use case you're trying to address here, or are you really trying to enforce a style rule of "don't use CTAD ever"?

Essentially the latter.

• I would be quite happy to move the diagnostic outside of -Wc++14-compat.
• I specifically do not want the diagnostic to become suppressed on explicitly-declared deduction guides; that would have the effect of suppressing it for e.g. std::vector and std::pair. That would also have the effect of nerfing it for user-defined types: if Alice is relying on the diagnostic to avoid accidental CTAD, and then Bob adds a deduction guide that solves his case without perfectly solving Alice's case, then the diagnostic will go silent for Alice even though the consequences of Alice's slip-up are still just as severe.
• I think I don't understand your third bullet. My use-case requires a diagnostic about the call site, opted into by the writer of the call-site (Alice). IIUC, you ask for a way that the callee (the author of the class type, or perhaps Bob) could unilaterally suppress that diagnostic. But only the caller is able to make the judgment call that says "I don't trust these deduction guides (either they're incorrect, or I think they're prone to change in the next release, or both); I want a diagnostic every time I accidentally rely on one of them." The callee can't tell whether any particular use was accidental or intentional.

[thakis]

(From a user point of view, I have no idea what "CTAD" means, and I sometimes work on a C++ compiler.)

[• Quuxplusone]

Rename -Wc++14-compat-ctad to just -Wctad, per @rsmith's comment to "move this out of -Wc++14-compat."

@rsmith ping?

[• Quuxplusone]

@rsmith ping!

@thakis: You said, "From a user point of view, I have no idea what "CTAD" means, and I sometimes work on a C++ compiler." Did you mean "I think the command-line option should be named something more verbose than -W[c++14-compat-]ctad"? Or "I don't know what CTAD is and I would indeed like a diagnostic if I use it accidentally"? Or something else?

[• Quuxplusone]

@rsmith ping! It's been a week since the last ping...

[thakis]

In D54565#1331293, @Quuxplusone wrote:

@rsmith ping!

@thakis: You said, "From a user point of view, I have no idea what "CTAD" means, and I sometimes work on a C++ compiler." Did you mean "I think the command-line option should be named something more verbose than -W[c++14-compat-]ctad"? Or "I don't know what CTAD is and I would indeed like a diagnostic if I use it accidentally"? Or something else?

The former. Since I don't know what it means, I don't know if I want a diagnostic for it :-)

[• Quuxplusone]

In D54565#1339454, @thakis wrote:

Since I don't know what it means, I don't know if I want a diagnostic for it :-)

Fair point. ;) But the full text of the diagnostic message does say "class template argument deduction is...," which does hint at the meaning of "CTAD."
I just checked and the text of the diagnostic for -Wvla is "variable length array used"; maybe I should follow its lead and just say "class template argument deduction used," thus even further divorcing this option from -Wc++14-compat.

[rsmith]

I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.

Personally I would go stronger: using CTAD on class templates that were designed for it (1) is dangerous and (2) may create source compatibility problems for future changes to that library!

I've given this a lot of thought, and while I'm very sympathetic to your desire here, I do not think that we have sufficient justification to provide a warning to allow (effectively) disabling this language features. There's an important difference here:

• using CTAD on arbitrary class templates that weren't designed for it creates a source compatibility problem that the class template author has no control over nor say in
• using CTAD on class templates that were designed for it does not create a new source compatibility problem -- the class template author created the source compatibility problem themselves when they designed how their class templates would interact with CTAD

Either way there's a source compatibility problem, but in the second case, it's the class template author's choice to extend their source compatibility contract. So:

• I still think it's a good idea to have a warning to warn on use of CTAD on a class where there is no evidence the class was designed for CTAD (this warning would be suppressed by either an explicit deduction guide or by an attribute on the class template definition indicating that the constructors are designed to support CTAD)
• it would also seem like a good idea to have warnings for CTAD cases that we have strong evidence are dangerous (eg, for T(x...) where x expands to a pack containing a single T, we should warn if we use the copy-deduction candidate and there's another candidate that would have been selected if the copy-deduction candidate didn't get a priority boost)

... but I don't think we can justify warning on all CTAD.

Have you considered the clang-tidy route? Given that it seems like your goal is to avoid accidental use of CTAD, an out-of-band (not part of regular compilations) check that you satisfy that goal would seem like it could be part of a solution.

[• Quuxplusone]

• using CTAD on arbitrary class templates that weren't designed for it creates a source compatibility problem that the class template author has no control over nor say in
• using CTAD on class templates that were designed for it does not create a new source compatibility problem -- the class template author created the source compatibility problem themselves when they designed how their class templates would interact with CTAD

I don't see the distinction you're making. It sounds like you're saying, "std::vector is a 'CTAD-enabled' type. You (the client programmer) opted into CTAD when you started using std::vector in your project. If you don't want accidental CTAD, stop using std::vector" — which I don't think is practically useful advice.

it would also seem like a good idea to have warnings for CTAD cases that we have strong evidence are dangerous (eg, for T(x...) where x expands to a pack containing a single T, we should warn if we use the copy-deduction candidate and there's another candidate that would have been selected if the copy-deduction candidate didn't get a priority boost)

That sounds like a good idea (although I am not qualified to implement it). I would also ask for a warning on T t{x}; within a generic algorithm where x is of some dependent type that might or might not happen to be a specialization of T. And ideally we'd be able to produce the warning at parse time (i.e. generic-algorithm-testing time) rather than waiting until the offending template instantiation is actually instantiated (i.e. generic-algorithm-usage-in-the-field time), because by then it's too late for the client programmer to do anything about it.

For an (artificial) example of how accidental CTAD in a generic algorithm can cause interesting misbehavior, see https://quuxplusone.github.io/blog/2018/12/11/dont-inherit-from-std-types/#now-we-feed-them-to-this-functio

[rjmccall]

I doubt "CTAD" is going to survive as a term that people recognize and remember, and I don't think -Wclass-template-argument-deduction is outrageously wordy compared to some of our other diagnostic groups. With that change, this would be fine with me.

[rjmccall]

Oh, I see this review might be dead in the water; I'm not sure why I was just added as a reviewer to it (by a non-author, no less).

I personally have no problem with pulling out specific features as sub-groups of the compatibility warning. I agree with Richard, though, that if the real goal is to warn about "unsafe" uses of this feature then a more tailored warning would be better.

include/clang/Basic/DiagnosticGroups.td

 def GNUFlexibleArrayInitializer : DiagGroup<"gnu-flexible-array-initializer">;
 def GNUFlexibleArrayUnionMember : DiagGroup<"gnu-flexible-array-union-member">;
 def GNUFoldingConstant : DiagGroup<"gnu-folding-constant">;
 def FormatExtraArgs : DiagGroup<"format-extra-args">;
 def FormatZeroLength : DiagGroup<"format-zero-length">;
 
 def InvalidIOSDeploymentTarget : DiagGroup<"invalid-ios-deployment-target">;
 
+def CTAD : DiagGroup<"ctad">;
 def CXX17CompatMangling : DiagGroup<"c++17-compat-mangling">;
 def : DiagGroup<"c++1z-compat-mangling", [CXX17CompatMangling]>;
 // Name of this warning in GCC.
 def NoexceptType : DiagGroup<"noexcept-type", [CXX17CompatMangling]>;
 
 // Warnings for C++1y code which is not compatible with prior C++ standards.
 def CXXPre14Compat : DiagGroup<"c++98-c++11-compat">;
 def CXXPre14CompatPedantic : DiagGroup<"c++98-c++11-compat-pedantic",
                                        [CXXPre14Compat,
                                         CXXPre14CompatBinaryLiteral]>;
-def CXXPre17Compat : DiagGroup<"c++98-c++11-c++14-compat">;
+def CXXPre17Compat : DiagGroup<"c++98-c++11-c++14-compat",
+                                       [CTAD]>;
 def CXXPre17CompatPedantic : DiagGroup<"c++98-c++11-c++14-compat-pedantic",
                                        [CXXPre17Compat]>;
 def CXXPre2aCompat : DiagGroup<"c++98-c++11-c++14-c++17-compat">;
 def CXXPre2aCompatPedantic : DiagGroup<"c++98-c++11-c++14-c++17-compat-pedantic",
                                        [CXXPre2aCompat]>;
 
 def CXX98CompatBindToTemporaryCopy :
   DiagGroup<"c++98-compat-bind-to-temporary-copy">;

include/clang/Basic/DiagnosticSemaKinds.td

   "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;
+  InGroup<CTAD>, DefaultIgnore;
 
 // 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<

test/SemaCXX/cxx14-compat-ctad.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wctad -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++2a -Wctad -verify %s
+
+template<typename T = int> struct X {};
+X x; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'X<int>'}}
+
+template<template<typename> class> struct Y {};
+Y<X> yx; // ok, not class template argument deduction
+
+template<typename T> void f(T t) {
+  X x = t; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17}}
+}
+
+template<typename T> void g(T t) {
+  typename T::X x = t; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'typename A::X<A>' (aka 'A::X<A>')}}
+}
+
+struct A { template<typename T> struct X { X(T); }; };
+void h(A a) { g(a); } // expected-note {{in instantiation of function template specialization 'g<A>'}}
+
+template<class T> struct V { V(const T&) {} };
+
+V(int) -> V<int*>;  // ok, deduction guide is not a use of class template argument deduction
+
+void f2() { V v('a'); } // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'V<char>'}}
+void g2() { V v(0); } // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'V<int *>'}}
+
+void h2() {
+    auto lam = [](){};
+    V v(lam); // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17}}
+}