1679522 - Enable SortIncludes option of clang-format

Status: REOPENED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Simon Giesecke [:sg] [he/him]]

clang-format provides the ability to sort include directives via the SortIncludes option. We should check if we can use that to automate adherence to the coding style, at least to some degree. There are various rules that can be configured. If it's not possible to implement the rules written down in the coding style, these could be adapted to match clang-format's abilities.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Other relevant options here are IncludeBlocks and IncludeCategories.

It seems that clang-format always sorts ignoring case, so we should probably adapt the coding style draft section accordingly. (The current codebase, here there is a visible sorting, is divided on whether case is ignored or not.)

Comment 2

[Simon Giesecke [:sg] [he/him]]

Sorting includes requires that the headers are self-contained, i.e. they include all other headers they need. I will check to what degree this causes issues.

Conditional includes are however, not affected by this. They are preserved as is, resp. if a conditional block includes multiple includes, these are sorted only within that block.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Activate SortIncludes in the clang-format configuration. r=sylvestre

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat mfbt after activating SortIncludes. r=sylvestre

Depends on D98078

Comment 5

[Simon Giesecke [:sg] [he/him]]

Another issue here are macros defined in system headers, which need to be undefined after they are included somewhere, possibly indirectly. A notorious case is None, which is defined by some X11 headers, which is undefined through X11UndefineNone.h. This must be immediately included after including the system header that caused it to be defined. This could be addressed by using a wrapper for that system header that ensures it is undefined appropriately.

Comment 6

[Simon Giesecke [:sg] [he/him]]

check_spidermonkey_style.py checks some convention for sorting & grouping includes that I don't fully understand. Some things might (probably should) be incorporated into the global rules, e.g. oddly_ordered_inclnames. Others are entangled with custom header naming conventions for SpiderMonkey.

Maybe this functionality can be retired at some point in favor of using clang-format to handle this? Maybe with a custom rule for js/src

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Pin order of includes where required before resorting. r=andi

Depends on D98078

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat mfbt after activating SortIncludes.

Depends on D98220

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat js after activating SortIncludes.

Depends on D98222

Comment 10

[Simon Giesecke [:sg] [he/him]]

The js code is the only (?) part of the code base that already does automatic sorting/grouping of include directives, using a custom mechanism. There are different options on how to proceed with this when activating SortIncludes for the codebase.

• Use the existing clang-format configuration for js as well (see https://phabricator.services.mozilla.com/D98312 on how that would currently look)
• Make some extensions to the general clang-format configuration, and use that for js as well (e.g. I think we could add the rules for grouping *-inl headers separately to the general rules, but some other rules are very js-specific, e.g. for grouping js includes separately)
• Make a separate clang-format configuration for js
• Disable SortIncludes for js (so for js the situation would stay unchanged after this bug)

(In the first three cases, the existing code for checking and fixing include directive ordering in config/check_spidermonkey_style.py could be removed)

:jandem, WDYT?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #10)

• Disable SortIncludes for js (so for js the situation would stay unchanged after this bug)

I'd prefer this option. I think SM engineers are happy with the #include order and grouping enforced by the check_spidermonkey_style.py script.

Let me drop a link to this bug in our Matrix room to see what others think.

Comment 12

[Simon Giesecke [:sg] [he/him]]

(In reply to Jan de Mooij [:jandem] from comment #11)

(In reply to Simon Giesecke [:sg] [he/him] from comment #10)

• Disable SortIncludes for js (so for js the situation would stay unchanged after this bug)

I'd prefer this option. I think SM engineers are happy with the #include order and grouping enforced by the check_spidermonkey_style.py script.

Let me drop a link to this bug in our Matrix room to see what others think.

Thanks! I left a note there as well. I also updated the config and patch for js to follow the existing rules more closely. There are a few things that clang-format can't do as it seems:

• Keep the order of subdirectories and casing as is. clang-format seems to always sort subdirectories after the base directory (so all foo/bar/.h includes after all foo/.h includes), and otherwise always case-insensitive.
• Use the corresponding *-inl.h as the main include. It can be configured to do the reverse, i.e. regard foo.h as the main include when ordering includes in foo-inl.h.

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat dom after activating SortIncludes. r=sylvestre

Depends on D98312

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat layout after activating SortIncludes. r=sylvestre

Depends on D98786

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat gfx after activating SortIncludes. r=sylvestre

Depends on D98788

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat toolkit after activating SortIncludes. r=sylvestre

Depends on D98789

Comment 17

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Reformat everything else after activating SortIncludes. r=sylvestre

Depends on D98790

Comment 18

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - SM fixes.

Depends on D98078

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Enable include sorting for SM code.

Depends on D98884

Comment 20

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Fix include directives and forward declarations. r=andi

• Add missing include directives and forward declarations.
• Remove some extra include directives.
• Add missing namespace qualifications.
• Move include directives out of namespace in toolkit/xre/GlobalSemaphore.h

Depends on D98604

Comment 21

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Use <> style for including windows system headers. r=andi

Depends on D98894

Comment 22

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1679522 - Fix macro naming conflict in libjar.

Unfortunately, mozzconf defines a macro named crc32, which causes a naming
conflict with the struct members in zipstruct.h. This patch renames the
latter to avoid that conflict. Otherwise, there's a brittle dependency on
include order.

Depends on D98895

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c3d3c0ed93d
Fix include directives and forward declarations. r=andi,necko-reviewers,jgilbert
https://hg.mozilla.org/integration/autoland/rev/5ac5ac506bc6
Use <> style for including windows system headers. r=andi

Comment 24

[Simon Giesecke [:sg] [he/him]]

I have landed the prerequisites that were ready for landing. andi, maybe you can move this forward?

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4c3d3c0ed93d
https://hg.mozilla.org/mozilla-central/rev/5ac5ac506bc6

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:andi, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 27

[Andi [:andi]]

This is a major blocker since we don't have support for self contained headers.