This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Clean up pair's constructors and assignment operators
ClosedPublic

Authored by ldionne on Feb 13 2023, 7:23 AM.

Details

Summary

This patch makes std::pair's constructors and assignment operators
closer to conforming in C++23. The only missing bit I am aware of
now is reference_constructs_from_temporary_v checks, which we
don't have the tools for yet.

This patch also refactors a long-standing non-standard extension where
we'd provide constructors for tuple-like types in all standard modes. The
criteria for being a tuple-like type are different from pair-like types
as introduced recently in the standard, leading to a lot of complexity
when trying to implement recent papers that touch the pair constructors.

After this patch, the pre-C++23 extension is provided in a self-contained
block so that we can easily deprecate and eventually remove the extension
in future releases.

Diff Detail

Event Timeline

ldionne created this revision.Feb 13 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 7:23 AM
ldionne requested review of this revision.Feb 13 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 7:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added subscribers: huixie90, tcanens, Restricted Project.Feb 13 2023, 7:31 AM

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp
81

@huixie90 @tcanens

While implementing this, I found out that stuff like this would compile just fine:

std::pair<long, std::string> p;
p = std::tuple<long, float>{};

This is because std::string can be assigned-to from float. While this has nothing to do with pair specifically, this seems completely bonkers to me -- this is almost Javascript bad. I'm curious to know whether that has been discussed and acknowledged?

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

@tcanens @huixie90

This behavior seems to be inconsistent with the non-const-qualified operator= from pair-like. For operator=(PairLike) (not const), it seems like we go through subrange's operator pair-like(), which makes this work. But with operator=(pair-like) const, it looks like this conversion doesn't trigger and we end up not being able to perform the assignment. Does that match the LWG design intent?

tcanens added inline comments.Feb 13 2023, 7:53 AM
libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp
81
libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

It doesn't trigger because ConstAssignable is not convertible from int*? Are the const/non-const cases actually different?

IIRC the constraint is there to prevent bypassing the slicing check on subrange's operator pair-like.

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

This one is a bit bigger of a breakage for us. Do you have a target date for landing this patch?

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

This one is a bit bigger of a breakage for us. Do you have a target date for landing this patch?

I'd like to do it before the LLVM 17 release, and ideally not too late in the cycle to allow everyone to see this breakage well in advance. How bad is it?

I mainly glossed over the code. I like to do a review with a paper or is this the exact wording in the current WP?

libcxx/include/__utility/pair.h
67

Does this patch complete a paper/LWG issue?

135

Would it be hard to keep these removed parts optionally available when using C++ < 23?
If so we could do our normal deprecation method.

For me it's not a blocker if it's too hard.

ldionne marked an inline comment as done.Feb 13 2023, 11:26 AM

I mainly glossed over the code. I like to do a review with a paper or is this the exact wording in the current WP?

This is the wording in the current draft, modulo the reference_constructs_from_temporary bits. I had to do a full survey of what we do implement and what we didn't implement in various standard modes to try and come up with this.

libcxx/include/__utility/pair.h
67

In itself, no. It is preparing the terrain for being able to resolve P2255. That was originally my goal, in fact, and then I had to go down the rabbit hole when I saw that our pair constructors and assignment operators were not conforming at all.

135

Our usual deprecation method is to write it in the release notes, however in this case I am not sure how helpful that would be -- I doubt a lot of users read it and this extension has been there so long that it is probably taken for granted. Unless it breaks an unreasonable amount of code, IMO we should just go for it, it's not worse than a bunch of other fixes we've done in the past.

The other downside with deprecating this is that we'd now have a behavior that works in C++11/C++14/C++17/C++20 but is deprecated, and a subtly different non-deprecated behavior in C++23. As a user, I think I would find this rather confusing.

Before deciding to add complexity around this, I'd like to see how bad this is in real world code bases -- I'm waiting on Google's feedback here and I also started internal builds 105406484&105406488&105406497 (for my own ref).

Pinging vendors since this is technically a source break. I'll also take this for a run internally to see how much breakage this causes, if any.

This one is a bit bigger of a breakage for us. Do you have a target date for landing this patch?

I'd like to do it before the LLVM 17 release, and ideally not too late in the cycle to allow everyone to see this breakage well in advance. How bad is it?

Oh, it's not nearly that bad -- I meant if you were trying to land this in the next day or two :)

My initial sample-based test found 15 breakages. I'll know later how many actual breakages there are.

Still chugging away, looks like this is going to be at least 50 breakages. Most of them are trivial to fix (e.g. return std::make_tuple() in a method returning std::pair should be return std::make_pair()). There are a few that are a little surprising, such as this one which doesn't seem related to tuple compatibility:

void func() {
  std::map<unsigned long, std::string> m;

  // OK
  std::vector<std::pair<unsigned long, std::string>> v1(m.begin(), m.end());

  // OK before, but now: error: no matching constructor for initialization
  std::vector<
      std::pair<const unsigned long, std::reference_wrapper<std::string>>>
      v2(m.begin(), m.end());
}

With the full backtrace:

D143914.cc:14:7: error: no matching constructor for initialization of 'std::vector<std::pair<const unsigned long, std::reference_wrapper<std::string>>>' (aka 'vector<pair<const unsigned long, reference_wrapper<basic_string<char>>>>')
      v2(m.begin(), m.end());
      ^  ~~~~~~~~~~~~~~~~~~
vector:393:66: note: candidate constructor not viable: no known conversion from 'iterator' (aka '__map_iterator<__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>') to 'size_type' (aka 'unsigned long') for 1st argument
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit vector(size_type __n, const allocator_type& __a);
                                                                 ^
vector:395:57: note: candidate constructor not viable: no known conversion from 'iterator' (aka '__map_iterator<__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>') to 'size_type' (aka 'unsigned long') for 1st argument
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(size_type __n, const value_type& __x);
                                                        ^
vector:463:57: note: candidate constructor not viable: no known conversion from 'iterator' (aka '__map_iterator<__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>') to 'const vector<pair<const unsigned long, reference_wrapper<string>>>' for 1st argument
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(const vector& __x, const __type_identity_t<allocator_type>& __a);
                                                        ^
vector:472:5: note: candidate constructor not viable: no known conversion from 'iterator' (aka '__map_iterator<__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>') to 'initializer_list<value_type>' (aka 'initializer_list<std::pair<const unsigned long, std::reference_wrapper<std::string>>>') for 1st argument
    vector(initializer_list<value_type> __il, const allocator_type& __a);
    ^
vector:488:5: note: candidate constructor not viable: no known conversion from 'iterator' (aka '__map_iterator<__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>') to 'vector<pair<const unsigned long, reference_wrapper<string>>>' for 1st argument
    vector(vector&& __x, const __type_identity_t<allocator_type>& __a);
    ^
vector:414:55: note: candidate template ignored: requirement '__is_exactly_cpp17_input_iterator<std::__map_iterator<std::__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>>::value' was not satisfied [with _InputIterator = iterator]
  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_InputIterator __first, _InputIterator __last);
                                                      ^
vector:427:55: note: candidate template ignored: requirement 'is_constructible<std::pair<const unsigned long, std::reference_wrapper<std::string>>, std::pair<const unsigned long, std::string> &>::value' was not satisfied [with _ForwardIterator = iterator]
  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_ForwardIterator __first, _ForwardIterator __last);
                                                      ^
vector:381:66: note: candidate constructor not viable: requires single argument '__a', but 2 arguments were provided
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit vector(const allocator_type& __a)
                                                                 ^
vector:391:66: note: candidate constructor not viable: requires single argument '__n', but 2 arguments were provided
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit vector(size_type __n);
                                                                 ^
vector:462:57: note: candidate constructor not viable: requires single argument '__x', but 2 arguments were provided
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(const vector& __x);
                                                        ^
vector:469:5: note: candidate constructor not viable: requires single argument '__il', but 2 arguments were provided
    vector(initializer_list<value_type> __il);
    ^
vector:480:5: note: candidate constructor not viable: requires single argument '__x', but 2 arguments were provided
    vector(vector&& __x)
    ^
vector:399:5: note: candidate constructor template not viable: requires 3 arguments, but 2 were provided
    vector(size_type __n, const value_type& __x, const allocator_type& __a)
    ^
vector:420:3: note: candidate constructor template not viable: requires 3 arguments, but 2 were provided
  vector(_InputIterator __first, _InputIterator __last, const allocator_type& __a);
  ^
