Use clang-format to sort includes

[npaun]

In my experience, a consistent include order is very beneficial. It saves time during PR reviews, avoiding comments when a dev has inserted an include in the wrong place, or has slightly different IDE settings than another.

It's been a month since we've had clang-format on, and things have been stable, so let's bring back @danlapid's include order config (#2505 (comment)) and apply it.

As last time, two manual adjustments were required:

1. Disable clang-format for the includes at the bottom of jsg.h
2. Ensure we're including jsg.h and not its internal implementation headers like jsg/function.h

[danlapid]

LOVE IT ⭐
LGTM so long as tests are passing of course.
I'm gonna add a requires-internal-pr just in case

[justin-mp]

Are we doing to do the same thing with the internal repository?

[danlapid]

Are we doing to do the same thing with the internal repository?

I definitely think we should but the 2 don't need to be done together. we can do the internal change at a later time.

[justin-mp]

Are we doing to do the same thing with the internal repository?

I definitely think we should but the 2 don't need to be done together. we can do the internal change at a later time.

Agreed that they don't need to be done together. I just hope we'll also get these nice things in the internal repo too!

[npaun]

Internal PR passes

[anonrig]

Before landing this, I think we should make sure that we are all using the same exact clang-format-18 version. Different clang-format versions (even if they're under the same exact major/minor version) might and will introduce breaking changes.

For example, I have 18.1.3, and whenever I format, it formats every single file and makes changes right now.

[danlapid]

changes

I think the correct version would be debian llvm deb most updated clang-format-18.
Currently that's 18.1.8

[anonrig]

@danlapid My clang-format version makes the following change for all files. I think we should assert that in the repository, before making this change. We might have more conflicting differences.

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

[danlapid]

@danlapid My clang-format version makes the following change for all files. I think we should assert that in the repository, before making this change. We might have more conflicting differences.

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

Yeah I'm in favor of asserting major,minor,patch clang-format versions then if we're seeing such differences even on patch releases.
This specific difference though seems like we might just be missing some flag in our .clang-format file.

[npaun]

Yeah I'm a bit surprised by the differences. We know the version in CI produces the same results as on my Mac.

[fhanau]

@danlapid My clang-format version makes the following change for all files. I think we should assert that in the repository, before making this change. We might have more conflicting differences.

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

Yeah I'm in favor of asserting major,minor,patch clang-format versions then if we're seeing such differences even on patch releases. This specific difference though seems like we might just be missing some flag in our .clang-format file.

I noticed this type of change when trying to update the internal repo from using a tagged commit to 18.1.8 – I ultimately couldn't figure out what caused the change but there doesn't seem to be a flag to control it. It's unfortunate that clang-format's behavior is not really stable. Agree that we should enforce a major/minor version – 18.1.8 should be our best bet but apparently Ubuntu is still on 18.1.3 which lines up with Yagiz' comment https://packages.ubuntu.com/search?keywords=clang-format&searchon=names&suite=noble&section=all

[npaun]

@fhanau Do you think we should try to vendor clang-format in workerd?

[danlapid]

@danlapid My clang-format version makes the following change for all files. I think we should assert that in the repository, before making this change. We might have more conflicting differences.

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

Yeah I'm in favor of asserting major,minor,patch clang-format versions then if we're seeing such differences even on patch releases. This specific difference though seems like we might just be missing some flag in our .clang-format file.

I noticed this type of change when trying to update the internal repo from using a tagged commit to 18.1.8 – I ultimately couldn't figure out what caused the change but there doesn't seem to be a flag to control it. It's unfortunate that clang-format's behavior is not really stable. Agree that we should enforce a major/minor version – 18.1.8 should be our best bet but apparently Ubuntu is still on 18.1.3 which lines up with Yagiz' comment packages.ubuntu.com/search?keywords=clang-format&searchon=names&suite=noble&section=all

Did some digging with Yagiz, seems like SpaceBeforeCpp11BracedList is the flag that controls that but for some reason with older and newer clang versions it is not enforced if not set to true or false but with 18.1.3 it is enforces as false if not set.

We could just set it to true then both versions would emit the same result supposedly, even though it would mean another small formatting chage.

[anonrig]

While fixing this issue, can we make sure we are on the same clang-format version via the format.py file? Ubuntu default clang-format might change at any time (or not).

[npaun]

I think we should make sure everyone is using apt.llvm.org - we've had some problems when people don't use it.

[fhanau]

@fhanau Do you think we should try to vendor clang-format in workerd?

I don't want to bikeshed too much here – I think this is best addressed in a follow-up PR to sorting the includes.
My preferred approach would be to rely on the system for installing the package since vendoring a version looks complex based on what we have internally. 18.1.8 is the last 18.x version so if https://bugs.launchpad.net/ubuntu/+source/llvm-toolchain-18/+bug/2075477 gets resolved and macOS users install clang-format via Homebrew and pin it to that version (LLVM 19 got released today and Homebrew clang-format already got updated). Vendoring might still require less maintenance though.
Edit: If folks use apt.llvm.org as Nicholas suggested that makes things easier of course.

[npaun]

If we're in favour, I'll rewrite the commits with SpaceBeforeCpp11BracedList: true and merge.

[fhanau]

I don't agree with some of the spacing decisions clang-format makes here (perhaps newlines are not needed between all groups), but also don't care enough to figure out how to do it differently.

[npaun]

I actually ended up setting SpaceBeforeCpp11BracedList: false to minimize the diff. I also personally think it looks better for cases like designated initialization or empty struct initializers.

[anonrig]

@npaun @danlapid The problem still exists. We should have set SpaceBeforeCpp11BracedList: true.

--- a/src/workerd/api/actor-state-iocontext-test.c++
+++ b/src/workerd/api/actor-state-iocontext-test.c++
@@ -40,7 +40,7 @@ public:
     return kj::heap<MockActorId>(kj::heapString(id));
   }

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

[danlapid]

@npaun @danlapid The problem still exists. We should have set SpaceBeforeCpp11BracedList: true.

--- a/src/workerd/api/actor-state-iocontext-test.c++
+++ b/src/workerd/api/actor-state-iocontext-test.c++
@@ -40,7 +40,7 @@ public:
     return kj::heap<MockActorId>(kj::heapString(id));
   }

-  virtual ~MockActorId() {};
+  virtual ~MockActorId(){};

Can you try to push a PR with those changes and see if it passes lint?