This is an archive of the discontinued LLVM Phabricator instance.

[libc++] P0433R2: test that deduction guides are properly SFINAEd away.
ClosedPublic

Authored by var-const on Oct 31 2021, 8:35 PM.

Details

Summary

Deduction guides for containers should not participate in overload
resolution when called with certain incorrect types (e.g. when called
with a template argument in place of an InputIterator that doesn't
qualify as an input iterator). Similarly, class template argument
deduction should not select unique_ptr constructors that take a
a pointer.

The tests try out every possible incorrect parameter (but never more
than one incorrect parameter in the same invocation).

Also add deduction guides to the synopsis for associative and unordered
containers (this was accidentally omitted from D112510).

Diff Detail

Event Timeline

var-const created this revision.Oct 31 2021, 8:35 PM
var-const requested review of this revision.Oct 31 2021, 8:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2021, 8:35 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 383709.Oct 31 2021, 8:43 PM

Uncomment working test

var-const updated this revision to Diff 383710.Oct 31 2021, 8:45 PM

Missing includes

ldionne requested changes to this revision.Nov 1 2021, 7:40 AM

Thanks a lot for the patch! I have some requests for changes but overall this is looking pretty good!

libcxx/docs/Status/Cxx17Papers.csv
97

The next release this gets in will be 14.0, not 13.0! 13.0 is the current latest release.

libcxx/include/__memory/allocator_traits.h
352–354

You can replace the link once you have a LWG issue number!

libcxx/include/map
229

Those should be marked as being new in // C++17. This applies elsewhere, so you might want to do a NFC patch that adjusts that in all the containers, or just fix it everywhere in this commit. I think both are reasonable, fixing everything right now is probably less churn so I'd go for that.

libcxx/include/set
477

I checked and this wasn't used anywhere -- that's why it's being removed right?

libcxx/test/std/containers/associative/set/set.cons/deduct.pass.cpp
190–191

This file is indented at 2 columns.

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp
251–263

I don't feel like this list adds very much since you commented the individual test cases below (thanks a bunch for doing that BTW). Feel free to keep them if you want, that's just my .02.

273

This is never used, and that's why the GCC build is failing. Did you forget a test case?

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

Is there a reason why this one isn't based on SFINAE checks like the container tests?