vector:434:3: note: candidate constructor template not viable: requires 3 arguments, but 2 were provided
  vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a);
  ^
vector:377:5: note: candidate constructor not viable: requires 0 arguments, but 2 were provided
    vector() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
    ^

The only other breakage I found so far that might be worthy of mention is that constructing containers requires begin/end iterators of pairs, not tuples, but frameworks for doing functional-style programming often tuples to be fully generic, so this breaks. This seems like an intended effect of this patch, but makes things a little less ergonomic. Is there an idiomatic way to do that w/o having to write your own tuple->pair boilerplate?

void func2() {
  std::vector<int> v1 = {1, 2, 3};
  std::vector<std::string> v2 = {"One", "Two", "Three"};

  // Using a framework like https://github.com/ryanhaining/cppitertools
  auto v = iter::zip(v1, v2);  // Elements are `tuple<int &, std::string &>`

  // std::map constructor accepts iterator<pair>, not iterator<tuple>, so this no longer works
  std::map<int, std::string> m(v.begin(), v.end());

  // Now mapping it to pairs is required:
  auto v_pairs =
      v | iter::imap([](const auto& tuple) {
        return std::make_pair(std::get<0>(tuple), std::get<1>(tuple));
      });
  // OK
  std::map<int, std::string> m2(v_pairs.begin(), v_pairs.end());
}
ldionne marked an inline comment as done.Feb 14 2023, 2:40 PM

The only other breakage I found so far that might be worthy of mention is that constructing containers requires begin/end iterators of pairs, not tuples, but frameworks for doing functional-style programming often tuples to be fully generic, so this breaks. This seems like an intended effect of this patch, but makes things a little less ergonomic. Is there an idiomatic way to do that w/o having to write your own tuple->pair boilerplate?

Switching to -std=c++2b would fix that -- IMO that sounds reasonable but I am interested to know if you disagree.

Thanks a lot for the back and forth here, this is invaluable. I will take a look at the vector issue when I have a bit of time.

I would recommend you get started on fixing the obvious ones at least, but this is probably going to be paused for ~2 weeks since I'll go OOO next week.

The only other breakage I found so far that might be worthy of mention is that constructing containers requires begin/end iterators of pairs, not tuples, but frameworks for doing functional-style programming often tuples to be fully generic, so this breaks. This seems like an intended effect of this patch, but makes things a little less ergonomic. Is there an idiomatic way to do that w/o having to write your own tuple->pair boilerplate?

Switching to -std=c++2b would fix that -- IMO that sounds reasonable but I am interested to know if you disagree.

It isn't an option for us, but I don't think it's worth it to keep a non-conformant extension + complexity in libc++ just so we can avoid a little boilerplate to do tuple->pair manually in a couple places.

Thanks a lot for the back and forth here, this is invaluable. I will take a look at the vector issue when I have a bit of time.

I would recommend you get started on fixing the obvious ones at least, but this is probably going to be paused for ~2 weeks since I'll go OOO next week.

Sounds good, that timeline should work. Have a good break!

hans added a subscriber: hans.Feb 15 2023, 4:54 AM

Pinging vendors since this is technically a source break.

I didn't hit any issues with this in Chromium (Linux and Windows).

Mordante added inline comments.Feb 15 2023, 11:46 AM
libcxx/include/__utility/pair.h
67

I know the feeling of the rabbit hole :-/

135

Sound fine to me, just wanted to make sure it's a deliberate choice.

327–328

IIRC the style using an int is the preferred style. Feel free to ignore it when I'm wrong.

huixie90 added inline comments.Feb 15 2023, 12:38 PM
libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

speaking of subrange, I think it is worth adding more tests such as

Pair p = subrange1;
Pair p{subrange1};
auto p = static_cast<Pair>(subrange);

where the pair's constructor and subrange's conversion operator both can work.
(apparently subrange's conversion operator is more constrained than pair's constructor but I think that does not change anything?)

ldionne marked 4 inline comments as done.Feb 17 2023, 6:59 PM

Still chugging away, looks like this is going to be at least 50 breakages. Most of them are trivial to fix (e.g. return std::make_tuple() in a method returning std::pair should be return std::make_pair()). There are a few that are a little surprising, such as this one which doesn't seem related to tuple compatibility:

