IWYU in Chromium

[Jean-Philippe Gravel]

Hi all,

I have been experimenting with using include-what-you-use (IWYU) and have had very encouraging results. I managed to clean up all the files under `third_party/blink/renderer/modules/canvas`. See pending review here: https://crrev.com/c/3939183. Moving forward with this CL will have repercussions on the rest of the code, so I'm opening a discussion here to discuss this.

In case anyone needs convincing that good a include hygiene is important, I fixed several latent problems by cleaning this folder. For instance, there are lots of unused includes in the code. For some files, as much as 1/3rd of the includes were unused. Removing these would improve compile time. Files are also missing lots of includes, making them non-hermetic. Depending on indirect includes means that changes in dependencies can break .cc files, sometimes in surprising ways. I also found that some files had illegal dependencies, though we never noticed because they were not including the corresponding headers.

I have experimented with both the internal and external versions of IWYU. The public version appears to have some issues, for which I have been creating bugs. None of these were deal breakers for the canvas folder though. The internal version seems to work better in some areas, but I also got some glitches, probably related to the hacky way I used it outside of google3.

Regardless of what IWYU version we use, fixing the includes in one file often requires fixes in many other files in the dependency tree. These are often trivial, like adding missing headers or fixing DEPS files. Some other problems however require special IWYU pragma annotations. For instance, if header "foo.h" includes a private "foo_impl.h", that impl file must be annotated so IWYU knows it's private and "foo.h" must be included instead. The full list of annotations is documented here.

This brings me to the main point of this email: if we start using IWYU, we need to agree to maintain these annotations going forward. Note that these annotations provide value irrespective of whether we run IWYU, as they are also understood by clangd. If you enable clangd's "UnusedIncludes: Strict" mode, the IWYU annotations help clangd in providing correct diagnostics.

It would be great to progressively start using IWYU where it works. This tool has some quirks, but we have to start somewhere and we can progressively improve things as we go.

Thoughts?

JP

[Peter Kasting]

On Wed, Oct 12, 2022 at 1:20 PM Jean-Philippe Gravel <jpgr...@chromium.org> wrote:

It would be great to progressively start using IWYU where it works. This tool has some quirks, but we have to start somewhere and we can progressively improve things as we go.

Thoughts?

thestig@ (+CCed) has done the most work to try both the external and internal IWYU tools on Chromium before, as recently as earlier this year. It can make helpful suggestions, but there are a large number of gotchas, especially in code that has platform-specific preprocessor directives. Additionally, the internal tool is basically unmaintained, and the external tool is woefully behind it in a variety of ways. To date, we've decided that the problems are too great to roll this out in any widespread way.

Google internally is currently working on an IWYU initiative that would lean heavily on the current Google style guidance to prefer #includes over forward declarations (which is guidance Chromium intentionally contradicts today). We're in discussions with the internal folks about whether and how the internal tooling could be used for Chromium.

For now, I would suggest using the tool locally to try to write clean patches, but I'm not sure we can do anything more than that.

PK

[dan...@chromium.org]

Maybe it would be useful in parts of the codebase that don't include system headers - like blink?

 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFB%2B53z_nw4UPxgEGf6YUiwEaeOWtE%2BrthftGFOHWaGCFA%40mail.gmail.com.

[Jean-Philippe Gravel]

I'm happy to hear that there are discussions internally on this.

Regarding platform-specific includes, I would be interested in having a look at a concrete example. Note that IWYU ignores code disabled macro directives. Therefore, if both the #include and its corresponding usage are both enabled/disabled by the same preprocessor condition, IWYU will either correctly analyze the code if it's enabled, or ignore it otherwise. In general, any changes suggested by IWYU should be verified manually. If IWYU suggests removing platform specific includes, that's certainly a red flag. If IWYU's recommendations are invalid, they should be corrected with a pragma comment, or by correctly wrapping the code and #include in the corresponding macro condition. It's also possible to run IWYU for several platform configs and combine the results. Ultimately, I hope that CQ can be trusted to validate that the modified code compiles for all platforms.

> For now, I would suggest using the tool locally to try to write clean patches

This is exactly what I have in mind for now. Though, to make that work, we still need to annotate base headers. If we can't annotate base files, anyone trying to run IWYU locally like me would have to assemble a mapping file to tell IWYU how to deal with base headers. That isn't a great or a scalable solution, it would be much better to annotate the base file once for everyone. Putting annotations would be a step in the right direction, leaving the code in a better state than it was before. The annotations are the same, whether we use clangd, the internal IWYU or the external one. If a tool comes along to replace both IWYUs in the future, it will likely also use the same pragma. 

> Additionally, the internal tool is basically unmaintained, and the external tool is woefully behind it in a variety of ways.

I see active contributions in both the internal and external versions. Besides, the fact that the external tool is behind is unfortunate, but that doesn't put a lot of weight in this discussion. Even with its imperfection, IWYU can still provide value. Using it where it works is definitely better than not doing anything. 

[Peter Boström]

I wouldn't want to upstream base annotations unless we have a good path to getting IWYU correctness and coverage in most places in the code.

Something something wish we had C++ modules.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAN3LA9U9aLYPdQQFr7BjdvtK-RG-XwHHiQrvbq3SeL%2B_Tz8T8A%40mail.gmail.com.

[dan...@chromium.org]

The annotations seem to have no real cost to developers and help tools, they are not invasive and you dont have to worry about them unless you're trying to work on IWYU.

I would be fine with adding the annotations, given we write something explaining about them and stick that in a doc in the tree.

[Peter Kasting]

On Wed, Oct 12, 2022 at 5:37 PM <dan...@chromium.org> wrote:

The annotations seem to have no real cost to developers and help tools, they are not invasive and you dont have to worry about them unless you're trying to work on IWYU.

I would be fine with adding the annotations, given we write something explaining about them and stick that in a doc in the tree.

My concern would be any of the following three scenarios:

• We abandon work on IWYU or go a different route that stops using these annotations. The file becomes useless cruft in the tree, but no one removes it.
• Updating out-of-line annotations becomes a best practice for a variety of reasons, but no tooling exists to remind authors to do this or help them do it easily.
• The annotations are not sufficiently maintained in some way that results in tools (especially editor suggestions) making actively-incorrect recommendations, which is worse than no recommendations.

If we can make these scenarios reasonably unlikely, AND we believe that there is potential value in the current IWYU route (my perception is that google3 considers it a dead end), I would be OK with this.

PK

[dan...@chromium.org]

On Wed, Oct 12, 2022 at 5:43 PM Peter Kasting <pkas...@google.com> wrote:

On Wed, Oct 12, 2022 at 5:37 PM <dan...@chromium.org> wrote:

The annotations seem to have no real cost to developers and help tools, they are not invasive and you dont have to worry about them unless you're trying to work on IWYU.

I would be fine with adding the annotations, given we write something explaining about them and stick that in a doc in the tree.

My concern would be any of the following three scenarios:

• We abandon work on IWYU or go a different route that stops using these annotations. The file becomes useless cruft in the tree, but no one removes it.

IIUC we're talking about putting `// IWYU pragma: private, include "public.h"` comment in some foo_impl.h headers to point to foo.h

 

• Updating out-of-line annotations becomes a best practice for a variety of reasons, but no tooling exists to remind authors to do this or help them do it easily.

The failure mode here would be IWYU suggesting the wrong thing when someone runs it, which we would expect to be caught in review and then the annotations corrected. I think this is sufficient pressure on keeping them correct, proportional to how much they are being used.

• The annotations are not sufficiently maintained in some way that results in tools (especially editor suggestions) making actively-incorrect recommendations, which is worse than no recommendations.

If we can make these scenarios reasonably unlikely, AND we believe that there is potential value in the current IWYU route (my perception is that google3 considers it a dead end), I would be OK with this.

Google3 hasn't spent a year+ trying to remove includes, they are actively adding more. Not sure what this speaks to exactly.. modules? Build systems? Platform differences?

Anyhow I think we would agree that using IWYU as a way to reduce our includes can lead to improving our build times which has been something our compiler teams have spent a lot of time on, and perhaps tooling can free up human time for other stuff?

Cheers,

Dana

 

PK

[Jean-Philippe Gravel]

My concern would be any of the following three scenarios:

• We abandon work on IWYU or go a different route that stops using these annotations. The file becomes useless cruft in the tree, but no one removes it.

It's certainly worth considering whether there are better alternatives, available now or in the near future. I do not however buy the argument that we should keep living with technical debt, and keep accumulating it, waiting for a tool that does not exist, or in fear that the tool we choose now might get replaced in the future. If we ever have to, removing all IWYU pragma is just one mass-regex-replace script away.

• Updating out-of-line annotations becomes a best practice for a variety of reasons, but no tooling exists to remind authors to do this or help them do it easily.

Instrumentation would certainly be useful. A presubmit check would be easy to implement, and would be a very good start.

• The annotations are not sufficiently maintained in some way that results in tools (especially editor suggestions) making actively-incorrect recommendations, which is worse than no recommendations.

This is worth clarifying: no one should ever accept IWYU's result without manually verifying it. That's the same for any changes for that matter, automated or not. We have review processes for a reason. Then, the includes across the code base are already broken as they stand, so if someone submitted a invalid include because of IWYU, it wouldn't be any different than what's happening today. In fact, the net average quality would be much better if IWYU was used, I have the impression that IWYU-assisted humans would do much better than humans on their own.

 

If we can make these scenarios reasonably unlikely, AND we believe that there is potential value in the current IWYU route (my perception is that google3 considers it a dead end), I would be OK with this.

I would be curious to know what gave you this impression. It would certainly be helpful to base this discussion on concrete evidence. I don't know what can be shared on this public forum, but suffice to say that I have been using IWYU in google3 for years with great success. You can search google3 for "IWYU pragma" to get a feel of how widely the feature is used.

PK

[Jean-Philippe Gravel]

> IIUC we're talking about putting `// IWYU pragma: private, include "public.h"` comment in some foo_impl.h headers to point to foo.h

Yes. And, `//IWYU pragma: export`, which is the flip-side of `//IWYU pragma: private`. You would use a `private, include X` if there is a one to one mapping between an impl and its public version. `export` is useful if several headers export content form the same private header, for instance, both Map and Set types are usually implemented using the same Table base type. Users of Map and Set need to use objects defined in Table (e.g. the Iterator class for instance), but shouldn't be bothered to include "table.h" manually.

> Google3 hasn't spent a year+ trying to remove includes, they are actively adding more.

Is this a reference to the ban on forward declarations? Just to be clear, google3 is not spending any time adding useless includes. From that point of view, we are probably adding more includes than google3 as time passes by, due to the fact that we have no visibility on what's needed and what's not.

> Something something wish we had C++ modules.

I think modules are an interesting point to bring up. Modules are the future, we will definitely want to use them when that's possible. But how will we want to support them? Will we want every single module to export every single of their own dependencies, like we do with headers right now? That seems hardly a good idea. Using modules effectively will likely require higher hermeticity standards. Perhaps, if we ever want to migrate to modules, we will need to clean up our dependency tree and make sure files include all their dependencies directly.

[K. Moon]

Perhaps this should be written up in a Google Doc as a proposal, rather than a cxx@ thread? I think the potential blast radius of this change rises above the level of, "Well, let's just try it," and could use a well-composed plan. One of the main ways any project accumulates technical debt is by going off and trying things without sufficient planning, and then not cleaning them up later when they inevitably hit unexpected difficulties.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAN3LA9UH7f3XSjGSaik6a7otS1Bhd2TUaAD6Yv%2BgLi6W3CVsyQ%40mail.gmail.com.

[Jean-Philippe Gravel]

Sounds good, I'll write a proposal document.

[Daniel Cheng]

Thanks for doing this. I do think it would be useful to have some tooling to maintain this. It is quite frustrating to try to clean up headers/move code around and have to fix IWYU fallout across the codebase, so improving that would be really great.

A suggestion for some questions that the doc could help answer:

- whether the external or internal IWYU will fight each other in generating changes, e.g. the two suggest persistently conflicting results. Should we specifically recommend one over the other?

- the preferred style if IWYU pragmas and nogncheck comments need to intersect. Similarly, the preferred style if IWYU pragmas and other explanatory comments on headers intersect

- any potential bad interactions between clang-format wrapping and 80 columns (a quick test seems to show that clang-format does not wrap this, which is good)

- how, if at all, IWYU pragmas will affect clang-format auto-sorting headers. Currently, I believe clang-format treats a comment in the middle of a group of headers as a block delimiter, so headers will not be sorted across that comment.

- general guidance and recommendations for which IWYU pragmas we should be using:

  - do mostly expect the use of private/export? Do we need keep? What about no_forward_declare/private?

  - should we prefer end-of-line comment style always due to the aforementioned clang-format header sorting behavior (e.g. no begin_exports/end_exports and just use export)