libcxx/test/support/sfinaes_away.h
15–28 ↗(On Diff #383710)

IMO this utility is a bit too narrow in usefulness to make it its own header, since it only works for checking that a class template's CTAD SFINAEs away. If this were for checking that a general expression SFINAEs away, then I'd be in favour of keeping it standalone.

I would suggest either moving it to deduction_guides_sfinae_checks.h or generalizing it.

This revision now requires changes to proceed.Nov 1 2021, 7:40 AM
Quuxplusone requested changes to this revision.Nov 1 2021, 8:02 AM

I've looked only at the new deque code, but my comments probably apply equally to all the containers.

libcxx/include/deque
1269

I don't believe this is needed. By the time we are instantiating the members of deque<T, allocator_type>, we are guaranteed that allocator_type is an Allocator type.

1572

FWIW, __iter_value_type<_InputIterator> means typename iterator_traits<_InputIterator>::value_type, which is ill-formed (SFINAE-friendly) unless _InputIterator is an iterator type. However, I guess what you're doing here is protecting against the possibility that _InputIterator could be an output iterator? ...But you have added no tests for that new behavior, AFAICT.

Please replace all your new tests with something simple that demonstrates the new behavior, e.g. https://godbolt.org/z/aKeh1fE9Y

template<class... Args>
auto deque_has_ctad_from(int, Args&&... args) -> decltype(std::deque(std::forward<Args>(args)...), true) { return true; }
template<class... Args>
bool deque_has_ctad_from(long, Args&&...) { return false; }

int main() {
    std::vector<int> v;
    auto out = std::back_inserter(v);
    assert(!deque_has_ctad_from(42, out, out));
}
libcxx/test/support/sfinaes_away.h
15–28 ↗(On Diff #383710)

+1 and more so: I don't think this PR should add any new headers. Each "deduct.pass.cpp" should just introduce a four-line bool foo_has_ctad_from(42, args...) (for the appropriate container-name foo).

ldionne added inline comments.Nov 1 2021, 10:59 AM
libcxx/include/deque
1572

FWIW, the Standard does not request that we check for anything more than "an integral type is not an input iterator". I agree we should change the tests that currently pass a struct BadIter { }; to make them pass a using BadIter = some-output-iterator;. And we need to mark those as libc++ specific, since that's technically just our QOI.

libcxx/test/support/deduction_guides_sfinae_checks.h
58

Per our offline discussion, let's add a test where we pass an integer type as an iterator and watch it fail, and also a libc++ specific test that passes an output iterator.

Note that we can't add an integer type test to the sequence containers, because those won't SFINAE away (the sequence containers can deduce from FOO(size_type, value_type) when passed two integers).

libcxx/test/support/sfinaes_away.h
15–28 ↗(On Diff #383710)

I disagree with that. I looked at the PR in a previous stage where the checks were duplicated, and there was *a lot* of code duplication. Since most of these requirements are specified in http://eel.is/c++draft/container.requirements.general, I think it makes sense to colocate the tests the way they are right now.

var-const updated this revision to Diff 383920.Nov 1 2021, 4:36 PM
var-const marked 6 inline comments as done.

Address feedback

libcxx/include/__memory/allocator_traits.h
352–354

Done, thanks!

libcxx/include/deque
1269

Sorry, I actually had a comment about this but forgot to post it before sending the patch out for review. Unfortunately, both of these changes (not using __base here and using the newly-added __allocator_traits) are necessary -- without them, the compiler would attempt to instantiate allocator_traits with allocator_type, which would result in instantiation errors if the given allocator_type doesn't have value_type. This affects all sequential containers and leads to verbose errors when trying to deduce from a bad allocator rather than silently sfinae'ing away a bad invocation.

I can dig more into this if you feel it warrants investigation.

1572

For adding this check, my main motivation was to make the requirement explicit, and also to be consistent with other similar checks. It seems to me that the fact that __iter_value_type also SFINAEs away "bad" input iterators is arguably an implementation detail, or at the very least, it's not as obvious without knowing the implementation as an explicit check.

I changed the tests that deal with bad iterators to use output iterators and also, where possible, to use integral types. PTAL.

libcxx/include/map
229

Fixed everything in this patch.

libcxx/include/set
477

Sorry, I wrote a comment about it but forgot to post it. Yes, I was investigating a compiler error stemming from the instantiation of __base requested here and noticed that __node_holder appears unused, so it seemed reasonable to remove it. I can split it into a separate small patch if you prefer.

504

Without this change (which is used in map and multimap) certain incorrect invocations try to do instantiations that depend on _Compare, leading to verbose errors.

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp
251–263

Yeah, I was on the fence about leaving these in -- removed now.

273

Thanks for spotting! Yes, looks like I missed 3 test cases.

libcxx/test/std/containers/container.adaptors/stack/stack.cons/deduct.pass.cpp
140

stack and queue have the same tests, but it didn't seem worthwhile to refactor them out in a separate function.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

For containers, the requirement is for deduction guides to not participate in overload resolution, but for unique pointers, the requirement is that If class template argument deduction would select the function template corresponding to this constructor, then the program is ill-formed. I interpreted ill-formed as "resulting in a hard error" -- please let me know if this is incorrect.

libcxx/test/support/deduction_guides_sfinae_checks.h
23

Please let me know if you'd prefer shorter names for these functions.

libcxx/test/support/sfinaes_away.h
15–28 ↗(On Diff #383710)

Moved SFINAEs_away to deduction_guides_sfinae_checks.h.

I'd prefer to avoid duplication if possible. I had each test file have its own tests and definitions of everything in earlier states of this patch. It was a pain to modify, and I made some inevitable bugs forgetting to apply a change to all the relevant files.

var-const added inline comments.Nov 1 2021, 4:42 PM
libcxx/test/support/deduction_guides_sfinae_checks.h
23

(I mean SequenceContainerDeductionGuidesSfinaeAway and similar functions)

175

Note: alternatively, I could create a new struct for OutputIter, similar to the priority_queue test. Let me know if you'd prefer that.

var-const edited the summary of this revision. (Show Details)Nov 1 2021, 7:24 PM
ldionne accepted this revision.Nov 2 2021, 10:18 AM

LGTM with the LIBCPP_STATIC_ASSERT simplification.

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp
291

You can replace this by LIBCPP_STATIC_ASSERT and avoid the #ifdef dance.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

Oh, then this is correct. It's a really unusual specification for this kind of failure though.

var-const updated this revision to Diff 384262.Nov 2 2021, 3:52 PM
  • use LIBCPP_STATIC_ASSERT;
  • rebase on main.
var-const updated this revision to Diff 384277.Nov 2 2021, 4:47 PM

Update the unique_ptr tests for the new wording in C++20.

var-const marked an inline comment as done.Nov 2 2021, 4:52 PM
var-const added inline comments.
libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp
291

Done, thanks!

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

I looked into this more closely, and there's actually been a change on this between C++17 and C++20.

In C++17, the requirement is:

If class template argument deduction would select the function template corresponding to this constructor, then the program is ill-formed.

However, [P1460](wg21.link/p1460) changed this in C++20 to:

Mandates: This constructor is not selected by class template argument deduction.

Now the words ill-formed are gone, and with the new wording, it seems like these constructors should just SFINAE away without causing a hard error. Moreover, this seems like a slight change in behavior. I don't see any discussion of this case in the paper, and I presume the new behavior is slightly more user-friendly.

It seems reasonable to me to only implement and test the new behavior, including in C++17 mode as well (FWIW, it was already implemented in a SFINAE-friendly way). Because of that, I replaced the previous deduct.fail.cpp file with a new deduct.pass.cpp test that uses SFINAEs_away. Let me know what you think!

var-const edited the summary of this revision. (Show Details)Nov 2 2021, 4:55 PM
var-const marked an inline comment as done.Nov 2 2021, 5:27 PM
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

"Mandates" always means "static_assert, not SFINAE." When the standard means SFINAE, it uses "Constraints" instead. However, I think this is an editorial defect in p1460's changes: it replaced a soft "Remark" with a hard "Mandates." It should have said:

Remarks: This constructor is never selected by class template argument deduction.

or simply

unique_ptr(type_identity_t<pointer> p)

libstdc++ and MSVC both interpret the intent as "this constructor shouldn't contribute to CTAD," which matches what you settled on here, IIUC.
I'll email LWG suggesting a new issue to clarify this wording.

Quuxplusone added inline comments.Nov 3 2021, 6:57 PM
libcxx/include/set
504

I vaguely recall hitting similar issues around CTAD in these containers, and suggesting that we firewall several (maybe all) of these member typedefs with __identity_t. But back then there was some reason we didn't do that. Probably just inertia, but I'm not sure. Looks like some discussion exists in D58587.

libcxx/include/vector
492

I still don't get this diff; but is it possible that it'll be a moot point after D112976?

libcxx/test/support/deduction_guides_sfinae_checks.h
24–33

Even after seeing @ldionne's comment, personally, I'd still write these tests without any additional headers. For a sequence container, we'd have this:

template<class... Args>
auto deque_has_no_ctad_from() -> decltype(std::deque(std::declval<Args>()...));
template<class... Args>
constexpr bool deque_has_no_ctad_from() { return true; }

int main(int, char**)
{
    // all the deduct.pass.cpp tests, and then these lines:

    using OutIt = std::insert_iterator<std::vector<int>>;
    struct NotAnAllocator {};

    LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt>());
    LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt, std::allocator<int>>());
    static_assert(deque_has_no_ctad_from<int*, int*, NotAnAllocator>());
}

And for an associative container like map we'd have this:

template<class... Args>
auto map_has_no_ctad_from() -> decltype(std::map(std::declval<Args>()...));
template<class... Args>
constexpr bool map_has_no_ctad_from() { return true; }

int main(int, char**)
{
    // all the deduct.pass.cpp tests, and then these lines:

    using VT = std::pair<const int, int>;
    using OutIt = std::insert_iterator<std::vector<int>>;
    struct NotAnAllocator {};

    static_assert(map_has_no_ctad_from<int, int>());
    static_assert(map_has_no_ctad_from<int, int, std::less<int>>());
    static_assert(map_has_no_ctad_from<int, int, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<int, int, std::less<int>, std::allocator<VT>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::allocator<VT>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<VT*, VT*, std::allocator<VT>, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<VT*, VT*, std::less<int>, NotAnAllocator>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::allocator<VT>, std::allocator<VT>>);
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::less<int>, NotAnAllocator>);
}

I just don't see the need for 300 lines of code off in test/support/ (plus another 100 lines in the priority_queue test) when we could have ~20 lines per container, right there in each container's test directory.

var-const added inline comments.Nov 5 2021, 6:36 PM
libcxx/include/vector
492

Without this change, the compiler errors out trying to instantiate allocator_traits with an incorrect allocator instead of silently SFINAE'ing away the deduction guide. However, like you said, it's a moot point after the other patch lands, so I don't think we need to spend time investigating this issue.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp
9 ↗(On Diff #383710)

I'll email LWG suggesting a new issue to clarify this wording.

Thanks!

libcxx/test/support/deduction_guides_sfinae_checks.h
24–33

A better example would be the unordered containers which have 36 test cases per container. While we could reduce the line count by omitting comments, I find this state less readable and much harder to modify or to verify that every case is covered. While I was working on this patch, I had to change the tests multiple times, and I found comments to be very helpful when making changes. Moreover, duplicating each change at least 4 times is annoying and error-prone. Finally, if the tests were duplicated, the reader might have to manually compare different files to make sure they are exactly the same -- I have found several inconsistencies between similar duplicated tests while working on this patch. For these reasons, I'd prefer to keep the current state, even if it is more verbose.

I don't see the fact that the tests covering the same aspect of functionality are all in one place as a downside, necessarily. After all, most of these requirements are placed by the Standard on different classes of containers, not on individual containers.

What is the downside of adding a new header? I'm not sure why significant code duplication would be preferable, and my experience working on this patch suggests otherwise.

var-const updated this revision to Diff 385219.Nov 5 2021, 6:38 PM

Rebase on main.

libcxx/test/support/deduction_guides_sfinae_checks.h
24–33

What is the downside of adding a new header?

Just that it's always easier to understand code that's in one place, as opposed to having to skip back and forth between different files to get the full picture. Also for "attribution": when a test fails, it's nice to be able to point to the line that failed, which should be in the test that failed; having asserts in .h files included from multiple tests makes that just a bit harder. Also in case of later refactoring: suppose set and map have the same CTAD guides today, but then map gains one new guide tomorrow; with the "one line of code per test case" version, that's a one-line diff to add the new test case, but with the "everything calls into the same .h file" version, it might require arbitrarily invasive refactoring of the shared .h file. ("Copy-on-write," in other words.)
Basically, this is the software-engineering version of the "aliasing plus mutation" problem. Aliasing alone isn't a problem if everything is const; mutation alone isn't a problem if nothing is aliased; but combine aliasing with mutation and suddenly you have the potential for bugs and race conditions.
Which is not to say that plenty of programmers can't deal just fine with aliasing plus mutation. I mean, C++ does it. ;) So, having stated my view, I'll now let this thread be.

var-const added inline comments.Nov 8 2021, 12:09 AM
libcxx/test/support/deduction_guides_sfinae_checks.h
24–33

Thank you for the writeup! I still think that, given the amount of duplication, having common functions is the lesser evil in this case, but I understand your concerns much better now.

var-const updated this revision to Diff 385585.Nov 8 2021, 12:05 PM

Rebase on main

var-const updated this revision to Diff 385617.Nov 8 2021, 1:34 PM

Fix vector after merge.

ldionne accepted this revision.Nov 9 2021, 5:00 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 9 2021, 9:32 AM
This revision was automatically updated to reflect the committed changes.