Include what you use pass on headers

[MichaelChirico]

Following up the tidyteam meeting today with a first suggested PR as an earmark/test balloon lest I forget. A few notes:

1. We could also consider fully "modularizing" by splitting up the sources into .h, .cpp pairs, where the .h file declares the routines and gives some documentary comments, and the .cpp file gives the implementations. Often, this further cleans up the inclusions by making clear what's needed for the API aspect of the routines (input/return types, etc.) vs. what's required for the implementation. Omitted this for now as it's a bit more involved / not sure preferences.
2. <cstddef> for NULL gave me a bit of pause because src/init.c uses <stdlib.h> for that. I think this is partly just a C vs. C++ thing, e.g. https://stackoverflow.com/q/12023476/3576984; we might consider <stddef.h> for a narrower inclusion in init.c.
3. If you'd like me to play around with getting this enforced in GHA, let me know. IINM it's https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html.
4. I added the annotations of which header gives which symbol (1) for easier review and (2) to match a few other files like src/xml2_doc.cpp. The risk is that these annotations fall out of sync with the actual code over time.
5. Some style guides recommendation around the order to use for inclusions (e.g.). I'm indifferent here and didn't make any changes, besides that my new inclusions here are roughly alphabetized.

[MichaelChirico]

nudge @jennybc :)

[jennybc]

I don't really look after xml2. Am I forgetting something I signed up to do here?

[MichaelChirico]

I may have picked a bad repo to start -- but my memory from the tidyteam meeting in February is that you were keen on doing this header clean-up for tidyverse+adjacent packages. CMIIW, or if you'd prefer to see this pilot exercise on some specific repos instead. Thanks!

[jeroen]

After giving this some more thought but I'm not so sure anymore it's a good idea to do this everywhere.

I agree that it might be good to scan your package for API headers that you forgot to include, and which happen to be included transitively in a way that might not be guaranteed to keep working.

However the opposite is also true: many libraries have an API header (that you are supposed to include in the app) and this header then transitively includes some implementation headers, which might be platform/version specific. So the portable solution is to include the API header only. In those cases enforcing that we include the implementation header might actually be counter productive in some cases.

For example in the case of this PR, I think we are supposed to include Rinternals.h in packages, which then includes the required headers such as R_ext/Error.h automatically. I think including R_ext/Error.h ourselves again is not needed and maybe even more fragile as in theory that header could be renamed or removed in some future R version.

[MichaelChirico]

I do think there are tradeoffs in both directions -- certainly, that the ecosystem generally works by just including Rinternals.h means that's the better-lit path. I do think the risk of things being unexported from underlying headers like R_ext/Error.h is mitigated by other CRAN packages doing selective imports already: https://github.com/search?q=org%3Acran+%2F%3CR_ext%5C%2Ferror%5C.h%3E%2F&type=code

FWIW, my understanding is this is also more of a C++ thing for clang (IWYU: https://github.com/include-what-you-use/include-what-you-use). Personally, the biggest appeal of this approach is readability -- you make it perfectly clear to readers where each symbol comes from in the narrowest way possible.

Anyway, within R as of today it's really more a matter of taste than correctness.