Include What You Use: transitive includes from associated .h file

[Charles Harrison]

It seems like the Google style guide has (somewhat) recently changed to include the following rule:

foo.cc should include bar.h if it uses a symbol from it even if foo.h includes bar.h.

I wanted to ask this group if we are comfortable with this rule, or if we wanted to add a special chromium style guide exception.

Pros to adding a new exception:

• Most code in Chromium becomes compliant, and we have much less inconsistency / developer confusion in the codebase. Otherwise cleaning up this inconsistency would be a large-scale effort.
• I believe (though I'm no expert here), that reducing the amount of included files, even those with include guards, will speed up compile times. This argument is similar to the arguments in favor of our exception on forward declarations.
• We have alignment with the public IWYU tool, which goes against the Google style guide.

Cons to adding a new exception:

• We are out of alignment with the Google style guide, causing developer confusion when folks move between Chromium and other Google projects.
• Maybe some code in Chromium is inconsistent if it has been adhering to the new rule.

Let me know your thoughts!

[Peter Boström]

In general I'm against Chromium exceptions unless we have stronger reasons to do so.

I believe the public IWYU tool (or any IWYU tool for that matter) hasn't been able to successfully clean up our codebase. +the...@chromium.org can help me stay honest here.

If the IWYU tool could automate a cleanup but would be inconsistent with the style guide in this aspect I'd be OK with the IWYU tool's output because it would fix more than it violates (but even with the IWYU tool's violations I wouldn't make that a Chromium exception).

I think we should align with Google style but not do any cleanup. I don't think any cleanup that's not fully automated is worth doing here (in either direction).

Peter

--
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/CADjAqN588KQpysRo1pxYeOLjHxgxBwDu1sAudBrioVXrakKfKw%40mail.gmail.com.

[K. Moon]

This change happened a while ago, and we talked about it. The conclusion was that we're comfortable with forward declarations, as they do have a meaningful impact for Chromium build times.

This may change with C++20 modules, but support for those is quite incomplete right now.

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

[Charles Harrison]

kmoon: just to clarify, this proposal is not about forward declarations but about whether foo.cc needs to include bar.h if foo.h also includes bar.h.

[K. Moon]

Sorry, I seem to be conflating two different aspects of the style change:

1. Include X from bar.h directly, not transitively through foo.h.

2. Don't forward declare X, include bar.h instead.

As far as I know, we follow (1) already (but there's no tooling support, so it's aspirational); we explicit rejected (2).

[K. Moon]

Appreciate the clarification!

There's another often-discussed aspect of this rule, which is what a header intends to provide. I've never been able to pin down a style arbiter on what this really means, and it seems to be subject to interpretation. So transitive includes are OK for some headers that intend to "re-export" the contents of another header.

[Charles Harrison]

kmoon: the Google guide is even stronger than what you wrote in (1), which I agree Chromium code generally follows. The rule goes further and says that even foo.cc should include X if its related header foo.h also includes X. I think this relates to your "re-export" point below, where perhaps you could make the argument that foo.h always re-exports to its associated .cc file.

[Peter Kasting]

When we last informally discussed this, the conclusion was to follow the style guide and make no exception for chromium. I have been guiding accordingly since that time.

The public iwyu tool is kind of a red herring. It was forked from the internal tool long ago and is not maintained by googlers.

I don't think there's a compile time argument here -- just scanning the lines of a header for the trailing end of an include guard don't cost a measurable amount afaik.  It's parsing that's expensive.

I don't think we need to try and fix up chromium to follow this. If we ever get iwyu tooling working (which will likely come from the internal version if so), we can tell it to fix things up then.

But I don't feel violently and if group consensus is against this, so be it.

PK

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

[Jeremy Roman]

It's probably not worth making an exception, but this isn't a style rule I see enforced much by reviewers anyway because we have no automatic enforcement and manual enforcement is extremely tedious.

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

[Charles Harrison]

On Fri, May 27, 2022 at 10:38 AM Jeremy Roman <jbr...@chromium.org> wrote:

It's probably not worth making an exception, but this isn't a style rule I see enforced much by reviewers anyway because we have no automatic enforcement and manual enforcement is extremely tedious.

On Fri, May 27, 2022 at 11:35 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

When we last informally discussed this, the conclusion was to follow the style guide and make no exception for chromium. I have been guiding accordingly since that time.

The public iwyu tool is kind of a red herring. It was forked from the internal tool long ago and is not maintained by googlers.

I don't think there's a compile time argument here -- just scanning the lines of a header for the trailing end of an include guard don't cost a measurable amount afaik.  It's parsing that's expensive.

Ah, so I was operating under the assumption that the compiler would still open the file, which would itself cause a slowdown. It appears clang has an optimization around this though:

https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fshow-skipped-includes

[Gabriel Charette]

I wouldn't enforce either way in code reviews (short of IWYU tools, it's a pain to get this right). It seems like this new rule is only a simplification when a tool is adding the includes (per file, no implicit includes), does google3 have IWYU tooling?

Without IWYU tools, I do generally like to depend on the assumption that APIs (or my own header) provide the includes I need to use them (I wouldn't want to require also including location.h, task_traits.h and sequenced_task_runner.h just to use thread_pool.h).

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

[Peter Kasting]

On Fri, May 27, 2022 at 10:02 AM Gabriel Charette <g...@chromium.org> wrote:

does google3 have IWYU tooling?

Yes.

We have done some experiments with said tools on Chromium.  The big problem with any set of IWYU tools is they depend on the compiler seeing all the code, which breaks in the presence of e.g. platform-specific #ifdefs.  So it's very easy for IWYU tools to cause havoc on files with #ifdefs, which is much more prevalent in Chromium than google3.  :(

PK

[K. Moon]

google3 has IWYU tooling, but there's a lot of duct tape holding it together, too. (Lots of headers with special case pragmas.) The IWYU rule itself needs to be simplified to make it more tractable.

We originally discussed removal of the related header rule back in 2019:

https://groups.google.com/a/chromium.org/g/cxx/c/ETW3zeyeT0o/m/dhTGmVNdBgAJ

There wasn't much controversy at the time (I'll do some more archive trawling); I think there may have been more discussion later. It boils down to the principle of following the Google C++ style guide unless there's a really good reason not to, and in this case, there wasn't a good reason not to. Compliance is uneven because it's a hard rule to automate, but it's still the official rule.

[dan...@chromium.org]

On Fri, May 27, 2022 at 1:07 PM K. Moon <km...@chromium.org> wrote:

google3 has IWYU tooling, but there's a lot of duct tape holding it together, too. (Lots of headers with special case pragmas.) The IWYU rule itself needs to be simplified to make it more tractable.

We originally discussed removal of the related header rule back in 2019:

https://groups.google.com/a/chromium.org/g/cxx/c/ETW3zeyeT0o/m/dhTGmVNdBgAJ

There wasn't much controversy at the time (I'll do some more archive trawling); I think there may have been more discussion later. It boils down to the principle of following the Google C++ style guide unless there's a really good reason not to, and in this case, there wasn't a good reason not to. Compliance is uneven because it's a hard rule to automate, but it's still the official rule.

IMO the failure case is this coming up in code review at all, and we now see that it is. The result on the readability of the code is none, the impact on IWYU is none, right? I think we should explicitly say reviewers should accept either.

[Peter Kasting]

On Fri, May 27, 2022 at 11:26 AM <dan...@chromium.org> wrote:

On Fri, May 27, 2022 at 1:07 PM K. Moon <km...@chromium.org> wrote:

google3 has IWYU tooling, but there's a lot of duct tape holding it together, too. (Lots of headers with special case pragmas.) The IWYU rule itself needs to be simplified to make it more tractable.

We originally discussed removal of the related header rule back in 2019:

https://groups.google.com/a/chromium.org/g/cxx/c/ETW3zeyeT0o/m/dhTGmVNdBgAJ

There wasn't much controversy at the time (I'll do some more archive trawling); I think there may have been more discussion later. It boils down to the principle of following the Google C++ style guide unless there's a really good reason not to, and in this case, there wasn't a good reason not to. Compliance is uneven because it's a hard rule to automate, but it's still the official rule.

IMO the failure case is this coming up in code review at all, and we now see that it is. The result on the readability of the code is none, the impact on IWYU is none, right? I think we should explicitly say reviewers should accept either.

The impact on the internal IWYU is not none, which is why the rule was adopted internally.

Why do we need to carve out exceptions in our style guide when questions that the style guide answers come up?  "The fact that the question came up" is not a good reason to add the cognitive load of Yet Another Chromium Style Rule, IMO.

PK

[K. Moon]

I believe the rationale for the original change is that it was simpler not to have the related header rule. It's still possible for the implementation file (.cpp) to require a header that's not needed by the declaration file (.h), even in the "related header" case (which then makes later removal from the related header harder), so IWYU considerations aren't actually any simpler in this case.

[dan...@chromium.org]

Because if you copy existing code, it no longer complies with the style guide and you get forced into making low-value changes, or having some back and forth about it. Our code all looks this way, so we should explicitly accept it until we have used tools to make it not.

 

PK

[Peter Kasting]

That's a general argument that we should never adopt a style rule until we can make it true across the codebase.  Some folks (e.g. sky@) would agree with you.  Other folks (e.g. titus@, me) would disagree.  The status quo is that both Google and Chrommium have explicitly rejected that reasoning before.  I don't think we should change to using it now.

When you copy code, make the changes to make it comply because you know the rules, or don't because you don't know them or you don't care.  And if reviewers notice them, ask for a fix if they know the rules and think it's worth the time to fix it, or don't.  But if a reviewer asks for something against the styleguide, which is what happened here, the reviewer is simply mistaken and should learn the updated rule.  And if a coder receives a request to do something style-compliant they think is low value, they should just do it, because the reviewer has the style guide on their side.

In no case should there be "back and forth".  "But there was!"  Yes, because Charlie didn't know the Google style rule.  Now he does.  Problem solved.

PK

[Peter Kasting]

On Fri, May 27, 2022 at 11:36 AM Peter Kasting <pkas...@google.com> wrote:

In no case should there be "back and forth".  "But there was!"  Yes, because Charlie didn't know the Google style rule.  Now he does.  Problem solved.

Woof, that last line was pretty dismissive.  I'm sorry.  I think I've said more than I need to say and should stay off the keyboard now.

PK 

[dan...@chromium.org]

On Fri, May 27, 2022 at 2:37 PM Peter Kasting <pkas...@google.com> wrote:

On Fri, May 27, 2022 at 11:33 AM <dan...@chromium.org> wrote:

On Fri, May 27, 2022 at 2:29 PM Peter Kasting <pkas...@google.com> wrote:

On Fri, May 27, 2022 at 11:26 AM <dan...@chromium.org> wrote:

On Fri, May 27, 2022 at 1:07 PM K. Moon <km...@chromium.org> wrote:

google3 has IWYU tooling, but there's a lot of duct tape holding it together, too. (Lots of headers with special case pragmas.) The IWYU rule itself needs to be simplified to make it more tractable.

We originally discussed removal of the related header rule back in 2019:

https://groups.google.com/a/chromium.org/g/cxx/c/ETW3zeyeT0o/m/dhTGmVNdBgAJ

There wasn't much controversy at the time (I'll do some more archive trawling); I think there may have been more discussion later. It boils down to the principle of following the Google C++ style guide unless there's a really good reason not to, and in this case, there wasn't a good reason not to. Compliance is uneven because it's a hard rule to automate, but it's still the official rule.

IMO the failure case is this coming up in code review at all, and we now see that it is. The result on the readability of the code is none, the impact on IWYU is none, right? I think we should explicitly say reviewers should accept either.

The impact on the internal IWYU is not none, which is why the rule was adopted internally.

