[PATCH] D114418: [clang][ASTImporter] Update lookup table correctly at deduction guides.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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.



================
Comment at: clang/lib/AST/ASTImporter.cpp:6081
+  // FunctionTemplateDecl objects are created, but in different order. In this
+  // way DeclContext of these template parameters may change relative to the
+  // "from" context. Because these DeclContext values look already not stable
----------------
martong wrote:
> ?
Probably better:
In this way the DeclContext of these template parameters is not necessary the same as in the "from" context.


================
Comment at: clang/lib/AST/ASTImporter.cpp:6082-6085
+  // "from" context. Because these DeclContext values look already not stable
+  // and unimportant this change looks acceptable.
+  // For these reasons the old DeclContext must be saved to change the lookup
+  // table later.
----------------
martong wrote:
> 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...
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.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:7336
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuide) {
+  TranslationUnitDecl *FromTU = getTuDecl(
----------------
martong wrote:
> Does this test provide an assertion failure in the base?
We get the "trying to remove not contained Decl" in ASTImporterLookupTable.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:7348-7353
+  // Get the implicit deduction guide for (non-default) constructor of 'B'.
+  auto *FromDG1 = FirstDeclMatcher<FunctionTemplateDecl>().match(
+      FromTU, functionTemplateDecl(templateParameterCountIs(3)));
+  // User defined deduction guide.
+  auto *FromDG2 = FirstDeclMatcher<CXXDeductionGuideDecl>().match(
+      FromTU, cxxDeductionGuideDecl(unless(isImplicit())));
----------------
martong wrote:
> Could you please formulate expectations (assertions) on the DeclContext's of the two template parameters? I'd expect them to be different.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114418/new/

https://reviews.llvm.org/D114418