void func() {
  std::map<unsigned long, std::string> m;

  // OK
  std::vector<std::pair<unsigned long, std::string>> v1(m.begin(), m.end());

  // OK before, but now: error: no matching constructor for initialization
  std::vector<
      std::pair<const unsigned long, std::reference_wrapper<std::string>>>
      v2(m.begin(), m.end());
}

This seems to be the relevant part:

include/c++/v1/vector:427:55: note: candidate template ignored: requirement 'is_constructible<std::pair<const unsigned long, std::reference_wrapper<std::string>>, std::pair<const unsigned long, std::string> &>::value' was not satisfied [with _ForwardIterator = std::__map_iterator<std::__tree_iterator<std::__value_type<unsigned long, std::string>, std::__tree_node<std::__value_type<unsigned long, std::string>, void *> *, long>>]
  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_ForwardIterator __first, _ForwardIterator __last);
                                                      ^

It basically boils down to this:

#include <map>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

int main(int, char**) {
  std::map<unsigned long, std::string> m;

  // OK
  using Pair1 = std::pair<unsigned long, std::string>;
  std::vector<Pair1> v1(m.begin(), m.end());

  // OK before, but not anymore
  using Pair2 = std::pair<const unsigned long, std::reference_wrapper<std::string>>;
  static_assert(std::is_constructible_v<Pair2, std::pair<const unsigned long, std::string> &>);
}

This used to be accepted by libc++ but isn't anymore. This was never accepted by other standard libraries though: https://godbolt.org/z/c4svx1YTG.

Incidentally, checking for static_assert(std::is_constructible_v<Pair2, std::pair<const unsigned long, std::string> const&>); instead (notice I made the right hand side a const&) never works, on all stdlib implementations: https://godbolt.org/z/M8odqfvKM.

Hence, I'd argue this is somewhat brittle and I think this is both a corner case and also something that I'd be fine with breaking. Thoughts?

So, for the next steps:

  1. I want to get the subrange operator= const vs operator= situation understood
  2. I got back our internal build results and I did see a few failures similar to what @rupprecht saw. I think this will bite a few users, however the fix in most cases should be to use -std=c++2b and the code will work out of the box. None of these failures seemed too hard to fix.
libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

@tcanens

It doesn't trigger because ConstAssignable is not convertible from int*

Ah, yes, obviously that's *one* problem. But even after fixing it, the behavior for const and non-const is different AFAICT:

// const assignment
struct ConstAssignable {
  mutable int* ptr = nullptr;
  ConstAssignable() = default;
  constexpr ConstAssignable(int* p) : ptr(p) { } // enable `subrange::operator pair-like`
  constexpr ConstAssignable const& operator=(int* p) const { ptr = p; return *this; }
};

static_assert( std::is_assignable_v<std::pair<ConstAssignable, ConstAssignable> const&, std::tuple<int*, int*>>); // test the test
static_assert(!std::is_assignable_v<std::pair<ConstAssignable, ConstAssignable> const&, std::ranges::subrange<int*>>);

// non-const assignment
struct Assignable {
  int* ptr = nullptr;
  Assignable() = default;
  constexpr Assignable(int* p) : ptr(p) { } // enable `subrange::operator pair-like`
  constexpr Assignable& operator=(int* p) { ptr = p; return *this; }
};
int data[] = {1, 2, 3, 4, 5};
std::ranges::subrange<int*> a(data);
std::pair<Assignable, Assignable> p;
p = a; // that's fine

Did I miss something else here? There must be something else going on that causes the operator pair-like for std::subrange to not trigger when the elements are const-assignable but not "just" assignable. Honestly, I am left wondering whether a const operator= triggers the same sequence of implicit conversions than the non-const operator=.

tcanens added inline comments.Feb 17 2023, 7:53 PM
libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

The difference is that ConstAssignable isn't const-assignable from ConstAssignable (but Assignable is assignable from Assignable).

operator pair-like converts the subrange to a std::pair<ConstAssignable, ConstAssignable> (or std::pair<Assignable, Assignable>) and the assignment is performed from that, so you need const-assignment from ConstAssignable`, not int*.

however the fix in most cases should be to use -std=c++2b and the code will work out of the box.

It was unfortunate that libc++ shipped this extension in the first place, but given that it did, it seems a little weird to require all users to migrate away from using the long-standing extension, only to bring it (mostly) right back in C++23 mode.

I wonder if it'd make sense to keep the extension in earlier standards modes, but modified to have the C++23 requirements. I'm assuming that the differences between the previous "tuple-like" requirement and new "pair-like" requirement is likely to be irrelevant to the real-world usages -- maybe worth verifying that assumption?

The total impact for us is ~400 files, but very trivial in most cases, and usually only a couple impacted lines per file.

AFAICT, the breakage is entirely internal. I haven't found a single breakage in any external code. There could be any number of reasons to explain this, but generally speaking I would expect this to not have a broad impact -- the large breakage we see is probably the outlier.

I don't have a personal opinion whether this is good to land, as I can see arguments either way. I'd still like some time to continue cleaning things up -- 2-3 weeks is likely good enough to get most if not all of it done.

This used to be accepted by libc++ but isn't anymore. This was never accepted by other standard libraries though: https://godbolt.org/z/c4svx1YTG.

Incidentally, checking for static_assert(std::is_constructible_v<Pair2, std::pair<const unsigned long, std::string> const&>); instead (notice I made the right hand side a const&) never works, on all stdlib implementations: https://godbolt.org/z/M8odqfvKM.

Hence, I'd argue this is somewhat brittle and I think this is both a corner case and also something that I'd be fine with breaking. Thoughts?

I'm fine accepting this outcome, but OOC, do you know why it's (erroneously) being accepted prior to this patch? Is it doing an implicit conversion roundtrip through something like pair<T, U> -> tuple<T, U> -> pair<const T, std::reference_wrapper<U>>?

ldionne marked 4 inline comments as done.Apr 19 2023, 12:27 PM

I wonder if it'd make sense to keep the extension in earlier standards modes, but modified to have the C++23 requirements.

Instead, here's what I suggest: I refactored the extension so that it would be self-contained in a separate set of constructors and assignment operators. That way, it's mostly out of the way for implementing the actual standard-specified constructors and assignment operators.

As it stands, this patch should not be a source break in most cases. There might be a few tiny cases where the new conversions will behave unlike the old conversions, but it should more or less work out of the box. I suggest that we then mark these extensions as deprecated with a message telling users to move to C++23 to get this behavior, and then remove the extensions in a future release. That will provide a better experience for folks.

libcxx/include/__utility/pair.h
327–328

We use nullptr elsewhere in pair.h, so I'll stick to that.

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp
81

Thanks for the pointer!

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp
58

Ah, of course, thanks!

ldionne updated this revision to Diff 515042.Apr 19 2023, 12:28 PM
ldionne marked 3 inline comments as done.
ldionne edited the summary of this revision. (Show Details)

Rebase, reintroduce extension pre C++23.

ldionne updated this revision to Diff 515043.Apr 19 2023, 12:31 PM

Fix C++03

@rupprecht This version of the patch should significantly reduce the number of small failures you're seeing due to tuple/array/pair incompatibilities. Would you be able to confirm that this is landable as far as you're concerned?

ldionne updated this revision to Diff 515098.Apr 19 2023, 2:38 PM

Add missing include

ldionne updated this revision to Diff 515383.Apr 20 2023, 10:27 AM

Fix some GCC issues.

ldionne updated this revision to Diff 515703.Apr 21 2023, 5:47 AM

Add missing includes

@rupprecht I'd like to land this within the next few days. If you're able to confirm that this doesn't cause as many issues for you anymore, that would be great. Otherwise, i'll land this without your confirmation -- I am fairly confident this new patch should solve the vast majority of issues you were seeing.

@rupprecht I'd like to land this within the next few days. If you're able to confirm that this doesn't cause as many issues for you anymore, that would be great. Otherwise, i'll land this without your confirmation -- I am fairly confident this new patch should solve the vast majority of issues you were seeing.

Sorry, I've been out for a few days. Indeed, the impact seems much smaller.

The one breakage I found so far is:

std::vector<std::pair<int, Foo>> foo_pairs;
for (std::pair<int, Foo&> foo : foo_pairs) {...}

I expect not many breakages, so I'm fine with this landing. Thanks for checking in.

ldionne accepted this revision.Apr 28 2023, 12:15 PM

Awesome, let's do this then!

This revision is now accepted and ready to land.Apr 28 2023, 12:15 PM