- do we need to get IWYU working for a set of core libraries first to make IWYU in a random directory more likely to produce useful results? e.g. //base, some of the public API directories, et cetera?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAN3LA9W1pzFbRO%2BHh8OMGzcKd%2B%3DOQ06tobxNoFokXgOgXxjQSw%40mail.gmail.com.

[Jeroen Dhollander]

I'd like to add 2 more reasons why I like the IWYU pragmas, both of which can be summed up as `clangd uses them`:

   * Since a few months, clangd has enabled unused includes warnings by default. This makes it possible to easily spot (potentially) unused includes in the long include lists on top of our files. Take for example a look at the included screenshot; It's easy to spot the 7 potentially unused includes (they're blue). Unfortunately at least one of them is a false positive (`base/bind.h`), because that just contains a include for `base/functional/bind.h`. Adding a `// IWYU pragma: export` there will stop clangd from flagging this include as unused (as I proposed in https://crrev.com/c/3972491 which then lead me to this thread).

   * Conversely, I rely heavily on clangd adding my includes. I start typing, select the type from the dropdown and bam, the include is added. This is a huge time-saver. Unfortunately this will include these private headers which I then manually have to fix up, unless the `// IWYU pragma: private` is used.

With regards,

Jeroen

[Jeroen Dhollander]

Gentle ping, I'd love to hear a conclusion on whether we can use `// IWYU pragma: export` to help clangd's unused-include feature.

Without this cleaning up includes is just that bit harder.

I can spin out a separate thread about this if people prefer that, since I know this thread is actually about running the IWYU tool which requires more changes.

Thanks,

Jeroen

[Jean-Philippe Gravel]

Wanting to allow IWYU annotations was the main reason I first started this thread:

> This brings me to the main point of this email: if we start using IWYU, we need to agree to maintain these annotations going forward. Note that these annotations provide value irrespective of whether we run IWYU, as they are also understood by clangd.

Though you are right that the thread evolved to much more than this question. This thread is currently blocked on me writing a document, but this is no small task. Previously from this thread:

"I wouldn't want to upstream base annotations unless we have a good path to getting IWYU correctness and coverage in most places in the code." This would mean, figuring out tooling, automation, enforcement, improvements to the IYWU tool, improvement to build toolchain, developer education, etc. I still hope to come up with a proposal, but this is just a side project for me.

I still wish we could agree on allowing IWYU annotations. This is orthogonal to the much bigger problem of permanently integrating the IWYU tool in the toolchain. Does the bar have to be set so high, to the point where everyone is forced to a standstill?

[Peter Boström]

I'm not sure we need to be stalled on something that's just my opinion.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAL0AKM9waU2339%3DU67GgZGC9ha1v23P_gmmLb4TuEcErEn3%3DCw%40mail.gmail.com.

[K. Moon]

Maybe the document could start with a brief description of why this shouldn't cause any long-term problems? And then people can comment if they anticipate any other issues.

If we can't write up even that, I don't think we should proceed.

[dan...@chromium.org]

There's a larger project that could be here but I think the thing that needs to be demonstrated before using annotations is that they won't be a pain for us.

At first glance it seemed fine to me. Daniel raised some good questions. I think it would be wise/pragmatic to scope your effort to answer specifically these questions (and if I missed another one in the thread then shout/address that too).

Once we have answers to where pain may come from, we can identify mitigations or rollout strategies if they are needed. In particular I am wondering about multiple pragma comment systems for a single header (GN + IWYU). The answer may be that it never happens though, since perhaps the annotations are used in different scenarios, or maybe not.

[Jean-Philippe Gravel]

This certainly lowers the bar, thank you ;)

Regarding GN+IWYU, note that IWYU and clangd are happy with comments like:

