[C++20] [Modules] Don't complain about duplicated default template argument across modules

[ChuanqiXu]

Summary

See https://github.com/cplusplus/draft/pull/5204 for a detailed background.

Simply, the test redundant-template-default-arg.cpp attached to this patch should be accepted instead of being complained about the redefinition.

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2681–2697 ↗

(On Diff #402472)

while I do approve of good commenting, this is a little wordy and I think misleading. It is only from global module fragments that you can get duplicate declarations and those must match default parameters (I think). so perhaps 'IsGlobalModule'? or somehting like that.

Perhaps all of this comment shoould be at the point of checking? After all the preceding comment mentions 'variables used to ...'?

2852 ↗

(On Diff #402472)

Yes, I think this is a better place for the comment. If there are duplicates we need to check they are the same -- pedantically the same tokens, but at least AST equivalence?

[ChuanqiXu]

clang/lib/Sema/SemaTemplate.cpp

2852 ↗

(On Diff #402472)

Yeah, it should be necessary to check the ODR. I found it is not trivial to do so. It needs more work. I sent https://reviews.llvm.org/D118223 as a forward patch.

[ChuanqiXu]

Address comment to check if the duplicated default template arguments are the same and add tests to address the negative cases.

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2654–2656 ↗

(On Diff #409157)

Can this ever trigger and the below expression test not trigger, given the above type comparison?

2658–2682 ↗

(On Diff #409157)

I'm kind of surprised how complex this check is. Isn't there an AST comparator available somewhere?

[ChuanqiXu]

Use ODRHash to simplify code.

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

I found ODRHash. I think it is much better now.

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

hm that suggests there there must be a comparator too -- this isn't a cryptographically strong hash is it? How would the compiler currently make use of 'definitely different' and 'probably the same' without such a comparator?

[ChuanqiXu]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

Yeah, I am sure there is not an such comparator. Or it has some methods like: ASTContext::hasSameType for type and ASTContext::isSameEntity() for Decl. But it lacks such methods now for Stmt and Expr.

How would the compiler currently make use of 'definitely different' and 'probably the same' without such a comparator?

Now it uses the two methods I listed above and ODRHash to compare. I think the two methods works for 'definitely different' and ODRHash works for 'probably the same'. So it's the reason why my previous implementation looks lengthy. Since I want to handle it by hand. (The previous method only works for simple Expr. I think it would be large work to implement comparator for whole Expr or Stmt).

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

Hm, how do template instantations work -- there must be some way of determining 'is this instantation just here the same as one I've already seen'

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

also, the same functionality (with more generality) is needed for comparing regular function default arguments with multiple definitions in the GM. How is that done (or is it yet to be implemented?)

[ChuanqiXu]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

also, the same functionality (with more generality) is needed for comparing regular function default arguments with multiple definitions in the GM. How is that done (or is it yet to be implemented?)

We implemented ODR check in ASTReader by ODRHash.

Hm, how do template instantations work -- there must be some way of determining 'is this instantation just here the same as one I've already seen'

First, previously we don't allow the redefinition of template default argument. And for the case without template default argument, previously we would try find the definition for the synthesized type, if we could find one, use it. And if we couldn't find one, initialize one.

---

Given we already checks ODR by ODRHash in ASTReader, I think what we do here might not be bad. I agree that it is odd at the first sight to use Hash to detect difference from the perspective of CS. But if this is what we used to do, I think it is acceptable.

[urnathan]

clang/lib/Sema/SemaTemplate.cpp

2658–2682 ↗

(On Diff #409157)

indeed. I would be more comfortable with a comment by someone more familiar with this @rsmith ?

[ChuanqiXu]

@rsmith gentle ping~

[ChuanqiXu]

@urnathan It looks @rsmith is not available this time. Would you like to accept this one?

[ChuanqiXu]

@rsmith gentle ping~

[ChuanqiXu]

@rsmith gentle ping~

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

2651 ↗

(On Diff #409507)

ODR hashing is intended to produce a hash that's stable across compilations (so it can't use pointer comparisons to check types are identical, for example). For use cases that don't need a hash code that's stable across processes, using Stmt::Profile should be preferred. Can you use that here?

2720–2728 ↗

(On Diff #409507)

Please rephrase these comments to remove the "Variables used to" part and to explain instead what value is stored in the variable. (Eg: "Whether we've seen a duplicate default argument in the same translation unit.")

2762 ↗

(On Diff #409507)

This is the wrong thing to check -- whether the owning module is imported (from an AST file) is irrelevant. We might parse multiple translation units in the same compilation, and we might split a single translation unit into multiple compilations (eg with a precompiled preamble) resulting in some of the same translation unit being in an AST file. What matters is whether the declaration is in the current translation unit or a different translation unit.

I'm not sure what the best way to detect that is currently. You might be able to just compare getOwningModule() of the two declarations, but I don't think that will properly handle cases in the global module fragment, or cases where one template is in the primary module interface unit and another is in a module implementation unit of the same module.

Ideally I would like for us to create one TranslationUnitDecl per translation unit, rather than shoving all imported code into the translation unit of the importer, but that's a moderately large change, which I'd like to not block this change on if we can avoid it. But I think that's the right longer-term approach. Then here we would just need to find the lexically-enclosing TranslationUnitDecl for each template parameter and see if they're the same.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Sema/SemaTemplate.cpp

2762 ↗

(On Diff #409507)

I found we could determine whether the declaration is in the current TU or not by using Sema::ModuleScopes, which contains the modules we're currently parsing.

[ChuanqiXu]

Remove unused header ODRHash.h

[ChuanqiXu]

@rsmith @iains ping

[ChuanqiXu]

@rsmith @iains gentle ping~

[ChuanqiXu]

Add @ilya-biryukov since he did similar things in https://reviews.llvm.org/D128921

[ChuanqiXu]

Given @rsmith and @iains is busy and this revision is relatively small, innocent and looks good to me, I plan to land this in Friday in case there is no further comments.

[ChuanqiXu]

Minor changes.

[ChuanqiXu]

Minor changes.

[ChuanqiXu]

Committed in https://reviews.llvm.org/rG354a597b9f3aad2a6a37518d47351adb99688cd2

clang/test/Modules/Inputs/redundant-template-default-arg/foo.h

-template <typename T>
-T u;
-
-template <typename T = int>
-T v;
-
-template <int T = 8>
-int v2;
-
-template <typename T>
-class my_array {};
-
-template <template <typename> typename C = my_array>
-int v3;
-
-template <typename T, int *i = nullptr>
-T v4;
-
-template <typename T, T *i = nullptr>
-T v5;
-
-inline int a = 43;
-template <typename T, int *i = &a>
-T v6;
-
-inline int b = 43;
-template <typename T, T *i = &b>
-T v7;
-
-template <int T = (3 > 2)>
-int v8;
-
-consteval int getInt() {
-  return 55;
-}
-template <int T = getInt()>
-int v9;

clang/test/Modules/Inputs/redundant-template-default-arg/foo.cppm

-module;
-#include "foo.h"
-export module foo;

clang/test/Modules/Inputs/redundant-template-default-arg2/foo.cppm

-export module foo;
-export template <typename T = int>
-T v;
-
-export template <int T = 8>
-int v2;
-
-export template <typename T>
-class my_array {};
-
-export template <template <typename> typename C = my_array>
-int v3;

clang/test/Modules/Inputs/redundant-template-default-arg3/foo.h

-template <typename T = int>
-T v;
-
-template <int T = 8>
-int v2;
-
-template <typename T>
-class my_array {};
-
-template <template <typename> typename C = my_array>
-int v3;
-
-template <typename T, int *i = nullptr>
-T v4;
-
-template <typename T, T *i = nullptr>
-T v5;
-
-inline int a = 43;
-template <typename T, int *i = &a>
-T v6;
-
-inline int b = 43;
-template <typename T, T *i = &b>
-T v7;
-
-template <int T = (3 > 2)>
-int v8;
-
-consteval int getInt() {
-  return 55;
-}
-template <int T = getInt()>
-int v9;

clang/test/Modules/Inputs/redundant-template-default-arg3/foo.cppm

-module;
-#include "foo.h"
-export module foo;

clang/test/Modules/Inputs/redundant-template-default-arg3/foo_bad.h

-template <typename T = double>
-T v;
-
-template <int T = 9>
-int v2;
-
-template <typename T>
-class others_array {};
-
-template <template <typename> typename C = others_array>
-int v3;
-
-static int a;
-consteval int *getIntPtr() {
-  return &a;
-}
-template <typename T, int *i = getIntPtr()>
-T v4;
-
-consteval void *getVoidPtr() {
-  return &a;
-}
-template <typename T, T *i = getVoidPtr()>
-T v5;
-
-inline int a_ = 43;
-template <typename T, int *i = &a_>
-T v6;
-
-inline int b_ = 43;
-template <typename T, T *i = &b_>
-T v7;
-
-template <int T = -1>
-int v8;
-
-consteval int getInt2() {
-  return 55;
-}
-template <int T = getInt2()>
-int v9;

clang/test/Modules/fwd-declaration.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 --precompile %t/A.cppm -o %t/M-A.pcm
+// RUN: %clang -std=c++20 --precompile %t/B.cppm -o %t/M-B.pcm
+// RUN: %clang -std=c++20 --precompile %t/M.cppm -o %t/M.pcm -fprebuilt-module-path=%t
+// RUN: %clang -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -S -o -
+//--- fwd.h
+template <class T>
+class A;
+
+//--- impl.h
+template <class T>
+class A;
+typedef A<int> a;
+template <class T>
+class A {};
+
+template <>
+class A<char> {
+public:
+    A() {}
+
+    A(int, double) {}
+};
+
+//--- A.cppm
+module;
+#include "fwd.h"
+export module M:A;
+export using ::A;
+
+//--- B.cppm
+module;
+#include "impl.h"
+export module M:B;
+export using ::A;
+
+//--- M.cppm
+export module M;
+export import :A;
+export import :B;
+
+//--- Use.cpp
+import M;
+void use() {
+    A<int> a;
+}

clang/test/Modules/redundant-template-default-arg.cpp

 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1  -std=c++20 %S/Inputs/redundant-template-default-arg/foo.cppm -I%S/Inputs/redundant-template-default-arg/. -emit-module-interface -o %t/foo.pcm
-// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %s -I%S/Inputs/redundant-template-default-arg/. -fsyntax-only -verify
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1  -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -I%t -fsyntax-only -verify
+
+//--- foo.h
+template <typename T>
+T u;
+
+template <typename T = int>
+T v;
+
+template <int T = 8>
+int v2;
+
+template <typename T>
+class my_array {};
+
+template <template <typename> typename C = my_array>
+int v3;
+
+template <typename T, int *i = nullptr>
+T v4;
+
+template <typename T, T *i = nullptr>
+T v5;
+
+inline int a = 43;
+template <typename T, int *i = &a>
+T v6;
+
+inline int b = 43;
+template <typename T, T *i = &b>
+T v7;
+
+template <int T = (3 > 2)>
+int v8;
+
+consteval int getInt() {
+  return 55;
+}
+template <int T = getInt()>
+int v9;
+
+//--- foo.cppm
+module;
+#include "foo.h"
+export module foo;
+
+
+//--- use.cpp
 // expected-no-diagnostics
 import foo;
 #include "foo.h"

clang/test/Modules/redundant-template-default-arg2.cpp

 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1  -std=c++20 %S/Inputs/redundant-template-default-arg2/foo.cppm -I%S/Inputs/redundant-template-default-arg2/. -emit-module-interface -o %t/foo.pcm
-// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %s -fsyntax-only -verify
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1 -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -fsyntax-only -verify
+
+//--- foo.cppm
+export module foo;
+export template <typename T = int>
+T v;
+
+export template <int T = 8>
+int v2;
+
+export template <typename T>
+class my_array {};
+
+export template <template <typename> typename C = my_array>
+int v3;
+
+//--- use.cpp
 import foo;
 template <typename T = int>
 T v; // expected-error {{declaration of 'v' in the global module follows declaration in module foo}}
-     // expected-note@Inputs/redundant-template-default-arg2/foo.cppm:3 {{previous declaration is here}}
+     // [e-mail]:3 {{previous declaration is here}}
 
 template <int T = 8>
 int v2; // expected-error {{declaration of 'v2' in the global module follows declaration in module foo}}
-        // expected-note@Inputs/redundant-template-default-arg2/foo.cppm:6 {{previous declaration is here}}
+        // [e-mail]:6 {{previous declaration is here}}
 
 template <typename T>
 class my_array {}; // expected-error {{redefinition of 'my_array'}}
-                   // expected-note@Inputs/redundant-template-default-arg2/foo.cppm:9 {{previous definition is here}}
+                   // [e-mail]:9 {{previous definition is here}}
 
 template <template <typename> typename C = my_array>
 int v3; // expected-error {{declaration of 'v3' in the global module follows declaration in module foo}}
-        // expected-note@Inputs/redundant-template-default-arg2/foo.cppm:12 {{previous declaration is here}}
+        // [e-mail]:12 {{previous declaration is here}}

clang/test/Modules/redundant-template-default-arg3.cpp

 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1  -std=c++20 %S/Inputs/redundant-template-default-arg3/foo.cppm -I%S/Inputs/redundant-template-default-arg3/. -emit-module-interface -o %t/foo.pcm
-// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %s -I%S/Inputs/redundant-template-default-arg3/. -fsyntax-only -verify
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1  -std=c++20 %t/foo.cppm -I%t -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1  -fprebuilt-module-path=%t -std=c++20 %t/use.cpp -I%t/. -fsyntax-only -verify
 
+//--- foo.h
+template <typename T = int>
+T v;
+
+template <int T = 8>
+int v2;
+
+template <typename T>
+class my_array {};
+
+template <template <typename> typename C = my_array>
+int v3;
+
+template <typename T, int *i = nullptr>
+T v4;
+
+template <typename T, T *i = nullptr>
+T v5;
+
+inline int a = 43;
+template <typename T, int *i = &a>
+T v6;
+
+inline int b = 43;
+template <typename T, T *i = &b>
+T v7;
+
+template <int T = (3 > 2)>
+int v8;
+
+consteval int getInt() {
+  return 55;
+}
+template <int T = getInt()>
+int v9;
+
+//--- foo_bad.h
+template <typename T = double>
+T v;
+
+template <int T = 9>
+int v2;
+
+template <typename T>
+class others_array {};
+
+template <template <typename> typename C = others_array>
+int v3;
+
+static int a;
+consteval int *getIntPtr() {
+  return &a;
+}
+template <typename T, int *i = getIntPtr()>
+T v4;
+
+consteval void *getVoidPtr() {
+  return &a;
+}
+template <typename T, T *i = getVoidPtr()>
+T v5;
+
+inline int a_ = 43;
+template <typename T, int *i = &a_>
+T v6;
+
+inline int b_ = 43;
+template <typename T, T *i = &b_>
+T v7;
+
+template <int T = -1>
+int v8;
+
+consteval int getInt2() {
+  return 55;
+}
+template <int T = getInt2()>
+int v9;
+
+//--- foo.cppm
+module;
+#include "foo.h"
+export module foo;
+
+//--- use.cpp
 import foo;
 #include "foo_bad.h"
 
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:1 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:1 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:4 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:4 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:10 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:10 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:17 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:13 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:23 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:16 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:27 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:20 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:31 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:24 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:34 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:27 {{previous default template argument defined in module foo.<global>}}
-// expected-error@Inputs/redundant-template-default-arg3/foo_bad.h:40 {{template parameter default argument is inconsistent with previous definition}}
-// expected-note@Inputs/redundant-template-default-arg3/foo.h:33 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:1 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:1 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:4 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:4 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:10 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:10 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:17 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:13 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:23 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:16 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:27 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:20 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:31 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:24 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:34 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:27 {{previous default template argument defined in module foo.<global>}}
+// expected-error@foo_bad.h:40 {{template parameter default argument is inconsistent with previous definition}}
+// expected-note@foo.h:33 {{previous default template argument defined in module foo.<global>}}