[Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

[martong]

Summary

http://lists.llvm.org/pipermail/cfe-dev/2020-November/067252.html

[martong]

clang/unittests/AST/ASTImporterTest.cpp

5912

Note, this "expectation" fails in baseline.

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

5925

These dump calls should be removed (in the other case too)?

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

2356–2359

I think it would be equivalent and simpler to check only OldTypedefDecl->getDeclContext()->isDependentContext(). If we can resolve the name of the typedef to a dependently-scoped typedef, it must be a member of the current instantiation, and that's what we really care about here. This would also avoid any concerns about whether we're looking at the right declaration of the enclosing class (in the case where multiple definitions from different modules got merged), whether we need to skip over transparent contexts, and so on.

[martong]

• Check for dependent DeclContext
• Remove "dump()" calls from tests

clang/lib/Sema/SemaTemplate.cpp

2356–2359

Thanks for the review! I've updated to check for the dependent context.

[shafik]

Should we add a test for the regular function template case as well?

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

2077

The check to see if we should clone a typedef or not should be moved into this function.

2352

Hmm, we're only applying the logic in the case where the top-level type of a parameter is a typedef type. That doesn't seem right; this won't do the right thing for Constructor(Typedef*) and similar. Also, we don't want to skip the parameter transform entirely if we find a such a typedef; instead, we should only skip cloning the typedef itself.

[martong]

In D92101#2423685, @shafik wrote:

Should we add a test for the regular function template case as well?

I think this issue is specific for CXXDeductionGuideDecl.

[martong]

• Move dependent context check into TransformTypedefType
• Add new test case for param with pointer type

[martong]

• Add to MaterializedTypedefs only when copying the Decl

[martong]

@rsmith I have moved the check into TransformTypedefType. We do not add the typedef to the MaterializedTypedefs list if it's context is dependent because later we set the deduction guide as the DC for all typedefs in the list:

for (auto *TD : MaterializedTypedefs)
  TD->setDeclContext(Guide);

Also, we have to push the original TypedefDecl to the TypeLocBuilder, otherwise an assertion would fire:

llvm-project/clang/lib/Sema/TypeLocBuilder.h:103: clang::TypeSourceInfo* clang::TypeLocBuilder::getTypeSourceInfo(clang::ASTContext&, clang::QualType): Assertion `T == LastTy && "type doesn't match last type pushed!"' failed.

[martong]

In D92101#2423685, @shafik wrote:

Should we add a test for the regular function template case as well?

I think this issue is specific for a CXXDeductionGuideDecl which is a FunctionDecl. I am not sure if I can follow what kind of test do you suggest, could you please be more specific?

[shafik]

In D92101#2428104, @martong wrote:

In D92101#2423685, @shafik wrote:

Should we add a test for the regular function template case as well?

I think this issue is specific for a CXXDeductionGuideDecl which is a FunctionDecl. I am not sure if I can follow what kind of test do you suggest, could you please be more specific?

Apologies, I think I meant this for another PR

[rsmith]

Looks good with a minor cleanup. Thanks!

clang/lib/Sema/SemaTemplate.cpp

2080–2084

We only need to transform the underlying type of the typedef if we're cloning the typedef.

clang/lib/Sema/SemaTemplate.cpp

public:
   typedef TreeTransform<ExtractTypeForDeductionGuide> Base;
   ExtractTypeForDeductionGuide(
       Sema &SemaRef,
       llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs)
       : Base(SemaRef), MaterializedTypedefs(MaterializedTypedefs) {}
 
   TypeSourceInfo *transform(TypeSourceInfo *TSI) { return TransformType(TSI); }
 
   QualType TransformTypedefType(TypeLocBuilder &TLB, TypedefTypeLoc TL) {
     ASTContext &Context = SemaRef.getASTContext();
     TypedefNameDecl *OrigDecl = TL.getTypedefNameDecl();
     TypeLocBuilder InnerTLB;
     QualType Transformed =
         TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
     TypeSourceInfo *TSI =
         TransformType(InnerTLB.getTypeSourceInfo(Context, Transformed));
 
     TypedefNameDecl *Decl = nullptr;
 
     if (isa<TypeAliasDecl>(OrigDecl))
       Decl = TypeAliasDecl::Create(
           Context, Context.getTranslationUnitDecl(), OrigDecl->getBeginLoc(),
           OrigDecl->getLocation(), OrigDecl->getIdentifier(), TSI);
     else {
for (unsigned I = 0, E = NewTL.getNumParams(); I != E; ++I)
       NewTL.setParam(I, Params[I]);
 
     return Result;
   }
 
   ParmVarDecl *transformFunctionTypeParam(
       ParmVarDecl *OldParam, MultiLevelTemplateArgumentList &Args,
       llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs) {
     TypeSourceInfo *OldDI = OldParam->getTypeSourceInfo();
+
+    if (auto *OldTypedefT = OldParam->getType()->getAs<TypedefType>()) {
+      TypedefNameDecl *OldTypedefDecl = OldTypedefT->getDecl();
+      // If the typedef does not have a dependent context then skip the
+      // transform.
+      if (!OldTypedefDecl->getDeclContext()->isDependentContext())
+        return OldParam;
+    }
+
     TypeSourceInfo *NewDI;
     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;

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
   EXPECT_FALSE((bool)Err);
   consumeError(std::move(Err));
 
   // Make sure the class was completed once.
   EXPECT_EQ(1U, CompletedTags.size());
   EXPECT_EQ(Record, CompletedTags.front());
 }
 
+// FIXME Move these tests out of ASTImporterTest. For that we need to factor
+// out the ASTImporter specific pars from ASTImporterOptionSpecificTestBase
+// into a new test Fixture. Then we should lift up this Fixture to its own
+// implementation file and only then could we reuse the Fixture in other AST
+// unitttests.
+struct CTAD : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(CTAD, DeductionGuideShouldReferToANonLocalTypedef) {
+  Decl *TU = getTuDecl(
+      R"(
+      typedef int U;
+      template <typename T> struct A {
+        A(U, T);
+      };
+      A a{(int)0, (int)0};
+      )",
+      Lang_CXX17, "input.cc");
+  auto *Guide = FirstDeclMatcher<CXXDeductionGuideDecl>().match(
+      TU, cxxDeductionGuideDecl());
+  auto *Typedef = FirstDeclMatcher<TypedefNameDecl>().match(
+      TU, typedefNameDecl(hasName("U")));
+  ParmVarDecl *Param = Guide->getParamDecl(0);
+  // The type of the first param (which is a typedef) should match the typedef
+  // in the global scope.
+  EXPECT_EQ(Param->getType()->getAs<TypedefType>()->getDecl(), Typedef);
+}
+
+TEST_P(CTAD, DeductionGuideShouldCopyALocalTypedef) {
+  Decl *TU = getTuDecl(
+      R"(
+      template <typename T> struct A {
+        typedef T U;
+        A(U, T);
+      };
+      A a{(int)0, (int)0};
+      )",
+      Lang_CXX17, "input.cc");
+  auto *Guide = FirstDeclMatcher<CXXDeductionGuideDecl>().match(
+      TU, cxxDeductionGuideDecl());
+  auto *Typedef = FirstDeclMatcher<TypedefNameDecl>().match(
+      TU, typedefNameDecl(hasName("U")));
+  ParmVarDecl *Param = Guide->getParamDecl(0);
+  EXPECT_NE(Param->getType()->getAs<TypedefType>()->getDecl(), Typedef);
+}
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, CTAD,
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(std::vector<std::string>()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );