[libc++] Reduces the number of transitive includes.

[Mordante]

Summary

This defines a new policy for removal of transitive includes.
The goal of the policy it to make it relatively easy to remove
headers when needed, but avoid breaking developers using and
vendors shipping libc++.

The method used is to guard transitive includes based on the
C++ language version. For the upcoming C++23 we can remove
headers when we want, but for other language versions we try
to keep it to a minimum.

In this code the transitive include of <chrono> is removed
since D128577 introduces a header cycle between <format>
and <chrono>. This cycle is indirectly required by the
Standard. Our cycle dependency tool basically is a grep based
tool, so it needs some hints to ignore cycles. With the input
of our transitive include tests we can create a better tool.
However that's out of the scope of this patch.

Note the flag _LIBCPP_REMOVE_TRANSITIVE_INCLUDES remains
unchanged. So users can still opt-out of transitives includes
entirely.

[Mordante]

Test.

[Mordante]

Change the includes.

[Mordante]

Test

[Mordante]

Polishing, adding documentation, fixing some CI issues.

[Mordante]

Fix CI.

[Mordante]

Attempt to fix Windows.

[Mordante]

libcxx/include/chrono

692

Note this doesn't belong in this patch. It's a test to see whether we can fix the build issues in D128577.

[ldionne]

I think this is a great step forward, but it does raise some interesting/annoying questions. I think we can solve all of them, though.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

18–20 ↗

(On Diff #454298)

22–25 ↗

(On Diff #454298)

27–31 ↗

(On Diff #454298)

33 ↗

(On Diff #454298)

44–47 ↗

(On Diff #454298)

49 ↗

(On Diff #454298)

Instead of this, I would suggest:

1. Adding a new libcxx-generate-public-header-graph job that produces all the expected.FOO headers using clang --trace-includes
2. This should allow getting rid of regenerate_expected_results = False/True in libcxx/test/libcxx/transitive_includes.sh.cpp.
3. Rewrite graph_header_deps.py to use that data instead of the naive grep-based parsing it has right now.

This is fine to do as a follow-up.

libcxx/docs/ReleaseNotes.rst

48–62 ↗

(On Diff #454298)

libcxx/include/chrono

686

I would like to investigate this instead:

#if _BUILDING_ON_WINDOWS_DLL // don't require usage of libexperimental cause it doesn't work yet
  virtual ~format_error() noexcept { }
#endif

I'm not sure it will be better, but we should try it out. It seems more localized to me.

libcxx/test/libcxx/transitive_includes/expected.algorithm

1 ↗

(On Diff #454298)

This is extremely unfortunate, but I think that we *have* to generate these headers for each language version if we move forward with this patch. Otherwise, we defeat the whole purpose of having this framework in place to prevent transitive include removal regressions.

17 ↗

(On Diff #454298)

We should keep the C++20 removals as minimal as possible. You should keep including <iterator>, <utility> and <variant> explicitly from <algorithm> in <= C++20, but drop them in C++23.

[Mordante]

Test Windows build based on ldionne's suggestion.

[Mordante]

Try to fix the build.

[Mordante]

Fix CI.

[Mordante]

Fix other Windows build.

[ldionne]

libcxx/utils/ci/run-buildbot

570

I think we should define _LIBCPP_INLINE_FORMAT_ERROR_DTOR inside __config instead. This is a general property of clang-cl / DLL builds, not only our CI build. IOW, we should enable the inline dtor if someone builds DLLs using clang-cl at their desk, which wouldn't be the case right now.

[Mordante]

Test another way to fix the Windows build.

[Mordante]

Addresses review comments.
Removes unrelated changes, these have been moved to other patches.
Rebased on top of D132584, to include the new transitve include test framework.

[Mordante]

libcxx/include/algorithm

1903–1906

During the live code review we discussed to move these transitive includes to the end of the file. That will be done in a followup patch.

libcxx/include/chrono

686

Fixed in D132667 in the suggested way. The changes are removed here.

libcxx/test/libcxx/transitive_includes/expected.algorithm

1 ↗

(On Diff #454298)

Solved in D132534 and D132584.

[mumbleskates]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

34 ↗

(On Diff #455641)

Sentence fragment terminates here

libcxx/docs/ReleaseNotes.rst

62–64 ↗

(On Diff #455641)

Is "the future" here major releases? years? If this deeply affected me I would want to know a lot more about what kind of timeline I was guaranteed, given "regardless of the language version in use".

[philnik]

Assuming we still plan to remove transitive includes at some point I'm OK with this approach, as it gives us a lot more flexibility when to remove them.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

11 ↗

(On Diff #455641)

32–33 ↗

(On Diff #455641)

Would the plan still be to remove transitive includes at some point? If not the includes will probably end up in a huge mess.

34 ↗

(On Diff #455641)

I think you are missing something here.

[Mordante]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

32–33 ↗

(On Diff #455641)

I'm not sure. We surely need to remove them when it breaks us, like I did for <chrono>.
The big issue of removal is that may break the packagers who then need to fix it.
If it was only developers I would have less of an issue with the breakage.

We can still remove them without repercussions from C++23 and I want to look at some more TODO removals and use the granularized headers in more places. Some header include concepts or type_traits which can be minimized now.

I'm not sure whether we would get in a huge mess with the current header method.
Do you have any concrete concerns with this approach?

34 ↗

(On Diff #455641)

Seems I misapplied the suggested changes,thanks.

[avogelsgesang]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

30 ↗

(On Diff #455641)

when do we consider a C++ version as "supported"?
As soon as we compile under that -std=c++XX flag without errors?
As soon as clang flips from -std=c++2b to -std=c++23?
As soon as we implemented all papers from that C++ version?

E.g., would we currently consider C++20 to be supported, despite not having implemented all papers, yet? If our bar is "some papers are implemented", which papers are those? Would we already consider C++23 as supported, given that we have implemented some of the C++23 papers?

71 ↗

(On Diff #455641)

[philnik]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

32–33 ↗

(On Diff #455641)

I'm fine with it if they're removed in a release where we don't get that much done that we can ship stable. LLVM 16 will definitely contain ranges and probably format, std::pmr (PRs coming soon), maybe the PSTL (@ldionne friendly reminder to work on D103198) and more. So that's most likely not a good option, but I don't think that there will be huge additions in LLVM 17. LLVM 15 would actually have been a really good release in hind-sight. The main reason we didn't do it then was because of ranges, but that didn't actually happen.

TL;DR I don't think it would be a huge problem if people don't have a really good reason to update (not that you shouldn't keep your tools up-to-date).

My main concern is that we will have to move stuff around to implement the rest of C++17 and C++20. Having

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 14
...
#endif

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
...
#endif


#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
...
#endif

seems to me like a bad idea. Adding a few includes also isn't that huge of a deal. I'm not saying we should do it right now, but it might be a good idea to say: We'll remove all transitive includes in LLVM 17 (maybe?), so, vendors, please enable _LIBCPP_REMOVE_TRANSITIVE_INCLUDES for your internal testing and file bugs against any libraries/programs that break. That would give vendors lots of time to resolve any issues.

[Mordante]

Thanks for all the feedback!

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

11 ↗

(On Diff #455641)

I agree with removing the only but the other part but the the headers for should remain.

30 ↗

(On Diff #455641)

C++20 is considered supported, that's why we put ranges and format under special flags.
I left supported here intentionally since it's already defined in another part of our policy.

https://libcxx.llvm.org/UsingLibcxx.html#using-a-different-version-of-the-c-standard

32–33 ↗

(On Diff #455641)

First of all I hope we can clean up our transitive includes before LLVM 16. Most of our larger headers have been granularized, we need to use them were appropriate.

I initially also considered to remove them fast, but we're not sure how much of effort that will be for vendors. If they have to file bugs against hundreds of packages it may take them a lot of effort. (I expect the transitive removal of algorithm and type_traits will uncover a lot of missing includes.) So I think keeping them when it doesn't "hurt" us might be even beneficial for us. If a vendor needs to fix a lot of packages or file a lot of bugs it will take them more effort to package, which may lead them to take a longer time to adopt a new libc++.

The policy still allows us the move things around when it "hurts" us. So if implementing PMR means we need to remove additional transitive includes we still should do that, but only try to minimize that. Like in this commit I removed chrono since it makes it hard to implement format.

The end goal is to move these transitive includes to the end of the header.

So I would really like to use this policy and when it turns out it doesn't work we can revise it. We explicitly mention we're free to remove them here and in the release notes.

libcxx/docs/ReleaseNotes.rst

62–64 ↗

(On Diff #455641)

We kind of intentionally don't want to provide any guarantee. We still miss some C++17 features like parallel algorithms and polymorphic allocators. They may cause header cycles we want to break by removing headers.

Technically code not using the proper includes is wrong. But often you "get away" with it. But not including the proper headers makes code less portable. Some code accepted by libstdc++ will be rejected by libc++ since both libraries have different transitive includes. If you want to make sure your code doesn't miss any includes you can use _LIBCPP_REMOVE_TRANSITIVE_INCLUDES to test before it becomes an issue.

Note that in the C++ Standard there are few required headers and some might even be surprising; <regex> doesn't require <string>. Based on the interface of <regex> I'm not convinced it can be implemented (in a practical way) without providing <string>.

[Mordante]

Addresses review comments.

[philnik]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

32–33 ↗

(On Diff #455641)

I get that it might be some effort for vendors to file bugs and stuff. That's why my proposal is to remove transitive includes at any point when there is a technical problem with keeping them. When we think we removed most/all the transitive includes we want to remove we tell everyone well in advance that we will remove the transitive includes in LLVM ab. If we think that we don't remove many after LLVM 16, we can say that transitive includes will be removed in LLVM 18 or whatever.

While this does result in some work IMO it's worth it. For example, including <string> already only takes 2/3 as long when adding -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES to the command line and I'm certain this can be reduced further. Including <memory> currently adds about 100ms (the compilation took ~385ms).

[Mordante]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

32–33 ↗

(On Diff #455641)

Let's discuss this further on Discord.

[jloser]

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

71 ↗

(On Diff #456104)

Nit: this reads a bit weird to me. Maybe s/at their/with their?

[ldionne]

I think this strikes the right balance between unblocking important work, not creating immense blockers for vendors to ship an updated libc++, and providing a way forward for the library to reduce its tech debt.

Per the policy, we might decide to do a major "let's drop all transitive includes in all standard modes" move, however that would be a bit of a project on its own and we would have to contact our main vendors to see what the impact is. This can totally be done, and I might do it a couple months down the road when things are a bit less busy, however right now I think this is the right way forward.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst

30 ↗

(On Diff #455641)

I'll note that we may want to revisit that definition, but it is indeed the current status quo.

32–33 ↗

(On Diff #455641)

The plan is definitely to eventually remove incidental transitive includes. In fact, the bigger plan would even be to have users migrate to newer standard versions, and perhaps one day drop support for older versions (we can dream, right?).

The policy is worded so that

(1) we try to minimize disruptive breakage for vendors, yet
(2) we still have the ability to remove incidental transitive includes when technically necessary (e.g. the <format> - <chrono> circular dep that brought us here).

I understand that some folks would like to have a clearer ETA for when we're going to remove those includes, however I would like to avoid committing to something specific because we still have a lot of more important things to do for the next few releases.

[philnik]

In D132284#3761464, @ldionne wrote:

I think this strikes the right balance between unblocking important work, not creating immense blockers for vendors to ship an updated libc++, and providing a way forward for the library to reduce its tech debt.

Per the policy, we might decide to do a major "let's drop all transitive includes in all standard modes" move, however that would be a bit of a project on its own and we would have to contact our main vendors to see what the impact is. This can totally be done, and I might do it a couple months down the road when things are a bit less busy, however right now I think this is the right way forward.

Then I'm happy with this patch. I just wanted to make sure that the plan is still to remove the transitive includes and this approach is just there to make it easier for us to continue implementing new features while we're working on removing the transitive includes.

[Mordante]

Thanks for all reviews!

[Mordante]

libcxx/include/algorithm

1903–1906

See D133212

libcxx/include/algorithm

 #include <__algorithm/stable_sort.h>
 #include <__algorithm/swap_ranges.h>
 #include <__algorithm/transform.h>
 #include <__algorithm/unique.h>
 #include <__algorithm/unique_copy.h>
 #include <__algorithm/unwrap_iter.h>
 #include <__algorithm/upper_bound.h>
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
+#endif
+
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
 #  include <iterator>
 #  include <utility>
 #endif
 
 // standard-mandated includes
 #include <initializer_list>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #if defined(_LIBCPP_HAS_PARALLEL_ALGORITHMS) && _LIBCPP_STD_VER >= 17
 #   include <__pstl_algorithm>
 #endif
 
 #endif // _LIBCPP_ALGORITHM

libcxx/include/any

 #include <__utility/unreachable.h>
 #include <cstdlib>
 #include <initializer_list>
 #include <memory>
 #include <type_traits>
 #include <typeinfo>
 #include <version>
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 namespace std {

libcxx/include/atomic

 #include <cstring>
 #include <type_traits>
 #include <version>
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 # include <__threading_support>
 #endif
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifdef _LIBCPP_HAS_NO_ATOMIC_HEADER

libcxx/include/chrono

 #include <__chrono/weekday.h>
 #include <__chrono/year.h>
 #include <__chrono/year_month.h>
 #include <__chrono/year_month_day.h>
 #include <__chrono/year_month_weekday.h>
 #include <__config>
 #include <version>
 
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+#  include <format>
+#endif
+
 // standard-mandated includes
 #include <compare>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #endif // _LIBCPP_CHRONO

libcxx/include/future

 #include <__utility/move.h>
 #include <exception>
 #include <memory>
 #include <mutex>
 #include <system_error>
 #include <thread>
 #include <version>
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifdef _LIBCPP_HAS_NO_THREADS

libcxx/include/optional

 #include <__utility/move.h>
 #include <__utility/swap.h>
 #include <initializer_list>
 #include <new>
 #include <stdexcept>
 #include <type_traits>
 #include <version>
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
-#  include <atomic>
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
+#endif
+
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+#  include <atomic>
 #  include <climits>
 #  include <concepts>
 #  include <ctime>
 #  include <iterator>
 #  include <memory>
 #  include <ratio>
 #  include <tuple>
 #  include <typeinfo>

libcxx/include/thread

 #include <cstddef>
 #include <iosfwd>
 #include <memory>
 #include <system_error>
 #include <tuple>
 #include <type_traits>
 #include <version>
 
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
 #  include <chrono>
+#endif
+
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
 #  include <functional>
 #endif
 
 // standard-mandated includes
 #include <compare>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header

libcxx/utils/ci/run-buildbot

if [ -s ${BUILD_DIR}/generated_output.status ]; then
         false
     fi
 
     # Reject patches that introduce non-ASCII characters or hard tabs.
     # Depends on LC_COLLATE set at the top of this script.
     ! grep -rn '[^ -~]' libcxx/include/ || false
 
     # Reject patches that introduce dependency cycles in the headers.
-    python3 libcxx/utils/graph_header_deps.py >/dev/null
+    # python3 libcxx/utils/graph_header_deps.py >/dev/null
 ;;
 generic-cxx03)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx03.cmake"
     check-runtimes
     check-abi-list
 ;;
 generic-cxx11)

 clang-cl-dll)
     clean
     # TODO: Currently, building with the experimental library breaks running
     # tests (the test linking look for the c++experimental library with the
     # wrong name, and the statically linked c++experimental can't be linked
     # correctly when libc++ visibility attributes indicate dllimport linkage
     # anyway), thus just disable the experimental library. Remove this
     # setting when cmake and the test driver does the right thing automatically.
     generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False"
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 ;;
 clang-cl-static)
     clean
     generate-cmake-libcxx-win -DLIBCXX_ENABLE_SHARED=OFF
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx