This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][NFC] Make sequence containers slightly more SFINAE-friendly during CTAD.
ClosedPublic

Authored by var-const on Nov 19 2021, 8:11 PM.

Details

Summary

Disable the constructors taking `(size_type, const value_type&,
allocator_type) if allocator_type` is not a valid allocator.
Otherwise, these constructors are considered when resolving e.g.
(int*, int*, NotAnAllocator()), leading to a hard error during
instantiation. A hard error makes the Standard's requirement to not
consider deduction guides of the form `(Iterator, Iterator,
BadAllocator)` during overload resolution essentially non-functional.

The previous approach was to SFINAE away allocator_traits. This patch
SFINAEs away the specific constructors instead, for consistency with
basic_string -- see [LWG3076](wg21.link/lwg3076) which describes
a very similar problem for strings (note, however, that unlike LWG3076,
no valid constructor call is affected by the bad instantiation).

Diff Detail

Event Timeline

var-const requested review of this revision.Nov 19 2021, 8:11 PM
var-const created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 8:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Note: testing the difference between the old and new SFINAE (differing in whether the parameter is deduced) is tricky. The non-deduced version works well on Clang for SFINAE purposes -- I can be wrong, but it seems like implementation-specific behavior (or worse, undefined behavior that happens to "work") that we cannot rely on. In tests, the difference only comes up in type traits like static_assert(std::is_default_constructible<std::vector<int, BadAlloc>>) -- it looks like the compiler checks all function signatures but not function bodies in that case, leading to a situation where there's a hard error due to incorrect SFINAE while "obvious" errors in the function bodies when the bad allocator is used are ignored.

For testing, I think you've got the right idea: the only way to test this (besides ugly/fragile .verify.cpp tests) will be SFINAE. You just implemented SequenceContainerDeductionGuidesSfinaeAway in a previous patch, right? So shouldn't this just be a one- or two-line addition to that function?

// Cannot deduce from (size_type, value_type, BAD_alloc)
static_assert(SFINAEs_away<Container, size_t, void*, BadAlloc>);

If that doesn't work as a regression-test, then I'm confused about the effect of this patch.

libcxx/include/deque
1275

Why the extra space on all these lines? Just to highlight this section in Phab? (I would guess "removing hard tabs," but we reject hard tabs at buildkite time.)

1293–1294

This change seemed OK until I realized that I didn't understand the maze that gets us from _Allocator (on line 1293) to allocator_type (on line 1294). From the context, I assume that allocator_type here is understood by the compiler as an alias for __base::allocator_type i.e. __deque_base<_Tp, _Allocator>::allocator_type i.e. _Allocator i.e. deque's own _Allocator parameter, and then CTAD applies. I just don't fully understand how the CTAD machinery is working — it has to instantiate __deque_base<SomeType, BadAllocator> in order to find out that __deque_base<_Tp, _Allocator>::allocator_type is _Allocator?
But when I try that myself on Godbolt, the compiler can't thread that maze.
https://godbolt.org/z/oMTqqvhhW
I must be doing something wrong in that Godbolt, but what?

libcxx/include/string
855

While you're here, please remove , nullptr_t from the existing default argument.

var-const added a comment.EditedNov 20 2021, 8:43 PM

For testing, I think you've got the right idea: the only way to test this (besides ugly/fragile .verify.cpp tests) will be SFINAE. You just implemented SequenceContainerDeductionGuidesSfinaeAway in a previous patch, right? So shouldn't this just be a one- or two-line addition to that function?

// Cannot deduce from (size_type, value_type, BAD_alloc)
static_assert(SFINAEs_away<Container, size_t, void*, BadAlloc>);

If that doesn't work as a regression-test, then I'm confused about the effect of this patch.

Yes, this works (the existing

// Cannot deduce from (iter, iter, BAD_alloc)
static_assert(SFINAEs_away<Container, Iter, Iter, BadAlloc>);

works as well -- it would give a hard error without either this patch or the previous approach that SFINAEd away allocator_traits).

What I meant to say is that it's hard to test for the difference between having one or two arguments in the template function:

// This is, IIUC, the correct form because it makes sure that `__is_allocator` is called on a deduced type.
template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value> >
// However, this works as well, even though `_Allocator` is not, AFAICT, a deduced type. It seems to me that
// it shouldn't work and perhaps Clang is being permissive here -- otherwise I must be missing something.
template <class = __enable_if_t<__is_allocator<_Allocator>::value> >

I couldn't think of a great way to test this difference. In existing tests, it only shows up in certain type traits, e.g. static_assert(std::is_default_constructible_v<T>) works with the 2-argument form but not 1-argument form. However, it seemed like something that _happened_ to depend on this difference rather than actively check for it, if that makes sense.

libcxx/include/forward_list
670

(1) Don't you need to guard this one on __is_allocator<allocator_type>, too? I think we're missing this case in forwardlist.cons/deduct.fail.cpp.
(2) Personally I'd be interested in just disabling the implicit guides for all these ctors (via __identity_t), and using explicit deduction guides for everything instead. Seems like explicit deduction guides would be a lot less error-prone and a lot more self-documenting, and I believe we'd be allowed to do it under the As-If Rule (I don't think a user could detect our trick).

673–680

Yes, this works (the existing SFINAEs_away<Container, Iter, Iter, BadAlloc> works as well -- it would give a hard error without either this patch or the previous approach that SFINAEd away allocator_traits)

Well, an existing test can't be a regression test, by definition. It already passed without this patch. :) Unless you're calling this PR "NFC [no functional change [intended]]", then it ought to have some functional effect and that means we ought to be able to write a regression test for that specific effect.

What I meant to say is that it's hard to test for the difference between having one or two arguments in the template function:

// This is, IIUC, the correct form because it makes sure that `__is_allocator` is called on a deduced type.
template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value> >
// However, this works as well, even though `_Allocator` is not, AFAICT, a deduced type. It seems to me that
// it shouldn't work and perhaps Clang is being permissive here -- otherwise I must be missing something.
template <class = __enable_if_t<__is_allocator<_Allocator>::value> >

I couldn't think of a great way to test this difference.

I think it's going to be difficult/impossible in this case, yeah. Here's a reduced test case:
https://godbolt.org/z/cbaM9hsxj
Normally the one-template-argument version is wrong because the __enable_if_t it will be instantiated relatively eagerly. (As soon as the class definition is instantiated, it'll instantiate all of the member functions' declarations, which will include the one that depends on a non-existent enable_if_t<false> type.)
However, in this case, CTAD happens "more eagerly than that." First we transform the constructor template declaration into CTAD's fictional function template declaration, discover that it'd be ill-formed, and CTAD fails; so then we decide not to instantiate the class definition at all.
However, if someone later attempts to instantiate vector<int, NotAnAllocator> by name, the single-template-argument ctor will cause failure, where the two-template-argument ctor would not cause failure.
However, in this case, we don't care because instantiating vector<int, NotAnAllocator> is not allowed.
Basically, the two-template-argument version template<class Y=X, class=enable_if_t<f(Y)>> is absolutely needed when !f(X) is legal but you're just trying to SFINAE away part of the class's API in that case. When !f(X) is flat-out illegal and you're just trying to teach that fact to CTAD, you can get away with the one-template-argument version.

ldionne requested changes to this revision.Nov 22 2021, 10:03 AM
ldionne added a subscriber: ldionne.

My understanding is that this is effectively a NFC, since you were previously using __allocator_traits to achieve the exact same effect. This is only making all the containers consistent. That's why there are no test changes, right?

libcxx/include/deque
1293

It's not really a "maybe" allocator. It's definitely _Allocator. I've also used _DependentAlloc in the past (e.g. in <tuple>) for the same purpose, in case you like that better.

libcxx/include/forward_list
670

On (2), my vote would be towards keeping our implementation as close to the Standard as possible, which means keeping a mix of implicit and explicit deduction guides like we do right now. I don't like how the Standard is specified in that regard (I'd rather have explicit deduction guides only*), however at least we can more easily report issues against the Standard if we don't stray away from it, and it's easier to validate whether we are conforming.

(*) I'd be even happier if implicit deduction guides didn't even exist, but that's a different story.

673–680

Interesting, thanks for your analysis. I still think we should go with the two argument case for simple consistency and to avoid confusion. The fact that CTAD deduction guide generation will not cause the class to be instantiated is IMO an interesting detail, but one what folks shouldn't have to think about when reading this code.

