IWYU annotations

Peter Eisentraut <peter@eisentraut.org> writes:

I have done a pass over much of the source code with
include-what-you-use (IWYU) to remove superfluous includes (commits
dbbca2cf299, 9be4e5d293b, ecb5af77987). Along the way I have collected
some pragma annotations to deal with exceptions and special cases and
peculiarities of the PostgreSQL source code header structures (see [0]
for description). Here I'm proposing a set of patches to add such
annotations in commonly useful cases that should deal with most of the
noise.

This seems to be going in the direction that there will be Yet Another
tool that committers have to know everything about in order to not
commit bad code. I'm feeling resistant to that, mainly because I'm
far from convinced that IWYU brings us enough value to justify
everybody having to learn about it. (The fact that the predecessor
tool pgrminclude hasn't been used in a dozen years, and nobody seems
to care, speaks volumes here.)

In particular, this patchset introduces what seem like very
error-prone setups, such as in rmgrdesc.c where there's now one
group of #include's with "pragma: begin_keep/pragma: end_keep"
around it and another group without. Most of us are likely
to just blindly stick a new #include into alphabetical order
somewhere in there and not notice that there's now an additional
concern. The fact that that you've added precisely zero
documentation about what these pragmas are doesn't help.

regards, tom lane

On 09.12.24 17:37, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I have done a pass over much of the source code with
include-what-you-use (IWYU) to remove superfluous includes (commits
dbbca2cf299, 9be4e5d293b, ecb5af77987). Along the way I have collected
some pragma annotations to deal with exceptions and special cases and
peculiarities of the PostgreSQL source code header structures (see [0]
for description). Here I'm proposing a set of patches to add such
annotations in commonly useful cases that should deal with most of the
noise.

This seems to be going in the direction that there will be Yet Another
tool that committers have to know everything about in order to not
commit bad code. I'm feeling resistant to that, mainly because I'm
far from convinced that IWYU brings us enough value to justify
everybody having to learn about it.

It's not realistic at the moment for it to be a tool that everyone needs
to use and everyone needs to keep 100% clean. We're certainly very far
from that being feasible at the moment.

I see it useful in two areas:

First, when you add new files or move lots of code around, you can have
it provide an informed opinion about what includes to keep and add.
Because in practice that's mostly a crapshoot nowadays. An example from
a current patch under review:

$ iwyu_tool.py -p build src/backend/libpq/auth-oauth.c
...
../src/backend/libpq/auth-oauth.c should remove these lines:
- #include <fcntl.h> // lines 19-19
- #include <unistd.h> // lines 18-18
- #include "storage/fd.h" // lines 28-28

Second, clangd (the language server) has support for this also, and so
depending on local configuration and preferences, it can highlight
missing or redundant includes or even add some automatically as you
edit. (The latter obviously needs some manual supervision, but it is
arguably kind of neat that you don't need to jump to the top and
manually add includes as you type in new code that needs an additional
header.)

But in order for either of this to work, it needs to have some
information about basic PostgreSQL code conventions. Otherwise, it will
also do this:

../src/backend/libpq/auth-oauth.c should add these lines:
...
#include <stdbool.h> // for false, bool, true
#include <stddef.h> // for NULL, size_t
#include "c.h" // for Assert, explicit_bzero,
pg_strncasecmp
...

because it doesn't know that the convention is that you are supposed to
include "postgres.h" (or one of the other always-first headers) and then
everything that it brings it should usually not be included again
directly. (Or conversely in some cases it will suggest to remove the
include of "postgres.h" because it doesn't provide anything of use.)

So right now you get a bunch of noise and misleading information for
each file and the whole clangd support is of limited use. That's what
my patch set is mainly trying to address.

In particular, this patchset introduces what seem like very
error-prone setups, such as in rmgrdesc.c where there's now one
group of #include's with "pragma: begin_keep/pragma: end_keep"
around it and another group without. Most of us are likely
to just blindly stick a new #include into alphabetical order
somewhere in there and not notice that there's now an additional
concern. The fact that that you've added precisely zero
documentation about what these pragmas are doesn't help.

It's a fair point that some documentation could be provided. I suppose
we don't want to verbosely explain each pragma individually. Should
there be some central explanation, maybe in src/tools/pginclude/README?

Note that if you google like "IWYU pragma: export" it will take you to
the upstream documentation as the first hit, so the full documentation
is pretty easy to find.

Peter Eisentraut <peter@eisentraut.org> writes:

On 09.12.24 17:37, Tom Lane wrote:

In particular, this patchset introduces what seem like very
error-prone setups, such as in rmgrdesc.c where there's now one
group of #include's with "pragma: begin_keep/pragma: end_keep"
around it and another group without. Most of us are likely
to just blindly stick a new #include into alphabetical order
somewhere in there and not notice that there's now an additional
concern. The fact that that you've added precisely zero
documentation about what these pragmas are doesn't help.

It's a fair point that some documentation could be provided. I suppose
we don't want to verbosely explain each pragma individually. Should
there be some central explanation, maybe in src/tools/pginclude/README?

That might do, but perhaps instead in the "PostgreSQL Coding
Conventions" chapter of the SGML docs? Actually, I think we could do
with a centralized explanation of our inclusion conventions --- I'm
not sure that the whole business of "postgres.h or a sibling must be
first" is explained in any easy-to-find place. This topic would
likely fit well with such an explanation.

But really, the point I was trying to make above is that I don't
want this to break our very long-standing convention that headers
should be #include'd alphabetically and there is never a need to
impose some other order (at least not without lots of commentary
about it at the scene of the crime). The way you've done it here
is just asking for trouble, IMO. If that means redundant pragma
commands, so be it.

regards, tom lane

On 02.01.25 17:15, Tom Lane wrote:

It's a fair point that some documentation could be provided. I suppose
we don't want to verbosely explain each pragma individually. Should
there be some central explanation, maybe in src/tools/pginclude/README?

That might do, but perhaps instead in the "PostgreSQL Coding
Conventions" chapter of the SGML docs? Actually, I think we could do
with a centralized explanation of our inclusion conventions --- I'm
not sure that the whole business of "postgres.h or a sibling must be
first" is explained in any easy-to-find place. This topic would
likely fit well with such an explanation.

In this updated patch set, I've just added a bit of info into
src/tools/pginclude/README. Updating the coding convention
documentation like you suggest also sounds like a good idea, but from my
perspective I would need to do further research to pin down what those
actually are, so I'm leaving that as a separate project.

(See for example the business in src/tools/pginclude/headerscheck about
guessing which the appropriate first header file should be for each
component. That kind of thing perhaps ought to be more formalized.)

But really, the point I was trying to make above is that I don't
want this to break our very long-standing convention that headers
should be #include'd alphabetically and there is never a need to
impose some other order (at least not without lots of commentary
about it at the scene of the crime). The way you've done it here
is just asking for trouble, IMO. If that means redundant pragma
commands, so be it.

Yeah, that's a fair point. I have removed that part from my patch set.

On 10.01.25 09:10, Peter Eisentraut wrote:

On 02.01.25 17:15, Tom Lane wrote:

It's a fair point that some documentation could be provided.  I suppose
we don't want to verbosely explain each pragma individually.  Should
there be some central explanation, maybe in src/tools/pginclude/README?

That might do, but perhaps instead in the "PostgreSQL Coding
Conventions" chapter of the SGML docs?  Actually, I think we could do
with a centralized explanation of our inclusion conventions --- I'm
not sure that the whole business of "postgres.h or a sibling must be
first" is explained in any easy-to-find place.  This topic would
likely fit well with such an explanation.

In this updated patch set, I've just added a bit of info into src/tools/
pginclude/README.  Updating the coding convention documentation like you
suggest also sounds like a good idea, but from my perspective I would
need to do further research to pin down what those actually are, so I'm
leaving that as a separate project.

(See for example the business in src/tools/pginclude/headerscheck about
guessing which the appropriate first header file should be for each
component.  That kind of thing perhaps ought to be more formalized.)

But really, the point I was trying to make above is that I don't
want this to break our very long-standing convention that headers
should be #include'd alphabetically and there is never a need to
impose some other order (at least not without lots of commentary
about it at the scene of the crime).  The way you've done it here
is just asking for trouble, IMO.  If that means redundant pragma
commands, so be it.

Yeah, that's a fair point.  I have removed that part from my patch set.

I have committed this. Thanks for the feedback.

On Wed, Jan 15, 2025 at 2:21 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I have committed this. Thanks for the feedback.

Thanks for working on this.

Is it worth documenting how to get clangd working sensibly with
"--header-insertion=iwyu"?

I enabled it just now. I find that I can now use completion with a
type name, and have clangd automatically include the necessary header
file. For example, if I use completion to pull in a "StringInfoData" I
find that clangd will automatically add the necessary #include
"lib/stringinfo.h", without any special directions from me -- no
context switching required. Importantly, now that we have these
annotations, clangd won't constantly be adding useless includes that
actually come from c.h -- that made IWYU header cleaning unusable
before now.

Note that I'm relying on my clang-tidy config for this. clangd is able
to respect our conventions around headers through clang-tidy, which
has been taught Postgres-specific rules around header conventions. So
we'd actually be documenting something about clang-tidy + clangd +
Postgres header conventions, if we went through with something like
this.

It might be worth holding off for now. It's possible that I'll find
--header-insertion=iwyu has big problems in some unforeseen way. But
offhand it looks like a real improvement, even though I foresee
certain minor downsides. What do you think?

--
Peter Geoghegan

On 15.01.25 20:51, Peter Geoghegan wrote:

On Wed, Jan 15, 2025 at 2:21 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I have committed this. Thanks for the feedback.

Thanks for working on this.

Is it worth documenting how to get clangd working sensibly with
"--header-insertion=iwyu"?

I enabled it just now. I find that I can now use completion with a
type name, and have clangd automatically include the necessary header
file. For example, if I use completion to pull in a "StringInfoData" I
find that clangd will automatically add the necessary #include
"lib/stringinfo.h", without any special directions from me -- no
context switching required. Importantly, now that we have these
annotations, clangd won't constantly be adding useless includes that
actually come from c.h -- that made IWYU header cleaning unusable
before now.

Note that I'm relying on my clang-tidy config for this. clangd is able
to respect our conventions around headers through clang-tidy, which
has been taught Postgres-specific rules around header conventions. So
we'd actually be documenting something about clang-tidy + clangd +
Postgres header conventions, if we went through with something like
this.

It might be worth holding off for now. It's possible that I'll find
--header-insertion=iwyu has big problems in some unforeseen way. But
offhand it looks like a real improvement, even though I foresee
certain minor downsides. What do you think?

I had turned automatic header insertion off until now and didn't think
to turn it back on yet. I'll give it another try sometime.

I'm unclear on what role clang-tidy would play in this.

On Wed, Jan 22, 2025 at 9:08 AM Peter Eisentraut <peter@eisentraut.org> wrote:

It might be worth holding off for now. It's possible that I'll find
--header-insertion=iwyu has big problems in some unforeseen way. But
offhand it looks like a real improvement, even though I foresee
certain minor downsides. What do you think?

I had turned automatic header insertion off until now and didn't think
to turn it back on yet. I'll give it another try sometime.

I'm now a week into this experiment. So far, the results have been mixed.

I'm tempted to turn --header-insertion=iwyu off now, since clangd
header insertion with completion is in fact still adding what seem to
me to be superfluous includes in some cases (though much less so
compared to before your recent work). So it's still useful, but it
might be more annoying than useful. Particularly compared to my
original approach of relying on clangd errors + quick-fix actions to
add required headers -- that doesn't have these problems (I seem to
only be offered the option of a quick-fix when I get certain compile
errors), and requires only minimal effort.

I'm unclear on what role clang-tidy would play in this.

Sorry, I meant clang-format.

The relevant section of my .clang-format file:

SortIncludes: true
IncludeIsMainSourceRegex: '(postgres\.h)$'
IncludeCategories:
- Regex: '^<.*\.h>'
Priority: 1
SortPriority: 1
- Regex: 'postgres.h'
Priority: 2
SortPriority: 2
- Regex: '.*'
Priority: 3
SortPriority: 3

I'm guessing that you already have something like this (I believe I
copied it from somebody else). This config is respected by clangd
whenever it adds a header file (whether it's fully automatic or
whether it's via a "missing header" quick-fix).

--
Peter Geoghegan