[Sema] Fix a crash while converting constructors to deduction guides

[erik.pilkington]

Summary

The problem was with constructors that have parameters that refer to previous ones, such as D below:

template <size_t> struct A {};
template <class T> struct D {
  D(T t, A<sizeof(t)>);
};
int main() { D d(0, A<4>()); }

We were canonicalizing the type of A<sizeof(t)> to refer to the t in the constructor as opposed to the new one in the deduction guide. This is because ParmVarDecls are profiled based on their type and offset, so the two ParmVarDecls profiled to the same ID, despite them belonging to distinct functions. The fix for this here is to add a bit to ParmVarDecl, IsParamOfDeductionGuide, that tracks if this ParmVarDecl is "spliced" from another function. This makes references to the new ParmVarDecl not canonicalize to the previous ParmVarDecl.

This patch also fixes a closely related bug where newly created ParmVarDecls were not being added to the CurrentInstantiationScope, leading to an assert when we try to find it later.

rdar://41330135

Thanks for taking a look!
Erik

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

1899–1907

This is really the problem: we shouldn't be doing a full canonicalization step here. I expect that even with your patch applied we'll still see problems for cases like:

template<unsigned long> struct X {};
template<typename T> struct A {
  A(T t, X<sizeof(t)>);
};
A a(0, {});
template<typename U> struct B {
  B(U u, X<sizeof(u)>);
};
B b(0, {});

... because we'll canonicalize the second parameter of B's deduction guide to have type X<sizeof(t)> (where that's the t from A's deduction guide).

So I think we should look at fixing the FIXME here properly. There seem to be at least two viable options:

1. don't canonicalize the type; instead, extend template instantiation to be able to cope with one template referring to members of another template directly, without instantiating the class containing those members, or
2. add a custom TreeTransform to do the canonicalization we want to do here, and to avoid the canonicalization we don't want to do

Both of these seem pretty tricky to get right, though, which is why we currently use the canonicalization hack :(

[erik.pilkington]

Implement @rsmith's second suggestion. Thanks!

clang/lib/Sema/SemaTemplate.cpp

1899–1907

Yep, that still crashes :/

I started to implement 2 in the new patch. This implementation just unwraps typedefs into the deduction guide, but that is already enough to pass libcxx's test suite. This doesn't handle everything that we could, such as expression in a decltype [1]. This is fine for now though, because the canonicalization hack doesn't either. In fact, I couldn't find any cases where this patch fails but the canonicalization succeeds. I'm inclined to fix the crash now, address any extra cases in a follow-up if we actually want to support them. Does this seem reasonable to you?

It also seems we're reading pretty far between the lines of the standard here, do you think a DR should be filed?

[1]: I think we could support this is we wanted by stubbing out DeclRefExprs to members for a new (or existing?) opaque reference AST node that acted like a declval<T>() analog. This would allow us to do sfinae using the member's type without relying on it's context.

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

1899–1907

In core discussion, we agreed to handwave vigorously about such things in the wording for now... (the other option was that we'd spend many meetings refining wording to express what we mean here). A DR to keep us honest seems like a very good thing, especially if you have examples (such as the decltype example) where the correct behavior is unclear.

I think it's fine to do as you've done here (fix the crash for now, and address any subsequent necessary transformations as we find we need them). I'm pleasantly surprised by how simple the TreeTransform turned out to be (though I bet it makes the clang binary significantly larger).

Please can you add the example I gave above (or something like it) as a test case?

clang/include/clang/AST/Decl.h

     unsigned IsObjCMethodParam : 1;
 
     /// If IsObjCMethodParam, a Decl::ObjCDeclQualifier.
     /// Otherwise, the number of function parameter scopes enclosing
     /// the function parameter scope in which this parameter was
     /// declared.
     unsigned ScopeDepthOrObjCQuals : 7;
 
+    /// If this ParmVarDecl is a parameter of a deduction guide. This ensures
+    /// that we don't profile it the same as the parameter of the constructor.
+    unsigned IsParamOfDeductionGuide : 1;
+
     /// The number of parameters preceding this parameter in the
     /// function parameter scope in which it was declared.
     unsigned ParameterIndex : NumParameterIndexBits;
   };
 
   class NonParmVarDeclBitfields {
     friend class ASTDeclReader;
     friend class ImplicitParamDecl;

   ParmVarDecl(Kind DK, ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
               SourceLocation IdLoc, IdentifierInfo *Id, QualType T,
               TypeSourceInfo *TInfo, StorageClass S, Expr *DefArg)
       : VarDecl(DK, C, DC, StartLoc, IdLoc, Id, T, TInfo, S) {
     assert(ParmVarDeclBits.HasInheritedDefaultArg == false);
     assert(ParmVarDeclBits.DefaultArgKind == DAK_None);
     assert(ParmVarDeclBits.IsKNRPromoted == false);
     assert(ParmVarDeclBits.IsObjCMethodParam == false);
+    assert(ParmVarDeclBits.IsParamOfDeductionGuide == false);
     setDefaultArg(DefArg);
   }
 
 public:
   static ParmVarDecl *Create(ASTContext &C, DeclContext *DC,
                              SourceLocation StartLoc,
                              SourceLocation IdLoc, IdentifierInfo *Id,
                              QualType T, TypeSourceInfo *TInfo,

   ///   are promoted to double.
   bool isKNRPromoted() const {
     return ParmVarDeclBits.IsKNRPromoted;
   }
   void setKNRPromoted(bool promoted) {
     ParmVarDeclBits.IsKNRPromoted = promoted;
   }
 
+  void setIsParamOfDeductionGuide(bool Value = true) {
+    ParmVarDeclBits.IsParamOfDeductionGuide = Value;
+  }
+  bool isParamOfDeductionGuide() const {
+    return ParmVarDeclBits.IsParamOfDeductionGuide;
+  }
+
   Expr *getDefaultArg();
   const Expr *getDefaultArg() const {
     return const_cast<ParmVarDecl *>(this)->getDefaultArg();
   }
 
   void setDefaultArg(Expr *defarg);
 
   /// Retrieve the source range that covers the entire default

clang/lib/AST/StmtProfile.cpp

void VisitDecl(const Decl *D) override {
           // function parameters, so we will use the parameter's type for
           // establishing function parameter identity. That way, our
           // definition of "equivalent" (per C++ [temp.over.link]) is at
           // least as strong as the definition of "equivalent" used for
           // name mangling.
           VisitType(Parm->getType());
           ID.AddInteger(Parm->getFunctionScopeDepth());
           ID.AddInteger(Parm->getFunctionScopeIndex());
+          ID.AddBoolean(Parm->isParamOfDeductionGuide());
           return;
         }
 
         if (const TemplateTypeParmDecl *TTP =
                 dyn_cast<TemplateTypeParmDecl>(D)) {
           ID.AddInteger(TTP->getDepth());
           ID.AddInteger(TTP->getIndex());
           ID.AddBoolean(TTP->isParameterPack());

clang/lib/Sema/SemaTemplate.cpp

     return Result;
   }
 
   ParmVarDecl *
   transformFunctionTypeParam(ParmVarDecl *OldParam,
                              MultiLevelTemplateArgumentList &Args) {
     TypeSourceInfo *OldDI = OldParam->getTypeSourceInfo();
     TypeSourceInfo *NewDI;
-    if (!Args.getNumLevels())
-      NewDI = OldDI;
-    else if (auto PackTL = OldDI->getTypeLoc().getAs<PackExpansionTypeLoc>()) {
+    if (auto PackTL = OldDI->getTypeLoc().getAs<PackExpansionTypeLoc>()) {
       // Expand out the one and only element in each inner pack.
       Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(SemaRef, 0);
       NewDI =
           SemaRef.SubstType(PackTL.getPatternLoc(), Args,
                             OldParam->getLocation(), OldParam->getDeclName());
       if (!NewDI) return nullptr;
       NewDI =
           SemaRef.CheckPackExpansion(NewDI, PackTL.getEllipsisLoc(),
                                      PackTL.getTypePtr()->getNumExpansions());
     } else
       NewDI = SemaRef.SubstType(OldDI, Args, OldParam->getLocation(),
                                 OldParam->getDeclName());
     if (!NewDI)
       return nullptr;
 
     // Canonicalize the type. This (for instance) replaces references to
     // typedef members of the current instantiations with the definitions of
     // those typedefs, avoiding triggering instantiation of the deduced type
     // during deduction.
     // FIXME: It would be preferable to retain type sugar and source
     // information here (and handle this in substitution instead).
     NewDI = SemaRef.Context.getTrivialTypeSourceInfo(
         SemaRef.Context.getCanonicalType(NewDI->getType()),
         OldParam->getLocation());
 
     // Resolving a wording defect, we also inherit default arguments from the
     // constructor.
     ExprResult NewDefArg;
     if (OldParam->hasDefaultArg()) {
-      NewDefArg = Args.getNumLevels()
-                      ? SemaRef.SubstExpr(OldParam->getDefaultArg(), Args)
-                      : OldParam->getDefaultArg();
+      NewDefArg = SemaRef.SubstExpr(OldParam->getDefaultArg(), Args);
       if (NewDefArg.isInvalid())
         return nullptr;
     }
 
     ParmVarDecl *NewParam = ParmVarDecl::Create(SemaRef.Context, DC,
                                                 OldParam->getInnerLocStart(),
                                                 OldParam->getLocation(),
                                                 OldParam->getIdentifier(),
                                                 NewDI->getType(),
                                                 NewDI,
                                                 OldParam->getStorageClass(),
                                                 NewDefArg.get());
     NewParam->setScopeInfo(OldParam->getFunctionScopeDepth(),
                            OldParam->getFunctionScopeIndex());
+    NewParam->setIsParamOfDeductionGuide();
+    SemaRef.CurrentInstantiationScope->InstantiatedLocal(OldParam, NewParam);
     return NewParam;
   }
 
   NamedDecl *buildDeductionGuide(TemplateParameterList *TemplateParams,
                                  bool Explicit, TypeSourceInfo *TInfo,
                                  SourceLocation LocStart, SourceLocation Loc,
                                  SourceLocation LocEnd) {
     DeclarationNameInfo Name(DeductionGuideName, Loc);

clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

private: // expected-note {{declared private by intervening access specifier}}
     Protected(int) -> Protected<int>; // expected-error {{different access}}
   public: // expected-note 2{{declared public by intervening access specifier}}
     template<typename T> Public(T) -> Public<T>;
     template<typename T> Protected(T) -> Protected<T>; // expected-error {{different access}}
     template<typename T> Private(T) -> Private<T>; // expected-error {{different access}}
   };
 }
 
+namespace rdar41330135 {
+template <int> struct A {};
+template <class T>
+struct S {
+  template <class U>
+  S(T a, U t, A<sizeof(t)>);
+};
+template <class T> struct D {
+  D(T t, A<sizeof(t)>);
+};
+int f() {
+  S s(0, 0, A<sizeof(int)>());
+  D d(0, A<sizeof(int)>());
+}
+}
+
 #else
 
 // expected-no-diagnostics
 namespace undefined_warnings {
   // Make sure we don't get an "undefined but used internal symbol" warning for the deduction guide here.
   namespace {
     template <typename T>
     struct TemplDObj {