// IWYU pragma: keep // nogncheck
We could make sure that GN supports this too (which might already be the case, I'm not sure).

[Joe Mason]

Catching up on this thread:

On Thu, Oct 13, 2022 at 2:11 PM Daniel Cheng <dch...@chromium.org> wrote:

- the preferred style if IWYU pragmas and nogncheck comments need to intersect. Similarly, the preferred style if IWYU pragmas and other explanatory comments on headers intersect

Can IWYU annotations contain extra unrecognized text? (eg. "// IWYU: private, include "public.h", nogncheck" - I believe GN would correctly see the "nogncheck" in that line and ignore the rest. Would it cause a warning or error in IWYU?)

 

  - should we prefer end-of-line comment style always due to the aforementioned clang-format header sorting behavior (e.g. no begin_exports/end_exports and just use export)

Even without IWYU tools there's some value to knowing that a block of headers contain re-exported symbols, so I would support putting begin_exports/end_exports headers in a separate block, similar to headers in BUILDFLAG conditionals.

I don't think we should use begin_keep/end_keep, though, because it doesn't give any semantic information since it doesn't say why IWYU needed to be overridden. I would allow inline "keep", but only if an IWYU problem can't be resolved with an annotation that carries more semantic information.

[Jeroen Dhollander]

clangd's `unused-include` check doesn't care about trailing characters, so it is perfectly happy with

```

#include "base/functional/bind.h"  // IWYU pragma: export, nogncheck

```

(It does not like trailing characters on the `// IWYU pragma: private, include "base/bind.h` line, but that's a non-issue as `nogncheck` is specified per include and `IWYU pragma: private` is specified once per file).

On the other hand, it only half-likes the `// IWYU pragma: export` to be on its own line - if you do that it stops complaining about unused includes when the header is used, but it does complain about an unused include in the header itself) - see attached screenshot.

Which sounds like a bug we should file with the clangd team so it's not necessary something we have to care about.

Jeroen

[Peter Kasting]

Possibly-useful note: libc++ trunk just added a mapping file for IWYU ( https://reviews.llvm.org/D138189 ) that might make results a bit nicer for us here.

PK

[Jean-Philippe Gravel]

This is great! It will definitely be useful. These mappings are required for IWYU to work properly, but now, we won't have to maintain them ourselves.

I have experimented some more with IWYU and wrote down a document summarizing the workflow I came up with so far. In particular, I wrote a "run_iwyu.py" that makes it very easy to run IWYU on Chromium files. I used it to fix a few files and folders and it worked great.

https://docs.google.com/document/d/1rUJfTwkezPUhyxodlkZ76qksNqIzsB5yt1fMOVR2_BE/edit#heading=h.4f9946en7wca

[Jean-Philippe Gravel]

Another update. I wrote a presubmit that runs IWYU on modified files. So, I now have:

 - A proposal document.

 - A CL for building IWYU (crrev.com/c/4061289).

 - A "run_iwyu.py" tool (crrev.com/c/4062547).

 - An IWYU presubmit implementation (crrev.com/c/4074586).

 - A demo where I fixed all include files under base/numerics and added a presubmit to keep that folder clean (crrev.com/c/4067888).

 - A CL where I cleaned a very large CC file in Blink, with a presubmit targeting that file only (crrev.com/c/4073943).

I have added a FAQ section in my document answering Daniel's questions directly. Please review these and let me know if there are still concerns. Otherwise, I will move forward with my CLs.

[dan...@chromium.org]

Thank you for the doc, and all the clearly excellent work on the tooling side too.

From what I can see the document answers the questions that have been raised and I have no concerns. Let's use this as a ping to this thread to read the document and if nothing else comes to light let's move forward by the end of next week?

Cheers,
Dana

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAL0AKM99C%2BX%3DS-3RbHFEjBvDm2kbHueqqv-NfrmS_SQQ-8hK4Q%40mail.gmail.com.

[Łukasz Anforowicz]

TIL about https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html - it seems to be another tool that we may be able to use in the quest toward IWYU cleaniness? 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSRWh4VMHmhkuNdQsemJPogyXg-G4KH9QqnkzFgLiFT_g%40mail.gmail.com.

[Dana Jansens]

On Mon, Jul 10, 2023 at 12:05 PM Łukasz Anforowicz <luk...@chromium.org> wrote:

TIL about https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html - it seems to be another tool that we may be able to use in the quest toward IWYU cleaniness? 

I think it would need to understand the same annotations so as to not fight with the other tools. It's not clear if it does - if not maybe a nice contribution to make if someone wants to use this in Chrome.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAA_NCUGxWw1icOq5LEsmS7HY0mDMiWwnuW8QeYcz3O0D%2BMiovA%40mail.gmail.com.

[Hardik Goyal]

Maybe worth reviving this. I just started making contributions to chromium and thought it could really use misc-include-headers in chromium.   Ran clang-tidy locally here https://chromium-review.googlesource.com/c/chromium/src/+/5378632 to fix.  Thought it did a good job here. 

I will be happy to write a proposal, if needed