Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++20] [Modules] <ranges> cannot be included in more than one module #61317

Closed
camio opened this issue Mar 10, 2023 · 6 comments
Closed

[C++20] [Modules] <ranges> cannot be included in more than one module #61317

camio opened this issue Mar 10, 2023 · 6 comments
Assignees
Labels
clang:modules C++20 modules and Clang Header Modules

Comments

@camio
Copy link

camio commented Mar 10, 2023

// A.cppm
module;
#include <ranges>
export module A;
// B.cppm
module;
#include <ranges>
export module B;
import A;
clang++ -fprebuilt-module-path=. -std=c++20  --precompile A.cppm -o A.pcm
clang++ -fprebuilt-module-path=. -std=c++20  --precompile B.cppm -o B.pcm

Compilation of the second file results in several errors like this

In module 'A' imported from B.cppm:6:
/usr/sbin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/ranges:1258:2: error: 'std::ranges::views::_All::operator()' from module 'A.<global>' is not present in definition of 'std::ranges::views::_All' provided earlier
        operator() [[nodiscard]] (_Range&& __r) const
        ^
/usr/sbin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/ranges:1258:2: note: declaration of 'operator()' does not match
        operator() [[nodiscard]] (_Range&& __r) const
        ^

This is using Clang trunk and libstdc++ that is distributed with GCC 12.2.1.

@EugeneZelenko EugeneZelenko added clang:modules C++20 modules and Clang Header Modules and removed new issue labels Mar 10, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 10, 2023

@llvm/issue-subscribers-clang-modules

@ChuanqiXu9
Copy link
Member

Thanks for reporting this! Would you like to reduce the defect report? It will be pretty helpful.

We can reduce the defect report by:

  1. preprocess <ranges> to a local header my_ranges.
  2. include my_ranges in the reproducer.
  3. Remove unnecessary things in my_ranges until we can't make it any more.

@camio
Copy link
Author

camio commented Mar 11, 2023

This reduced version reproduces the issue.

// A.cppm
module;
#include "foo.h"
export module A;
// B.cppm
module;
#include "foo.h"
export module B;
import A;
// foo.h
#ifndef _FOO
#define _FOO

template <typename T> struct Foo {
  Foo(T) {}
};

template <typename T> Foo(T &) -> Foo<T>;

struct Bar {
  template <typename T>
    requires requires { Foo{T()}; }
  void baz() const {}
};

#endif

The issue seems to be related to the deduction guide. If the requires clause is changed to Foo<T>{T()};, the issue appears to go away.

@davidstone
Copy link
Contributor

davidstone commented Mar 11, 2023

This feels a lot like #60486. Note in particular "The practical effect of this bug is that #include <ranges> fails when used in two modules with libstdc++."

@ChuanqiXu9 ChuanqiXu9 self-assigned this Mar 14, 2023
@camio
Copy link
Author

camio commented Mar 14, 2023

Thank you!