Why do we need to carve out exceptions in our style guide when questions that the style guide answers come up?  "The fact that the question came up" is not a good reason to add the cognitive load of Yet Another Chromium Style Rule, IMO.

Because if you copy existing code, it no longer complies with the style guide and you get forced into making low-value changes, or having some back and forth about it. Our code all looks this way, so we should explicitly accept it until we have used tools to make it not.

That's a general argument that we should never adopt a style rule until we can make it true across the codebase.

I don't think it is, because I would not argue for that position, and I'm not trying to here. However I don't think Chrome and Google are the same here, because Google has a ton of investment in mass migrations so they can in general make rules that don't match the codebase and in a lower-cost way bring the codebase into alignment.

That aside, I don't think this rule is representative of all style rules. It was a rule we had previously, that Google changed, and (I would claim that) the change is not an improvement to readability nor an assistance to process. So, for all rules that match those criteria, yes I'd prefer to just stick with our historical rule, or explicitly accept both, until such time as we have similar sorts of investment in migrating our codebase to match our words.

[K. Moon]

The thing I like most about having a style guide is not arguing about points covered in the style guide. If we reach a consensus on an alternative rule, great. Until then, I think we should apply the rules as they exist; anything else encourages more confusion, rather than less.

In particular, new contributors (I include myself in this category, even though I've been poking away at this code base for a few years now) aren't going to have a lot of context about what's "consistent" with the Chromium code base, and are going to look to the style guide and other resources for the "right" thing to do. To the extent that we aren't having arguments about, "Well, but this is how the code base actually is, so feel free to ignore the rules..." I think that'd be good.

[Bruce Dawson]

cpplint is the main enforcer of IWYU in Chromium and it is not run everywhere in Chromium. This is problematic because a developer may assume that if they don't get any include ordering or IWYU warnings that their CL must be fine, when in fact it is probably just not being checked. For instance, base was not having cpplint run on it. I recently enabled it just for base/win because enabling it for all of base was more than I wanted to take on.

Which is to say, it might be worth improving the coverage of cpplint so that whatever rules we agree on are somewhat better (if imperfectly) enforced. Doing so also reduces the confusion when moving code (new warnings may be applied) or when writing new code (when expected checks may not be applied).

[Glen Robertson]

IMO the failure case is this coming up in code review at all, and we now see that it is. The result on the readability of the code is none, the impact on IWYU is none, right? I think we should explicitly say reviewers should accept either.

+1 to this. The IWYU tool makes it easy to get includes mostly-correct (modulo ifdefs) which saves time when writing code. I'd like to be able to use that tool without being called upon by the reviewer to add includes to the cc file that the tool will tell me to remove again next time I run it.

--

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/dd6b0001-936b-4913-9b77-23144118b23fn%40chromium.org.

[Gabriel Charette]

Re. "IWYU is hard because it relies on the compiler": I think we're limiting ourselves by excluding a much simpler solution that covers 99% of cases (regex matching).

Literally all of the "fix IWYU" CLs I've written over the years (because lack of IWYU prevented deleting an unused include in a widely used header) have been a simple python regex script.

We would get a lot of mileage with a simple list of widely used APIs and the Chromium header needed to use them. The vast majority of the IWYU tooling complexity is in the system headers and that's not where the Chromium issue lies.

[K. Moon]

I like this idea. What would a prototype look like?

[Gabriel Charette]

On Tue, May 31, 2022 at 11:14 AM K. Moon <km...@chromium.org> wrote:

I like this idea. What would a prototype look like?

Kind of like PRESUBMIT.py, with an auto-apply option?

i.e. just a list of [

  {"include": "base/foo.h", "include_if": "base::Foo", "exclude_if": "class Foo;"},

  ...

]

(need `exclude_if` from experience, e.g. in this case for fwd-decls)

Force-applied on the codebase for new entries.

Could probably get messy though (i.e. my scripts usually ignore base::Foo* pointers)... Alternatively, can we just restrain an existing IWYU tool to Chromium headers only (no system, no third-party by default)?

[dan...@chromium.org]

On Tue, May 31, 2022 at 3:58 PM Gabriel Charette <g...@chromium.org> wrote:

On Tue, May 31, 2022 at 11:14 AM K. Moon <km...@chromium.org> wrote:

I like this idea. What would a prototype look like?

Kind of like PRESUBMIT.py, with an auto-apply option?

i.e. just a list of [

  {"include": "base/foo.h", "include_if": "base::Foo", "exclude_if": "class Foo;"},

  ...

]

(need `exclude_if` from experience, e.g. in this case for fwd-decls)

Force-applied on the codebase for new entries.

Could probably get messy though (i.e. my scripts usually ignore base::Foo* pointers)... Alternatively, can we just restrain an existing IWYU tool to Chromium headers only (no system, no third-party by default)?

It would also be possible to build this list into a clang-tidy check. Then you don't have to do brace tracking to know what namespace you're in and such.

[Bruce Dawson]

Wouldn't we want to extend cpplint to support Chromium header files, and then expand the coverage of cpplint?

Potentially we should be expanding the coverage of cpplint first because right now if it is run on, say, all of base, it finds lots of iwyu issues even without any expansion of what is checked.

--

Bruce Dawson, he/him

[dan...@chromium.org]

On Tue, May 31, 2022 at 4:30 PM Bruce Dawson <bruce...@google.com> wrote:

Wouldn't we want to extend cpplint to support Chromium header files, and then expand the coverage of cpplint?

Potentially we should be expanding the coverage of cpplint first because right now if it is run on, say, all of base, it finds lots of iwyu issues even without any expansion of what is checked.

I thought cpplint finds missing headers but doesn't find headers that are no longer needed, so it only solves half the problem. Maybe that's enough..?

[Bruce Dawson]

You are correct that cpplint will only tell you about missing headers, such as string, etc.

I thought that it warned about excessive includes but that is CheckForSuperfluousStlIncludesInHeaders(), which simply warns if various STL headers are included but std:: not mentioned (but only in header files for some reason).

--

Bruce Dawson, he/him

[Gabriel Charette]

I'm okay with one-way IWYU as it's strictly better than today.

Today is kind of one-way anyways as many devs only add headers when things don't compile.

And I can say from completing many migrations that unused headers are quite common already... Would be nice to solve that too but not mandatory?

[Xiaohan Wang (王消寒)]

FWIW, what IWYU tool do people use when working on Chromium? Is there a doc/page on this to help Chromium contributors? Thanks!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7L%2BM9_kgr%2BPErDvXQTFo4NytrYNd8vsmV8mu-6Mig-87PA%40mail.gmail.com.

[Peter Kasting]

On Wed, Jun 1, 2022 at 9:07 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

FWIW, what IWYU tool do people use when working on Chromium? Is there a doc/page on this to help Chromium contributors? Thanks!

No, there's not, because there is no supported tool.  All available tools are Use At Your Own Risk.  The best tool is the Google internal one, but I don't know how to get it working.

We've tried to fix this, it looks intractable.

PK 

[Bruce Dawson]

Perhaps relevant to this discussion, I just found that cpplint's IWYU implementation is designed such that it can cause latent presubmit failures to creep in. It looks like cpplint counts an include as having been legitimately included if it is included by the associated header file. So, if a change removes an include from, say, view.h, then that could trigger a presubmit error in view.cc. If view.cc was not modified in that change then the presubmit system would not analyze it, and the latent presubmit failure would be hidden until the next person modified the file.

To be more specific, crrev.com/c/3953060 (pkasting@, FWIW) deleted "#include <algorithm>" from view.h, leading to this error:

    ui\views\view.cc(372): (cpplint)  Add #include <algorithm> for max  [build/include_what_you_use] [4]

The presubmit --all bot is almost functional so it will find these things in a timely manner. I'm not sure what else we can do until then. crrev.com/c/3956668 is the CL I just uploaded to fix the error.

--

Bruce Dawson, he/him