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

Ranges pipe syntax broken after commit #c1086532d4d5 when using std module. #89898

Closed
stripe2933 opened this issue Apr 24, 2024 · 4 comments · Fixed by #90071
Closed

Ranges pipe syntax broken after commit #c1086532d4d5 when using std module. #89898

stripe2933 opened this issue Apr 24, 2024 · 4 comments · Fixed by #90071
Assignees
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`

Comments

@stripe2933
Copy link
Contributor

The following code

#include <print>
#include <ranges>

int main(){
    using namespace std::string_view_literals;
    std::println("{}", "abcde"sv | std::views::transform([](char c) { return c - 'a'; }));
    
}

works correctly (godbolt link) in Clang trunk. However, when changing the header inclusion to import std;, then the code will emit error:

main.cpp:5:34: error: invalid operands to binary expression ('basic_string_view<char>' and '__range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at main.cpp:5:58)>>>' (aka 'std::__1::ranges::__range_adaptor_closure_t<std::__bind_back_t<std::__1::ranges::views::__transform::__fn, std::__1::tuple<(lambda at main.cpp:5:58)>>>'))
    5 |     std::println("{}", "abcde"sv | std::views::transform([](char c) { return c - 'a'; }));
      |                        ~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/build/include/c++/v1/cstddef:73:45: note: candidate function not viable: no known conversion from 'basic_string_view<char>' to 'byte' for 1st argument
   73 | _LIBCPP_HIDE_FROM_ABI inline constexpr byte operator|(byte __lhs, byte __rhs) noexcept {
      |                                             ^         ~~~~~~~~~~
llvm-project/build/include/c++/v1/__charconv/chars_format.h:34:53: note: candidate function not viable: no known conversion from 'basic_string_view<char>' to 'chars_format' for 1st argument
   34 | inline _LIBCPP_HIDE_FROM_ABI constexpr chars_format operator|(chars_format __x, chars_format __y) {
      |                                                     ^         ~~~~~~~~~~~~~~~~
llvm-project/build/include/c++/v1/future:433:55: note: candidate function not viable: no known conversion from 'basic_string_view<char>' to 'launch' for 1st argument
  433 | inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR launch operator|(launch __x, launch __y) {
      |                                                       ^         ~~~~~~~~~~
llvm-project/build/include/c++/v1/bitset:929:1: note: candidate template ignored: could not match 'bitset' against 'basic_string_view'
  929 | operator|(const bitset<_Size>& __x, const bitset<_Size>& __y) _NOEXCEPT {
      | ^
llvm-project/build/include/c++/v1/valarray:2858:1: note: candidate template ignored: requirement '__is_val_expr<std::__1::basic_string_view<char, std::__1::char_traits<char>>>::value' was not satisfied [with _Expr1 = basic_string_view<char>, _Expr2 = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at main.cpp:5:58)>>>]
 2858 | operator|(const _Expr1& __x, const _Expr2& __y) {
      | ^
llvm-project/build/include/c++/v1/valarray:2867:5: note: candidate template ignored: requirement '__is_val_expr<std::__1::basic_string_view<char, std::__1::char_traits<char>>>::value' was not satisfied [with _Expr = basic_string_view<char>]
 2867 |     operator|(const _Expr& __x, const typename _Expr::value_type& __y) {
      |     ^
llvm-project/build/include/c++/v1/valarray:2876:5: 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<(lambda at main.cpp:5:58)>>>>::value' was not satisfied [with _Expr = __range_adaptor_closure_t<__bind_back_t<__fn, tuple<(lambda at main.cpp:5:58)>>>]
 2876 |     operator|(const typename _Expr::value_type& __x, const _Expr& __y) {
      |     ^
1 error generated.

Compiler: Clang 18.1.4, Libc++: trunk.

I followed the standard library module instruction documented in libc++ document.

I noticed that the commits before c1086532d4d5, which corresponds to the PR #89148 does not print this error. It seems that the PR is problem.

@EugeneZelenko EugeneZelenko added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>` and removed new issue labels Apr 24, 2024
@ldionne
Copy link
Member

ldionne commented Apr 24, 2024

@stripe2933 Is this still an issue after #89793?

CC @xiaoyang-sde

@xiaoyang-sde
Copy link
Member

xiaoyang-sde commented Apr 24, 2024

@stripe2933 Is this still an issue after #89793?

CC @xiaoyang-sde

I tested the code with clang 18.1.4 and libc++ trunk. It's still an issue after #89793. It seems that ADL can't find the operator| in the std::ranges namespace when using import std. The issue is solved when I moved the operator| back into std::ranges::__range_adaptor_closure:

template <class _Tp>
  requires is_class_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>>
struct __range_adaptor_closure {
  template <ranges::range _Range, _RangeAdaptorClosure _Closure>
    requires same_as<_Tp, remove_cvref_t<_Closure>> && invocable<_Closure, _Range>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr decltype(auto)
  operator|(_Range&& __range, _Closure&& __closure) noexcept(is_nothrow_invocable_v<_Closure, _Range>) {
    return std::invoke(std::forward<_Closure>(__closure), std::forward<_Range>(__range));
  }

  template <_RangeAdaptorClosure _Closure, _RangeAdaptorClosure _OtherClosure>
    requires same_as<_Tp, remove_cvref_t<_Closure>> && constructible_from<decay_t<_Closure>, _Closure> &&
             constructible_from<decay_t<_OtherClosure>, _OtherClosure>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2) noexcept(
      is_nothrow_constructible_v<decay_t<_Closure>, _Closure> &&
      is_nothrow_constructible_v<decay_t<_OtherClosure>, _OtherClosure>) {
    return __range_adaptor_closure_t(std::__compose(std::forward<_OtherClosure>(__c2), std::forward<_Closure>(__c1)));
  }
};

@xiaoyang-sde xiaoyang-sde self-assigned this Apr 24, 2024
@ldionne
Copy link
Member

ldionne commented Apr 25, 2024

@ChuanqiXu9 Is this possibly a bug in the C++20 Modules implementation?

@mordante
Copy link
Member

There seem to be two issues

  • c108653 moved operator| to the ranges namespace but didn't export the named declaration
  • clang-tidy no longer runs in the CI so the error was not detected

I'll look at both issues.

mordante added a commit to mordante/llvm-project that referenced this issue Apr 25, 2024
This was omitted in c108653 and not detected by the CI since
clang-tidy is not running. This fixes the exports.

Fixes: llvm#89898
mordante added a commit that referenced this issue Apr 26, 2024
This was omitted in c108653 and not detected by the CI since
clang-tidy is not running. This fixes the exports.

Fixes: #89898
mordante added a commit that referenced this issue May 8, 2024
The patch does several things:
- fixes module exports
- disables clang-tidy with Clang-17 due to known issues
- disabled clang-tidy on older libstdc++ versions since it lacks C++20
features used
- fixes the CMake dependency

The issue why clang-tidy was not used in the CI was the last issue; the
plugin was not a
dependency of the tests. Without a plugin the tests disable clang-tidy.

This was noticed while investigating
#89898
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants