[Clang][Sema] Add -Wctad-selects-copy to diagnose copy deduction candidate

[rZhBoYao]

Summary

1. '-Wctad-selects-copy' warns when copy deduction candidate is selected in CTAD, and the initialization is a direct-initialization. The warning is suppressed when the class template is in a system header and has at least one explicit deduction guide.
2. Add a group '-Wctad-maybe-unintended' controlling '-Wctad-selects-copy' and '-Wctad-maybe-unsupported'.

Fixes #63288

[shafik]

Adding Aaron.

[cor3ntin]

I have a couple questions but overall I think this is a great warning to add.
Sorry it took us a while to get to your PR

clang/include/clang/Basic/DiagnosticGroups.td

1384

Maybe we should just call that Wctad?

clang/include/clang/Basic/DiagnosticSemaKinds.td

2491–2494

This message is a bit difficult to understand.
Maybe The type of this expression is Y<Z> because it was deduced by CTAD from the copy constructor of Y?
It's not an easy warning to phrase!

clang/lib/Sema/SemaInit.cpp

22

I don't think that adding this header is necessary

10932

So the idea here is that the standard library would have protected against that by providing deduction guides?
Which is funny because the original issue was reported because they did not.
Do you have an example of when we should not emit the diagnostic for a library type?

@ldionne @philnik Opinions on this warning?

clang/test/SemaTemplate/ctad.cpp

51

[philnik]

clang/lib/Sema/SemaInit.cpp

10932

I think this definitely has to be suppressed in system headers somehow. e.g. as_rvalue_view uses the copy ctor CTAD to avoid double-wrapping iterators. (see https://github.com/llvm/llvm-project/blob/main/libcxx/include/__ranges/as_rvalue_view.h#L75). OTOH I'm not sure whether this should be suppressed if the type is from a system header, since reverse_iterator{some-other-reverse-iterator} might not be intended either, regardless of deduction guides.

clang/docs/ReleaseNotes.rst

.. code-block:: text
     void func(int aa, int bb);
          ^            ~~~~~~
 
 - ``-Wformat`` cast fix-its will now suggest ``static_cast`` instead of C-style casts
   for C++ code.
 - ``-Wformat`` will no longer suggest a no-op fix-it for fixing scoped enum format
   warnings. Instead, it will suggest casting the enum object to the type specified
   in the format string.
+- Clang now warns when the class template argument deduction deduces the type to be the same as the initializer
+  of a direct-initialization. Turn this on with ``-Wctad-selects-copy``. If the class template is in a system header
+  and a reachable explicit deduction guide is present, the warning is suppressed.
+  (`#63288 <https://github.com/llvm/llvm-project/issues/63288>`_).
+
+  .. code-block:: c++
+
+    auto vec = std::vector{42};
+    auto v = std::vector{vec}; // no warning, user-defined deduction guides exist
+                               // and the template is in a system header
+
+    std::reverse_iterator it1 = v.rbegin();       // no warning, copy-initialization
+    auto it2 = std::reverse_iterator{v.rbegin()}; // warns, the intent might be the next line
+    auto it3 = std::reverse_iterator<decltype(v.rbegin())>(v.rbegin()); // no warning, no CTAD
 
 Bug Fixes in This Version
 -------------------------
 
 - Added a new diagnostic warning group
   ``-Wdeprecated-redundant-constexpr-static-def``, under the existing
   ``-Wdeprecated`` group. This controls warnings about out-of-line definitions
   of 'static constexpr' data members that are unnecessary from C++17 onwards.

clang/include/clang/Basic/DiagnosticGroups.td

def FunctionMultiVersioning
     : DiagGroup<"function-multiversion", [TargetClonesMixedSpecifiers]>;
 
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
 def CrossTU : DiagGroup<"ctu">;
 
 def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;
+def CTADSelectsCopy      : DiagGroup<"ctad-selects-copy">;
+def CTADMaybeUnintended  : DiagGroup<"ctad-maybe-unintended", [
+    CTADMaybeUnsupported, CTADSelectsCopy
+  ]>;
 
 def FortifySource : DiagGroup<"fortify-source">;
 
 def MaxTokens : DiagGroup<"max-tokens"> {
   code Documentation = [{
 The warning is issued if the number of pre-processor tokens exceeds
 the token limit, which can be set in three ways:
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

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">;
+def warn_ctad_selects_copy : Warning<
+  "%select{move|copy}0-constructing not wrapping a %1, "
+  "use copy-%select{list-|}2initialization to suppress this warning">,
+  InGroup<CTADSelectsCopy>, 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">;

clang/lib/Sema/SemaInit.cpp

 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Designator.h"
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/PointerIntPair.h"
QualType Sema::DeduceTemplateSpecializationFromInitializer(
   //  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 the copy deduction candidate is selected in direct-initialization.
+  auto DiagnoseCTADCopy = [&] {
+    if (auto IK = Kind.getKind();
+        IK <= InitializationKind::IK_DirectList &&
+        cast<CXXDeductionGuideDecl>(Best->Function)
+                ->getDeductionCandidateKind() == DeductionCandidate::Copy) {
+      Diag(Kind.getLocation(), diag::warn_ctad_selects_copy)
+          << Kind.getRange() << Inits.front()->isLValue() << DeducedType
+          << !ListInit;
+    }
+  };
+
   // Warn if CTAD was used on a type that does not have any user-defined
   // deduction guides.
   if (!FoundDeductionGuide) {
     Diag(TSInfo->getTypeLoc().getBeginLoc(),
          diag::warn_ctad_maybe_unsupported)
         << TemplateName;
     Diag(Template->getLocation(), diag::note_suppress_ctad_maybe_unsupported);
+    DiagnoseCTADCopy();
+  } else if (!PP.getSourceManager().isInSystemHeader(Template->getLocation())) {
+    DiagnoseCTADCopy();
   }
 
   return DeducedType;
 }

clang/test/SemaTemplate/ctad.cpp

-// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++23 -verify %s
 
 namespace pr41427 {
   template <typename T> class A {
   public:
     A(void (*)(T)) {}
   };
 
   void D(int) {}
namespace Access {
   D y = {Y(), {}};
 
   class Z {
     template <typename T> friend class D;
     struct type {};
   };
   D z = {Z(), {}};
 }
+
+namespace warn_ctad_copy {
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wctad-selects-copy"
+// like std::revrse_iterator, the motivating example
+template <typename T> struct CTAD {
+  template <typename From>
+    requires(!__is_same(From, T) && __is_convertible_to(From, T))
+  CTAD(const CTAD<From>&) {} // #1
+  CTAD(T) {}
+};
+CTAD(void) -> CTAD<void>; // does not suppress -Wctad-selects-copy
+
+CTAD<int>&   fc1();
+CTAD<double> fc2(),
+             c1 = fc1();         // uses #1
+CTAD c2 = fc2(),                 // OK, uses copy-initialization
+     c3 = CTAD(4.2);             // OK, copy deduction candidate not selected
+CTAD c4{fc2()};                  // test prvalue expression
+// expected-warning@-1{{move-constructing not wrapping a 'CTAD<double>' \
+(aka 'warn_ctad_copy::CTAD<double>'), use copy-list-initialization to suppress this warning}}
+CTAD c5 = CTAD{fc1()};           // test lvalue  expression
+// expected-warning@-1{{copy-constructing not wrapping a 'CTAD<int>' \
+(aka 'warn_ctad_copy::CTAD<int>'), use copy-list-initialization to suppress this warning}}
+auto c6 = CTAD((CTAD<int>&&)c5); // test xvalue  expression
+// expected-warning@-1{{move-constructing not wrapping a 'CTAD<int>' \
+(aka 'warn_ctad_copy::CTAD<int>'), use copy-initialization to suppress this warning}}
+#pragma clang diagnostic pop
+} // namespace warn_ctad_copy