Skip to content

[libc++] Stabilize transitive includes for C++23 #134143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Apr 2, 2025

Our mechanism to retain transitive includes for backwards compatibility was previously not taking into account C++23: this means that users on C++23 would not get any protection against the removal of transitive includes. This was fine when C++23 was still not used widely and it allowed us to make build time improvements for such "bleeding edge" users.

However, now that C++23 is used pretty widely, we should start providing transitive includes backwards compatibility for users of that language mode too. This patch documents that requirement.

There are no actual changes to the code since we haven't removed transitive includes since the last release. However, starting now, we should guard any removal of transitive includes behind

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 23

instead of

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

Our mechanism to retain transitive includes for backwards compatibility
was previously not taking into account C++23: this means that users on
C++23 would not get any protection against the removal of transitive
includes. This was fine when C++23 was still not used widely.

However, now that C++23 is used pretty widely, we should start providing
transitive includes backwards compatibility for users of that language
mode too. This patch documents that requirement.

There are no actual changes to the code since we haven't removed
transitive includes since the last release. However, starting now,
we should guard any removal of transitive includes behind

    #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 23

instead of

    #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
@ldionne ldionne requested a review from a team as a code owner April 2, 2025 19:25
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 2, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Our mechanism to retain transitive includes for backwards compatibility was previously not taking into account C++23: this means that users on C++23 would not get any protection against the removal of transitive includes. This was fine when C++23 was still not used widely.

However, now that C++23 is used pretty widely, we should start providing transitive includes backwards compatibility for users of that language mode too. This patch documents that requirement.

There are no actual changes to the code since we haven't removed transitive includes since the last release. However, starting now, we should guard any removal of transitive includes behind

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) &amp;&amp; _LIBCPP_STD_VER &lt;= 23

instead of

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) &amp;&amp; _LIBCPP_STD_VER &lt;= 20

Full diff: https://github.com/llvm/llvm-project/pull/134143.diff

1 Files Affected:

  • (modified) libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst (+4-1)
diff --git a/libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst b/libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
index 02cbc162318ef..03b5f4ac4b5d6 100644
--- a/libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
+++ b/libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
@@ -38,7 +38,7 @@ guarded by something of the form:
 
 .. code-block:: cpp
 
-   #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+   #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 23
    #  include <algorithm>
    #  include <iterator>
    #  include <utility>
@@ -49,6 +49,9 @@ include transitive headers, regardless of the language version. This can be
 useful for users to aid the transition to a newer language version, or by users
 who simply want to make sure they include what they use in their code.
 
+We currently provide transitive inclusions for backwards compatibility in all
+Standard modes older than C++26.
+
 
 Rationale
 ---------

@philnik777
Copy link
Contributor

Is there a specific reason to do this now? Did you see any significant breakage so far? FWIW I'm not actually sure our current approach is that helpful in C++23, since we're only rarely including public headers nowadays.

@mordante
Copy link
Member

mordante commented Apr 7, 2025

It seems I missed/forgot C++20's headers are considered stable. Is there a way we can automatically validate this policy in the CI?

@ldionne
Copy link
Member Author

ldionne commented May 6, 2025

Is there a specific reason to do this now? Did you see any significant breakage so far? FWIW I'm not actually sure our current approach is that helpful in C++23, since we're only rarely including public headers nowadays.

Yes, this change is prompted by fairly wide breakage we saw internally with LLVM 20. It's not as wide as the breakage we initially saw when we started the granularization effort, but it's very noticeable.

It seems I missed/forgot C++20's headers are considered stable. Is there a way we can automatically validate this policy in the CI?

We already do validate the stability of headers with the transitive inclusion tests that you're aware of. However, so far we were happy to approve patches that changed the .csv files describing transitive includes for C++23 -- this patch documents that we shouldn't do that anymore. It would technically be possible to test this policy in the CI by adding a test to ensure that we don't modify the list of transitive includes for C++23, however I think the value of such a test would be almost none (and we'd have other change it whenever we make additive changes to the transitive includes). So really I think this comes down to the review process.

@ldionne
Copy link
Member Author

ldionne commented May 9, 2025

Gentle ping @philnik777 @mordante , WDYT of my rationale?

@philnik777
Copy link
Contributor

I think the rationale makes sense, but I'm not actually sure the solution is the right one. Would keeping the public headers actually help with the breakages you've seen?

@ldionne
Copy link
Member Author

ldionne commented May 14, 2025

I think the rationale makes sense, but I'm not actually sure the solution is the right one. Would keeping the public headers actually help with the breakages you've seen?

I think it would, as with all the other transitive includes breakages? You seem to imply that we'd want another solution, but I'm not certain I follow your thought here. Can you elaborate?

@philnik777
Copy link
Contributor

I think the rationale makes sense, but I'm not actually sure the solution is the right one. Would keeping the public headers actually help with the breakages you've seen?

I think it would, as with all the other transitive includes breakages? You seem to imply that we'd want another solution, but I'm not certain I follow your thought here. Can you elaborate?

