[Bug libstdc++/106201] New: filesystem::directory_iterator is a borrowable range?

[Bug libstdc++/106201] New: filesystem::directory_iterator is a borrowable range?

[prlw1 at cam dot ac.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

            Bug ID: 106201
           Summary: filesystem::directory_iterator is a borrowable range?
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: prlw1 at cam dot ac.uk
  Target Milestone: ---

In the following:

#include <algorithm>
#include <filesystem>
#include <iostream>
#include <ranges>

int main()
{
        auto print = [](const auto& x) {std::cout << x << '\n';} ;

        std::ranges::for_each(
                std::views::iota(1) | std::views::take(5),
                print
        );

        std::ranges::for_each(
                std::filesystem::directory_iterator("/"),
                print
        );

        std::ranges::for_each(
                std::filesystem::directory_iterator("/") | std::views::take(3),
                print
        );      
}

the first two for_each clauses compile and execute correctly, even with g++
10.3.0.

Is my expectation that the third clause is valid incorrect?

(I just tried with today's head, g++ (GCC) 13.0.0 20220705 (experimental) and
it failed to compile.)

Oh: https://cplusplus.github.io/LWG/issue3480
But: it says "could be fixed ... (as libstdc++ currently does anyway)"
Issue looks resolved?
Thoughts?

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Last reconfirmed|                            |2022-07-05
           Keywords|                            |rejects-valid
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Patrick Welche from comment #0)
> Oh: https://cplusplus.github.io/LWG/issue3480
> But: it says "could be fixed ... (as libstdc++ currently does anyway)"
> Issue looks resolved?

That ensures that directory iterators are ranges, which is what your second
for_each call depends on.

The third one fails but not because of anything to do with borrowable ranges.
The problem is that the filesystem::swap(path&, path&) overload is found by ADL
and considered by overload resolution when checking whether
counted_iterator<filesystem::directory_iterator> is swappable. Overload
resolution checks whether the iterator can be converted to a path, which causes
constraint recursion.

The following change fixes this by removing filesystem::swap(path&, path&) from
the ADL result set:

--- a/libstdc++-v3/include/bits/fs_path.h
+++ b/libstdc++-v3/include/bits/fs_path.h
@@ -591,6 +591,8 @@ namespace __detail
       return __result;
     }

+    friend void swap(path& __lhs, path& __rhs) noexcept { __lhs.swap(__rhs); }
+
   private:
     enum class _Type : unsigned char {
       _Multi = 0, _Root_name, _Root_dir, _Filename
@@ -732,8 +734,6 @@ namespace __detail
   /// @{
   /// @relates std::filesystem::path

-  inline void swap(path& __lhs, path& __rhs) noexcept { __lhs.swap(__rhs); }
-
   size_t hash_value(const path& __p) noexcept;

   /// @}

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> The third one fails but not because of anything to do with borrowable
> ranges. The problem is that the filesystem::swap(path&, path&) overload is
> found by ADL and considered by overload resolution when checking whether
> counted_iterator<filesystem::directory_iterator> is swappable. Overload
> resolution checks whether the iterator can be converted to a path, which
> causes constraint recursion.

The full explanation is that views::take creates a view whose iterator is
counted_iterator<filesystem::directory_iterator> and the constraints for
ranges::for_each check that that iterator is copyable, movable, and swappable.
That fails with libstdc++:

#include <filesystem>
#include <ranges>
using I = std::counted_iterator<std::filesystem::directory_iterator>;
static_assert( std::swappable<I> );

The swappable concept finds filesystem::swap(path&, path&) via ADL. Overload
resolution checks if counted_iterator<filesystem::directory_iterator> can be
converted to path, which considers the path constructors, which checks if
counted_iterator is an iterator, which uses std::iterator_traits, which depends
on the iterator being copyable, which depends on it being swappable, and we're
recursing.

However, I *think* there's a compiler bug here. We should not be considering
constructors of path to see if we can get a path& lvalue. That would never
work, constructing a path would give us an rvalue. Clang compiles the example
when using libstdc++ headers.

Clang and EDG both compile this, but G++ doesn't:

namespace effing
{
  template<typename T>
    concept effable = requires (T& t) { f(t); };

  template <class T>
    requires effable<T> || true
  void eff(T&&) { }
}

struct path {
  template<effing::effable T> path(const T&) { }
};

void f(path&);

struct iterator { };

int main()
{
  iterator i;
  effing::eff(i);
}

conv.C: In substitution of 'template<class T>  requires  effable<T>
path::path(const T&) [with T = iterator]':
conv.C:4:42:   recursively required by substitution of 'template<class T> 
requires  effable<T> path::path(const T&) [with T = iterator]'
conv.C:4:42:   required by substitution of 'template<class T>  requires
(effable<T>) || true void effing::eff(T&&) [with T = iterator&]'
conv.C:22:14:   required from here
conv.C:4:13:   required for the satisfaction of 'effable<T>' [with T =
iterator]
conv.C:4:23:   in requirements with 'T& t' [with T = iterator]
conv.C:4:23: error: satisfaction of atomic constraint 'requires(T& t) {f(t);}
[with T = T]' depends on itself
    4 |     concept effable = requires (T& t) { f(t); };
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Reduced to not use concepts:

struct foo
{
  template<typename T, typename = decltype(f(*(T*)nullptr))>
    foo(const T&) { }
};

struct bar { };

struct baz
{
  template<typename T> baz(const T&) { }
};

void f(foo&);
void f(baz, ...);

int main()
{
  bar b;
  f(b);
}

G++ goes into infinite recursion, Clang, EDG and MSVC accept it.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |94894

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It looks like this is PR c++/94894 but we'll need a workaround in the library
for now.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94894
[Bug 94894] Premature instantiation of conversion function template during
overload resolution

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:68f37670eff0b872ce5dfd382c8d8f3206bdfc27

commit r13-1755-g68f37670eff0b872ce5dfd382c8d8f3206bdfc27
Author: Patrick Palka <ppalka@redhat.com>
Date:   Tue Jul 19 14:04:13 2022 -0400

    c++: shortcut bad reference binding [PR94894]

    In case of l/rvalue or cv-qual mismatch during reference binding, we
    try to give more helpful diagnostics by computing a bad conversion that
    allows the mismatch.  But in doing so, we may end up considering and
    instantiating a conversion function that could induce a hard error and
    in turn cause us to reject otherwise valid code.  We could just give up
    on producing a better diagnostic here, but ideally we'd preserve the
    better diagnostics for invalid code while avoiding unnecessary template
    instantiations for valid code.

    To that end, this patch adapts the bad conversion shortcutting mechanism
    from r12-3346-g47543e5f9d1fc5 to additionally handle this situation.
    The main observation from there is that during overload resolution, if we
    know we have a strictly viable candidate then we don't need to distinguish
    between an unviable and non-strictly viable candidate.  Thus we don't
    need to distinguish between an invalid and bad conversion either, which
    is what this patch exploits.  Of course, we don't know whether we have a
    strictly viable candidate until after the fact, so we still need to
    remember when we deferred distinguishing between an invalid and bad
    conversion.  This patch adds a special conversion kind ck_deferred_bad
    for this purpose.

            PR c++/94894
            PR c++/105766
            PR c++/106201

    gcc/cp/ChangeLog:

            * call.cc (enum conversion_kind): Add ck_deferred_bad enumerator.
            (has_next): Return false for it.
            (reference_binding): Return a ck_deferred_bad conversion instead
            of an actual bad conversion when LOOKUP_SHORTCUT_BAD_CONVS is set.
            Remove now obsolete early exit for the incomplete TO case.
            (implicit_conversion_1): Don't mask out LOOKUP_SHORTCUT_BAD_CONVS.
            (add_function_candidate): Set LOOKUP_SHORTCUT_BAD_CONVS iff
            shortcut_bad_convs.
            (missing_conversion_p): Also return true for a ck_deferred_bad
            conversion.
            * cp-tree.h (LOOKUP_SHORTCUT_BAD_CONVS): Define.

    gcc/testsuite/ChangeLog:

            * g++.dg/conversion/ref8.C: New test.
            * g++.dg/conversion/ref9.C: New test.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[prlw1 at cam dot ac.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

Patrick Welche <prlw1 at cam dot ac.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #6 from Patrick Welche <prlw1 at cam dot ac.uk> ---
I can confirm that this is now fixed. Thank you!

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |ASSIGNED
         Resolution|FIXED                       |---

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Reopening because I want to add a workaround in the library for the release
branches.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
... because the compiler change isn't suitable to be backported to the stable
releases, but the problem exists there too.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[prlw1 at cam dot ac.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #9 from Patrick Welche <prlw1 at cam dot ac.uk> ---
sorry for jumping the gun...

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm planning to use this workaround for gcc-11 and gcc-12:

--- a/libstdc++-v3/include/bits/fs_path.h
+++ b/libstdc++-v3/include/bits/fs_path.h
@@ -737,7 +737,14 @@ namespace __detail
   /// @{
   /// @relates std::filesystem::path

+#if __cpp_concepts >= 201907L
+  // Workaround for PR libstdc++/106201
+  inline void
+  swap(same_as<path> auto& __lhs, same_as<path> auto& __rhs) noexcept
+  { __lhs.swap(__rhs); }
+#else
   inline void swap(path& __lhs, path& __rhs) noexcept { __lhs.swap(__rhs); }
+#endif

   size_t hash_value(const path& __p) noexcept;

This will prevent that swap overload from being a candidate for swapping
counted_iterator<filesystem::directory_iterator>, so we won't check for
conversion to path& and hit the constraint recursion.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:6b859736bb1e707778627b2e58ef6088e475a54c

commit r13-4242-g6b859736bb1e707778627b2e58ef6088e475a54c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Nov 22 12:17:27 2022 +0000

    libstdc++: Add testcase for fs::path constraint recursion [PR106201]

    libstdc++-v3/ChangeLog:

            PR libstdc++/106201
            * testsuite/27_io/filesystem/iterators/106201.cc: New test.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:477b3f9d58589b3af7a5a7d038d78352ad66406e

commit r12-8927-g477b3f9d58589b3af7a5a7d038d78352ad66406e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Nov 22 18:15:56 2022 +0000

    libstdc++: Add workaround for fs::path constraint recursion [PR106201]

    This works around a compiler bug where overload resolution attempts
    implicit conversion to path in order to call a function with a path&
    parameter. Such conversion would produce a prvalue, which would not be
    able to bind to the lvalue reference anyway. Attempting to check the
    conversion causes a constraint recursion because the arguments to the
    path constructor are checked to see if they're iterators, which checks
    if they're swappable, which tries to use the swap function that
    triggered the conversion in the first place.

    This replaces the swap function with an abbreviated function template
    that is constrained with same_as<path> auto& so that the invalid
    conversion is never considered.

    libstdc++-v3/ChangeLog:

            PR libstdc++/106201
            * include/bits/fs_path.h (filesystem::swap(path&, path&)):
            Replace with abbreviated function template.
            * include/experimental/bits/fs_path.h (filesystem::swap):
            Likewise.
            * testsuite/27_io/filesystem/iterators/106201.cc: New test.
            * testsuite/experimental/filesystem/iterators/106201.cc: New test.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3892251498c16c9507cf8471f4f10676212e9ead

commit r13-4292-g3892251498c16c9507cf8471f4f10676212e9ead
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Nov 22 21:51:06 2022 +0000

    libstdc++: Update tests on trunk [PR106201]

    This copies the better tests from gcc-12 to trunk.

    libstdc++-v3/ChangeLog:

            PR libstdc++/106201
            * testsuite/27_io/filesystem/iterators/106201.cc: Improve test.
            * testsuite/experimental/filesystem/iterators/106201.cc: New test.

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm not planning to backport the workaround to gcc-11, so I'm going to close
this as fixed for 12.3

[Bug libstdc++/106201] filesystem::directory_iterator is a borrowable range?

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106201
Bug 106201 depends on bug 94894, which changed state.

Bug 94894 Summary: avoidable instantiation of conversion function template during overload resolution
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94894

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED