This is an archive of the discontinued LLVM Phabricator instance.

[clang][ASTImporter] Update lookup table correctly at deduction guides.
ClosedPublic

Authored by balazske on Nov 23 2021, 1:22 AM.

Details

Summary

Declaration context of template parameters of a FunctionTemplateDecl
may be different for each one parameter if the template is a
deduction guide. This case is handled correctly after this change.

Diff Detail

Event Timeline

balazske created this revision.Nov 23 2021, 1:22 AM
balazske requested review of this revision.Nov 23 2021, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 1:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong added inline comments.Nov 23 2021, 8:06 AM
clang/lib/AST/ASTImporter.cpp
6081

?

6082–6085

I think this sentence does not provide any meaningful information and does not increase the understand-ability. Plus the word change is overloaded, first I though you meant the patch itself...

clang/unittests/AST/ASTImporterTest.cpp
7336

Does this test provide an assertion failure in the base?

7348–7353

Could you please formulate expectations (assertions) on the DeclContext's of the two template parameters? I'd expect them to be different.

If the test is changed to print AST:

TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuide) {
  TranslationUnitDecl *FromTU = getTuDecl(
      R"(
      template<class> class A { };
      template<class T> class B {
          template<class T1, typename = A<T>> B(T1);
      };
      template<class T>
      B(T, T) -> B<int>;
      )",
      Lang_CXX17);

  // Get the implicit deduction guide for (non-default) constructor of 'B'.
  auto *FromDG1 = FirstDeclMatcher<FunctionTemplateDecl>().match(
      FromTU, functionTemplateDecl(templateParameterCountIs(3)));

  FromTU->dumpColor();
  llvm::errs()<<cast<Decl>(FromDG1->getTemplateParameters()->getParam(0)->getDeclContext())<<",";
  llvm::errs()<<cast<Decl>(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext())<<",";
  llvm::errs()<<cast<Decl>(FromDG1->getTemplateParameters()->getParam(2)->getDeclContext())<<"\n";
}

The output is:

...
|-ClassTemplateDecl 0x17931e0 <input.cc:2:7, col:33> col:29 A
| |-TemplateTypeParmDecl 0x1793090 <col:16> col:21 class depth 0 index 0
| `-CXXRecordDecl 0x1793150 <col:23, col:33> col:29 class A definition
|   |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
|   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
|   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveConstructor exists simple trivial needs_implicit
|   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveAssignment exists simple trivial needs_implicit
|   | `-Destructor simple irrelevant trivial needs_implicit
|   `-CXXRecordDecl 0x1793420 <col:23, col:29> col:29 implicit class A
|-ClassTemplateDecl 0x17935f0 <line:3:7, line:5:7> line:3:31 B
| |-TemplateTypeParmDecl 0x17934c8 <col:16, col:22> col:22 referenced class depth 0 index 0 T
| |-CXXRecordDecl 0x1793560 <col:25, line:5:7> line:3:31 class B definition
| | |-DefinitionData empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
| | | |-DefaultConstructor defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x1793830 <col:25, col:31> col:31 implicit referenced class B
| | `-FunctionTemplateDecl 0x1793cb0 <line:4:11, col:51> col:47 B<T>
| |   |-TemplateTypeParmDecl 0x17938c0 <col:20, col:26> col:26 referenced class depth 1 index 0 T1
| |   |-TemplateTypeParmDecl 0x1793a00 <col:30, <scratch space>:2:1> input.cc:4:39 typename depth 1 index 1
| |   | `-TemplateArgument type 'A<T>'
| |   |   `-TemplateSpecializationType 0x1793980 'A<T>' dependent A
| |   |     `-TemplateArgument type 'T'
| |   |       `-TemplateTypeParmType 0x1793520 'T' dependent depth 0 index 0
| |   |         `-TemplateTypeParm 0x17934c8 'T'
| |   `-CXXConstructorDecl 0x1793c08 <col:47, col:51> col:47 B<T> 'void (T1)'
| |     `-ParmVarDecl 0x1793ad8 <col:49> col:51 'T1'
| `-ClassTemplateSpecializationDecl 0x16faa28 <line:3:7, line:5:7> line:3:31 class B
|   `-TemplateArgument type 'int'
|     `-BuiltinType 0x1728a70 'int'
|-FunctionTemplateDecl 0x16fb050 <line:4:11, col:51> col:47 implicit <deduction guide for B>
| |-TemplateTypeParmDecl 0x17934c8 <line:3:16, col:22> col:22 referenced class depth 0 index 0 T
| |-TemplateTypeParmDecl 0x16fac88 <line:4:20, col:26> col:26 class depth 0 index 1 T1
| |-TemplateTypeParmDecl 0x16fad38 <col:30, <scratch space>:2:1> input.cc:4:39 typename depth 0 index 2
| | `-TemplateArgument type 'A<T>'
| |   `-TemplateSpecializationType 0x16fae00 'A<T>' dependent A
| |     `-TemplateArgument type 'T'
| |       `-TemplateTypeParmType 0x1793520 'T' dependent depth 0 index 0
| |         `-TemplateTypeParm 0x17934c8 'T'
| `-CXXDeductionGuideDecl 0x16faf98 <col:47, col:51> col:47 implicit <deduction guide for B> 'auto (type-parameter-0-1) -> B<T>'
|   `-ParmVarDecl 0x16faea8 <col:49> col:51 'type-parameter-0-1'
|-FunctionTemplateDecl 0x16fb268 <line:3:7, col:31> col:31 implicit <deduction guide for B>
| |-TemplateTypeParmDecl 0x17934c8 <col:16, col:22> col:22 referenced class depth 0 index 0 T
| `-CXXDeductionGuideDecl 0x16fb1b0 <col:31> col:31 implicit <deduction guide for B> 'auto (B<T>) -> B<T>'
|   `-ParmVarDecl 0x16fb148 <col:31> col:31 'B<T>'
`-FunctionTemplateDecl 0x16fb380 <line:6:7, line:7:23> col:7 <deduction guide for B>
  |-TemplateTypeParmDecl 0x16fa860 <line:6:16, col:22> col:22 referenced class depth 0 index 0 T
  `-CXXDeductionGuideDecl 0x16fb2d0 <line:7:7, col:23> col:7 <deduction guide for B> 'auto (T, T) -> B<int>'
    |-ParmVarDecl 0x16fa930 <col:9> col:10 'T'
    `-ParmVarDecl 0x16fa9a8 <col:12> col:13 'T'
0x16fb1b0,0x16faf98,0x16faf98
In file included from output.cc:1:
input.cc:4:11: warning: template parameter lists have a different number of parameters (1 vs 3) [-Wodr]
          template<class T1, typename = A<T>> B(T1);
          ^
input.cc:6:7: note: template parameter list also declared here
      template<class T>

The interesting is the printed DeclContext: The first value is the deduction guide 'auto (B<T>) -> B<T>'. Not the class declaration and not the "correct" deduction guide. I think that when the deduction guide 'auto (B<T>) -> B<T>' is created the parameters are "adopted", and after B<T> the DeclContext of T is changed too. This is the same object that was originally the template parameter of the CXXRecordDecl for class A. Probably this is a bug in the AST creation but currently it works this way.

Previously I did not see this ODR warning. It needs more investigation to check why it appears.

clang/lib/AST/ASTImporter.cpp
6081

Probably better:
In this way the DeclContext of these template parameters is not necessary the same as in the "from" context.

6082–6085

It is still good to have an explanation of why the DeclContext is not exactly preserved at import. And the DeclContext is really not "stable", not easily predictable from the source code.

clang/unittests/AST/ASTImporterTest.cpp
7336

We get the "trying to remove not contained Decl" in ASTImporterLookupTable.

7348–7353

I left this out because the "instability" mentioned above. It is possible to make the assertions for this exact code. We can better say that it comes from a set of possible values, these are the current DeductionGuideDecl, or another one for the same class, or the class itself (but I am not sure in this any more). It is possible to get different value for different template parameters of the same template.

Probably this is a bug in the AST creation but currently it works this way.

I don't think this is a bug, deduction guides have a convoluted implementation. See the related DeclContext issue with them when local typdefs are involved:
https://reviews.llvm.org/D92209

clang/lib/AST/ASTImporter.cpp
6081

okay, but necessary->necessarily

6082–6085

Okay, then we could write "Consequently, the DeclContext of these Decls may change several times until the top-level import call is finished."

clang/unittests/AST/ASTImporterTest.cpp
7336

ok.

7348–7353

So, we could have

ASSERT_NE(cast<Decl>(FromDG1->getTemplateParameters()->getParam(0)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext()));
ASSERT_NE(cast<Decl>(FromDG1->getTemplateParameters()->getParam(0)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(2)->getDeclContext()));
ASSERT_EQ(cast<Decl>(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(2)->getDeclContext()))

And then after the Import calls we could have the same expectations on ToDG1->getParam(0) and the rest. Couldn't we?

balazske added inline comments.Nov 25 2021, 6:05 AM
clang/lib/AST/ASTImporter.cpp
6082–6085

The meaning of this is still different from what my original intent was: The DeclContext in the "To" context is different from the "From", even after the top-level import call. I wanted to emphasize here that this change of DeclContext after (top-level) import has no relevance, should cause no problems.

clang/unittests/AST/ASTImporterTest.cpp
7348–7353

Probably at the "explicit" template parameters (the ones not coming from the class template) the DeclContext is always the deduction guide, so an ASSERT_EQ(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext(), FromDG1->getTemplatedDecl()) do work (for param 2 similar). I would not say that param 0 has always necessary a different DeclContext than the others, in this one case yes but not generally. This is why I do not like an assert for that. (Such an assert would not point out an invariant that is true always in the AST: An "unrelated" change in the source, for example different order of declarations, change in template-template parameters, may result in change of the DeclContext.)

martong added inline comments.Nov 25 2021, 6:34 AM
clang/lib/AST/ASTImporter.cpp
6082–6085

So, you say that is it possible that the DeclContext of the params are equal in the "from" context, but after the import we may end up having different DeclContexts for the imported params in the "to" context?

clang/unittests/AST/ASTImporterTest.cpp
7348–7353

An "unrelated" change in the source, for example different order of declarations, change in template-template parameters, may result in change of the DeclContext.)

I think having a different order of the declarations would be a different test case since the source code in the raw string literal would change.

balazske added inline comments.Nov 25 2021, 7:19 AM
clang/lib/AST/ASTImporter.cpp
6082–6085

They can be (are in the current case) not all equal in the From context and can be different in the To context (related to the corresponding AST nodes in the From context). At least the code does not ensure that these will be the same. There was originally a FIXME at this place, I wanted to change it to this new comment. Because from semantic point of view it should not matter if the DeclContext of a template parameter is some implicit deduction guide or another one, probably both are correct (this is why I don't wanted to add assertion for exactly one case).

clang/unittests/AST/ASTImporterTest.cpp
7348–7353

OK, I add the assertions for every DeclContext. If a future change (in Sema) results in different behavior the test will fail and needs to be updated. Without the assertions it will probably not fail and the test remains not updated for the new behavior.

balazske updated this revision to Diff 391035.Dec 1 2021, 8:11 AM

Improved the test.
Added another test to show effect of import in different order.
Updated the comment in ASTImporter.cpp.

The new tests produce some -Wodr warnings. The reason is that all deduction guides for the same class B are in the ASTImporterLookupTable with the same name "deduction guide for B". The message may come from the import of the function template declaration, when an existing non-matching object is found. This is only a printed message, does not cause failure.

martong accepted this revision.Dec 6 2021, 1:44 AM

LGTM! Thanks for the update!

This revision is now accepted and ready to land.Dec 6 2021, 1:44 AM