[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

1384

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 (Result.isNull()) {
       Info.SecondArg = NewDeduced;
       return Sema::TDK_Inconsistent;
     }
 
     Deduced[Index] = Result;
     return Sema::TDK_Success;
   }
 
-  // Set up the template argument deduction information for a failure.
-  Info.FirstArg = TemplateArgument(ParamIn);
-  Info.SecondArg = TemplateArgument(ArgIn);
-
   // If the parameter is an already-substituted template parameter
   // pack, do nothing: we don't know which of its arguments to look
   // at, so we have to wait until all of the parameter packs in this
   // expansion have arguments.
   if (isa<SubstTemplateTypeParmPackType>(Param))
     return Sema::TDK_Success;
 
+  // Use this when returning a failure result in order to fill in the
+  // corresponding failure info. Don't use this when returning the
+  // result of a recursive call, as the callee will have already set
+  // their own failure info.
+  const auto WithFailureInfo =
+    [&Info, &ParamIn, &ArgIn](Sema::TemplateDeductionResult Result) {
+      Info.FirstArg = TemplateArgument(ParamIn);
+      Info.SecondArg = TemplateArgument(ArgIn);
+      return Result;
+    };
+
   // Check the cv-qualifiers on the parameter and argument types.
   CanQualType CanParam = S.Context.getCanonicalType(Param);
   CanQualType CanArg = S.Context.getCanonicalType(Arg);
   if (!(TDF & TDF_IgnoreQualifiers)) {
     if (TDF & TDF_ParamWithReferenceType) {
       if (hasInconsistentOrSupersetQualifiersOf(Param, Arg))
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     } else if (!IsPossiblyOpaquelyQualifiedType(Param)) {
       if (Param.getCVRQualifiers() != Arg.getCVRQualifiers())
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     }
 
     // If the parameter type is not dependent, there is nothing to deduce.
     if (!Param->isDependentType()) {
       if (!(TDF & TDF_SkipNonDependent)) {
         bool NonDeduced =
             (TDF & TDF_AllowCompatibleFunctionType)
                 ? !S.isSameOrCompatibleFunctionType(CanParam, CanArg)
                 : Param != Arg;
-        if (NonDeduced) {
-          return Sema::TDK_NonDeducedMismatch;
-        }
+        if (NonDeduced)
+          return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
       }
       return Sema::TDK_Success;
     }
   } else if (!Param->isDependentType()) {
     CanQualType ParamUnqualType = CanParam.getUnqualifiedType(),
                 ArgUnqualType = CanArg.getUnqualifiedType();
     bool Success =
         (TDF & TDF_AllowCompatibleFunctionType)
case Type::ObjCObjectPointer: {
       if (TDF & TDF_SkipNonDependent)
         return Sema::TDK_Success;
 
       if (TDF & TDF_IgnoreQualifiers) {
         Param = Param.getUnqualifiedType();
         Arg = Arg.getUnqualifiedType();
       }
 
-      return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+      if (Param == Arg)
+        return Sema::TDK_Success;
+
+      return WithFailureInfo(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;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
     //     _Atomic T   [extension]
     case Type::Atomic:
       if (const AtomicType *AtomicArg = Arg->getAs<AtomicType>())
         return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                        cast<AtomicType>(Param)->getValueType(),
                                        AtomicArg->getValueType(),
                                        Info, Deduced, TDF);
 
-      return Sema::TDK_NonDeducedMismatch;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
     //     T *
     case Type::Pointer: {
       QualType PointeeType;
       if (const PointerType *PointerArg = Arg->getAs<PointerType>()) {
         PointeeType = PointerArg->getPointeeType();
       } else if (const ObjCObjectPointerType *PointerArg
                    = Arg->getAs<ObjCObjectPointerType>()) {
         PointeeType = PointerArg->getPointeeType();
       } else {
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
       }
 
       unsigned SubTDF = TDF & (TDF_IgnoreQualifiers | TDF_DerivedClass);
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                      cast<PointerType>(Param)->getPointeeType(),
                                      PointeeType,
                                      Info, Deduced, SubTDF);
     }
 
     //     T &
     case Type::LValueReference: {
       const LValueReferenceType *ReferenceArg =
           Arg->getAs<LValueReferenceType>();
       if (!ReferenceArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                            cast<LValueReferenceType>(Param)->getPointeeType(),
                            ReferenceArg->getPointeeType(), Info, Deduced, 0);
     }
 
     //     T && [C++0x]
     case Type::RValueReference: {
       const RValueReferenceType *ReferenceArg =
           Arg->getAs<RValueReferenceType>();
       if (!ReferenceArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                              cast<RValueReferenceType>(Param)->getPointeeType(),
                              ReferenceArg->getPointeeType(),
                              Info, Deduced, 0);
     }
 
     //     T [] (implied, but not stated explicitly)
     case Type::IncompleteArray: {
       const IncompleteArrayType *IncompleteArrayArg =
         S.Context.getAsIncompleteArrayType(Arg);
       if (!IncompleteArrayArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       unsigned SubTDF = TDF & TDF_IgnoreQualifiers;
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                     S.Context.getAsIncompleteArrayType(Param)->getElementType(),
                     IncompleteArrayArg->getElementType(),
                     Info, Deduced, SubTDF);
     }
 
     //     T [integer-constant]
     case Type::ConstantArray: {
       const ConstantArrayType *ConstantArrayArg =
         S.Context.getAsConstantArrayType(Arg);
       if (!ConstantArrayArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       const ConstantArrayType *ConstantArrayParm =
         S.Context.getAsConstantArrayType(Param);
       if (ConstantArrayArg->getSize() != ConstantArrayParm->getSize())
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       unsigned SubTDF = TDF & TDF_IgnoreQualifiers;
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                            ConstantArrayParm->getElementType(),
                                            ConstantArrayArg->getElementType(),
                                            Info, Deduced, SubTDF);
     }
 
     //     type [i]
     case Type::DependentSizedArray: {
       const ArrayType *ArrayArg = S.Context.getAsArrayType(Arg);
       if (!ArrayArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       unsigned SubTDF = TDF & TDF_IgnoreQualifiers;
 
       // Check the element type of the arrays
       const DependentSizedArrayType *DependentArrayParm
         = S.Context.getAsDependentSizedArrayType(Param);
       if (Sema::TemplateDeductionResult Result
             = DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
case Type::DependentSizedArray: {
       if (const DependentSizedArrayType *DependentArrayArg
             = dyn_cast<DependentSizedArrayType>(ArrayArg))
         if (DependentArrayArg->getSizeExpr())
           return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
                                                DependentArrayArg->getSizeExpr(),
                                                Info, Deduced);
 
       // Incomplete type does not match a dependently-sized array type
-      return Sema::TDK_NonDeducedMismatch;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     }
 
     //     type(*)(T)
     //     T(*)()
     //     T(*)(T)
     case Type::FunctionProto: {
       unsigned SubTDF = TDF & TDF_TopLevelParameterTypeList;
       const FunctionProtoType *FunctionProtoArg =
         dyn_cast<FunctionProtoType>(Arg);
       if (!FunctionProtoArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       const FunctionProtoType *FunctionProtoParam =
         cast<FunctionProtoType>(Param);
 
       if (FunctionProtoParam->getTypeQuals()
             != FunctionProtoArg->getTypeQuals() ||
           FunctionProtoParam->getRefQualifier()
             != FunctionProtoArg->getRefQualifier() ||
           FunctionProtoParam->isVariadic() != FunctionProtoArg->isVariadic())
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       // Check return types.
       if (auto Result = DeduceTemplateArgumentsByTypeMatch(
               S, TemplateParams, FunctionProtoParam->getReturnType(),
               FunctionProtoArg->getReturnType(), Info, Deduced, 0))
         return Result;
 
       // Check parameter types.
#include "clang/AST/TypeNodes.def"
     //     type (T::*)(T)
     //     T (type::*)(T)
     //     T (T::*)()
     //     T (T::*)(T)
     case Type::MemberPointer: {
       const MemberPointerType *MemPtrParam = cast<MemberPointerType>(Param);
       const MemberPointerType *MemPtrArg = dyn_cast<MemberPointerType>(Arg);
       if (!MemPtrArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       QualType ParamPointeeType = MemPtrParam->getPointeeType();
       if (ParamPointeeType->isFunctionType())
         S.adjustMemberFunctionCC(ParamPointeeType, /*IsStatic=*/true,
                                  /*IsCtorOrDtor=*/false, Info.getLocation());
       QualType ArgPointeeType = MemPtrArg->getPointeeType();
       if (ArgPointeeType->isFunctionType())
         S.adjustMemberFunctionCC(ArgPointeeType, /*IsStatic=*/true,
#include "clang/AST/TypeNodes.def"
     //     type(^)(T)
     //     T(^)()
     //     T(^)(T)
     case Type::BlockPointer: {
       const BlockPointerType *BlockPtrParam = cast<BlockPointerType>(Param);
       const BlockPointerType *BlockPtrArg = dyn_cast<BlockPointerType>(Arg);
 
       if (!BlockPtrArg)
-        return Sema::TDK_NonDeducedMismatch;
+        return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
       return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                                 BlockPtrParam->getPointeeType(),
                                                 BlockPtrArg->getPointeeType(),
                                                 Info, Deduced, 0);
     }
 
     //     (clang extension)
     //
     //     T __attribute__(((ext_vector_type(<integral constant>))))
     case Type::ExtVector: {
       const ExtVectorType *VectorParam = cast<ExtVectorType>(Param);
       if (const ExtVectorType *VectorArg = dyn_cast<ExtVectorType>(Arg)) {
         // Make sure that the vectors have the same number of elements.
         if (VectorParam->getNumElements() != VectorArg->getNumElements())
-          return Sema::TDK_NonDeducedMismatch;
+          return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
         // Perform deduction on the element types.
         return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                                   VectorParam->getElementType(),
                                                   VectorArg->getElementType(),
                                                   Info, Deduced, TDF);
       }
 
       if (const DependentSizedExtVectorType *VectorArg
                                 = dyn_cast<DependentSizedExtVectorType>(Arg)) {
         // We can't check the number of elements, since the argument has a
         // dependent number of elements. This can only occur during partial
         // ordering.
 
         // Perform deduction on the element types.
         return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams,
                                                   VectorParam->getElementType(),
                                                   VectorArg->getElementType(),
                                                   Info, Deduced, TDF);
       }
 
-      return Sema::TDK_NonDeducedMismatch;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     }
 
     //     (clang extension)
     //
     //     T __attribute__(((ext_vector_type(N))))
     case Type::DependentSizedExtVector: {
       const DependentSizedExtVectorType *VectorParam
         = cast<DependentSizedExtVectorType>(Param);
case Type::DependentSizedExtVector: {
         if (!NTTP)
           return Sema::TDK_Success;
 
         return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
                                              VectorArg->getSizeExpr(),
                                              Info, Deduced);
       }
 
-      return Sema::TDK_NonDeducedMismatch;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     }
 
     //     (clang extension)
     //
     //     T __attribute__(((address_space(N))))
     case Type::DependentAddressSpace: {
       const DependentAddressSpaceType *AddressSpaceParam =
           cast<DependentAddressSpaceType>(Param);
case Type::DependentAddressSpace: {
         if (!NTTP)
           return Sema::TDK_Success;
 
         return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
                                              ArgAddressSpace, S.Context.IntTy,
                                              true, Info, Deduced);
       }
 
-      return Sema::TDK_NonDeducedMismatch;
+      return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
     }
 
     case Type::TypeOfExpr:
     case Type::TypeOf:
     case Type::DependentName:
     case Type::UnresolvedUsing:
     case Type::Decltype:
     case Type::UnaryTransform:

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}}
+  }
+}