[Bug libstdc++/97600] New: [ranges] result of static assertion depends on unrelated statement

[Bug libstdc++/97600] New: [ranges] result of static assertion depends on unrelated statement

[ppalka at gcc dot gnu.org]

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

            Bug ID: 97600
           Summary: [ranges] result of static assertion depends on
                    unrelated statement
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ppalka at gcc dot gnu.org
  Target Milestone: ---

$ cat testcase.C
$#include <sstream>
#include <ranges>

int main() {
  using namespace std::ranges;
  basic_istream_view<int, char, std::char_traits<char>> v;
  v.begin();
  static_assert(range<decltype(v)>);
}

$ g++ -std=c++20 testcase.C
testcase.C:8:17: error: static assertion failed
    8 |   static_assert(range<decltype(v)>);
      |                 ^~~~~~~~~~~~~~~~~~


Unexpectedly, this static assert fails.  Even more unexpectedly, if we remove
the 'v.begin()' line before it, then the static assert succeeds.

[Bug libstdc++/97600] [ranges] result of static assertion depends on unrelated statement

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.2.0, 11.0

--- Comment #1 from Patrick Palka <ppalka at gcc dot gnu.org> ---
This happens because of the following chain of events:

1. Calling basic_istream_view::begin() entails instantiating its definition to
perform return type deduction.
2. We perform overload resolution of the direct-initialization _Iterator{*this}
inside its definition.
3. As part of overload resolution, we consider the copy ctor _Iterator(const
_Iterator&).
4. We try to find an implicit conversion sequence from basic_istream_view to
const _Iterator&.
5. We consider a conversion sequence that goes through the conversion function
view_interface::operator bool().
6. We check the conversion function's constraint 'requires {
ranges::empty(_M_derived()); }'.
7. ranges::empty() checks that view_interface::empty() is callable and
therefore has satisfied constraints
8. view_interface::empty() requires range<basic_istream_view>, which requires
that basic_istream_view::begin() is callable, which requires that we deduce its
return type.  But we're already in the the middle of deducing it, so we hit a
SFINAE error, which causes satisfaction of range<basic_istream_view> to fail.
9. We cache this negative satisfaction result for (the atomic constraint of)
range<basic_istream_view>.
10. We rule out the copy ctor during overload resolution and eventually resolve
the direct-initialization in the expected way.

When we later evaluate the static assert, we just return the cached
satisfaction result.

I have an experimental patch here
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557117.html that makes
us rule out the conversion function in step 5 sooner without first checking its
constraints, which would be one way to fix this.

[Bug libstdc++/97600] [ranges] result of static assertion depends on unrelated statement

[ppalka at gcc dot gnu.org]

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

--- Comment #2 from Patrick Palka <ppalka at gcc dot gnu.org> ---
I think all views whose begin()/end() has a placeholder return type and
performs direct initialization of a _Iterator/_Sentinel from *this are affected
by some variant of this issue.

A library-side workaround would be to templatize view_interface::operator
bool() like so:

diff --git a/libstdc++-v3/include/bits/ranges_util.h
b/libstdc++-v3/include/bits/ranges_util.h
index cc50e2ad4e4..8563a5dfcb0 100644
--- a/libstdc++-v3/include/bits/ranges_util.h
+++ b/libstdc++-v3/include/bits/ranges_util.h
@@ -86,13 +86,15 @@ namespace ranges
       empty() const requires forward_range<const _Derived>
       { return ranges::begin(_M_derived()) == ranges::end(_M_derived()); }

-      constexpr explicit
-      operator bool() requires requires { ranges::empty(_M_derived()); }
-      { return !ranges::empty(_M_derived()); }
-
-      constexpr explicit
-      operator bool() const requires requires { ranges::empty(_M_derived()); }
-      { return !ranges::empty(_M_derived()); }
+      template<same_as<bool> _Tp>
+       constexpr explicit
+       operator _Tp() requires requires { ranges::empty(_M_derived()); }
+       { return !ranges::empty(_M_derived()); }
+
+      template<same_as<bool> _Tp>
+       constexpr explicit
+       operator _Tp() const requires requires { ranges::empty(_M_derived()); }
+       { return !ranges::empty(_M_derived()); }

       constexpr auto
       data() requires contiguous_iterator<iterator_t<_Derived>>

So that when we considering this conversion function in step 5, we deduce
_Tp=_Iterator and short circuit satisfaction of the conversion function's
associated constraints in step 6.

[Bug libstdc++/97600] [ranges] satisfaction value of range<basic_istream_view> affected by prior use of basic_istream_view::begin()

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |ppalka at gcc dot gnu.org
   Last reconfirmed|                            |2020-10-28
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

[Bug libstdc++/97600] [ranges] satisfaction value of range<basic_istream_view> affected by prior use of basic_istream_view::begin()

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 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:afb8da7faa9dfe5a0d94ed45a373d74c076784ab

commit r11-4584-gafb8da7faa9dfe5a0d94ed45a373d74c076784ab
Author: Patrick Palka <ppalka@redhat.com>
Date:   Fri Oct 30 20:33:19 2020 -0400

    libstdc++: Don't initialize from *this inside some views [PR97600]

    This works around a subtle issue where instantiating the begin()/end()
    member of some views (as part of return type deduction) inadvertently
    requires computing the satisfaction value of range<foo_view>.

    This is problematic because the constraint range<foo_view> requires the
    begin()/end() member to be callable.  But it's not callable until we've
    deduced its return type, so evaluation of range<foo_view> yields false
    at this point.  And if after both members are instantiated (and their
    return types deduced) we evaluate range<foo_view> again, this time it
    will yield true since the begin()/end() members are now both callable.
    This makes the program ill-formed according to [temp.constr.atomic]/3:

      If, at different points in the program, the satisfaction result is
      different for identical atomic constraints and template arguments, the
      program is ill-formed, no diagnostic required.

    The views affected by this issue are those whose begin()/end() member
    has a placeholder return type and that member initializes an _Iterator
    or _Sentinel object from a reference to *this.  The second condition is
    relevant because it means explicit conversion functions are considered
    during overload resolution (as per [over.match.copy], I think), and
    therefore it causes g++ to check the constraints of the conversion
    function view_interface<foo_view>::operator bool().  And this conversion
    function's constraints indirectly require range<foo_view>.

    This issue is observable on trunk only with basic_istream_view (as in
    the testcase in the PR).  But a pending patch that makes g++ memoize
    constraint satisfaction values indefinitely (it currently invalidates
    the satisfaction cache on various events) causes many existing tests for
    the other affected views to fail, because range<foo_view> then remains
    false for the whole compilation.

    This patch works around this issue by adjusting the constructors of the
    _Iterator and _Sentinel types of the affected views to take their
    foo_view argument by pointer instead of by reference, so that g++ no
    longer considers explicit conversion functions when resolving the
    direct-initialization inside these views' begin()/end() members.

    libstdc++-v3/ChangeLog:

            PR libstdc++/97600
            * include/std/ranges (basic_istream_view::begin): Initialize
            _Iterator from 'this' instead of '*this'.
            (basic_istream_view::_Iterator::_Iterator): Adjust constructor
            accordingly.
            (filter_view::_Iterator::_Iterator): Take a filter_view*
            argument instead of a filter_view& argument.
            (filter_view::_Sentinel::_Sentinel): Likewise.
            (filter_view::begin): Initialize _Iterator from 'this' instead
            of '*this'.
            (filter_view::end): Likewise.
            (transform_view::_Iterator::_Iterator): Take a _Parent* instead
            of a _Parent&.
            (filter_view::_Iterator::operator+): Adjust accordingly.
            (filter_view::_Iterator::operator-): Likewise.
            (filter_view::begin): Initialize _Iterator from 'this' instead
            of '*this'.
            (filter_view::end): Likewise.
            (join_view::_Iterator): Take a _Parent* instead of a _Parent&.
            (join_view::_Sentinel): Likewise.
            (join_view::begin): Initialize _Iterator from 'this' instead of
            '*this'.
            (join_view::end): Initialize _Sentinel from 'this' instead of
            '*this'.
            (split_view::_OuterIter): Take a _Parent& instead of a _Parent*.
            (split_view::begin): Initialize _OuterIter from 'this' instead
            of '*this'.
            (split_view::end): Likewise.
            * testsuite/std/ranges/97600.cc: New test.

[Bug libstdc++/97600] [ranges] satisfaction value of range<basic_istream_view> affected by prior use of basic_istream_view::begin()

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:9324f7a25c7161a813bfae6cc2d180784b165740

commit r11-5954-g9324f7a25c7161a813bfae6cc2d180784b165740
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Dec 11 14:37:09 2020 -0500

    c++: Avoid considering some conversion ops [PR97600]

    Patrick's earlier patch to check convertibility before constraints for
    conversion ops wasn't suitable because checking convertibility can also
lead
    to unwanted instantiations, but it occurs to me that there's a smaller
check
    we can do to avoid doing normal consideration of the conversion ops in this
    case: since we're in the middle of a user-defined conversion, we can
exclude
    from consideration any conversion ops that return a type that would need an
    additional user-defined conversion to reach the desired type: namely, a
type
    that differs in class-ness from the desired type.

    [temp.inst]/9 allows optimizations like this: "If the function selected by
    overload resolution can be determined without instantiating a class
template
    definition, it is unspecified whether that instantiation actually takes
    place."

    gcc/cp/ChangeLog:

            PR libstdc++/97600
            * call.c (build_user_type_conversion_1): Avoid considering
            conversion functions that return a clearly unsuitable type.

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp2a/concepts-conv3.C: New test.

[Bug libstdc++/97600] [ranges] satisfaction value of range<basic_istream_view> affected by prior use of basic_istream_view::begin()

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:3d6bba85e1dd6cb7e213a7e6c060b9c8a0a346e2

commit r10-9737-g3d6bba85e1dd6cb7e213a7e6c060b9c8a0a346e2
Author: Patrick Palka <ppalka@redhat.com>
Date:   Fri Oct 30 20:33:19 2020 -0400

    libstdc++: Don't initialize from *this inside some views [PR97600]

    This works around a subtle issue where instantiating the begin()/end()
    member of some views (as part of return type deduction) inadvertently
    requires computing the satisfaction value of range<foo_view>.

    This is problematic because the constraint range<foo_view> requires the
    begin()/end() member to be callable.  But it's not callable until we've
    deduced its return type, so evaluation of range<foo_view> yields false
    at this point.  And if after both members are instantiated (and their
    return types deduced) we evaluate range<foo_view> again, this time it
    will yield true since the begin()/end() members are now both callable.
    This makes the program ill-formed according to [temp.constr.atomic]/3:

      If, at different points in the program, the satisfaction result is
      different for identical atomic constraints and template arguments, the
      program is ill-formed, no diagnostic required.

    The views affected by this issue are those whose begin()/end() member
    has a placeholder return type and that member initializes an _Iterator
    or _Sentinel object from a reference to *this.  The second condition is
    relevant because it means explicit conversion functions are considered
    during overload resolution (as per [over.match.copy], I think), and
    therefore it causes g++ to check the constraints of the conversion
    function view_interface<foo_view>::operator bool().  And this conversion
    function's constraints indirectly require range<foo_view>.

    This issue is observable on trunk only with basic_istream_view (as in
    the testcase in the PR).  But a pending patch that makes g++ memoize
    constraint satisfaction values indefinitely (it currently invalidates
    the satisfaction cache on various events) causes many existing tests for
    the other affected views to fail, because range<foo_view> then remains
    false for the whole compilation.

    This patch works around this issue by adjusting the constructors of the
    _Iterator and _Sentinel types of the affected views to take their
    foo_view argument by pointer instead of by reference, so that g++ no
    longer considers explicit conversion functions when resolving the
    direct-initialization inside these views' begin()/end() members.

    libstdc++-v3/ChangeLog:

            PR libstdc++/97600
            * include/std/ranges (basic_istream_view::begin): Initialize
            _Iterator from 'this' instead of '*this'.
            (basic_istream_view::_Iterator::_Iterator): Adjust constructor
            accordingly.
            (filter_view::_Iterator::_Iterator): Take a filter_view*
            argument instead of a filter_view& argument.
            (filter_view::_Sentinel::_Sentinel): Likewise.
            (filter_view::begin): Initialize _Iterator from 'this' instead
            of '*this'.
            (filter_view::end): Likewise.
            (transform_view::_Iterator::_Iterator): Take a _Parent* instead
            of a _Parent&.
            (filter_view::_Iterator::operator+): Adjust accordingly.
            (filter_view::_Iterator::operator-): Likewise.
            (filter_view::begin): Initialize _Iterator from 'this' instead
            of '*this'.
            (filter_view::end): Likewise.
            (join_view::_Iterator): Take a _Parent* instead of a _Parent&.
            (join_view::_Sentinel): Likewise.
            (join_view::begin): Initialize _Iterator from 'this' instead of
            '*this'.
            (join_view::end): Initialize _Sentinel from 'this' instead of
            '*this'.
            (split_view::_OuterIter): Take a _Parent& instead of a _Parent*.
            (split_view::begin): Initialize _OuterIter from 'this' instead
            of '*this'.
            (split_view::end): Likewise.
            * testsuite/std/ranges/97600.cc: New test.

    (cherry picked from commit afb8da7faa9dfe5a0d94ed45a373d74c076784ab)

[Bug libstdc++/97600] [ranges] satisfaction value of range<basic_istream_view> affected by prior use of basic_istream_view::begin()

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

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

--- Comment #6 from Patrick Palka <ppalka at gcc dot gnu.org> ---
The library side workaround is in place for GCC 10.4 and 11, and GCC 11
additionally has a frontend optimization for ruling out unviable conversion ops
sooner which alone is sufficient to fix the PR.  I suppose we could revert the
library-side workaround for GCC 11/12 then, but it seems harmless to keep
around.