[Sema] Merge variable template specializations

[ilya-biryukov]

Summary

Clang used to produce redefinition errors, see tests for examples.

[inclyc]

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp

45 ↗

(On Diff #450283)

Missing newline

clang/test/Modules/merge-var-template-spec.cpp

71

Missing newline

[ChuanqiXu]

When I run this locally, it emits several unexpected errors:

While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:4:32: error: redefinition of 'zero'
template <class T> constexpr T zero = 0;
                               ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:4:32: note: previous definition is here
template <class T> constexpr T zero = 0;
                               ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:5:8: error: redefinition of 'Int'
struct Int {
       ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:5:8: note: previous definition is here
struct Int {
       ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:8:27: error: redefinition of 'zero<Int>'
template <> constexpr int zero<Int> = {0};
                          ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:8:27: note: previous definition is here
template <> constexpr int zero<Int> = {0};
                          ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:9:33: error: redefinition of 'zero<T *>'
template <class T> constexpr T* zero<T*> = nullptr;
                                ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:9:33: note: previous definition is here
template <class T> constexpr T* zero<T*> = nullptr;
                                ^
4 errors generated.

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp

1 ↗

(On Diff #450283)

Maybe it is better to rename this as merge-var-template-spec.cppm since we uses the suffix .cppm as a note for c++20 modules.

[ChuanqiXu]

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp

18–23 ↗

(On Diff #450283)

BTW, these errors are not expected by me since they are not in a named modules and they are in different TU with var_def.cppm.

[ilya-biryukov]

• Add the forgotten -fmodules-local-submodule-visibility flag to the test
• Add newlines to the test files
• Switch to C++14 mode for tests, they don't use C++17.

[ilya-biryukov]

In D131258#3705503, @ChuanqiXu wrote:

When I run this locally, it emits several unexpected errors:

Sorry, my bad. I was testing this with -std=c++20 and later switched to -std=c++17 as this does not have any C++20-related features.
I thought I reran the tests with -std=c++17 before sending out the patch, but apparently I did not.
The reason why this worked in C++20 mode is that C++20 implies -fcxx-modules, which imply -fmodules-local-submodule-visibility.
Without the latter flag, all declarations inside the same TU are visible, even when not imported.

I have added -fmodules-local-submodule-visibility to the tests and also included a comment explaining why we need it.
While here, also switched to C++14, which seems enough here.

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp

18–23 ↗

(On Diff #450283)

I am not sure about this one, but my understanding of the standard was that no two definitions are allowed if any is attached to a named module.
Am I reading the standard wrong here?
The relevant bits from my perspective are in {basic.def.odr}p14.3:

For any definable item D with definitions in multiple translation units,
...
- if the definitions in different translation units do not satisfy the following requirements,
the program is ill-formed; a diagnostic is required only if the definable item is attached to a named module and a prior definition is reachable at the point where a later definition occurs. 
...
  - Each such definition shall not be attached to a named module ([module.unit]).

The diagnostic seems to be required by the standard when looking at a declaration in the named module and this example is the other way around here.
However, I don't see why we can't provide a diagnostic in that case too.

[ChuanqiXu]

LGTM then.

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp

18–23 ↗

(On Diff #450283)

Oh, you are right. My bad. Sorry for wasting time.

clang/lib/Sema/SemaDecl.cpp

 }
 
 /// We've just determined that \p Old and \p New both appear to be definitions
 /// of the same variable. Either diagnose or fix the problem.
 bool Sema::checkVarDeclRedefinition(VarDecl *Old, VarDecl *New) {
   if (!hasVisibleDefinition(Old) &&
       (New->getFormalLinkage() == InternalLinkage ||
        New->isInline() ||
+       isa<VarTemplateSpecializationDecl>(New) ||
        New->getDescribedVarTemplate() ||
        New->getNumTemplateParameterLists() ||
        New->getDeclContext()->isDependentContext())) {
     // The previous definition is hidden, and multiple definitions are
     // permitted (in separate TUs). Demote this to a declaration.
     New->demoteThisDefinitionToDeclaration();
 
     // Make the canonical definition visible.

clang/test/Modules/merge-var-template-spec-cxx-modules.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/var_def.cppm -o %t/var_def.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/reexport1.cppm -o %t/reexport1.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/reexport2.cppm -o %t/reexport2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cppm -fsyntax-only -verify
+
+//--- use.cppm
+import reexport1;
+import reexport2;
+
+auto foo = zero<Int>;
+auto bar = zero<int*>;
+auto baz = zero<int>;
+
+template <class T> constexpr T zero = 0; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}}
+                                         // expected-note@* {{previous}}
+template <> constexpr Int zero<Int> = {0}; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}}
+                                           // expected-note@* {{previous}}
+template <class T> constexpr T* zero<T*> = nullptr; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}}
+                                                    // expected-note@* {{previous}}
+
+template <> constexpr int** zero<int**> = nullptr; // ok, new specialization.
+template <class T> constexpr T** zero<T**> = nullptr; // ok, new partial specilization.
+
+//--- var_def.cppm
+export module var_def;
+
+export template <class T> constexpr T zero = 0;
+export struct Int {
+    int value;
+};
+export template <> constexpr Int zero<Int> = {0};
+export template <class T> constexpr T* zero<T*> = nullptr;
+
+//--- reexport1.cppm
+export module reexport1;
+export import var_def;
+
+//--- reexport2.cppm
+export module reexport2;
+export import var_def;

clang/test/Modules/merge-var-template-spec.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// We need '-fmodules-local-submodule-visibility' to properly test merging when building a module from multiple
+// headers inside the same TU. C++20 mode would imply this flag, but we need it to set it explicitly for C++14.
+//
+// RUN: %clang_cc1 -xc++ -std=c++14 -fmodules -fmodules-local-submodule-visibility -fmodule-name=library \
+// RUN:     -emit-module %t/modules.map \
+// RUN:     -o %t/module.pcm
+//
+//
+// RUN: %clang_cc1 -xc++ -std=c++14 -fmodules -fmodules-local-submodule-visibility -fmodule-file=%t/module.pcm  \
+// RUN:     -fmodule-map-file=%t/modules.map \
+// RUN:     -fsyntax-only -verify %t/use.cpp
+//
+//--- use.cpp
+
+#include "var1.h"
+#include "var2.h"
+
+auto foo = zero<Int>;
+auto bar = zero<int*>;
+auto baz = zero<int>;
+
+template <class T> constexpr T zero = 0; // expected-error {{redefinition}} expected-note@* {{previous}}
+template <> constexpr Int zero<Int> = {0}; // expected-error {{redefinition}} expected-note@* {{previous}}
+template <class T> constexpr T* zero<T*> = nullptr; // expected-error {{redefinition}} expected-note@* {{previous}}
+
+template <> constexpr int** zero<int**> = nullptr; // ok, new specialization.
+template <class T> constexpr T** zero<T**> = nullptr; // ok, new partial specilization.
+
+//--- modules.map
+module "library" {
+  export *
+  module "var1" {
+    export *
+    header "var1.h"
+  }
+  module "var2" {
+    export *
+    header "var2.h"
+  }
+}
+
+//--- var1.h
+#ifndef VAR1_H
+#define VAR1_H
+
+template <class T> constexpr T zero = 0;
+struct Int {
+    int value;
+};
+template <> constexpr int zero<Int> = {0};
+template <class T> constexpr T* zero<T*> = nullptr;
+
+#endif // VAR1_H
+
+//--- var2.h
+#ifndef VAR2_H
+#define VAR2_H
+
+template <class T> constexpr T zero = 0;
+struct Int {
+    int value;
+};
+template <> constexpr int zero<Int> = {0};
+template <class T> constexpr T* zero<T*> = nullptr;
+
+#endif // VAR2_H