[ODRHash] Fix ODR hashing of template names

[Hahnfeld]

Summary

For hashing, we should not differentiate between templates and qualified
templates and only look at the underlying name. As an example, consider
the added test case of a template template parameter; the two modules
have slightly differing using declarations (using C = B<A> vs B<NS::A>)
which should be treated as identical.

[ChuanqiXu]

This looks not so good. In this way, we can't disambiguate other template types. At least we added the kind and a TODO here.

BTW, the attached test case is in fact in correct. See https://eel.is/c++draft/basic.def.odr#14.3, such mergeable declarations shouldn't be attached to named modules. (And this is also a defect that now we don't diagnose it correctly).

Even if you put them into the global module fragment, I think we should try to look other places like isSameEntity to solve this.

[Hahnfeld]

In D153003#4423926, @ChuanqiXu wrote:

This looks not so good. In this way, we can't disambiguate other template types. At least we added the kind and a TODO here.

Which template name types would we need to disambiguate? We could also normalize the Kind, for example from QualifiedTemplate to Template (which is the case I care about).

BTW, the attached test case is in fact in correct. See https://eel.is/c++draft/basic.def.odr#14.3, such mergeable declarations shouldn't be attached to named modules. (And this is also a defect that now we don't diagnose it correctly).

I'm not sure I understand, could you elaborate? "correct" as in "the merge error is right" or "should work as written". Also there are a number of export struct in the tests, but you are saying this should not be included in the modules? How would this work?

Even if you put them into the global module fragment, I think we should try to look other places like isSameEntity to solve this.

Sure, but this first requires the hash to be identical, no? My understanding is that two (structurally) identical Decls must always hash to the same value, but the same value does not imply that isSameEntity and friends eventually agree (due to hash collisions).

[ChuanqiXu]

In D153003#4424004, @Hahnfeld wrote:

In D153003#4423926, @ChuanqiXu wrote:

This looks not so good. In this way, we can't disambiguate other template types. At least we added the kind and a TODO here.

Which template name types would we need to disambiguate? We could also normalize the Kind, for example from QualifiedTemplate to Template (which is the case I care about).

For example, the template name with QualifiedTemplate kind has different hash name with the name with DependentTemplate kind. But it is not true after the patch.

BTW, the attached test case is in fact in correct. See https://eel.is/c++draft/basic.def.odr#14.3, such mergeable declarations shouldn't be attached to named modules. (And this is also a defect that now we don't diagnose it correctly).

I'm not sure I understand, could you elaborate? "correct" as in "the merge error is right" or "should work as written". Also there are a number of export struct in the tests, but you are saying this should not be included in the modules? How would this work?

I mean the error is correct. The test case itself is invalid. We shouldn't declare it as expected-no-diagnostics. The link above says the reason. The same declaration in multiple module purview is invalid.

Even if you put them into the global module fragment, I think we should try to look other places like isSameEntity to solve this.

Sure, but this first requires the hash to be identical, no? My understanding is that two (structurally) identical Decls must always hash to the same value, but the same value does not imply that isSameEntity and friends eventually agree (due to hash collisions).

Yeah, it will be better to be identical. I think the major problem is that the patch misses information instead of adding more information.

, but the same value does not imply that isSameEntity ...

No. IsSameEntity is a weaker check. It simply checks whether two named decls refers to the same name.

[Hahnfeld]

For example, the template name with QualifiedTemplate kind has different hash name with the name with DependentTemplate kind. But it is not true after the patch.

Yes, I do understand. However, what I'm asking is why we need to differentiate between the two, ie in which cases do we want these two to have a different hash?

No. IsSameEntity is a weaker check. It simply checks whether two named decls refers to the same name.

I'm confused: Hashing always works the way that two "identical" objects hash to the same value, while two "non-identical" objects may also hash to the same value (hash collision). The way I understand your statement is that two "identical" Decls, including isSame returning true, can have a different hashes. This doesn't make sense to me, could you maybe clarify?

[ChuanqiXu]

what I'm asking is why we need to differentiate between the two, ie in which cases do we want these two to have a different hash?

I don't understand. It should a basic requirement for different AST entities to have different values.

The way I understand your statement is that two "identical" Decls, including isSame returning true, can have a different hashes.

I mean when we can't work on ODRHash properly, we can choose that as a workaround.

An important node here is that ODRHash is used to check the AST Nodes are keeping the same across compilations. There is gap to use ODRHash to check the semantical equality.

---

Also I find I missed an important point. The standard says https://eel.is/c++draft/basic.def.odr#14.4. So that the example is not valid even if they are in global module fragments since they don't have same token sequences.

