Page MenuHomePhabricator

[libc++] Guard std::ranges under _LIBCPP_HAS_NO_INCOMPLETE_RANGES.
ClosedPublic

Authored by Quuxplusone on Feb 1 2022, 1:53 PM.

Details

Summary

For @ldionne and @jloser's perusal.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 1 2022, 1:53 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 1 2022, 1:53 PM
ldionne requested changes to this revision.Feb 1 2022, 2:32 PM

Apart from my comments, this looks reasonable and desirable in time for LLVM 14.

libcxx/include/__algorithm/in_in_out_result.h
23

Please only add _LIBCPP_HAS_NO_INCOMPLETE_RANGES in this patch. If we decide to change the meaning of _LIBCPP_HAS_NO_CONCEPTS to not include _LIBCPP_STD_VER > 17, we should do it in a separate patch.

libcxx/test/libcxx/ranges/has-no-incomplete-ranges.compile.pass.cpp
2

I don't think this test adds that much value -- instead, we should just add a CI configuration that tests without incomplete features, and also make sure not to needlessly mark tests as UNSUPPORTED (like we did for the span ones).

This revision now requires changes to proceed.Feb 1 2022, 2:32 PM
Quuxplusone added inline comments.Feb 1 2022, 2:36 PM
libcxx/test/libcxx/ranges/has-no-incomplete-ranges.compile.pass.cpp
2

We should definitely add a CI configuration that includes libcpp-has-no-incomplete-ranges and/or libcpp-has-no-incomplete-format. However, this test is the only test that actually tests the point of that config flag: that, when it is enabled, you don't get these features. Otherwise, we could just remove the macro from the entire codebase (or misspell it) and CI would never notice that the features had become enabled.
(Like how right now std::ranges::advance is enabled: do we want it to be? Maybe we do, in which case this test is not harmless but actually testing something we don't want. But we should consciously decide that.)

ldionne added inline comments.Feb 1 2022, 2:43 PM
libcxx/test/libcxx/ranges/has-no-incomplete-ranges.compile.pass.cpp
2

Okay, I agree. Can we instead use a .verify.cpp test that does namespace rng = std::ranges and checks for an error? As it stands, the test is technically IFNDR.

Also, regarding testing a configuration without incomplete features, see https://reviews.llvm.org/D118740.

Quuxplusone added inline comments.Feb 1 2022, 4:39 PM
libcxx/test/libcxx/ranges/has-no-incomplete-ranges.compile.pass.cpp
2

Can we instead use a .verify.cpp test that does namespace rng = std::ranges and checks for an error?

Sure, I suppose. The reason I did it this way specifically is that I needed to use the compiler diagnostics on failure to detect exactly which header(s) were still defining namespace ranges. (The accompanying note points to the offending namespace ranges line.) Just seeing "Expected an error and didn't get one" gave no clue as to where the problem was coming from, so I wasn't able to iterate on that. But if we keep this clean going forward, then the lack of helpful hints shouldn't be a huge deal.

UNSUPPORTED the affected tests. There are a lot of affected tests.

jloser added inline comments.Feb 2 2022, 9:14 AM
libcxx/test/support/test_iterators.h
494–495

Do we want to keep TEST_STD_VER > 17 check since we have !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)?

Quuxplusone added inline comments.Feb 2 2022, 9:36 AM
libcxx/test/support/test_iterators.h
494–495

Yes, we must, because the #if-ed-out code uses C++20 Concepts syntax, so it cannot be compiled e.g. by MSVC in -std:c++14 mode.

I am somewhat worried about this. On the one hand this is the "correct" thing to do, however I am concerned that this could actually end up removing some features we've shipped in LLVM 13 (accidentally), and hence break users.

So far, I get the feeling that we've (mistakenly) used _LIBCPP_HAS_NO_INCOMPLETE_RANGES as a proxy for something closer to "no ranges functions and types", not for "nothing in the ranges namespace". So, for example, we've been happily providing concepts that are related to ranges (which as a result makes it possible to provide stuff like span constructors). I suspect we'd have to touch way fewer files if we still allowed a couple of ranges-related functionality to creep in despite _LIBCPP_HAS_NO_INCOMPLETE_RANGES being defined, but I am having trouble defining what should and should not be guarded precisely. This is something that's harder to do in hindsight -- but in our defence, that's one of the first time trying to ship such a huge feature that spreads over months and months of development, so hiccups were to expect.

I'm not really sure how to cut the cake, but perhaps I'd try to only remove:

  • the content in __algorithm, __filesystem, __functional
  • "concrete types" in __iterator, like __iterator/common_iterator.h, counted_iterator.h, etc.
  • the uninitialized algorithms
  • the "concrete views" in __ranges/, like common_view.h, copyable_box.h, drop_view.h, etc.

In particular, that should leave us with most of the concepts defined unless I'm mistaken. And those concepts should be reasonable stable AFAICT, and they will enable most of the important stuff like span and string_view. What do you think?

libcxx/include/__iterator/concepts.h
29 ↗(On Diff #405300)

Geez, this one really hurts. We're so close to being able to provide these concepts. Are we only missing iter_move?

libcxx/include/span
213

This one hurts too, because that's amongst the only ways we have to construct a span.

I am somewhat worried about this. On the one hand this is the "correct" thing to do, however I am concerned that this could actually end up removing some features we've shipped in LLVM 13 (accidentally), and hence break users.

Yup.

So far, I get the feeling that we've (mistakenly) used _LIBCPP_HAS_NO_INCOMPLETE_RANGES as a proxy for something closer to "no ranges functions and types", not for "nothing in the ranges namespace". So, for example, we've been happily providing concepts that are related to ranges (which as a result makes it possible to provide stuff like span constructors). I suspect we'd have to touch way fewer files if we still allowed a couple of ranges-related functionality to creep in despite _LIBCPP_HAS_NO_INCOMPLETE_RANGES being defined, but I am having trouble defining what should and should not be guarded precisely.

Also yup. I think if you're worried about ABI issues, then it might make sense to restrict the disabled stuff to just the view adaptors — e.g. guard ranges::reverse_view, but don't guard ranges::distance because although the behavior of ranges::distance will change, the struct layout will not (it'll always be an empty type).

I'm not really sure how to cut the cake, but perhaps I'd try to only remove:

  • the content in __algorithm, __filesystem, __functional

The content in <filesystem> is just opting into view-ness and/or borrowed-ness for certain existing types. I think that's harmless. The only reason I disabled it in this PR right now is because my chosen criterion was "When the macro is present then namespace ranges is not present, period."

The content in <functional>, i.e. ranges::less etc., is also stateless empty types, like ranges::distance, where again it's quite possible that their implementation will change but their struct layout definitely won't. So if (if!) our criterion is going to be "enable the empty types and disable the non-empty types," then ranges::less should be enabled, as should all of the algorithms we've implemented so far (which maybe is the empty set ;)).

Is this kind of what you had in mind with "concrete views" and "concrete types" — basically a criterion of "any non-empty type should be guarded by the macro"?

In particular, that should leave us with most of the concepts defined unless I'm mistaken. And those concepts should be reasonable stable AFAICT, and they will enable most of the important stuff like span and string_view. What do you think?

I think certain concepts, like output_iterator, are known to be unstable because there's already changes coming in C++23 and/or late-breaking C++20. (Unless all the changes have already come? Aren't there still major changes in flight re: move-only iterator concepts?) But since concepts don't affect struct layout, maybe we don't care about them from an ABI perspective. I'm still extremely hazy on why we're guarding any of this stuff — what failure mode we're afraid of.

The content in <filesystem> is just opting into view-ness and/or borrowed-ness for certain existing types. I think that's harmless. The only reason I disabled it in this PR right now is because my chosen criterion was "When the macro is present then namespace ranges is not present, period."

Right.

The content in <functional>, i.e. ranges::less etc., is also stateless empty types, like ranges::distance, where again it's quite possible that their implementation will change but their struct layout definitely won't. So if (if!) our criterion is going to be "enable the empty types and disable the non-empty types," then ranges::less should be enabled, as should all of the algorithms we've implemented so far (which maybe is the empty set ;)).

Is this kind of what you had in mind with "concrete views" and "concrete types" — basically a criterion of "any non-empty type should be guarded by the macro"?

Yes, although I would also remove actual functionality-providing algorithms. I guess it's easier to explain it by saying what we'd keep. The only things we'd keep is whatever is necessary for the non-ranges concepts to work, which is (I think) iter_move (and perhaps a few others). WDYT?

I'm still extremely hazy on why we're guarding any of this stuff — what failure mode we're afraid of.

What we want to guard against is users starting to use functionality that is going to break in the future (either ABI or API wise). I also want to guard against shipping an inconsistent subset of <ranges> because it's going to be a frustrating experience for users. For example, I don't want to ship ranges::drop_view since we're not shipping ranges::drop. Same goes for ranges::begin, since we're not shipping ranges::cbegin (yet). [Unless ranges::begin is necessary for the non-ranges concepts to work, in which case I would still provide it]

Quuxplusone edited the summary of this revision. (Show Details)

Here's the new idea:

  • Omit everything in ranges:: to start with.
  • Then, permit everything in namespace std, including swappable, iterator_t, sentinel_t, counted_iterator, default_sentinel_t.
  • Then, permit everything transitively required by those: swap_ranges, iter_swap, input_range, iter_move, in_in_result, borrowed_range, begin, end, etc etc.

I believe we still achieve the goal of not permitting anything with struct-layout concerns.

Notice that we permit std::counted_iterator but not views::counted (because the former is in std:: and the latter isn't).
Notice that we permit ranges::in_in_result but not ranges::in_out_result (because the former is transitively needed by swappable and the latter is not needed by anyone).
Notice that we permit span and string_view's ctors taking (iter,sentinel), but not their ctors taking (range). This is double good, because string_view's range ctor is probably going to go away again before C++23 ships.

The test results look pretty positive, only 4 failures to fix?

Fix the CI failures. I hadn't removed the #ifndef guard around all-of-the-<ranges>-header, so the borrowed_range tests were failing because they only included <ranges> and no other headers. Also, span is supported-ish in this mode, but can't be constructed from an arbitrary contiguous range because we don't support contiguous_range in this mode. (Perhaps we should? It would drag in ranges::data and I think also ranges::size, as well as all the other *_range concepts; but that might not be too bad.)

I didn't look to closely at the patch, but one suggestion.

libcxx/test/std/containers/views/span.cons/deduct.pass.cpp
89

I'm working on removing _LIBCPP_FOO from the tests, can you add a TEST_HAS_NO_INCOMPLETE_RANGES in test_macros.h and use that?

ldionne accepted this revision.Feb 14 2022, 12:56 PM

LGTM, however:

  1. I would really prefer not opening namespace std when testing that we don't define std::ranges, since that's technically ill-formed.
  2. Consider applying @Mordante 's comment about getting rid of _LIBCPP_ macros inside the tests.

Let's ship this ASAP and I can cherry-pick it onto release/14.x. Thanks a lot for handling this -- we definitely didn't want to ship LLVM 14 without having this under control, since that could have removed our ability to improve several things in the future.

This revision is now accepted and ready to land.Feb 14 2022, 12:56 PM
This revision was landed with ongoing or failed builds.Feb 15 2022, 7:39 AM
This revision was automatically updated to reflect the committed changes.