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>: Avoid infinite recursion in ranges::to when the inner ranges match but the destination container is still not constructible #4142

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Nov 2, 2023

Fixes GH-4141.

The standard says ([range.utility.conv.to]):

  • (2.1) If C does not satisfy input_range or convertible_to<range_reference_t<R>, range_value_t<C>> is true:
    • [...]
  • (2.2) Otherwise, if input_range<range_reference_t<R>> is true:
    to<C>(r | views::transform([](auto&& elem) {
        return to<range_value_t<C>>(std::forward<decltype(elem)>(elem));
    }), std::forward<Args>(args)...);
    

So views::transform shouldn't be used if convertible_to<range_reference_t<R>, range_value_t<C>> is true.

Notably, when step (2.2) recursively calls ranges::to<C>, it passes a transform_view whose range_reference_t is the same as range_value_t<C>. So in this case convertible_to<range_reference_t<R>, range_value_t<C>> is convertible_to<range_value_t<C>, range_value_t<C>>, which should usually be true, which prevents infinite recursion.

But, convertible_to<range_value_t<C>, range_value_t<C>> may be false (i.e. range_value_t<C> may not be move-constructible):

#include <ranges>

struct ImmovableRange {
    ImmovableRange(int*, int*);
    ImmovableRange(ImmovableRange&&) = delete;

    int* begin();
    int* end();
};

struct C {
    ImmovableRange* begin();
    ImmovableRange* end();
};

using R = int[1][2];

void test() {
    (void)std::ranges::to<C>(R{});  // infinite recursion
}

This PR does not fix this case, as I can't find where the standard addresses this. Maybe it really is a defect in the standard after all.

@cpplearner cpplearner requested a review from a team as a code owner November 2, 2023 16:20
@CaseyCarter CaseyCarter added bug Something isn't working ranges C++20/23 ranges labels Nov 2, 2023
@StephanTLavavej
Copy link
Member

Thanks for the fix and the detailed explanation!

@hewillk
Copy link
Contributor

hewillk commented Nov 7, 2023

Hum, the nonmovable range always seems problematic to me (although format_parse_context is necessary for its existence).

#include <ranges>

struct ImmovableRange {
  using value_type = int;
  ImmovableRange();
  ImmovableRange(ImmovableRange&&) = delete;

  int* begin();
  int* end();
  void push_back(value_type);
};

using R = int[1];

void test() {
  (void)std::ranges::to<ImmovableRange>(R{}); // hard error
}

@CaseyCarter
Copy link
Member

Hum, the nonmovable range always seems problematic to me (although format_parse_context is necessary for its existence).

I don't understand what this comment is saying about the change in this PR.

@hewillk
Copy link
Contributor

hewillk commented Nov 8, 2023

I don't understand what this comment is saying about the change in this PR.

Not sure why I made this comment in the first place.
Here are the exact comments about the PR:
Why don't we follow the wording and use more nested if conditions, for example:

if constexpr (_Ref_converts<_Rng, _Container>) {
    if constexpr (/* */) {
        // ...
    } else if constexpr (/* */) {
        // ...
    } else if constexpr (/* */) {
        // ...
    } else if constexpr (/* */) {
        // ...
    } else {
        static_assert(_Always_false<_Container>, "the program is ill-formed per N4950 [range.utility.conv.to]/2.3");
    }
    // ...
} else if constexpr (input_range<range_reference_t<_Rng>>) {
    // ...
} else {
    static_assert(_Always_false<_Container>, "the program is ill-formed per N4950 [range.utility.conv.to]/2.3");
}

instead use a less-nested but repeatedly-checked structure?

if constexpr (_Ref_converts<_Rng, _Container> && /* */) {
    // ...
} else if constexpr (_Ref_converts<_Rng, _Container> && /* */) {
    // ...
} else if constexpr (_Ref_converts<_Rng, _Container> && /* */) {
    // ...
} else if constexpr (_Ref_converts<_Rng, _Container> && /* */) {
    // ...
} else if constexpr (!_Ref_converts<_Rng, _Container> && input_range<range_reference_t<_Rng>>) {
    // ...
} else {
    static_assert(_Always_false<_Container>, "the program is ill-formed per N4950 [range.utility.conv.to]/2.3");
}

I think it is more appropriate to strictly adhere to the first structure especially after the editorial enhancements #6568 (Although both forms are currently equivalent).

@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter
Copy link
Member

Here are the exact comments about the PR: Why don't we follow the wording and use more nested if conditions, for example:
[snip]

I don't recall now why I used this structure, maybe it was simply to avoid repeating the static_assert? In any case, I agree it would be clearer if we more closely followed the structure of the Standard wording. I'd be happy to approve a PR making that change.

@StephanTLavavej StephanTLavavej merged commit 5045784 into microsoft:main Nov 10, 2023
37 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing this bug where for your PR I commented, Thanks for fixing this bug where for your PR I commented, Thanks for fixing this bug where for your PR I commented, 😹 🎉 🤪

@cpplearner cpplearner deleted the gh-4141 branch November 11, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<ranges>: ranges::to<array<T, N>>() strikes fear into the hearts of compilers
5 participants