[Hahnfeld]

In D153003#4427412, @ChuanqiXu wrote:

An important node here is that ODRHash is used to check the AST Nodes are keeping the same across compilations. There is gap to use ODRHash to check the semantical equality.

Oh ok... I'm coming from the context of https://reviews.llvm.org/D41416 which uses ODRHashes to decide which Decls to load. The problem at hand is that the hash in the module was computed from a TemplateName::Template while the instantiating context has a fully qualified template name. I was thinking to "solve" this by making them hash to the same value, but I'm hearing that using ODRHash for this purpose is the wrong approach to begin with?

[ChuanqiXu]

I was thinking to "solve" this by making them hash to the same value, but I'm hearing that using ODRHash for this purpose is the wrong approach to begin with?

We can't say this abstractly. It should be fine to work in ODRHash for C++20 modules issues as long as we don't lose informations. And this was the major issue of the patch. (Now the major issue is that the case shouldn't be accepted.)

[Hahnfeld]

We can't say this abstractly. It should be fine to work in ODRHash for C++20 modules issues as long as we don't lose informations.

I honestly don't understand this: For the approach to work, we would need to hash the two cases of an unqualified and a qualified template to the same value. You oppose to this approach because it would lose information. I can only conclude that the approach doesn't work with ODRHash.

[ChuanqiXu]

In D153003#4427731, @Hahnfeld wrote:

We can't say this abstractly. It should be fine to work in ODRHash for C++20 modules issues as long as we don't lose informations.

I honestly don't understand this: For the approach to work, we would need to hash the two cases of an unqualified and a qualified template to the same value. You oppose to this approach because it would lose information. I can only conclude that the approach doesn't work with ODRHash.

I mean: for this case we couldn't solve it by ODRHash. But for other cases, it may be still good to solve the issues by improving ODRHash.

[Hahnfeld]

Ok, I understand that fixing ODRHash is the wrong approach for our needs - we'll likely need to implement a custom hashing of template arguments to work as a lookup for lazy loading.

[ChuanqiXu]

Yeah, but I feel the most important problem for the patch is that the reproducer is not valid according to the wording.

[rsmith]

I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

Two template-ids are the same if [...] their corresponding template template-arguments refer to the same template.

so B<A> and B<NS::A> are the same type. The stricter "same sequence of tokens" rule doesn't apply here, because using-declarations are not definitions.

But that also suggests that ODR hashing should not be visiting these template arguments in using-declarations at all, because the ODR does not apply to the types specified in a namespace-scope using-declaration. How to we even get into the ODR hasher here? I thought we only applied it to function and class definitions (to which the ODR does apply).

[ChuanqiXu]

In D153003#4456595, @rsmith wrote:

I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

Two template-ids are the same if [...] their corresponding template template-arguments refer to the same template.

so B<A> and B<NS::A> are the same type. The stricter "same sequence of tokens" rule doesn't apply here, because using-declarations are not definitions.

Got it. Thanks for your commenting. I can't reopen this page. So I file an issue here https://github.com/llvm/llvm-project/issues/63595. @Hahnfeld you can still work on this by following the @rsmith 's suggestion if you're still interested. Or I'd like to take it.

But that also suggests that ODR hashing should not be visiting these template arguments in using-declarations at all, because the ODR does not apply to the types specified in a namespace-scope using-declaration. How to we even get into the ODR hasher here? I thought we only applied it to function and class definitions (to which the ODR does apply).

I think this comes from we add ODRHash for RecordDecl. This is landed in https://reviews.llvm.org/D71734. Do you have suggestion on this? e.g., only limit this for C and Objective-C? So that we can solve the issue naturally.

[Hahnfeld]

In D153003#4458323, @ChuanqiXu wrote:

In D153003#4456595, @rsmith wrote:

I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

Two template-ids are the same if [...] their corresponding template template-arguments refer to the same template.

so B<A> and B<NS::A> are the same type. The stricter "same sequence of tokens" rule doesn't apply here, because using-declarations are not definitions.

Got it. Thanks for your commenting. I can't reopen this page. So I file an issue here https://github.com/llvm/llvm-project/issues/63595. @Hahnfeld you can still work on this by following the @rsmith 's suggestion if you're still interested. Or I'd like to take it.

You mean re-open this review? Which suggestions exactly?

[ChuanqiXu]

You mean re-open this review?

Not exactly. I feel it may be better to create a new patch if I understand @rsmith correctly.

In D153003#4458366, @Hahnfeld wrote:

In D153003#4458323, @ChuanqiXu wrote:

In D153003#4456595, @rsmith wrote:

I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

Two template-ids are the same if [...] their corresponding template template-arguments refer to the same template.

so B<A> and B<NS::A> are the same type. The stricter "same sequence of tokens" rule doesn't apply here, because using-declarations are not definitions.

Got it. Thanks for your commenting. I can't reopen this page. So I file an issue here https://github.com/llvm/llvm-project/issues/63595. @Hahnfeld you can still work on this by following the @rsmith 's suggestion if you're still interested. Or I'd like to take it.

You mean re-open this review? Which suggestions exactly?

It should be:

But that also suggests that ODR hashing should not be visiting these template arguments in using-declarations at all, because the ODR does not apply to the types specified in a namespace-scope using-declaration.

[rsmith]

In D153003#4458323, @ChuanqiXu wrote:

In D153003#4456595, @rsmith wrote:

How to we even get into the ODR hasher here? I thought we only applied it to function and class definitions (to which the ODR does apply).

I think this comes from we add ODRHash for RecordDecl.

The using-declarations in the testcase aren't inside RecordDecls.

Oh, I guess we're somehow adding a semi-resolved form of the base class type of D into the ODR hash, which then includes details of the using-declaration. That seems wrong -- we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

[ChuanqiXu]

Oh, I guess we're somehow adding a semi-resolved form of the base class type of D into the ODR hash, which then includes details of the using-declaration. That seems wrong -- we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

Got it. I'll try to fix it. Thanks for the suggestion.

[v.g.vassilev]

In D153003#4462388, @ChuanqiXu wrote:

Oh, I guess we're somehow adding a semi-resolved form of the base class type of D into the ODR hash, which then includes details of the using-declaration. That seems wrong -- we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

Got it. I'll try to fix it. Thanks for the suggestion.

Thanks @rsmith for the differential diagnosis!

@ChuanqiXu, could you add me and @Hahnfeld in the loop as that's critical for us.

[ChuanqiXu]

In D153003#4463426, @v.g.vassilev wrote:

In D153003#4462388, @ChuanqiXu wrote:

Oh, I guess we're somehow adding a semi-resolved form of the base class type of D into the ODR hash, which then includes details of the using-declaration. That seems wrong -- we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

Got it. I'll try to fix it. Thanks for the suggestion.

Thanks @rsmith for the differential diagnosis!

@ChuanqiXu, could you add me and @Hahnfeld in the loop as that's critical for us.

Got it. No problem : )

