[AST] TypeSourceInfo for params of inherited constructor should be null

[sammccall]

Summary

This matches behavior of other synthetic members like copy constructors,
and avoids RecursiveASTVisitor traversing TypeLocs that don't correspond
to any spelling of the type they describe.

[ymandel]

clang/lib/Sema/SemaDeclCXX.cpp

13895–13896

I'm now suspicious of all TypeLocs in this code, and If I'm reading this code correctly, this typeloc is never actually used.

13908

Once you're at it, should this use actually be nullptr?

13929

nit: label with /* TypeSourceInfo=*/?

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

2142–2144

why phrase it bottom up, instead of top-down (matching the record decl and then using hasDescendant)?

[sammccall]

+Aaron for guidance & in case I'm making things up, TypeLocs/TSI have always confused me...

clang/lib/Sema/SemaDeclCXX.cpp

13895–13896

My understanding is TypeLoc is really just a Type* + handle into some storage, TypeSourceInfo is typically that storage.
So TInfo is storage to hold the location info for a function type, initialized to ~empty ("trivial").
Then ProtoLoc is an accessor that we use to mutate that storage (ProtoLoc.setParam below).
Meanwhile we pass TInfo in as the TypeSourceInfo for the function.

The net effect: if we ask for the function's typeloc, and then ask that loc for the param decls, we'll get them, just the same as if we ask the function for its param decls directly.
That said, see next comment.

13908

... maybe? For implicit copy-constructors, the TSI is nullptr (SemaDeclCXX.cpp:15425).
That suggests that being able to get at the params via the TypeLoc isn't actually a critical invariant.

I've done this as I agree TypeSourceInfo for implicit code is dubious, and I was able to construct a similar ASTMatcher misbehavior that this fixes.

@aaron.ballman does this seem right?

[sammccall]

address review comments, also null the TSI of the constructor itself

[aaron.ballman]

clang/lib/Sema/SemaDeclCXX.cpp

13908

Errr... somewhere in the middle between right and wrong. I can see why TypeSourceInfo for implicit code is dubious, but we've had a theoretical long-standing goal that we've never seriously worked towards of replacing QualType/Type * with TypeSourceInfo * in many of our interfaces so that location information about types is more readily available.

On balance, I think passing nullptr here is reasonable -- this is an implicit node and we do this for other implicitly created special member functions.

13929

I think this change is potentially incorrect, at least judging by: https://github.com/llvm/llvm-project/blob/ce0d16f574f5bd81086e124345b0fe8dc4bfd95d/clang/lib/Sema/SemaDeclCXX.cpp#L15442

But then again: https://github.com/llvm/llvm-project/blob/ce0d16f574f5bd81086e124345b0fe8dc4bfd95d/clang/lib/Sema/SemaDeclCXX.cpp#L15584

So perhaps more investigation is needed here?

clang/lib/Sema/SemaDeclCXX.cpp

Sema::findInheritingConstructor(SourceLocation Loc,
   for (NamedDecl *Ctor : Derived->lookup(Name))
     if (declaresSameEntity(cast<CXXConstructorDecl>(Ctor)
                                ->getInheritedConstructor()
                                .getConstructor(),
                            BaseCtor))
       return cast<CXXConstructorDecl>(Ctor);
 
   DeclarationNameInfo NameInfo(Name, UsingLoc);
-  TypeSourceInfo *TInfo =
-      Context.getTrivialTypeSourceInfo(BaseCtor->getType(), UsingLoc);
-  FunctionProtoTypeLoc ProtoLoc =
-      TInfo->getTypeLoc().IgnoreParens().castAs<FunctionProtoTypeLoc>();
 
   // Check the inherited constructor is valid and find the list of base classes
   // from which it was inherited.
   InheritedConstructorInfo ICI(*this, Loc, Shadow);
 
   bool Constexpr =
       BaseCtor->isConstexpr() &&
       defaultedSpecialMemberIsConstexpr(*this, Derived, CXXDefaultConstructor,
                                         false, BaseCtor, &ICI);
 
   CXXConstructorDecl *DerivedCtor = CXXConstructorDecl::Create(
-      Context, Derived, UsingLoc, NameInfo, TInfo->getType(), TInfo,
-      BaseCtor->getExplicitSpecifier(), getCurFPFeatures().isFPConstrained(),
+      Context, Derived, UsingLoc, NameInfo, BaseCtor->getType(),
+      /*TInfo=*/nullptr, BaseCtor->getExplicitSpecifier(),
+      getCurFPFeatures().isFPConstrained(),
       /*isInline=*/true,
       /*isImplicitlyDeclared=*/true,
       Constexpr ? BaseCtor->getConstexprKind() : ConstexprSpecKind::Unspecified,
       InheritedConstructor(Shadow, BaseCtor),
       BaseCtor->getTrailingRequiresClause());
   if (Shadow->isInvalidDecl())
     DerivedCtor->setInvalidDecl();
 
   // Build an unevaluated exception specification for this fake constructor.
-  const FunctionProtoType *FPT = TInfo->getType()->castAs<FunctionProtoType>();
+  const FunctionProtoType *FPT =
+      BaseCtor->getType()->castAs<FunctionProtoType>();
   FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
   EPI.ExceptionSpec.Type = EST_Unevaluated;
   EPI.ExceptionSpec.SourceDecl = DerivedCtor;
   DerivedCtor->setType(Context.getFunctionType(FPT->getReturnType(),
                                                FPT->getParamTypes(), EPI));
 
   // Build the parameter declarations.
   SmallVector<ParmVarDecl *, 16> ParamDecls;
   for (unsigned I = 0, N = FPT->getNumParams(); I != N; ++I) {
-    TypeSourceInfo *TInfo =
-        Context.getTrivialTypeSourceInfo(FPT->getParamType(I), UsingLoc);
     ParmVarDecl *PD = ParmVarDecl::Create(
         Context, DerivedCtor, UsingLoc, UsingLoc, /*IdentifierInfo=*/nullptr,
-        FPT->getParamType(I), TInfo, SC_None, /*DefArg=*/nullptr);
+        FPT->getParamType(I), /*TInfo=*/nullptr, SC_None, /*DefArg=*/nullptr);
     PD->setScopeInfo(0, I);
     PD->setImplicit();
     // Ensure attributes are propagated onto parameters (this matters for
     // format, pass_object_size, ...).
     mergeDeclAttributes(PD, BaseCtor->getParamDecl(I));
     ParamDecls.push_back(PD);
-    ProtoLoc.setParam(I, PD);
   }
 
   // Set up the new constructor.
   assert(!BaseCtor->isDeleted() && "should not use deleted constructor");
   DerivedCtor->setAccess(BaseCtor->getAccess());
   DerivedCtor->setParams(ParamDecls);
   Derived->addDecl(DerivedCtor);
 

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

StringRef Code = R"(
     using typeAliasDecl = X<int>;
   )";
   EXPECT_TRUE(
       matches(Code, typeAliasTemplateDecl(hasName("typeAliasTemplateDecl"))));
   EXPECT_TRUE(
       notMatches(Code, typeAliasTemplateDecl(hasName("typeAliasDecl"))));
 }
 
+TEST_P(ASTMatchersTest, TypeLocTest_DoesNotBindToSyntheticParams) {
+  if (!GetParam().isCXX11OrLater()) {
+    return;
+  }
+  EXPECT_TRUE(notMatches(
+      R"cpp(
+        struct Base { Base(int); };
+        struct Derived : Base { using Base::Base; };
+        Derived d(42); // force constructor to exist
+      )cpp",
+      cxxRecordDecl(hasAnyName("Derived"),
+                    hasDescendant(typeLoc(
+                        anyOf(loc(functionType()), loc(asString("int"))))))));
+}
+
 TEST_P(ASTMatchersTest, QualifiedTypeLocTest_BindsToConstIntVarDecl) {
   EXPECT_TRUE(matches("const int x = 0;",
                       qualifiedTypeLoc(loc(asString("const int")))));
 }
 
 TEST_P(ASTMatchersTest, QualifiedTypeLocTest_BindsToConstIntFunctionDecl) {
   EXPECT_TRUE(matches("const int f() { return 5; }",
                       qualifiedTypeLoc(loc(asString("const int")))));