Page MenuHomePhabricator

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

Authored by martong on Nov 25 2020, 7:31 AM.

Diff Detail

Event Timeline

martong created this revision.Nov 25 2020, 7:31 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong requested review of this revision.Nov 25 2020, 7:31 AM
martong added inline comments.Nov 25 2020, 7:45 AM
clang/unittests/AST/ASTImporterTest.cpp
5985

Note, this "expectation" fails in baseline.

balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5998

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

rsmith added inline comments.Nov 30 2020, 1:01 AM
clang/lib/Sema/SemaTemplate.cpp
2357–2360

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 updated this revision to Diff 308335.Nov 30 2020, 5:29 AM
martong marked 2 inline comments as done.
  • Check for dependent DeclContext
  • Remove "dump()" calls from tests
clang/lib/Sema/SemaTemplate.cpp
2357–2360

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

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

rsmith added inline comments.Nov 30 2020, 1:11 PM
clang/lib/Sema/SemaTemplate.cpp
2077

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

2353

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.

hokein added a subscriber: hokein.Nov 30 2020, 11:53 PM

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

I think this issue is specific for CXXDeductionGuideDecl.

martong updated this revision to Diff 308920.Dec 2 2020, 2:23 AM
  • Move dependent context check into TransformTypedefType
  • Add new test case for param with pointer type
martong updated this revision to Diff 308922.Dec 2 2020, 2:30 AM
  • Add to MaterializedTypedefs only when copying the Decl

@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.

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 added a comment.Dec 2 2020, 2:21 PM

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 accepted this revision.Dec 2 2020, 6:35 PM

Looks good with a minor cleanup. Thanks!

clang/lib/Sema/SemaTemplate.cpp
2079–2080

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

This revision is now accepted and ready to land.Dec 2 2020, 6:35 PM
This revision was landed with ongoing or failed builds.Dec 3 2020, 2:36 AM
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.