[Sema] Improve diagnostics for template arg deduction

[jtbandes]

Summary

I noticed a relatively simple program produced a bad error message:

template<size_t N>
void foo(double (&arr)[N]) {}

void bar(double (&xs)[]) {
    foo(xs);
}

error: no matching function for call to 'foo'
    foo(xs);
    ^~~
note: candidate template ignored: could not match 'double' against 'double'
void foo(double (&arr)[N]) {}
     ^

This seems to happen because DeduceTemplateArgumentsByTypeMatch will sometimes do this

Info.FirstArg = TemplateArgument(ParamIn);
Info.SecondArg = TemplateArgument(ArgIn);

before it determines whether there's an error or not. Since the function is called recursively with the same Info, the FirstArg and SecondArg that actually cause the failure get overwritten.

My solution here is not very clean, although I tried to be minimally invasive; I'm open to suggestions for a better way to organize this. I considered llvm::SaveAndRestore but it seems to be unconditional and couldn't be "canceled" in the case of a real failure return value. I'm not familiar with any other such utilities, but maybe there is one that can make this nicer?

[jtbandes]

Adding @rsmith for review based on the commit history of SemaTemplateDeduction.cpp :)

[erik.pilkington]

This looks correct, but I definitely agree that RAII would make this a lot nicer. Have you considered adding a CancelableSaveAndRestore or something to SaveAndRestore.h? It seems useful and generic enough to make it worthwhile. Otherwise, you could just write your own RAII object special-cased to handle this. A less intrusive way of doing this might be to wrap calls to this function with another that checks if the return value is TDK_Success, and if so restores these fields.

lib/Sema/SemaTemplateDeduction.cpp

1395

What if this return TDK_Success?

[jtbandes]

@erik.pilkington Updated to use a wrapper function. This is definitely less invasive, but it could defeat some optimizations (any approach that checks the return value will defeat tail recursion, I suppose...)

[jtbandes]

Bump :)

[lichray]

I'm not confident enough about the effects of this patch with only one test; the last patch also passes the test. I hope I can understand the effects better.

[rsmith]

I would prefer that we make the more-invasive fix, and make each error case within template argument deduction set all the deduction failure information. (Consider factoring out the error setup into helper functions too.) The current approach is a maintenance problem in addition to creating incorrect diagnostics, as it makes it very hard to see what parameters are actually intended to be provided with each of the different forms of deduction failure.

[jtbandes]

Using a slightly more invasive fix. I haven't come up with any other test cases that exhibit the problem, which makes me unsure this fix is needed in all these locations. Maybe someone with more knowledge of this function can advise.

[jtbandes]

Please take another look when you get a chance :)

lib/Sema/SemaTemplateDeduction.cpp

if (auto *ParamRef = Param->getAs<RValueReferenceType>()) {
     if (ParamRef->getPointeeType().getQualifiers())
       return false;
     auto *TypeParm = ParamRef->getPointeeType()->getAs<TemplateTypeParmType>();
     return TypeParm && TypeParm->getIndex() >= FirstInnerIndex;
   }
   return false;
 }
 
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+    Sema &S, TemplateParameterList *TemplateParams, QualType ParamIn,
+    QualType ArgIn, TemplateDeductionInfo &Info,
+    SmallVectorImpl<DeducedTemplateArgument> &Deduced, unsigned TDF,
+    bool PartialOrdering, bool DeducedFromArrayBound);
+
 /// \brief Deduce the template arguments by comparing the parameter type and
 /// the argument type (C++ [temp.deduct.type]).
 ///
 /// \param S the semantic analysis object within which we are deducing
 ///
 /// \param TemplateParams the template parameters that we are deducing
 ///
 /// \param ParamIn the parameter type
 ///
 /// \param ArgIn the argument type
 ///
 /// \param Info information about the template argument deduction itself
 ///
 /// \param Deduced the deduced template arguments
 ///
 /// \param TDF bitwise OR of the TemplateDeductionFlags bits that describe
 /// how template argument deduction is performed.
 ///
 /// \param PartialOrdering Whether we're performing template argument deduction
 /// in the context of partial ordering (C++0x [temp.deduct.partial]).
 ///
 /// \returns the result of template argument deduction so far. Note that a
 /// "success" result means that template argument deduction has not yet failed,
 /// but it may still fail, later, for other reasons.
