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.

Sign up for GitHub

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

Jump to bottom

[clang][Modules] Import then include fails to merge inline lambdas #102721

Closed
davidstone opened this issue Aug 10, 2024 · 1 comment · Fixed by #106483
Closed

[clang][Modules] Import then include fails to merge inline lambdas #102721

davidstone opened this issue Aug 10, 2024 · 1 comment · Fixed by #106483
Assignees
@ChuanqiXu9
Labels
clang:modules C++20 modules and Clang Header Modules

Comments

@davidstone
Copy link
Contributor

Given the following valid translation units:

module;

inline auto x = []{};

export module a;
module;

import a;

inline auto x = []{};

export module b;

clang erroneously rejects with

b.cpp:5:13: error: redeclaration of 'x' with a different type: '(lambda at /app/b.cpp:5:17)' vs '(lambda at /app/a.cpp:3:17)'
    5 | inline auto x = []{};
      |             ^
a.cpp:3:13: note: previous definition is here
    3 | inline auto x = []{};
      |             ^
1 error generated.

See it live: https://godbolt.org/z/WxW47zW4v

This is the reduced version of the following reproducer:

module;

#include <compare>

export module a;
module;

import a;
#include <compare>

export module b;

in which clang with libc++ erroneously rejects with

In file included from /app/b.cpp:4:
In file included from /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/compare:156:
/opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:45: error: redeclaration of '__synth_three_way' with a different type: 'const std::__1::(lambda at /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:65)' vs 'const std::__1::(lambda at /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:65)'
   28 | _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way = []<class _Tp, class _Up>(const _Tp& __t, const _Up& __u)
      |                                             ^
/opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:45: note: previous definition is here
   28 | _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way = []<class _Tp, class _Up>(const _Tp& __t, const _Up& __u)
      |                                             ^
1 error generated.

See it live: https://godbolt.org/z/4dK717sr4

Looks like the library side of it was reintroduced by d043e4c @H-G-Hristov, @Zingam but I suspect that the clang side of it was never fully fixed. #57222 @ChuanqiXu9 shows a unit test for two modules that just include a header with an inline lambda, but this case of import module that included, followed by include appears to have slipped through.
@github-actions github-actions bot added the clang Clang issues not falling into any other category label Aug 10, 2024
@EugeneZelenko EugeneZelenko added clang:modules C++20 modules and Clang Header Modules and removed clang Clang issues not falling into any other category labels Aug 10, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 10, 2024

@llvm/issue-subscribers-clang-modules

Author: David Stone (davidstone)

Given the following valid translation units: 
module;

inline auto x = []{};

export module a;
module;

import a;

inline auto x = []{};

export module b;

clang erroneously rejects with

b.cpp:5:13: error: redeclaration of 'x' with a different type: '(lambda at /app/b.cpp:5:17)' vs '(lambda at /app/a.cpp:3:17)'
    5 | inline auto x = []{};
      |             ^
a.cpp:3:13: note: previous definition is here
    3 | inline auto x = []{};
      |             ^
1 error generated.

See it live: https://godbolt.org/z/WxW47zW4v

This is the reduced version of the following reproducer:

module;

#include &lt;compare&gt;

export module a;
module;

import a;
#include &lt;compare&gt;

export module b;

in which clang with libc++ erroneously rejects with

In file included from /app/b.cpp:4:
In file included from /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/compare:156:
/opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:45: error: redeclaration of '__synth_three_way' with a different type: 'const std::__1::(lambda at /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:65)' vs 'const std::__1::(lambda at /opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:65)'
   28 | _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way = []&lt;class _Tp, class _Up&gt;(const _Tp&amp; __t, const _Up&amp; __u)
      |                                             ^
/opt/compiler-explorer/clang-assertions-trunk-20240809/bin/../include/c++/v1/__compare/synth_three_way.h:28:45: note: previous definition is here
   28 | _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way = []&lt;class _Tp, class _Up&gt;(const _Tp&amp; __t, const _Up&amp; __u)
      |                                             ^
1 error generated.

See it live: https://godbolt.org/z/4dK717sr4

Looks like the library side of it was reintroduced by d043e4c @H-G-Hristov, @Zingam but I suspect that the clang side of it was never fully fixed. #57222 @ChuanqiXu9 shows a unit test for two modules that just include a header with an inline lambda, but this case of import module that included, followed by include appears to have slipped through.

@ChuanqiXu9 ChuanqiXu9 self-assigned this Aug 12, 2024
ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this issue Aug 13, 2024
@ChuanqiXu9
[C++20] [Modules] Merge lambdas from Sema to imported one
f7610cd 
Close llvm#102721

Previously we tried to merge lambdas between TUs. But we may still meet
problems if we import and then include the headers containing the same
lambda.

The solution in this patch is, when we assigned a lambda numbering in a
sema, try to see if there is already a same lambda imported. If yes, try
to merge them.
This was referenced Aug 13, 2024
Closed
Merged
@ChuanqiXu9 ChuanqiXu9 mentioned this issue Aug 29, 2024
Merged
ChuanqiXu9 added a commit that referenced this issue Aug 29, 2024
@ChuanqiXu9
[C++20] [Modules] Merge lambdas in source to imported lambdas (#106483)
55cdb3c 
Close #102721

Generally, the type of merged decls will be reused in ASTContext. But
for lambda, in the import and then include case, we can't decide its
previous decl in the imported modules so that we can't assign the
previous decl before creating the type for it. Since we can't decide its
numbering before creating it. So we have to assign the previous decl and
the canonical type for it after creating it, which is unusual and
slightly hack.
5c4lar pushed a commit to 5c4lar/llvm-project that referenced this issue Aug 29, 2024
@ChuanqiXu9 @5c4lar
[C++20] [Modules] Merge lambdas in source to imported lambdas (llvm#1…
2045e86 
…06483)

Close llvm#102721

Generally, the type of merged decls will be reused in ASTContext. But
for lambda, in the import and then include case, we can't decide its
previous decl in the imported modules so that we can't assign the
previous decl before creating the type for it. Since we can't decide its
numbering before creating it. So we have to assign the previous decl and
the canonical type for it after creating it, which is unusual and
slightly hack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ChuanqiXu9 ChuanqiXu9

Labels
clang:modules C++20 modules and Clang Header Modules
Projects
None yet
Milestone
No milestone
4 participants
@davidstone @EugeneZelenko @llvmbot @ChuanqiXu9