llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.24k stars 11.66k forks source link

[C++20] [Module] lambda in initializer of non-inline variable in named modules is internal to the TU incorrectly #110146

Open stripe2933 opened 4 days ago

stripe2933 commented 4 days ago

Following code works with Clang 19 + libc++.

main.cpp

import std;

#define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)

namespace ranges::views {
    constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
}

int main() {
    auto opts = { std::optional { 1 }, std::optional { 2 } };
    std::println("{}", opts | ranges::views::deref); // [1, 2]
}

But this does not:

main.cpp

import std;
import ranges;

int main() {
    auto opts = { std::optional { 1 }, std::optional { 2 } };
    std::println("{}", opts | ranges::views::deref);
}

ranges.cppm

export module ranges;

import std;

#define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)

namespace ranges::views {
    export constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
}
====================[ Build | hello | macos-clang-19 ]==========================
/opt/homebrew/Cellar/cmake/3.30.0/bin/cmake --build /Users/stripe2933/Desktop/hello/build/macos-clang-19 --target hello -j 6
[3/4] Building CXX object CMakeFiles/hello.dir/main.cpp.o
FAILED: CMakeFiles/hello.dir/main.cpp.o 
/opt/homebrew/opt/llvm@19/bin/clang++   -nostdinc++ -nostdlib++ -isystem /opt/homebrew/opt/llvm@19/include/c++/v1 -fexperimental-modules-reduced-bmi -std=gnu++23 -arch arm64 -isysroot /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.0.sdk -fcolor-diagnostics -MD -MT CMakeFiles/hello.dir/main.cpp.o -MF CMakeFiles/hello.dir/main.cpp.o.d @CMakeFiles/hello.dir/main.cpp.o.modmap -o CMakeFiles/hello.dir/main.cpp.o -c /Users/stripe2933/Desktop/hello/main.cpp
/Users/stripe2933/Desktop/hello/main.cpp:6:29: error: invalid operands to binary expression ('std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') and 'const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>' (aka 'const std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>'))
    6 |     std::println("{}", opts | ranges::views::deref);
      |                        ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm@19/include/c++/v1/cstddef:73:45: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'byte' for 1st argument
   73 | _LIBCPP_HIDE_FROM_ABI inline constexpr byte operator|(byte __lhs, byte __rhs) noexcept {
      |                                             ^         ~~~~~~~~~~
/opt/homebrew/opt/llvm@19/include/c++/v1/__charconv/chars_format.h:34:53: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'chars_format' for 1st argument
   34 | inline _LIBCPP_HIDE_FROM_ABI constexpr chars_format operator|(chars_format __x, chars_format __y) {
      |                                                     ^         ~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm@19/include/c++/v1/future:431:47: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'launch' for 1st argument
  431 | inline _LIBCPP_HIDE_FROM_ABI constexpr launch operator|(launch __x, launch __y) {
      |                                               ^         ~~~~~~~~~~
/opt/homebrew/opt/llvm@19/include/c++/v1/bitset:932:1: note: candidate template ignored: could not match 'bitset' against 'std::initializer_list'
  932 | operator|(const bitset<_Size>& __x, const bitset<_Size>& __y) _NOEXCEPT {
      | ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:71:1: note: candidate template ignored: constraints not satisfied [with _Range = std::initializer_list<std::optional<int>> &, _Closure = const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>> &]
   71 | operator|(_Range&& __range, _Closure&& __closure) noexcept(is_nothrow_invocable_v<_Closure, _Range>) {
      | ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:69:12: note: because 'invocable<const std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)> > > &, std::initializer_list<std::__1::optional<int> > &>' evaluated to false
   69 |   requires invocable<_Closure, _Range>
      |            ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__concepts/invocable.h:28:3: note: because 'std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...)' would be invalid: no matching function for call to 'invoke'
   28 |   std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...); // not required to be equality preserving
      |   ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:77:52: note: candidate template ignored: constraints not satisfied [with _Closure = std::initializer_list<std::optional<int>> &, _OtherClosure = const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>> &]
   77 | [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2) noexcept(
      |                                                    ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:75:11: note: because 'std::initializer_list<std::__1::optional<int>> &' does not satisfy '_RangeAdaptorClosure'
   75 | template <_RangeAdaptorClosure _Closure, _RangeAdaptorClosure _OtherClosure>
      |           ^
/opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:62:32: note: because '!ranges::range<remove_cvref_t<initializer_list<optional<int> > &> >' evaluated to false
   62 | concept _RangeAdaptorClosure = !ranges::range<remove_cvref_t<_Tp>> && requires {
      |                                ^
/opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2858:1: note: candidate template ignored: requirement '__is_val_expr<std::initializer_list<std::__1::optional<int>>>::value' was not satisfied [with _Expr1 = std::initializer_list<std::optional<int>>, _Expr2 = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>]
 2858 | operator|(const _Expr1& __x, const _Expr2& __y) {
      | ^
/opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2867:1: note: candidate template ignored: requirement '__is_val_expr<std::initializer_list<std::__1::optional<int>>>::value' was not satisfied [with _Expr = std::initializer_list<std::optional<int>>]
 2867 | operator|(const _Expr& __x, const typename _Expr::value_type& __y) {
      | ^
/opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2876:1: note: candidate template ignored: requirement '__is_val_expr<std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>>::value' was not satisfied [with _Expr = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>]
 2876 | operator|(const typename _Expr::value_type& __x, const _Expr& __y) {
      | ^
1 error generated.
ninja: build stopped: subcommand failed.

Even if I removed -fexperimental-modules-reduced-bmi argument, it still throws errors.

llvmbot commented 4 days ago

@llvm/issue-subscribers-clang-modules

Author: LEE KYOUNGHEON (stripe2933)

Following code works with Clang 19 + libc++. `main.cpp` ```c++ import std; #define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__) namespace ranges::views { constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); }); } int main() { auto opts = { std::optional { 1 }, std::optional { 2 } }; std::println("{}", opts | ranges::views::deref); // [1, 2] } ``` But this does not: `main.cpp` ```c++ import std; import ranges; int main() { auto opts = { std::optional { 1 }, std::optional { 2 } }; std::println("{}", opts | ranges::views::deref); } ``` `ranges.cppm` ```c++ export module ranges; import std; #define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__) namespace ranges::views { export constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); }); } ``` ``` ====================[ Build | hello | macos-clang-19 ]========================== /opt/homebrew/Cellar/cmake/3.30.0/bin/cmake --build /Users/stripe2933/Desktop/hello/build/macos-clang-19 --target hello -j 6 [3/4] Building CXX object CMakeFiles/hello.dir/main.cpp.o FAILED: CMakeFiles/hello.dir/main.cpp.o /opt/homebrew/opt/llvm@19/bin/clang++ -nostdinc++ -nostdlib++ -isystem /opt/homebrew/opt/llvm@19/include/c++/v1 -fexperimental-modules-reduced-bmi -std=gnu++23 -arch arm64 -isysroot /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.0.sdk -fcolor-diagnostics -MD -MT CMakeFiles/hello.dir/main.cpp.o -MF CMakeFiles/hello.dir/main.cpp.o.d @CMakeFiles/hello.dir/main.cpp.o.modmap -o CMakeFiles/hello.dir/main.cpp.o -c /Users/stripe2933/Desktop/hello/main.cpp /Users/stripe2933/Desktop/hello/main.cpp:6:29: error: invalid operands to binary expression ('std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') and 'const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>' (aka 'const std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>')) 6 | std::println("{}", opts | ranges::views::deref); | ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~ /opt/homebrew/opt/llvm@19/include/c++/v1/cstddef:73:45: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'byte' for 1st argument 73 | _LIBCPP_HIDE_FROM_ABI inline constexpr byte operator|(byte __lhs, byte __rhs) noexcept { | ^ ~~~~~~~~~~ /opt/homebrew/opt/llvm@19/include/c++/v1/__charconv/chars_format.h:34:53: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'chars_format' for 1st argument 34 | inline _LIBCPP_HIDE_FROM_ABI constexpr chars_format operator|(chars_format __x, chars_format __y) { | ^ ~~~~~~~~~~~~~~~~ /opt/homebrew/opt/llvm@19/include/c++/v1/future:431:47: note: candidate function not viable: no known conversion from 'std::initializer_list<std::optional<int>>' (aka 'initializer_list<optional<int>>') to 'launch' for 1st argument 431 | inline _LIBCPP_HIDE_FROM_ABI constexpr launch operator|(launch __x, launch __y) { | ^ ~~~~~~~~~~ /opt/homebrew/opt/llvm@19/include/c++/v1/bitset:932:1: note: candidate template ignored: could not match 'bitset' against 'std::initializer_list' 932 | operator|(const bitset<_Size>& __x, const bitset<_Size>& __y) _NOEXCEPT { | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:71:1: note: candidate template ignored: constraints not satisfied [with _Range = std::initializer_list<std::optional<int>> &, _Closure = const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>> &] 71 | operator|(_Range&& __range, _Closure&& __closure) noexcept(is_nothrow_invocable_v<_Closure, _Range>) { | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:69:12: note: because 'invocable<const std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)> > > &, std::initializer_list<std::__1::optional<int> > &>' evaluated to false 69 | requires invocable<_Closure, _Range> | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__concepts/invocable.h:28:3: note: because 'std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...)' would be invalid: no matching function for call to 'invoke' 28 | std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...); // not required to be equality preserving | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:77:52: note: candidate template ignored: constraints not satisfied [with _Closure = std::initializer_list<std::optional<int>> &, _OtherClosure = const __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>> &] 77 | [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2) noexcept( | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:75:11: note: because 'std::initializer_list<std::__1::optional<int>> &' does not satisfy '_RangeAdaptorClosure' 75 | template <_RangeAdaptorClosure _Closure, _RangeAdaptorClosure _OtherClosure> | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/__ranges/range_adaptor.h:62:32: note: because '!ranges::range<remove_cvref_t<initializer_list<optional<int> > &> >' evaluated to false 62 | concept _RangeAdaptorClosure = !ranges::range<remove_cvref_t<_Tp>> && requires { | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2858:1: note: candidate template ignored: requirement '__is_val_expr<std::initializer_list<std::__1::optional<int>>>::value' was not satisfied [with _Expr1 = std::initializer_list<std::optional<int>>, _Expr2 = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>] 2858 | operator|(const _Expr1& __x, const _Expr2& __y) { | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2867:1: note: candidate template ignored: requirement '__is_val_expr<std::initializer_list<std::__1::optional<int>>>::value' was not satisfied [with _Expr = std::initializer_list<std::optional<int>>] 2867 | operator|(const _Expr& __x, const typename _Expr::value_type& __y) { | ^ /opt/homebrew/opt/llvm@19/include/c++/v1/valarray:2876:1: note: candidate template ignored: requirement '__is_val_expr<std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<ranges::views::(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>>::value' was not satisfied [with _Expr = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at /Users/stripe2933/Desktop/hello/ranges.cppm:8:57)>>>] 2876 | operator|(const typename _Expr::value_type& __x, const _Expr& __y) { | ^ 1 error generated. ninja: build stopped: subcommand failed. ``` Even if I removed `-fexperimental-modules-reduced-bmi` argument, it still throws errors.
ChuanqiXu9 commented 3 days ago

I haven't figured it out completely. But it shows we can workaround it by:

export module ranges;

import std;

#define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)

namespace ranges::views {
+    export  inline constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
-    export constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
}
ChuanqiXu9 commented 3 days ago

Sadly this tends to be an ABI issue. According to https://itanium-cxx-abi.github.io/cxx-abi/abi.html:

5.1.8 Closure Types (Lambdas) In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

  • ...
  • the initializers of inline or templated variables

So that according to the Itanium C++ ABI spec, we only set lambda numbering for the cases listed above (including the initializers of inline variables, this is why the above workaround works). However, this is not true for modules clearly. The lambda is not unique to the translation units if they are used in initializers of varaibles in modules.

I am not sure if we can change the behavior of the compiler before the above proposed wording get approved in Itanium C++ ABI. Otherwise we can only wait for the action of Itanium C++ group, which may take a longer time.

CC @AaronBallman @erichkeane @cor3ntin @mizvekov for the policy issues.

mizvekov commented 3 days ago

The straight conversion of this example using preprocessor includes, without modules, doesn't work as well, so I tend to think this is not a bug, the user should have used inline as you suggested.

Wouldn't this be an ODR violation anyway, and the real problem here is that we miss diagnosing that?

ChuanqiXu9 commented 1 day ago

The straight conversion of this example using preprocessor includes, without modules, doesn't work as well

What's the example? In the single TU form, https://godbolt.org/z/1zre9eMjb, it works well. Do you mean split the definition of deref in other TUs? Then I think it wouldn't work well naturally.

Wouldn't this be an ODR violation anyway, and the real problem here is that we miss diagnosing that?

Then this is why I think this is an ABI issue. If we add a clause to https://itanium-cxx-abi.github.io/cxx-abi/abi.html#closure-types:

In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

  • ...
  • attached to named modules

it wouldn't be an ODR violation.

kamrann commented 14 hours ago

I don't see how it relates to ODR. In the given modules case, there is only a single definition, and the entity has module linkage.

Anyway I think this is something more fundamental. It can be reduced to:

// mod.cppm

export module mod;

namespace mod {
    export constexpr auto lambda = [](auto &&x) { return x; };
}
// main.cpp

import std;
import mod;

constexpr auto local_lambda = [](auto &&x) { return x; };

int main() {
    auto vals = std::array{ 1, 2 };
    std::views::transform(local_lambda)(vals); // ok
    std::views::transform(mod::lambda)(vals); // error
}

And indeed further, to:

// main.cpp

import mod;

constexpr auto local_lambda = [](auto &&x) { return x; };

auto a = local_lambda;
auto b = mod::lambda; // error

And now I see that it's the same issue as #59513, and also that you already realized that, @ChuanqiXu9. Anyway I'll leave this reduction here just in case it's useful.