clang/lib/AST/ODRHash.cpp

case NestedNameSpecifier::TypeSpecWithTemplate:
     break;
   case NestedNameSpecifier::Global:
   case NestedNameSpecifier::Super:
     break;
   }
 }
 
 void ODRHash::AddTemplateName(TemplateName Name) {
-  auto Kind = Name.getKind();
-  ID.AddInteger(Kind);
-
-  switch (Kind) {
-  case TemplateName::Template:
-    AddDecl(Name.getAsTemplateDecl());
-    break;
-  // TODO: Support these cases.
-  case TemplateName::OverloadedTemplate:
-  case TemplateName::AssumedTemplate:
-  case TemplateName::QualifiedTemplate:
-  case TemplateName::DependentTemplate:
-  case TemplateName::SubstTemplateTemplateParm:
-  case TemplateName::SubstTemplateTemplateParmPack:
-  case TemplateName::UsingTemplate:
-    break;
-  }
+  if (auto *TD = Name.getAsTemplateDecl())
+    AddDecl(TD);
 }
 
 void ODRHash::AddTemplateArgument(TemplateArgument TA) {
   const auto Kind = TA.getKind();
   ID.AddInteger(Kind);
 
   switch (Kind) {
     case TemplateArgument::Null:

clang/test/Modules/merge-template-template-parameters.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/module1.cppm -o %t/module1.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/module2.cppm -o %t/module2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/merge.cpp -verify -fsyntax-only
+
+//--- header.h
+namespace NS {
+template <int I>
+class A {
+};
+
+template <template <int I_> class T>
+class B {
+};
+}
+
+//--- module1.cppm
+// inside NS, using C = B<A>
+module;
+export module module1;
+#include "header.h"
+namespace NS {
+using C = B<A>;
+}
+export struct D : NS::C {};
+
+//--- module2.cppm
+// inside NS, using C = B<NS::A>
+module;
+export module module2;
+#include "header.h"
+namespace NS {
+using C = B<NS::A>;
+}
+export struct D : NS::C {};
+
+//--- merge.cpp
+// expected-no-diagnostics
+import module1;
+import module2;
+D d;