[Sema] PR27155: Fix a template argument deduction bug with base classes

[erik.pilkington]

Summary

Previously, clang would incorrectly reject the following:

struct S {};
template<class T, int i> struct D : T {};
template<class T> void Foo(D<T, 1>);
int fn() {
  D<D<S, 1>, 0> v;
  Foo(v);
}

The problem is that clang initially tries to apply D<S, 1> for T in Foo, storing the result (in Deduced). Then clang tries to match 0 for 1 in Foo, would fail, and begin to consider the base classes of v (per temp.deduct.call p4.3), without resetting the original faulty assumption. This patch simply saves the deduced arguments before attempting the faulty deduction, then restores them once the faulty deduction fails.

Note: This section of code still has a somewhat related latent bug where ambiguous base class deductions are left undiagnosed, for example:

int fn2() {
  D<D<D<S, 1>, 1>, 0> v;
  Foo(v); // error, is T deduced to be S or D<S, 1>?
}

Which is outlawed by temp.deduct.call p5. I have another patch that fixes this, which I'll submit once(/if) this goes through.

[rsmith]

lib/Sema/SemaTemplateDeduction.cpp

1424–1426

Can you also make this saving (and restoring) of the old deduced arguments conditional on TDF_DerivedClass and the argument type being a class type? Maybe duplicate the DeduceTemplateArguments call and put one copy inside the if (const RecordType *RecordT = ... block and the other on the else path, instead of moving the copy earlier.

[erik.pilkington]

Invert some control flow for clarity, avoid unnecessary copy of deduced template arguments.

[rsmith]

lib/Sema/SemaTemplateDeduction.cpp

1449–1451

Can you move this up and merge it with the TDF_DerivedClass check to also skip the copy in the case where the argument is not of class type?

1453–1457

... This too, if it's not too awkward.

[erik.pilkington]

Avoid another copy when Arg is not a record type.

Richard: I don't think it's possible to avoid copying Deduced when Arg is not a complete type, because we only try to complete it when we know that the deduction would otherwise fail (therefore clobbering Deduced).

If this looks good, would you mind committing it? Thanks for reviewing!

[erik.pilkington]

Ping!

[rsmith]

Committed as r267444.

I refactored it to use CXXRecordDecl::forallBases instead of a hand-rolled traversal of the base classes in r267453.

lib/Sema/SemaTemplateDeduction.cpp

     }
 
     //     template-name<T> (where template-name refers to a class template)
     //     template-name<i>
     //     TT<T>
     //     TT<i>
     //     TT<>
     case Type::TemplateSpecialization: {
-      const TemplateSpecializationType *SpecParam
-        = cast<TemplateSpecializationType>(Param);
-
-      // Try to deduce template arguments from the template-id.
-      Sema::TemplateDeductionResult Result
-        = DeduceTemplateArguments(S, TemplateParams, SpecParam, Arg,
-                                  Info, Deduced);
-
-      if (Result && (TDF & TDF_DerivedClass)) {
-        // C++ [temp.deduct.call]p3b3:
-        //   If P is a class, and P has the form template-id, then A can be a
-        //   derived class of the deduced A. Likewise, if P is a pointer to a
-        //   class of the form template-id, A can be a pointer to a derived
-        //   class pointed to by the deduced A.
-        //
-        // More importantly:
-        //   These alternatives are considered only if type deduction would
-        //   otherwise fail.
-        if (const RecordType *RecordT = Arg->getAs<RecordType>()) {
-          // We cannot inspect base classes as part of deduction when the type
-          // is incomplete, so either instantiate any templates necessary to
-          // complete the type, or skip over it if it cannot be completed.
-          if (!S.isCompleteType(Info.getLocation(), Arg))
-            return Result;
-
-          // Use data recursion to crawl through the list of base classes.
-          // Visited contains the set of nodes we have already visited, while
-          // ToVisit is our stack of records that we still need to visit.
-          llvm::SmallPtrSet<const RecordType *, 8> Visited;
-          SmallVector<const RecordType *, 8> ToVisit;
-          ToVisit.push_back(RecordT);
-          bool Successful = false;
-          SmallVector<DeducedTemplateArgument, 8> DeducedOrig(Deduced.begin(),
-                                                              Deduced.end());
-          while (!ToVisit.empty()) {
-            // Retrieve the next class in the inheritance hierarchy.
-            const RecordType *NextT = ToVisit.pop_back_val();
-
-            // If we have already seen this type, skip it.
-            if (!Visited.insert(NextT).second)
-              continue;
-
-            // If this is a base class, try to perform template argument
-            // deduction from it.
-            if (NextT != RecordT) {
-              TemplateDeductionInfo BaseInfo(Info.getLocation());
-              Sema::TemplateDeductionResult BaseResult
-                = DeduceTemplateArguments(S, TemplateParams, SpecParam,
-                                          QualType(NextT, 0), BaseInfo,
-                                          Deduced);
-
-              // If template argument deduction for this base was successful,
-              // note that we had some success. Otherwise, ignore any deductions
-              // from this base class.
-              if (BaseResult == Sema::TDK_Success) {
-                Successful = true;
-                DeducedOrig.clear();
-                DeducedOrig.append(Deduced.begin(), Deduced.end());
-                Info.Param = BaseInfo.Param;
-                Info.FirstArg = BaseInfo.FirstArg;
-                Info.SecondArg = BaseInfo.SecondArg;
-              }
-              else
-                Deduced = DeducedOrig;
-            }
-
-            // Visit base classes
-            CXXRecordDecl *Next = cast<CXXRecordDecl>(NextT->getDecl());
-            for (const auto &Base : Next->bases()) {
-              assert(Base.getType()->isRecordType() &&
-                     "Base class that isn't a record?");
-              ToVisit.push_back(Base.getType()->getAs<RecordType>());
-            }
-          }
+      const TemplateSpecializationType *SpecParam =
+          cast<TemplateSpecializationType>(Param);
+
+      // When Arg cannot be a derived class, we can just try to deduce template
+      // arguments from the template-id.
+      const RecordType *RecordT = Arg->getAs<RecordType>();
+      if (!(TDF & TDF_DerivedClass) || !RecordT)
+        return DeduceTemplateArguments(S, TemplateParams, SpecParam, Arg, Info,
+                                       Deduced);
+
+      SmallVector<DeducedTemplateArgument, 8> DeducedOrig(Deduced.begin(),
+                                                          Deduced.end());
+
+      Sema::TemplateDeductionResult Result = DeduceTemplateArguments(
+          S, TemplateParams, SpecParam, Arg, Info, Deduced);
+
+      if (Result == Sema::TDK_Success)
+        return Result;
+
+
+      // C++14 [temp.deduct.call] p4b3:
+      //   If P is a class and P has the form simple-template-id, then the
+      //   transformed A can be a derived class of the deduced A. Likewise if
+      //   P is a pointer to a class of the form simple-template-id, the
+      //   transformed A can be a pointer to a derived class pointed to by the
+      //   deduced A.
+      //
+      //   These alternatives are considered only if type deduction would
+      //   otherwise fail. If they yield more than one possible deduced A, the
+      //   type deduction fails.
+
+      // Reset the incorrectly deduced argument from above.
+      Deduced = DeducedOrig;
+
+      // Use data recursion to crawl through the list of base classes.
+      // Visited contains the set of nodes we have already visited, while
+      // ToVisit is our stack of records that we still need to visit.
+      llvm::SmallPtrSet<const RecordType *, 8> Visited;
+      SmallVector<const RecordType *, 8> ToVisit;
+      ToVisit.push_back(RecordT);
+      bool Successful = false;
+      while (!ToVisit.empty()) {
+        // Retrieve the next class in the inheritance hierarchy.
+        const RecordType *NextT = ToVisit.pop_back_val();
+
+        // If we have already seen this type, skip it.
+        if (!Visited.insert(NextT).second)
+          continue;
 
-          if (Successful)
-            return Sema::TDK_Success;
+        // If this is a base class, try to perform template argument
+        // deduction from it.
+        if (NextT != RecordT) {
+          TemplateDeductionInfo BaseInfo(Info.getLocation());
+          Sema::TemplateDeductionResult BaseResult =
+              DeduceTemplateArguments(S, TemplateParams, SpecParam,
+                                      QualType(NextT, 0), BaseInfo, Deduced);
+
+          // If template argument deduction for this base was successful,
+          // note that we had some success. Otherwise, ignore any deductions
+          // from this base class.
+          if (BaseResult == Sema::TDK_Success) {
+            Successful = true;
+            DeducedOrig.clear();
+            DeducedOrig.append(Deduced.begin(), Deduced.end());
+            Info.Param = BaseInfo.Param;
+            Info.FirstArg = BaseInfo.FirstArg;
+            Info.SecondArg = BaseInfo.SecondArg;
+          } else
+            Deduced = DeducedOrig;
         }
 
+        // Visit base classes
+        CXXRecordDecl *Next = cast<CXXRecordDecl>(NextT->getDecl());
+        for (const auto &Base : Next->bases()) {
+          assert(Base.getType()->isRecordType() &&
+                 "Base class that isn't a record?");
+          ToVisit.push_back(Base.getType()->getAs<RecordType>());
+        }
       }
 
+      if (Successful)
+        return Sema::TDK_Success;
+
       return Result;
     }
 
     //     T type::*
     //     T T::*
     //     T (type::*)()
     //     type (T::*)()
     //     type (type::*)(T)

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

 namespace PR9233 {
   template<typename T> void f(const T **q); // expected-note{{candidate template ignored: deduced type 'const int **' of 1st parameter does not match adjusted type 'int **' of argument [with T = int]}}
 
   void g(int **p) {
     f(p); // expected-error{{no matching function for call to 'f'}}
   }
 
 }
+
+namespace PR27155 {
+
+struct B {};
+
+template<class T, int i> struct D : T {};
+template<class T> void Foo(D<T, 1>);
+
+int fn() {
+  D<D<B, 1>, 0> f;
+  Foo(f);
+}
+
+}