Page MenuHomePhabricator

[libcxx][modules] protects users from relying on detail headers
Needs ReviewPublic

Authored by cjdb on Jul 15 2021, 9:31 PM.

Details

Reviewers
ldionne
zoecarver
Mordante
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Summary

libc++ has started splicing standard library headers into much more
fine-grained content for maintainability. It's very likely that outdated
and naive tooling (some of which is outside of LLVM's scope) will
suggest users include things such as <__ranges/access.h> instead of
<ranges>, and Hyrum's law suggests that users will eventually begin to
rely on this without the help of tooling. As such, this commit
intends to protect users from themselves, by making it a hard error for
anyone outside of the standard library to include libc++ detail headers.

Diff Detail

Event Timeline

cjdb requested review of this revision.Jul 15 2021, 9:31 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2021, 9:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.Jul 16 2021, 10:14 AM
libcxx/include/__ranges/drop_view.h
13

Two suggestions:

  1. could we make this a smaller macro? For example, could we make it so that in drop_view.h I only have to type out _LIBCPP_PRIVATE_HEADER(ranges, __ranges/drop_view.h)?
  1. Could we somehow roll this into _LIBCPP_PUSH_MACROS and _LIBCPP_POP_MACROS so that we don't even need to update it? This would require some creative design work but I think it might be doable.
libcxx/test/libcxx/diagnostics/detail.headers/ranges/view_interface.header.verify.cpp
14

Could these tests be turned into a custom shell test that automates all this? I don't really want to have to do another thing every time I add a header file.

(You can use RUN: <whatever> like this.)

ldionne requested changes to this revision.Jul 16 2021, 11:05 AM

Could this be implemented as a pragma? We have #pragma GCC system_header, perhaps we could have #pragma clang include_instead("<ranges>") or something along those lines? That would also be more generally useful outside of the standard library.

As it is, I find the benefit provided doesn't clearly surpass the amount of boilerplate needed to make it work. Also, if we want to make this worthwhile in any form, we'll have to automate the creation of those tests (or do that on the fly with a .sh.cpp test like Zoe suggests). We don't want to add a manual step like this every time we add a detail header -- we have many of those.

This revision now requires changes to proceed.Jul 16 2021, 11:05 AM
cjdb added inline comments.Jul 16 2021, 11:15 AM
libcxx/test/libcxx/diagnostics/detail.headers/ranges/view_interface.header.verify.cpp
14

The idea will be to use the same Python test for private modules. I did it by hand for this one for two reasons:

  1. I wanted to showcase a small patch to get the discussion going. Your RFC has vindicated this first one.
  2. I haven't yet worked out a way to safely automate code injection so it can be applied to all our headers at once.

But yes, automating this will happen by the time I look at <algorithm> (I am NOT writing ~120 of these by hand).

cjdb updated this revision to Diff 361462.EditedJul 24 2021, 9:09 AM
cjdb edited the summary of this revision. (Show Details)

Completely reworks patch to use #pragma clang include_instead(<header>). As with D105932, reviewers don't need to inspect individual files: reviewing the changes to utils/generate_private_header_tests.py should suffice.

I fully expect CI to fail for the time being, since D106394 hasn't been merged yet.

cjdb updated this revision to Diff 361463.Jul 24 2021, 9:26 AM

Properly documents find_header_name.

ldionne requested changes to this revision.Jul 26 2021, 6:32 AM

Overall LGTM, but I'd like to see this again before it's shipped. I'll request changes so it shows up correctly in the review queue, and I'll look again once the Clang patch has landed.

As it's been discussed, I think we'll want a solution to avoid having to generate N files every time we add a private header. We could look into .sh.cpp tests. But that's non-blocking.

libcxx/utils/generate_private_header_tests.py
90

Weird indentation?

98

Why are those not prefixed by their __?

This revision now requires changes to proceed.Jul 26 2021, 6:32 AM
cjdb updated this revision to Diff 362017.Jul 27 2021, 6:56 AM

rebases to activate CI

Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 6:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 363068.Jul 30 2021, 6:54 AM

rebase to update CI

cjdb updated this revision to Diff 363110.Jul 30 2021, 8:45 AM
cjdb retitled this revision from [libcxx][modules] protects users from relying on ranges detail headers to [libcxx][modules] protects users from relying on detail headers.

rebases to activate CI with generated scripts corrected!

cjdb updated this revision to Diff 366529.Aug 15 2021, 4:22 PM
cjdb edited the summary of this revision. (Show Details)

rebases onto main

cjdb updated this revision to Diff 367239.Aug 18 2021, 10:08 AM

rebases to activate CI