We're not including top-level headers anymore in most of places. Consider for example <functional>. The header provides vector, but <vector> isn't listed in the transitive includes of <functional> in C++23, since we only include <__vector/vector.h>. That means that if we managed to remove that dependency, the transitive includes test would be completely happy, even though we've probably broken some user by removing it. This change will make it less likely that we're breaking someone through granularizing non-granular headers, but that basically doesn't happen anymore anyways. We're more likely to move some code around and remove a dependency that way I think.

Also, somewhat tangential, IMO we'll need to give users some way to get include-clean. Keeping this cruft around is a huge hit on compile times, and the transitive include test probably made things a lot worse, since we've never dropped any transitive includes, no matter how long they were actually in the library. #139855 seems related.

@ldionne
Copy link
Member Author

ldionne commented May 15, 2025

I think the rationale makes sense, but I'm not actually sure the solution is the right one. Would keeping the public headers actually help with the breakages you've seen?

I think it would, as with all the other transitive includes breakages? You seem to imply that we'd want another solution, but I'm not certain I follow your thought here. Can you elaborate?

We're not including top-level headers anymore in most of places. Consider for example <functional>. The header provides vector, but <vector> isn't listed in the transitive includes of <functional> in C++23, since we only include <__vector/vector.h>. That means that if we managed to remove that dependency, the transitive includes test would be completely happy, even though we've probably broken some user by removing it. This change will make it less likely that we're breaking someone through granularizing non-granular headers, but that basically doesn't happen anymore anyways. We're more likely to move some code around and remove a dependency that way I think.

I see what you mean now, thanks for explaining. However, taking the <functional> example: std::vector must have been provided by <functional> pretty recently, no earlier than since we started splitting headers, right? Because if it was provided before then, the only way it would have been provided is via #include <vector>, and that would have been preserved through our transitive inclusion stability.

I do agree that what you describe is likely the bigger problem nowadays, and I do agree that keeping transitive public includes stable doesn't help that. However, we've come across breakage that would have been easily avoided by keeping the public transitive includes stable, so I'd like us to at least do that. The rationale is also consistent with us doing it for C++03 - C++20.

Also, somewhat tangential, IMO we'll need to give users some way to get include-clean. Keeping this cruft around is a huge hit on compile times, and the transitive include test probably made things a lot worse, since we've never dropped any transitive includes, no matter how long they were actually in the library. #139855 seems related.

Thanks for the heads up on that PR. I have thought about this several times in the past, and it's good to see people actually working on it. I agree that with something like a compiler warning to get include-clean, we should re-have the whole discussion about transitive includes. I think it might be reasonable to start dropping transitive includes entirely if we had that tool (and perhaps an automatic fix-it tool). But in the absence of such a tool, I'd like us to at least try to keep stable what we can.

@philnik777
Copy link
Contributor

I think the rationale makes sense, but I'm not actually sure the solution is the right one. Would keeping the public headers actually help with the breakages you've seen?

I think it would, as with all the other transitive includes breakages? You seem to imply that we'd want another solution, but I'm not certain I follow your thought here. Can you elaborate?

We're not including top-level headers anymore in most of places. Consider for example <functional>. The header provides vector, but <vector> isn't listed in the transitive includes of <functional> in C++23, since we only include <__vector/vector.h>. That means that if we managed to remove that dependency, the transitive includes test would be completely happy, even though we've probably broken some user by removing it. This change will make it less likely that we're breaking someone through granularizing non-granular headers, but that basically doesn't happen anymore anyways. We're more likely to move some code around and remove a dependency that way I think.

I see what you mean now, thanks for explaining. However, taking the <functional> example: std::vector must have been provided by <functional> pretty recently, no earlier than since we started splitting headers, right? Because if it was provided before then, the only way it would have been provided is via #include <vector>, and that would have been preserved through our transitive inclusion stability.

std::vector doesn't have to be a recent addition. Remember, we've removed transitive includes in C++23 until now, so we've removed dependencies that were there before the header split. The C++20 transitive includes list does have <vector> as a dependency on <functional>. That's why I'm not convinced the current solution is actually that helpful for C++23.

@ldionne
Copy link
Member Author

ldionne commented May 29, 2025

I did some more research and it seems like many of the issues we hit might actually be linked to the removal of implementation detail headers and wouldn't have been caught even by stabilizing the C++23 headers. For example, ffc7380 is likely to have been the cause of our largest breakage for LLVM 20, but there's no way to guard against that kind of stuff systematically.

That being said, even if the cause for a lot of the breakage we saw in LLVM 20 might not have been prevented by stabilizing transitive includes, do we agree that we'd be at risk of breaking users if we did remove a transitive include in C++23? If so, then clearly we should be considering C++23 transitive includes as stable just like we consider the ones for C++20 and older standard modes stable. It just so happens that removal of these transitive includes will happen more infrequently than it used to, so this may not actually result in us adding any more "transitive includes conservation" blocks to the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants