[clang-format] Add support for absl nullability macros #130346

jvoung · 2025-03-07T21:02:58Z

Add support for formatting w/ absl nullability macros (https://github.com/abseil/abseil-cpp/blob/c52afac4f87ef76e6293b84874e5126a62be1f15/absl/base/nullability.h#L237).
Example at https://godbolt.org/z/PYv19M1Gj
input:

std::vector<int* _Nonnull> x;
std::vector<int* absl_nonnull> y;

orig output:

std::vector<int* _Nonnull> x;
std::vector<int * absl_nonnull> y;

new output:

std::vector<int* _Nonnull> x;
std::vector<int* absl_nonnull> y;

credit to @ymand for the original patch

llvmbot · 2025-03-08T02:40:50Z

@llvm/pr-subscribers-clang-format

Author: Jan Voung (jvoung)

Changes

Add support for formatting w/ absl nullability macros (https://github.com/abseil/abseil-cpp/blob/c52afac4f87ef76e6293b84874e5126a62be1f15/absl/base/nullability.h#L237).
Example at https://godbolt.org/z/PYv19M1Gj
input:

std::vector&lt;int* _Nonnull&gt; x;
std::vector&lt;int* absl_nonnull&gt; y;

orig output:

std::vector&lt;int* _Nonnull&gt; x;
std::vector&lt;int * absl_nonnull&gt; y;

new output:

std::vector&lt;int* _Nonnull&gt; x;
std::vector&lt;int* absl_nonnull&gt; y;

credit to @ymand for the original patch

Full diff: https://github.com/llvm/llvm-project/pull/130346.diff

3 Files Affected:

(modified) clang/lib/Format/Format.cpp (+4)
(modified) clang/unittests/Format/ConfigParseTest.cpp (+5-2)
(modified) clang/unittests/Format/FormatTest.cpp (+17-2)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index b5f1241321891..1401b51586d03 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1508,6 +1508,10 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
+  // Abseil aliases to clang's `_Nonnull`, `_Nullable` and `_Null_unspecified`.
+  LLVMStyle.AttributeMacros.push_back("absl_nonnull");
+  LLVMStyle.AttributeMacros.push_back("absl_nullable");
+  LLVMStyle.AttributeMacros.push_back("absl_nullability_unknown");
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BinPackLongBracedList = true;
   LLVMStyle.BinPackParameters = FormatStyle::BPPS_BinPack;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 273bab87b1ee1..5c789b21c306f 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -907,8 +907,11 @@ TEST(ConfigParseTest, ParsesConfiguration) {
   CHECK_PARSE("IfMacros: [MYIF]", IfMacros, CustomIfs);
 
   Style.AttributeMacros.clear();
-  CHECK_PARSE("BasedOnStyle: LLVM", AttributeMacros,
-              std::vector<std::string>{"__capability"});
+  CHECK_PARSE(
+      "BasedOnStyle: LLVM", AttributeMacros,
+      std::vector<std::string>({"__capability", "absl_nonnull", "absl_nullable",
+                                "absl_nullability_unknown"}));
+  Style.AttributeMacros.clear();
   CHECK_PARSE("AttributeMacros: [attr1, attr2]", AttributeMacros,
               std::vector<std::string>({"attr1", "attr2"}));
 
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index bd335f4b6a21b..a17cad0b67b08 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -12375,6 +12375,9 @@ TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyFormat("vector<a *_Nonnull> v;");
   verifyFormat("vector<a *_Nullable> v;");
   verifyFormat("vector<a *_Null_unspecified> v;");
+  verifyFormat("vector<a *absl_nonnull> v;");
+  verifyFormat("vector<a *absl_nullable> v;");
+  verifyFormat("vector<a *absl_nullability_unknown> v;");
   verifyFormat("vector<a *__ptr32> v;");
   verifyFormat("vector<a *__ptr64> v;");
   verifyFormat("vector<a *__capability> v;");
@@ -12518,6 +12521,9 @@ TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyIndependentOfContext("MACRO(A *_Nonnull a);");
   verifyIndependentOfContext("MACRO(A *_Nullable a);");
   verifyIndependentOfContext("MACRO(A *_Null_unspecified a);");
+  verifyIndependentOfContext("MACRO(A *absl_nonnull a);");
+  verifyIndependentOfContext("MACRO(A *absl_nullable a);");
+  verifyIndependentOfContext("MACRO(A *absl_nullability_unknown a);");
   verifyIndependentOfContext("MACRO(A *__attribute__((foo)) a);");
   verifyIndependentOfContext("MACRO(A *__attribute((foo)) a);");
   verifyIndependentOfContext("MACRO(A *[[clang::attr]] a);");
@@ -12674,6 +12680,12 @@ TEST_F(FormatTest, UnderstandsAttributes) {
   verifyFormat("SomeType *__unused s{InitValue};", CustomAttrs);
   verifyFormat("SomeType s __unused(InitValue);", CustomAttrs);
   verifyFormat("SomeType s __unused{InitValue};", CustomAttrs);
+  verifyFormat("SomeType *absl_nonnull s(InitValue);", CustomAttrs);
+  verifyFormat("SomeType *absl_nonnull s{InitValue};", CustomAttrs);
+  verifyFormat("SomeType *absl_nullable s(InitValue);", CustomAttrs);
+  verifyFormat("SomeType *absl_nullable s{InitValue};", CustomAttrs);
+  verifyFormat("SomeType *absl_nullability_unknown s(InitValue);", CustomAttrs);
+  verifyFormat("SomeType *absl_nullability_unknown s{InitValue};", CustomAttrs);
   verifyFormat("SomeType *__capability s(InitValue);", CustomAttrs);
   verifyFormat("SomeType *__capability s{InitValue};", CustomAttrs);
 }
@@ -12687,7 +12699,9 @@ TEST_F(FormatTest, UnderstandsPointerQualifiersInCast) {
   verifyFormat("x = (foo *_Nonnull)*v;");
   verifyFormat("x = (foo *_Nullable)*v;");
   verifyFormat("x = (foo *_Null_unspecified)*v;");
-  verifyFormat("x = (foo *_Nonnull)*v;");
+  verifyFormat("x = (foo *absl_nonnull)*v;");
+  verifyFormat("x = (foo *absl_nullable)*v;");
+  verifyFormat("x = (foo *absl_nullability_unknown)*v;");
   verifyFormat("x = (foo *[[clang::attr]])*v;");
   verifyFormat("x = (foo *[[clang::attr(\"foo\")]])*v;");
   verifyFormat("x = (foo *__ptr32)*v;");
@@ -12701,7 +12715,8 @@ TEST_F(FormatTest, UnderstandsPointerQualifiersInCast) {
   LongPointerLeft.PointerAlignment = FormatStyle::PAS_Left;
   StringRef AllQualifiers =
       "const volatile restrict __attribute__((foo)) _Nonnull _Null_unspecified "
-      "_Nonnull [[clang::attr]] __ptr32 __ptr64 __capability";
+      "_Nullable absl_nonnull absl_nullable absl_nullability_unknown "
+      "[[clang::attr]] __ptr32 __ptr64 __capability";
   verifyFormat(("x = (foo *" + AllQualifiers + ")*v;").str(), LongPointerRight);
   verifyFormat(("x = (foo* " + AllQualifiers + ")*v;").str(), LongPointerLeft);

owenca

Why should we add those macros to the default list? Can't you use the AttributeMacros option instead?

jvoung · 2025-03-10T01:53:29Z

It would be nice to to avoid having each project configure via AttributeMacros.

The Abseil macro is meant to be a portable way to use the Clang-specific nullability attributes like _Nonnull (portable in that it will be empty for non-Clang compilers). So one would expect it to be treated the same as the attributes w.r.t. formatting. I don't think it makes sense to opt-out of it / configure it the other way. So users of these absl macros would want this on.

I see some other library-specific bits in Format.cpp like Q_FOREACH, KJ_IF_MAYBE, BOOST_PP_STRINGIZE, so it seemed like there is precedent for library-specific bits baked in?

owenca · 2025-03-10T04:20:22Z

I wish they had not been added to the default but can't take them off the lists for backward compatibility. I don't think we should add more. The reason is that people wouldn't be able to turn off the special meaning if they wanted to use e.g. absl_nonnull as a regular identifier.

Can you log an issue for std::vector<int * absl_nonnull> y;? It's a bug IMO and can be easily fixed.

mydeveloperday · 2025-03-10T12:28:16Z

Imagine if we did this for every "toolkit"... we'd be inundated with add this/that etc... why can't people who use Abseil not add them to their .clang-format? Outside of google are that many people even using Abseil?

Abseil itself doesn't even define them in their .clang-format file.

This is a no from me.

jvoung · 2025-03-10T14:51:42Z

I wish they had not been added to the default but can't take them off the lists for backward compatibility. I don't think we should add more. The reason is that people wouldn't be able to turn off the special meaning if they wanted to use e.g. absl_nonnull as a regular identifier.

What if this is limited to the Google style? +cc some folks involved in the Google style @ilya-biryukov
It is not yet in the style guide, but we are working on getting the style guide updated.

The chance of using absl_nonnull as a regular identifier at the same time as using the Google style is likely lower.

Can you log an issue for std::vector<int * absl_nonnull> y;? It's a bug IMO and can be easily fixed.

filed #130602 but I think there are still cases it wouldn't cover, right? (like ContainerWithSize<MyType* absl_nonnull, MySize * MyCount> fixed_array;)

Abseil itself doesn't even define them in their .clang-format file.

We're working toward getting it in the google style guide, so it will be covered by BasedOnStyle: Google (which is the main thing in the Abseil .clang-format file), so that folks don't need to specify this individually (and less risk of forgetting to set it and diverging from the style).

ilya-biryukov · 2025-03-10T15:20:14Z

EDIT: I did not read the previous version of the patch and did not realize the original proposal was to update the common style, not Google-specific style. Hence, my comment below was written under the assumptions that responses were towards a change that only updates the Google style. Sorry about a rushed comment here, please read what's below with a grain of salt.

Google C++ Style Guide and Abseil go hand-in-hand from Google's perspective. We are the stewards of both and we would prefer to keep the two in sync. We have opted to have the Google Style available as builtin in clang-format and have been sticking to that decision until now. I think it benefits everyone that each user of Abseil doesn't need to update their configuration files everywhere. We don't even have any downstream mechanisms to configure clang-format internally because we are committed to keep clang-format aligned with what we use.

#76239 was a former precedent where we got some mild pushback, but it ultimately landed.

The reason is that people wouldn't be able to turn off the special meaning if they wanted to use e.g. absl_nonnull as a regular identifier.

@owenca we deliberately use the prefixes to make sure it has almost zero chance of clashes with anyone else. What are the chances of somebody prefixing their code with absl_? I searched for absl_nonnull and found two results, both related to Abseil.

Imagine if we did this for every "toolkit"... we'd be inundated with add this/that etc... why can't people who use Abseil not add them to their .clang-format? Outside of google are that many people even using Abseil?
Abseil itself doesn't even define them in their .clang-format file.
This is a no from me.

People from Google were the original authors of clang-format and introduced google as one of default styles supported by the binary. We have never negotiated it explicitly, but having the ability to change the Google Style is definitely something we expected to keep going forward.

Do people on this thread feel it is a reasonable ask from Google? If not, could you elaborate and suggest a different governance model for the style defined by clang-format -style=google?
It is fairly important for us to clarify what folks are thinking about this and how we can move forward with these types of changes going further.

The question is mainly to @mydeveloperday @owenca, but also cc @HazardyKnusperkeks @rymiel who were mentioned in #76239.

ilya-biryukov · 2025-03-10T18:08:08Z

(please note the update comment above, I did not read the change history and didn't realize the original patch was touching the common default style, not Google's style)

HazardyKnusperkeks · 2025-03-10T19:56:15Z

I still think Google should have to say what their style in clang-format is, extending to the default values of AttributeMacros.

I also wouldn't have such a big issue with adding it to the base style, because of the prefix and I think it's nice that Boost or Qt mostly work out of the box.

The only problem I see is how clang-format parses those lists from yaml. I think there is no possibility to choose between adding or overwriting, if you put AttributeMacros in your .clang-format, but it just overwrites the values.

owenca · 2025-03-11T07:30:00Z

I wish they had not been added to the default but can't take them off the lists for backward compatibility. I don't think we should add more. The reason is that people wouldn't be able to turn off the special meaning if they wanted to use e.g. absl_nonnull as a regular identifier.

What if this is limited to the Google style? +cc some folks involved in the Google style @ilya-biryukov It is not yet in the style guide, but we are working on getting the style guide updated.

I would have no problem with that.

mydeveloperday · 2025-03-11T08:46:01Z

The only problem I see is how clang-format parses those lists from yaml. I think there is no possibility to choose between adding or overwriting, if you put AttributeMacros in your .clang-format, but it just overwrites the values.

if they can be overridden!, then I'm ok with putting them either in the Google style or the default style.

ilya-biryukov · 2025-03-11T09:36:51Z

From the Google's perspective, we are happy with any change that makes those macros available in -style=google.
It seems that either of the options gives us that, so up to the rest of the folks in the thread.

clang/unittests/Format/FormatTest.cpp

clang/unittests/Format/ConfigParseTest.cpp

jvoung

Thanks for the review!

clang/unittests/Format/FormatTest.cpp

clang/unittests/Format/ConfigParseTest.cpp

owenca

Loods good otherwise.

clang/unittests/Format/TokenAnnotatorTest.cpp

owenca · 2025-03-13T06:41:01Z

Please also left align the * (i.e. *absl_ → * absl_) for all the added Google style tests in FormatTest.cpp and TokenAnnotatorTest.cpp.

jvoung · 2025-03-14T13:35:45Z

Thank you Owen for the review and helping merge, and thank you all for helping arrive at a solution!

@ymand

Add support for formatting w/ absl nullability macros (https://github.com/abseil/abseil-cpp/blob/c52afac4f87ef76e6293b84874e5126a62be1f15/absl/base/nullability.h#L237). Example at https://godbolt.org/z/PYv19M1Gj input: ``` std::vector<int* _Nonnull> x; std::vector<int* absl_nonnull> y; ``` orig output: ``` std::vector<int* _Nonnull> x; std::vector<int * absl_nonnull> y; ``` new output: ``` std::vector<int* _Nonnull> x; std::vector<int* absl_nonnull> y; ``` credit to @ymand for the original patch

jvoung added 2 commits March 7, 2025 21:02

[clang-format] Add support for absl nullability macros

10df185

format

05dd2f2

jvoung marked this pull request as ready for review March 8, 2025 02:40

llvmbot added the clang-format label Mar 8, 2025

jvoung requested a review from sam-mccall March 8, 2025 02:41

owenca requested changes Mar 8, 2025

View reviewed changes

jvoung added 2 commits March 10, 2025 14:35

Switch to only in Google style

f3654a2

reformat

f7ecc7e

owenca reviewed Mar 12, 2025

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/ConfigParseTest.cpp Outdated Show resolved Hide resolved

jvoung added 2 commits March 12, 2025 18:46

Address comments

1fb4e5c

format

eb2a542

jvoung commented Mar 12, 2025

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/ConfigParseTest.cpp Outdated Show resolved Hide resolved

owenca approved these changes Mar 13, 2025

View reviewed changes

jvoung added 2 commits March 13, 2025 15:19

Address comments

f50660c

one more

1efd20d

owenca approved these changes Mar 13, 2025

View reviewed changes

Reflow comment in TokenAnnotatorTest.cpp

78d915f

owenca merged commit 467ad6a into llvm:main Mar 14, 2025
9 of 10 checks passed

llvm deleted a comment from llvm-ci Mar 15, 2025

owenca mentioned this pull request Mar 15, 2025

[clang-format] classifies * in template arguments as non-pointer in some cases #130602

Closed

[clang-format] Add support for absl nullability macros #130346

[clang-format] Add support for absl nullability macros #130346

Uh oh!

Conversation

jvoung commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 8, 2025

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

jvoung commented Mar 10, 2025

Uh oh!

owenca commented Mar 10, 2025

Uh oh!

mydeveloperday commented Mar 10, 2025

Uh oh!

jvoung commented Mar 10, 2025

Uh oh!

ilya-biryukov commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilya-biryukov commented Mar 10, 2025

Uh oh!

HazardyKnusperkeks commented Mar 10, 2025

Uh oh!

owenca commented Mar 11, 2025

Uh oh!

mydeveloperday commented Mar 11, 2025

Uh oh!

ilya-biryukov commented Mar 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvoung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jvoung commented Mar 14, 2025

Uh oh!

Uh oh!

jvoung commented Mar 7, 2025 •

edited

Loading

ilya-biryukov commented Mar 10, 2025 •

edited

Loading

owenca commented Mar 13, 2025 •

edited

Loading