camio added a commit to camio/coroutines-experiment that referenced this issue Mar 16, 2023
Clang issue [#61317](llvm/llvm-project#61317)
being fixed removed the need for a stdlibc++ patch for this project.
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this issue Mar 17, 2023
Close llvm/llvm-project#61317

The root cause of the problem is that we profile TemplateName by the
non-canonical decls so that the compiler thought they are two different
types. But this is not true. We fixed the issue after we profile the
template name by using the same name.
agozillon pushed a commit to ROCm-Developer-Tools/llvm-project that referenced this issue Mar 17, 2023
Close llvm/llvm-project#61317

The root cause of the problem is that we profile TemplateName by the
non-canonical decls so that the compiler thought they are two different
types. But this is not true. We fixed the issue after we profile the
template name by using the same name.
eymay pushed a commit to eymay/llvm-project that referenced this issue Mar 21, 2023
Close llvm#61317

The root cause of the problem is that we profile TemplateName by the
non-canonical decls so that the compiler thought they are two different
types. But this is not true. We fixed the issue after we profile the
template name by using the same name.
@mizvekov
Copy link
Contributor

I have come across this change while working on another problem.

The fix which was committed is not correct : The profile function is supposed to support structural comparison, not referential comparison. It's even inconsistent that it would arbitrarily pick referential comparisons for 'TemplateDecl' names, but pick structural for everything else.

This can't be the real solution: When we build canonical types, we start with canonical components, and profiling these still does the right thing, because for canonical stuff, structural and referential comparisons are equivalent.

This fix causes issues where TemplateNames would become canonicalized on the first spelling appearing in source, and their spellings in diagnostics becoming inconsistent.

For example, some libcxx tests emitting diagnostics with different spellings depending on whether they were built with modules or not.

mizvekov added a commit that referenced this issue Jun 12, 2024
This reverts the functional elements of commit 3e78fa8
and redoes it, by fixing the true root cause of #61317.

A TemplateName can be non-canonical; profiling it based on the canonical
name would result in inconsistent preservation of as-written information
in the AST.

The true problem in #61317 is that we would not consider the methods
with requirements expression which contain DTSTs as the same
in relation to merging of declarations when importing modules.

The expressions would never match because they contained DTSTs pointing
to different redeclarations of the same class template, but since
canonicalization for them was broken, their canonical types would not
match either.

---

No changelog entry because #61317 was already claimed as fixed in
previous release.
mizvekov added a commit that referenced this issue Jun 12, 2024
This reverts the functional elements of commit 3e78fa8
and redoes it, by fixing the true root cause of #61317.

A TemplateName can be non-canonical; profiling it based on the canonical
name would result in inconsistent preservation of as-written information
in the AST.

The true problem in #61317 is that we would not consider the methods
with requirements expression which contain DTSTs as the same
in relation to merging of declarations when importing modules.

The expressions would never match because they contained DTSTs pointing
to different redeclarations of the same class template, but since
canonicalization for them was broken, their canonical types would not
match either.

---

No changelog entry because #61317 was already claimed as fixed in
previous release.
mizvekov added a commit that referenced this issue Jun 12, 2024
This reverts the functional elements of commit 3e78fa8
and redoes it, by fixing the true root cause of #61317.

A TemplateName can be non-canonical; profiling it based on the canonical
name would result in inconsistent preservation of as-written information
in the AST.

The true problem in #61317 is that we would not consider the methods
with requirements expression which contain DTSTs as the same
in relation to merging of declarations when importing modules.

The expressions would never match because they contained DTSTs pointing
to different redeclarations of the same class template, but since
canonicalization for them was broken, their canonical types would not
match either.

---

No changelog entry because #61317 was already claimed as fixed in
previous release.
mizvekov added a commit that referenced this issue Jun 13, 2024
This reverts the functional elements of commit 3e78fa8
and redoes it, by fixing the true root cause of #61317.

A TemplateName can be non-canonical; profiling it based on the canonical
name would result in inconsistent preservation of as-written information
in the AST.

The true problem in #61317 is that we would not consider the methods
with requirements expression which contain DTSTs as the same
in relation to merging of declarations when importing modules.

The expressions would never match because they contained DTSTs pointing
to different redeclarations of the same class template, but since
canonicalization for them was broken, their canonical types would not
match either.

---

No changelog entry because #61317 was already claimed as fixed in
previous release.
mizvekov added a commit that referenced this issue Jun 13, 2024
…ype (#95202)

This reverts the functional elements of commit
3e78fa8 and redoes it, by fixing the
true root cause of #61317.

A TemplateName can be non-canonical; profiling it based on the canonical
name would result in inconsistent preservation of as-written information
in the AST.

The true problem in #61317 is that we would not consider the methods
with requirements expression which contain DTSTs as the same in relation
to merging of declarations when importing modules.

The expressions would never match because they contained DTSTs pointing
to different redeclarations of the same class template, but since
canonicalization for them was broken, their canonical types would not
match either.

---

No changelog entry because #61317 was already claimed as fixed in
previous release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules
Projects
None yet
Development

No branches or pull requests

6 participants