This revision now requires changes to proceed.Nov 22 2021, 10:03 AM
var-const marked 6 inline comments as done.

Feedback

libcxx/include/deque
1275

Removed (I think).

1293

What I was trying to convey is that the type is being tested for whether it is an allocator. Changed to _DependentAlloc.

1293–1294

I think that, unlike the other type aliases, allocator_type doesn't depend on the base class. See line 1257:

typedef _Allocator allocator_type;
libcxx/include/forward_list
670

(1) I'm not sure. My primary concern was with the overloads that overlap with explicit deduction guides, and I haven't seen a case where this two-argument overload would cause issues. I'd rather keep the scope limited because expanding it later if necessary is straightforward whereas removing the additional constraints, should the need arise, would likely break users.

673–680

+1, thanks a lot for the detailed analysis!

I have marked this patch as [NFC] because it simply changes the mechanism for achieving the same effect (making sure bad allocators don't cause hard errors).

We have to use the two-argument version because the single-argument version causes compilation errors on test code like static_assert(std::is_default_constructible<std::vector<int, BadAlloc>>).

libcxx/include/string
855

Done.

var-const retitled this revision from [libcxx] Make sequential containers slightly more SFINAE-friendly during CTAD. to [libcxx][NFC] Make sequential containers slightly more SFINAE-friendly during CTAD..Nov 30 2021, 12:18 AM
ldionne accepted this revision.Nov 30 2021, 3:17 AM

Thanks! LGTM but the C++03 CI will have to pass. I haven't looked too closely but I don't think it's a major issue.

libcxx/include/deque
1293

Oh, I see what you were doing now. Hmm I guess I'm neutral on the rename then. I guess I'm still slightly happier with _DependentAlloc since it's a tiny bit more consistent with other places in the library.

This revision is now accepted and ready to land.Nov 30 2021, 3:17 AM

No objection from me, modulo that the C++03 forward_list test is still failing for some reason.

libcxx/include/deque
1293–1294

Ah, I'd missed that line. Okay, there's less of a maze than I thought. :)
I still think it's confusingly subtle that the _Allocator on line 1293 is understood by the compiler as the same type as the allocator_type on line 1294, but I have no concrete suggestion to make it better. (We could use allocator_type on line 1293: _DependentAlloc = allocator_type: that's still kinda weird. Or we could use _Allocator on line 1294: size_type __n, const value_type& __v, const _Allocator& __a: that's also weird.) No action required AFAIC.

libcxx/include/forward_list
673–680

We have to use the two-argument version because the single-argument version causes compilation errors on test code like static_assert(std::is_default_constructible<std::vector<int, BadAlloc>>).

IIUC, that's expected and a good thing. A user who instantiates std::vector<int, BadAlloc> is in IFNDR-land and deserves a hard error, just as if they instantiated std::vector<int&, std::allocator<int&>> or std::function<int*>.
I don't think we should jump through any hoops to either permit or reject that bad code: let's just do whatever seems natural. I'm not necessarily agitating in favor of the single-argument version, just disagreeing with the conclusion that we have to do the two-argument version in order to accept this IFNDR code. In particular, let's not add any tests that rely on our specific behavior in this case.

Fix forward_list in C++03 mode.

ldionne added inline comments.Nov 30 2021, 1:24 PM
libcxx/include/forward_list
673–680

I think I agree with this. I guess we should be using the one argument version in that case, to avoid promoting the use of something crazy like std::vector<int, bad_alloc>. It's just unfortunate that it will work with CTAD through this kind of complex chain of things.

var-const updated this revision to Diff 390842.Nov 30 2021, 3:48 PM
var-const marked an inline comment as done.

Switch to use the one-argument version.

ldionne accepted this revision.Dec 1 2021, 8:06 AM

Thanks! Also, nice that we are fixing tests that were previously IFNDR!

var-const edited the summary of this revision. (Show Details)Dec 1 2021, 11:53 AM
var-const retitled this revision from [libcxx][NFC] Make sequential containers slightly more SFINAE-friendly during CTAD. to [libcxx][NFC] Make sequence containers slightly more SFINAE-friendly during CTAD..