-static Sema::TemplateDeductionResult
-DeduceTemplateArgumentsByTypeMatch(Sema &S,
-                                   TemplateParameterList *TemplateParams,
-                                   QualType ParamIn, QualType ArgIn,
-                                   TemplateDeductionInfo &Info,
-                            SmallVectorImpl<DeducedTemplateArgument> &Deduced,
-                                   unsigned TDF,
-                                   bool PartialOrdering,
-                                   bool DeducedFromArrayBound) {
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
+    Sema &S, TemplateParameterList *TemplateParams, QualType ParamIn,
+    QualType ArgIn, TemplateDeductionInfo &Info,
+    SmallVectorImpl<DeducedTemplateArgument> &Deduced, unsigned TDF,
+    bool PartialOrdering, bool DeducedFromArrayBound) {
+  // Because DeduceTemplateArgumentsByTypeMatchInner is recursive, and tends to
+  // modify Info.FirstArg and SecondArg even when deduction succeeds, save the
+  // original values and restore them if no error occurred.
+  const auto OriginalFirstArg = Info.FirstArg;
+  const auto OriginalSecondArg = Info.SecondArg;
+
+  const auto Result = DeduceTemplateArgumentsByTypeMatchInner(
+      S, TemplateParams, ParamIn, ArgIn, Info, Deduced, TDF, PartialOrdering,
+      DeducedFromArrayBound);
+
+  if (Result == Sema::TDK_Success) {
+    Info.FirstArg = OriginalFirstArg;
+    Info.SecondArg = OriginalSecondArg;
+  }
+  return Result;
+}
+
+/// \see DeduceTemplateArgumentsByTypeMatch()
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+    Sema &S, TemplateParameterList *TemplateParams, QualType ParamIn,
+    QualType ArgIn, TemplateDeductionInfo &Info,
+    SmallVectorImpl<DeducedTemplateArgument> &Deduced, unsigned TDF,
+    bool PartialOrdering, bool DeducedFromArrayBound) {
   // We only want to look at the canonical types, since typedefs and
   // sugar are not part of template argument deduction.
   QualType Param = S.Context.getCanonicalType(ParamIn);
   QualType Arg = S.Context.getCanonicalType(ArgIn);
 
   // If the argument type is a pack expansion, look at its pattern.
   // This isn't explicitly called out
   if (const PackExpansionType *ArgExpansion
case Type::ObjCObjectPointer: {
       }
 
       return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
     }
 
     //     _Complex T   [placeholder extension]
     case Type::Complex:
       if (const ComplexType *ComplexArg = Arg->getAs<ComplexType>())
         return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                     cast<ComplexType>(Param)->getElementType(),
                                     ComplexArg->getElementType(),
                                     Info, Deduced, TDF);
 
       return Sema::TDK_NonDeducedMismatch;
 
     //     _Atomic T   [extension]
     case Type::Atomic:

test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp

namespace test3 {
   template<typename T>
   void func(bar<T>) { // expected-note {{candidate template ignored: could not match 'bar' against 'foo'}}
   }
 
   void test() {
     func(foo<int>()); // expected-error {{no matching function}}
   }
 }
+
+namespace test4 {
+  // expected-note@+1 {{candidate template ignored: could not match 'int [N]' against 'int []'}}
+  template<int N> void f1(int (&arr)[N]);
+  template<int N> void f2(int (&arr)[]) {
+    f1(arr); // expected-error {{no matching function}}
+  }
+}