[clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[jaredgrubb]

Summary

Add a style option to specify the order that property attributes should appear in ObjC property declarations (property attributes are things like`nonatomic, strong, nullable`).

[rymiel]

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

[jaredgrubb]

Some implementation notes:

• The implementation was modeled after QualifierAlignmentFixer and sortCppIncludes.
• the additions to the .bazel/.gn build files was done naively based on searching for where "QualifierFixerTest" appeared in the repo; I can't really test these. It looks right, but it's a guess.
• I considered creating a top-level style similar to QualifierAlignment/QualifierOrder; that one has a few pre-canned values (Leave, Left, Right, Custom), where Custom opens up the style QualifierOrder, an array of qualifier names.
  • Pro: we could create a [Leave, LLVM] that would set them in the same order the DeclPrinter.cpp would dump them
  • Pro: users probably would like not having to think about 18 different attributes (I did try to document them in groups to make them easier to find)
  • Con: I couldn't find any style guides that provide a "standard order", so I don't know any other pre-canned settings that wouldn't just be an opinion of my own personal style. If there are, let me know and that might add weight to going this route.

I don't like that there are 18 attributes that users _probably_ want to specify for the style, so having some shortcut seems helpful. Perhaps I could just document it that way so it's easy to copy-paste? I also considered making it possible to have an alias so the style would only need to have a handful of them to get full coverage (eg, if you specify assign in the style, then the tool would infer you also mean to imply that the similar attributes retain, strong, copy, weak, unsafe_unretained would occupy the same "slot" unless you also specify them explicitly).

[jaredgrubb]

Update ClangFormatStyleOptions.rst as requested by the auto-comment (thanks auto-bot!).

[HazardyKnusperkeks]

LGTM, but someone other has to approve.

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

44–45

[MyDeveloperDay]

Interesting

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

70

You need a unit tests that tests all the keywords or if one becomes a non identifier then it will break

71

Make a function, isAttribute()

[MyDeveloperDay]

clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

78

might it be a good idea to actually test some/all of the keywords you intend to support? just incase some of them are not actually tok::identifiers?

[jaredgrubb]

Address review comments:

• fix some style
• add unit test for each ObjC attribute recognized by the compiler
• adjust the docs for the style-option to show a YAML example with all of them in a sane order (something people could copy-paste as a starter version)

[MyDeveloperDay]

clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

157

I'm not a massive fan of writing looping logic in tests, I prefer just to lay them all out so when it fails I know precisely where and why. I don't want to have to debug a failure. I want it to be very obvious. My concern here in the first place is if someone makes one of your words a keyword further down the link, this is why you should have specific examples.

[jaredgrubb]

Address review comments: unroll a loop in unit tests to explicitly test all the property attributes.

[jaredgrubb]

Are there any other comments on this patch? I would love to make this into clang-17!

[jaredgrubb]

Rebased (no other updates) to re-run build and hopefully get some eyes on this.

[MyDeveloperDay]

I don't have a problem with this, but I'm not an ObjC person so I can't really review from the validity of what its changing. If @owenpan and @HazardyKnusperkeks don't have an problem with this then I don't either as it likely won't impact most C++ users.

clang/lib/Format/QualifierAlignmentFixer.cpp

10 ↗

(On Diff #540492)

This is an unrelated change, it wrong and that was probably my fault, we should do this as a NFC

llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn

23 ↗

(On Diff #540492)

I never change these file either I think there is a bot or something that does this.

llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn

39 ↗

(On Diff #540492)

I never change these file either I think there is a bot or something that does this.

utils/bazel/llvm-project-overlay/clang/BUILD.bazel

1360 ↗

(On Diff #540492)

I never change these files I don't think we need to?

[MyDeveloperDay]

may remove the other build system files, I don't think its our responsiblity to keep them upto date.

[jaredgrubb]

Rebased and adjusted docs to reflect that this patch would appear in clang-format 18 (not 17 now).
Removed extraneous comment change (will do NFC later for that)
Removed other build-system changes (as requested).

[jaredgrubb]

@MyDeveloperDay addressed your comments. Thanks!

Would love to get this in before Phabricator closes. Anything else I can do to help?

[jaredgrubb]

This comment was removed by jaredgrubb.

[owenpan]

@jaredgrubb, a few quick comments:

• Please undo the (unrelated) changes to clang-formatted-files.txt.
• Add a warning to the option in Format.h. See here for an example.
• Rerun dump_format_style.py.
• Rebase if needed.

[jaredgrubb]

Address review feedback from @owenpan on Oct23. Thanks for helping me tune this patch!

[jaredgrubb]

Should I open a GitHub issue for this patch?

[owenpan]

In D150083#4655479, @jaredgrubb wrote:

Should I open a GitHub issue for this patch?

That'd be great.

[owenpan]

See also D153228.

clang/include/clang/Format/Format.h

3261

And reflow the comment.

clang/lib/Format/Format.cpp

3671–3675

Move down as it's Objective-C specific. (See below.)

3713–3720

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

19

Delete.

21–25

Delete.

36

40

Delete.

56

I strongly suggest that we bail out as well if there are leading and/or trailing comments.

76

No else after continue.

78

Do we need trim()?

85–91

Change:

if (cond) {
  ...
} else {
  return;
}

To:

if (!cond)
  return;
...

87

Ditto.

93–96

Ditto.

98

Can we assert PropertyAttributes.size() > 1 here?

105

105–107

Start names with uppercase letters except for functions.

114

Is it valid in Objective-C to have duplicate attributes?

121

Is it possible that Indices becomes empty or has only one element after deduplication?

127

Initialize with AppendAttribute. (See below.)

128–130

132–137

Make it a lambda e.g. AppendAttribute.

155

191

Must @property be the first non-comment tokens of an unwrapped line in Objective-C? And at most one @property per line?

clang/lib/Format/ObjCPropertyAttributeOrderFixer.h

22

Delete.

29

Delete. (See comments in ObjCPropertyAttributeOrderFixer.cpp.)

45–49

Move up to make it private.

[owenpan]

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

105

Oops! Please ignore.

[owenpan]

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

185

Why not InPPDirective?

[jaredgrubb]

Addressing all review comments to date.

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

56

I'm ok with that!

In our codebase we have some code like this:

@property (/*nullable*/ readonly, strong) Thing *thing;

Nullablility annotations are all-or-nothing in a header file (putting it once triggers clang to complain about it missing in other eligible places). We have places where a dev wanted to add the attribute to one line but didn't want to commit to auditing the whole header. Their comment is a hint to the lucky person who does that work one day.

I thought it would be easy to handle a leading/trailing comment in this patch, but I am very ok with skipping these lines completely as you suggest. Conservative is probably safer!

I've revised the tests to affirm that any comments get skipped now.

78

I removed both trim() and my tests still pass. I wasn't fully sure when trim would help, but it doesn't seem to in this case.

98

We shouldn't _assert_, as it is valid to have just one. But I can add an early-return for that though. (I'll also early-return if it's zero, which is also legal, eg @property () int x)

114

It's silly, but clang doesn't seem to care. I tried this, so duplicate was ok, but contradiction was flagged:

@property (strong, nonatomic, nonatomic) X *X;   // duplicate, but no error
@property (strong, nonatomic, weak) Y *y;   // error: Property attributes 'strong' and 'weak' are mutually exclusive

I wasn't sure whether to do this, but went that way since that's what "sort include files" did. However, it seems like an odd corner case so I'd be ok removing this uniquing if you prefer.

121

It is possible to reduce to one element (I have a unit test case for that scenario (a, a)).

I don't see how std::unique could take a non-empty list and output an empty list, so I'll claim "no" on the empty part.

185

I copy-pasted this from LeftRightQualifierAlignmentFixer::analyze, which I used as a template since I'm still getting used to the codebase. I wasn't sure whether this was important, so I left it in. But I don't think I have a good reason.

I've added a new test case SortsInPPDirective that spot-checks some macro definition examples (which did fail unless this Line->InPPDirective check was removed, as expected.).

191

I can't think of any token that should precede a @property. Aka, I don't think there are any __attribute__ that can fill that slot.

You could have @optional or @required floating around between property/method declarations. I've added a test-case for a @property that is preceded by these tokens and proved that the reordering is handled correctly.

As for multiple properties in one line, I've never seen a codebase with that. clang-format does split them already.

[jaredgrubb]

I opened https://github.com/llvm/llvm-project/issues/71323 for this patch.

[jaredgrubb]

Adjusting the Style comments, since the behavior changed and were not updated to reflect that leading/trailing comments no longer have special handling.

[jaredgrubb]

From my perspective, this patch is ready to go! Please let me know if there's anything more I can do to improve the patch! (@owenpan thanks so much for your help so far!)

[owenpan]

Thank you for your patience! There are still a few comments not done from the previous round.

In D150083#4655528, @owenpan wrote:

See also D153228.

Would this patch have a similar performance issue?

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

74

94

114

We should keep the duplicates if clang accepts them. What happens to e.g. @property(x=a, y=b, x=a, y=c)?

144

185

What about Line->Type != LT_ObjCProperty I suggested?

186

191

I asked the questions because if yes, we could move on to the next line before hitting the last token of the current line.

[owenpan]

BTW, it may be simpler and more efficient to use a set (e.g. llvm::SmallSet) for Indices, especially if we don't need/want to handle duplicate attributes that have a value. (See D150083#inline-1551778.)

[jaredgrubb]

In D150083#4656832, @owenpan wrote:

Thank you for your patience!

I appreciate the help :) I'm excited to get this in!

In D150083#4655528, @owenpan wrote:

See also D153228.

Would this patch have a similar performance issue?

Reading through the issue, I don't think so. As I understand that issue, the scale-problem is due to how the passes interact with one another as a whole (since that setting creates a Pass for _each_ qualifier). This ObjC pass uses only ONE pass to handle the ordering of all property attributes. I intentionally did not copy that part of the Qualifier pass because it didn't seem to actually help my use-case, which is much narrower (primarily that I only have to operate on lines that look like properties, not in arbitrary places like function args and variable declarations, etc.).

Please correct me if I missed something about that patch that could apply.

[jaredgrubb]

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

114

Looks like clang does not reject duplicates like that either, even though I can't imagine that it results in anything sane:

@property (getter=Foo, getter=Bar) int x;

I'll remove the de-dup code as you request -- it'll make the pass simpler and marginally faster :)

For dup-cases where the values differ (eg, y=b, y=c), the pass will just stable-sort them. This is the simplest thing to do and IMO is good-enough as I think dup's are questionable anyway.

I've added some test cases for this (and removed the original de-dup test cases).

I'll add a couple unit tests to confirm this and include your example.

185

Ah, I missed that suggestion.

I don't have enough understanding of clang-format to say whether that would be correct, but I'll trust you :)

When I add this check it doesn't break unit tests or cause any change-in-behavior on my ObjC test repo (~10K .h/.m files). So, it -seems- like this is a valid check to add, so I'll do it!

191

In testing, it seems that some other pass (not sure which) will split the line before this new pass sees it. So if I can rely on that (I think I can?), then we can early-break like you propose.

I've added a unit test to cover this scenario.

[jaredgrubb]

Update to address all the review comments.

[owenpan]

LGTM except the final nits.

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

49

56

72

74

83–85

Delete or assert instead.

98

Now we can. 😉

104

You can assert the equality of the sizes before the if if you like.

137

166

176

185

When I add this check it doesn't break unit tests or cause any change-in-behavior on my ObjC test repo (~10K .h/.m files).

If it does, then we probably have a bug in setting LT_ObjCProperty. 🙂

[jaredgrubb]

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

83–85

This line is an optimization to return early in a case like, eg, when there is only one attribute:

@property (nonatomic) int x;

Therefore I don't think it makes sense to assert. I could remove this check but then it would do the sort on vector-of-one before it returns early. This statement is worth keeping.

98

I want to keep the early-return above, which means any assert here would be redundant. Unless I'm missing what you're asking for completely?

104

I'll remove the check; now that we don't de-duplicate, there's not really any way for the size to change. Good catch!

185

hehe good point :)

[jaredgrubb]

Address review comments (adding the 'asserts' and fixing up a couple other minor nits). Thanks @owenpan! I don't know if you're in the US, but if you are Happy Thanksgiving, and if you're not, then Happy Thursday :)

[owenpan]

In D150083#4657394, @jaredgrubb wrote:

Address review comments (adding the 'asserts' and fixing up a couple other minor nits). Thanks @owenpan! I don't know if you're in the US, but if you are Happy Thanksgiving, and if you're not, then Happy Thursday :)

For some reason, I don't get email notifications anymore. :(

I'll land this for you in the next couple of days!

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
      {
         [self.test1 t:self
                      w:self
             callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
                  u = c;
              }]
      }
 
+.. _ObjCPropertyAttributeOrder:
+
+**ObjCPropertyAttributeOrder** (``List of Strings``) :versionbadge:`clang-format 18` :ref:`¶ <ObjCPropertyAttributeOrder>`
+  The order in which ObjC property attributes should appear.
+
+  Attributes in code will be sorted in the order specified. Any attributes
+  encountered that are not mentioned in this array will be sorted last, in
+  stable order. Comments between attributes will leave the attributes
+  untouched.
+
+  .. warning::
+
+   Using this option could lead to incorrect code formatting due to
+   clang-format's lack of complete semantic information. As such, extra
+   care should be taken to review code changes made by this option.
+
+  .. code-block:: yaml
+
+    ObjCPropertyAttributeOrder: [
+        class, direct,
+        atomic, nonatomic,
+        assign, retain, strong, copy, weak, unsafe_unretained,
+        readonly, readwrite, getter, setter,
+        nullable, nonnull, null_resettable, null_unspecified
+    ]
+
 .. _ObjCSpaceAfterProperty:
 
 **ObjCSpaceAfterProperty** (``Boolean``) :versionbadge:`clang-format 3.7` :ref:`¶ <ObjCSpaceAfterProperty>`
   Add a space after ``@property`` in Objective-C, i.e. use
   ``@property (readonly)`` instead of ``@property(readonly)``.
 
 .. _ObjCSpaceBeforeProtocolList:
 

clang/docs/ReleaseNotes.rst

 - Add ``dependentSizedExtVectorType``.
 - Add ``macroQualifiedType``.
 
 clang-format
 ------------
 - Add ``AllowBreakBeforeNoexceptSpecifier`` option.
 - Add ``AllowShortCompoundRequirementOnASingleLine`` option.
 - Change ``BreakAfterAttributes`` from ``Never`` to ``Leave`` in LLVM style.
+- Add ``ObjCPropertyAttributeOrder`` which can be used to sort ObjC property
+  attributes (like ``nonatomic, strong, nullable``).
 
 libclang
 --------
 
 - Exposed arguments of ``clang::annotate``.
 
 Static Analyzer
 ---------------

clang/include/clang/Format/Format.h

   ///           callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
   ///                u = c;
   ///            }]
   ///    }
   /// \endcode
   /// \version 11
   bool ObjCBreakBeforeNestedBlockParam;
 
+  /// The order in which ObjC property attributes should appear.
+  ///
+  /// Attributes in code will be sorted in the order specified. Any attributes
+  /// encountered that are not mentioned in this array will be sorted last, in
+  /// stable order. Comments between attributes will leave the attributes
+  /// untouched.
+  /// \warning
+  ///  Using this option could lead to incorrect code formatting due to
+  ///  clang-format's lack of complete semantic information. As such, extra
+  ///  care should be taken to review code changes made by this option.
+  /// \endwarning
+  /// \code{.yaml}
+  ///   ObjCPropertyAttributeOrder: [
+  ///       class, direct,
+  ///       atomic, nonatomic,
+  ///       assign, retain, strong, copy, weak, unsafe_unretained,
+  ///       readonly, readwrite, getter, setter,
+  ///       nullable, nonnull, null_resettable, null_unspecified
+  ///   ]
+  /// \endcode
+  /// \version 18
+  std::vector<std::string> ObjCPropertyAttributeOrder;
+
   /// Add a space after ``@property`` in Objective-C, i.e. use
   /// ``@property (readonly)`` instead of ``@property(readonly)``.
   /// \version 3.7
   bool ObjCSpaceAfterProperty;
 
   /// Add a space in front of an Objective-C protocol list, i.e. use
   /// ``Foo <Protocol>`` instead of ``Foo<Protocol>``.
   /// \version 3.7
return AccessModifierOffset == R.AccessModifierOffset &&
            MacroBlockEnd == R.MacroBlockEnd && Macros == R.Macros &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&
            NamespaceMacros == R.NamespaceMacros &&
            ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&
            ObjCBlockIndentWidth == R.ObjCBlockIndentWidth &&
            ObjCBreakBeforeNestedBlockParam ==
                R.ObjCBreakBeforeNestedBlockParam &&
+           ObjCPropertyAttributeOrder == R.ObjCPropertyAttributeOrder &&
            ObjCSpaceAfterProperty == R.ObjCSpaceAfterProperty &&
            ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList &&
            PackConstructorInitializers == R.PackConstructorInitializers &&
            PenaltyBreakAssignment == R.PenaltyBreakAssignment &&
            PenaltyBreakBeforeFirstCallParameter ==
                R.PenaltyBreakBeforeFirstCallParameter &&
            PenaltyBreakComment == R.PenaltyBreakComment &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&

clang/lib/Format/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangFormat
   AffectedRangeManager.cpp
   BreakableToken.cpp
   ContinuationIndenter.cpp
   DefinitionBlockSeparator.cpp
   Format.cpp
   FormatToken.cpp
   FormatTokenLexer.cpp
   IntegerLiteralSeparatorFixer.cpp
   MacroCallReconstructor.cpp
   MacroExpander.cpp
   NamespaceEndCommentsFixer.cpp
+  ObjCPropertyAttributeOrderFixer.cpp
   QualifierAlignmentFixer.cpp
   SortJavaScriptImports.cpp
   TokenAnalyzer.cpp
   TokenAnnotator.cpp
   UnwrappedLineFormatter.cpp
   UnwrappedLineParser.cpp
   UsingDeclarationsSorter.cpp
   WhitespaceManager.cpp
 
   LINK_LIBS
   clangBasic
   clangLex
   clangToolingCore
   clangToolingInclusions
   )

clang/lib/Format/Format.cpp

 #include "BreakableToken.h"
 #include "ContinuationIndenter.h"
 #include "DefinitionBlockSeparator.h"
 #include "FormatInternal.h"
 #include "FormatToken.h"
 #include "FormatTokenLexer.h"
 #include "IntegerLiteralSeparatorFixer.h"
 #include "NamespaceEndCommentsFixer.h"
+#include "ObjCPropertyAttributeOrderFixer.h"
 #include "QualifierAlignmentFixer.h"
 #include "SortJavaScriptImports.h"
 #include "TokenAnalyzer.h"
 #include "TokenAnnotator.h"
 #include "UnwrappedLineFormatter.h"
 #include "UnwrappedLineParser.h"
 #include "UsingDeclarationsSorter.h"
 #include "WhitespaceManager.h"
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("Macros", Style.Macros);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("NamespaceMacros", Style.NamespaceMacros);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
     IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
     IO.mapOptional("ObjCBreakBeforeNestedBlockParam",
                    Style.ObjCBreakBeforeNestedBlockParam);
+    IO.mapOptional("ObjCPropertyAttributeOrder",
+                   Style.ObjCPropertyAttributeOrder);
     IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
     IO.mapOptional("ObjCSpaceBeforeProtocolList",
                    Style.ObjCSpaceBeforeProtocolList);
     IO.mapOptional("PackConstructorInitializers",
                    Style.PackConstructorInitializers);
     IO.mapOptional("PenaltyBreakAssignment", Style.PenaltyBreakAssignment);
     IO.mapOptional("PenaltyBreakBeforeFirstCallParameter",
                    Style.PenaltyBreakBeforeFirstCallParameter);
if (Style.isCpp()) {
     if (Style.RemoveParentheses != FormatStyle::RPS_Leave) {
       FormatStyle S = Expanded;
       S.RemoveParentheses = Style.RemoveParentheses;
       Passes.emplace_back([&, S = std::move(S)](const Environment &Env) {
         return ParensRemover(Env, S).process(/*SkipAnnotation=*/true);
       });
     }
 
     if (Style.InsertBraces) {
       FormatStyle S = Expanded;
       S.InsertBraces = true;
       Passes.emplace_back([&, S = std::move(S)](const Environment &Env) {
         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
       });
     }
 
     if (Style.RemoveBracesLLVM) {
       FormatStyle S = Expanded;
       S.RemoveBracesLLVM = true;
       Passes.emplace_back([&, S = std::move(S)](const Environment &Env) {
         return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
if (Style.isCpp()) {
     }
   }
 
   if (Style.SeparateDefinitionBlocks != FormatStyle::SDS_Leave) {
     Passes.emplace_back([&](const Environment &Env) {
       return DefinitionBlockSeparator(Env, Expanded).process();
     });
   }
 
+  if (Style.Language == FormatStyle::LK_ObjC &&
+      !Style.ObjCPropertyAttributeOrder.empty()) {
+    Passes.emplace_back([&](const Environment &Env) {
+      return ObjCPropertyAttributeOrderFixer(Env, Expanded).process();
+    });
+  }
+
   if (Style.isJavaScript() &&
       Style.JavaScriptQuotes != FormatStyle::JSQS_Leave) {
     Passes.emplace_back([&](const Environment &Env) {
       return JavaScriptRequoter(Env, Expanded).process(/*SkipAnnotation=*/true);
     });
   }
 
   Passes.emplace_back([&](const Environment &Env) {

clang/lib/Format/ObjCPropertyAttributeOrderFixer.h

+//===--- ObjCPropertyAttributeOrderFixer.h ------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file declares ObjCPropertyAttributeOrderFixer, a TokenAnalyzer that
+/// adjusts the order of attributes in an ObjC `@property(...)` declaration,
+/// depending on the style.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_FORMAT_OBJCPROPERTYATTRIBUTEORDERFIXER_H
+#define LLVM_CLANG_LIB_FORMAT_OBJCPROPERTYATTRIBUTEORDERFIXER_H
+
+#include "TokenAnalyzer.h"
+
+namespace clang {
+namespace format {
+
+class ObjCPropertyAttributeOrderFixer : public TokenAnalyzer {
+  llvm::StringMap<unsigned> SortOrderMap;
+
+  void analyzeObjCPropertyDecl(const SourceManager &SourceMgr,
+                               const AdditionalKeywords &Keywords,
+                               tooling::Replacements &Fixes,
+                               const FormatToken *Tok) const;
+
+  void sortPropertyAttributes(const SourceManager &SourceMgr,
+                              tooling::Replacements &Fixes,
+                              const FormatToken *BeginTok,
+                              const FormatToken *EndTok) const;
+
+  std::pair<tooling::Replacements, unsigned>
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override;
+
+public:
+  ObjCPropertyAttributeOrderFixer(const Environment &Env,
+                                  const FormatStyle &Style);
+};
+
+} // end namespace format
+} // end namespace clang
+
+#endif

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

+//===--- ObjCPropertyAttributeOrderFixer.cpp -------------------*- C++--*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file implements ObjCPropertyAttributeOrderFixer, a TokenAnalyzer that
+/// adjusts the order of attributes in an ObjC `@property(...)` declaration,
+/// depending on the style.
+///
+//===----------------------------------------------------------------------===//
+
+#include "ObjCPropertyAttributeOrderFixer.h"
+
+#include "llvm/ADT/Sequence.h"
+
+#include <algorithm>
+
+namespace clang {
+namespace format {
+
+ObjCPropertyAttributeOrderFixer::ObjCPropertyAttributeOrderFixer(
+    const Environment &Env, const FormatStyle &Style)
+    : TokenAnalyzer(Env, Style) {
+
+  // Create an "order priority" map to use to sort properties.
+  unsigned index = 0;
+  for (const auto &Property : Style.ObjCPropertyAttributeOrder)
+    SortOrderMap[Property] = index++;
+}
+
+struct ObjCPropertyEntry {
+  StringRef Attribute; // eg, "readwrite"
+  StringRef Value;     // eg, the "foo" of the attribute "getter=foo"
+};
+
+static bool isObjCPropertyAttribute(const FormatToken *Tok) {
+  // Most attributes look like identifiers, but `class` is a keyword.
+  return Tok->isOneOf(tok::identifier, tok::kw_class);
+}
+
+void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes(
+    const SourceManager &SourceMgr, tooling::Replacements &Fixes,
+    const FormatToken *BeginTok, const FormatToken *EndTok) const {
+  assert(BeginTok);
+  assert(EndTok);
+  assert(EndTok->Previous);
+
+  // If there are zero or one tokens, nothing to do.
+  if (BeginTok == EndTok || BeginTok->Next == EndTok)
+    return;
+
+  // Collect the attributes.
+  SmallVector<ObjCPropertyEntry, 8> PropertyAttributes;
+  for (auto Tok = BeginTok; Tok != EndTok; Tok = Tok->Next) {
+    assert(Tok);
+    if (Tok->is(tok::comma)) {
+      // Ignore the comma separators.
+      continue;
+    }
+
+    if (!isObjCPropertyAttribute(Tok)) {
+      // If we hit any other kind of token, just bail.
+      return;
+    }
+
+    // Memoize the attribute. (Note that 'class' is a legal attribute!)
+    PropertyAttributes.push_back({Tok->TokenText, StringRef{}});
+
+    // Also handle `getter=getFoo` attributes.
+    // (Note: no check needed against `EndTok`, since its type is not
+    // BinaryOperator or Identifier)
+    assert(Tok->Next);
+    if (Tok->Next->is(tok::equal)) {
+      Tok = Tok->Next;
+      assert(Tok->Next);
+      if (Tok->Next->isNot(tok::identifier)) {
+        // If we hit any other kind of token, just bail. It's unusual/illegal.
+        return;
+      }
+      Tok = Tok->Next;
+      PropertyAttributes.back().Value = Tok->TokenText;
+    }
+  }
+
+  // There's nothing to do unless there's more than one attribute.
+  if (PropertyAttributes.size() < 2)
+    return;
+
+  // Create a "remapping index" on how to reorder the attributes.
+  SmallVector<unsigned, 8> Indices =
+      llvm::to_vector<8>(llvm::seq<unsigned>(0, PropertyAttributes.size()));
+
+  // Sort the indices based on the priority stored in 'SortOrderMap'; use Max
+  // for missing values.
+  const auto SortOrderMax = Style.ObjCPropertyAttributeOrder.size();
+  auto SortIndex = [&](const StringRef &Needle) -> unsigned {
+    auto I = SortOrderMap.find(Needle);
+    return (I == SortOrderMap.end()) ? SortOrderMax : I->getValue();
+  };
+  llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
+    return SortIndex(PropertyAttributes[LHSI].Attribute) <
+           SortIndex(PropertyAttributes[RHSI].Attribute);
+  });
+
+  // If the property order is already correct, then no fix-up is needed.
+  if (llvm::is_sorted(Indices))
+    return;
+
+  // Generate the replacement text.
+  std::string NewText;
+  const auto AppendAttribute = [&](const ObjCPropertyEntry &PropertyEntry) {
+    NewText += PropertyEntry.Attribute;
+
+    if (!PropertyEntry.Value.empty()) {
+      NewText += "=";
+      NewText += PropertyEntry.Value;
+    }
+  };
+
+  AppendAttribute(PropertyAttributes[Indices[0]]);
+  for (unsigned Index : llvm::drop_begin(Indices)) {
+    NewText += ", ";
+    AppendAttribute(PropertyAttributes[Index]);
+  }
+
+  auto Range = CharSourceRange::getCharRange(
+      BeginTok->getStartOfNonWhitespace(), EndTok->Previous->Tok.getEndLoc());
+  auto Replacement = tooling::Replacement(SourceMgr, Range, NewText);
+  auto Err = Fixes.add(Replacement);
+  if (Err) {
+    llvm::errs() << "Error while reodering ObjC property attributes : "
+                 << llvm::toString(std::move(Err)) << "\n";
+  }
+}
+
+void ObjCPropertyAttributeOrderFixer::analyzeObjCPropertyDecl(
+    const SourceManager &SourceMgr, const AdditionalKeywords &Keywords,
+    tooling::Replacements &Fixes, const FormatToken *Tok) const {
+  assert(Tok);
+
+  // Expect `property` to be the very next token or else just bail early.
+  const FormatToken *const PropertyTok = Tok->Next;
+  if (!PropertyTok || PropertyTok->isNot(Keywords.kw_property))
+    return;
+
+  // Expect the opening paren to be the next token or else just bail early.
+  const FormatToken *const LParenTok = PropertyTok->getNextNonComment();
+  if (!LParenTok || LParenTok->isNot(tok::l_paren))
+    return;
+
+  // Get the matching right-paren, the bounds for property attributes.
+  const FormatToken *const RParenTok = LParenTok->MatchingParen;
+  if (!RParenTok)
+    return;
+
+  sortPropertyAttributes(SourceMgr, Fixes, LParenTok->Next, RParenTok);
+}
+
+std::pair<tooling::Replacements, unsigned>
+ObjCPropertyAttributeOrderFixer::analyze(
+    TokenAnnotator & /*Annotator*/,
+    SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+    FormatTokenLexer &Tokens) {
+  tooling::Replacements Fixes;
+  const AdditionalKeywords &Keywords = Tokens.getKeywords();
+  const SourceManager &SourceMgr = Env.getSourceManager();
+  AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
+
+  for (AnnotatedLine *Line : AnnotatedLines) {
+    assert(Line);
+    if (!Line->Affected || Line->Type != LT_ObjCProperty)
+      continue;
+    FormatToken *First = Line->First;
+    assert(First);
+    if (First->Finalized)
+      continue;
+
+    const auto *Last = Line->Last;
+
+    for (const auto *Tok = First; Tok != Last; Tok = Tok->Next) {
+      assert(Tok);
+
+      // Skip until the `@` of a `@property` declaration.
+      if (Tok->isNot(TT_ObjCProperty))
+        continue;
+
+      analyzeObjCPropertyDecl(SourceMgr, Keywords, Fixes, Tok);
+
+      // There are never two `@property` in a line (they are split
+      // by other passes), so this pass can break after just one.
+      break;
+    }
+  }
+  return {Fixes, 0};
+}
+
+} // namespace format
+} // namespace clang

clang/unittests/Format/CMakeLists.txt

add_clang_unittest(FormatTests
   FormatTestTextProto.cpp
   FormatTestVerilog.cpp
   FormatTokenSourceTest.cpp
   FormatReplacementTest.cpp
   IntegerLiteralSeparatorTest.cpp
   MacroCallReconstructorTest.cpp
   MacroExpanderTest.cpp
   NamespaceEndCommentsFixerTest.cpp
+  ObjCPropertyAttributeOrderFixerTest.cpp
   QualifierFixerTest.cpp
   SortImportsTestJS.cpp
   SortImportsTestJava.cpp
   SortIncludesTest.cpp
   UsingDeclarationsSorterTest.cpp
   TokenAnnotatorTest.cpp
   )
 
 clang_target_link_libraries(FormatTests
   PRIVATE
   clangBasic
   clangFormat
   clangRewrite
   clangToolingCore
   )

clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)                                        \
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";          \
+  EXPECT_EQ(0, parseConfiguration(TEXT, &Style).value());                      \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE)                                         \
+  EXPECT_NE(0, parseConfiguration(TEXT, &Style).value());                      \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+                     const FormatStyle &Style = getLLVMStyle()) {
+    return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator<FormatToken> Allocator;
+  std::vector<std::unique_ptr<llvm::MemoryBuffer>> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+              std::vector<std::string>({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+              "class, direct, atomic, nonatomic, "
+              "assign, retain, strong, copy, weak, unsafe_unretained, "
+              "readonly, readwrite, getter, setter, "
+              "nullable, nonnull, null_resettable, null_unspecified"
+              "]",
+              ObjCPropertyAttributeOrder,
+              std::vector<std::string>({
+                  "class",
+                  "direct",
+                  "atomic",
+                  "nonatomic",
+                  "assign",
+                  "retain",
+                  "strong",
+                  "copy",
+                  "weak",
+                  "unsafe_unretained",
+                  "readonly",
+                  "readwrite",
+                  "getter",
+                  "setter",
+                  "nullable",
+                  "nonnull",
+                  "null_resettable",
+                  "null_unspecified",
+              }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Zero: nothing to do, but is legal.
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+
+  // Check that properties preceded by @optional/@required work.
+  verifyFormat("@optional\n"
+               "@property(a, b) int p;",
+               "@optional @property(b, a) int p;", Style);
+  verifyFormat("@required\n"
+               "@property(a, b) int p;",
+               "@required @property(b, a) int p;", Style);
+
+  // Check two `@property`s on one-line are reflowed (by other passes)
+  // and both have their attributes reordered.
+  verifyFormat("@property(a, b) int p;\n"
+               "@property(a, b) int q;",
+               "@property(b, a) int p; @property(b, a) int q;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsAttributesWithValues) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "getter", "c"};
+
+  // No change
+  verifyFormat("@property(getter=G, c) int p;", Style);
+  verifyFormat("@property(a, getter=G) int p;", Style);
+  verifyFormat("@property(a, getter=G, c) int p;", Style);
+
+  // Reorder
+  verifyFormat("@property(getter=G, c) int p;", "@property(c, getter=G) int p;",
+               Style);
+  verifyFormat("@property(a, getter=G) int p;", "@property(getter=G, a) int p;",
+               Style);
+  verifyFormat("@property(a, getter=G, c) int p;",
+               "@property(getter=G, c, a) int p;", Style);
+
+  // Multiple set properties, including ones not recognized
+  verifyFormat("@property(a=A, c=C, x=X, y=Y) int p;",
+               "@property(c=C, x=X, y=Y, a=A) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsUnspecifiedAttributesToBack) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  verifyFormat("@property(x) int p;", Style);
+
+  // No change in order.
+  verifyFormat("@property(a, x, y) int p;", Style);
+  verifyFormat("@property(b, x, y) int p;", Style);
+  verifyFormat("@property(a, b, c, x, y) int p;", Style);
+
+  // Reorder one unrecognized one.
+  verifyFormat("@property(a, x) int p;", "@property(x, a) int p;", Style);
+
+  // Prove the unrecognized ones have a stable sort order
+  verifyFormat("@property(a, b, x, y) int p;", "@property(x, b, y, a) int p;",
+               Style);
+  verifyFormat("@property(a, b, y, x) int p;", "@property(y, b, x, a) int p;",
+               Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesDuplicatedAttributes) {
+  // Duplicated attributes aren't rejected by the compiler even if it's silly
+  // to do so. Preserve them and sort them best-effort.
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Just a dup and nothing else.
+  verifyFormat("@property(a, a) int p;", Style);
+
+  // A dup and something else.
+  verifyFormat("@property(a, a, b) int p;", "@property(a, b, a) int p;", Style);
+
+  // Duplicates using `=`: stable-sort irrespective of their value.
+  verifyFormat("@property(a=A, a=A, b=X, b=Y) int p;",
+               "@property(a=A, b=X, a=A, b=Y) int p;", Style);
+  verifyFormat("@property(a=A, a=A, b=Y, b=X) int p;",
+               "@property(a=A, b=Y, a=A, b=X) int p;", Style);
+  verifyFormat("@property(a, a=A, b=B, b) int p;",
+               "@property(a, b=B, a=A, b) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsInPPDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Spot-check a few simple cases that require sorting in a macro definition.
+  verifyFormat("#define MACRO @property() int p;", Style);
+  verifyFormat("#define MACRO @property(a) int p;", Style);
+  verifyFormat("#define MACRO @property(a, b) int p;",
+               "#define MACRO @property(b, a) int p;", Style);
+  verifyFormat("#define MACRO @property(a, b, c) int p;",
+               "#define MACRO @property(c, b, a) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesAllAttributes) {
+  // 'class' is the only attribute that is a keyword, so make sure it works too.
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"FIRST",
+                                      "class",
+                                      "direct",
+                                      "atomic",
+                                      "nonatomic",
+                                      "assign",
+                                      "retain",
+                                      "strong",
+                                      "copy",
+                                      "weak",
+                                      "unsafe_unretained",
+                                      "readonly",
+                                      "readwrite",
+                                      "getter",
+                                      "setter",
+                                      "nullable",
+                                      "nonnull",
+                                      "null_resettable",
+                                      "null_unspecified",
+                                      "LAST"};
+
+  // No change: specify all attributes in the correct order.
+  verifyFormat("@property(class, LAST) int p;", Style);
+  verifyFormat("@property(direct, LAST) int p;", Style);
+  verifyFormat("@property(atomic, LAST) int p;", Style);
+  verifyFormat("@property(nonatomic, LAST) int p;", Style);
+  verifyFormat("@property(assign, LAST) int p;", Style);
+  verifyFormat("@property(retain, LAST) int p;", Style);
+  verifyFormat("@property(strong, LAST) int p;", Style);
+  verifyFormat("@property(copy, LAST) int p;", Style);
+  verifyFormat("@property(weak, LAST) int p;", Style);
+  verifyFormat("@property(unsafe_unretained, LAST) int p;", Style);
+  verifyFormat("@property(readonly, LAST) int p;", Style);
+  verifyFormat("@property(readwrite, LAST) int p;", Style);
+  verifyFormat("@property(getter, LAST) int p;", Style);
+  verifyFormat("@property(setter, LAST) int p;", Style);
+  verifyFormat("@property(nullable, LAST) int p;", Style);
+  verifyFormat("@property(nonnull, LAST) int p;", Style);
+  verifyFormat("@property(null_resettable, LAST) int p;", Style);
+  verifyFormat("@property(null_unspecified, LAST) int p;", Style);
+
+  verifyFormat("@property(FIRST, class) int p;", Style);
+  verifyFormat("@property(FIRST, direct) int p;", Style);
+  verifyFormat("@property(FIRST, atomic) int p;", Style);
+  verifyFormat("@property(FIRST, nonatomic) int p;", Style);
+  verifyFormat("@property(FIRST, assign) int p;", Style);
+  verifyFormat("@property(FIRST, retain) int p;", Style);
+  verifyFormat("@property(FIRST, strong) int p;", Style);
+  verifyFormat("@property(FIRST, copy) int p;", Style);
+  verifyFormat("@property(FIRST, weak) int p;", Style);
+  verifyFormat("@property(FIRST, unsafe_unretained) int p;", Style);
+  verifyFormat("@property(FIRST, readonly) int p;", Style);
+  verifyFormat("@property(FIRST, readwrite) int p;", Style);
+  verifyFormat("@property(FIRST, getter) int p;", Style);
+  verifyFormat("@property(FIRST, setter) int p;", Style);
+  verifyFormat("@property(FIRST, nullable) int p;", Style);
+  verifyFormat("@property(FIRST, nonnull) int p;", Style);
+  verifyFormat("@property(FIRST, null_resettable) int p;", Style);
+  verifyFormat("@property(FIRST, null_unspecified) int p;", Style);
+
+  verifyFormat("@property(FIRST, class, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, direct, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, atomic, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, nonatomic, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, assign, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, retain, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, strong, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, copy, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, weak, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, unsafe_unretained, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, readonly, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, readwrite, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, getter, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, setter, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, nullable, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, nonnull, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, null_resettable, LAST) int p;", Style);
+  verifyFormat("@property(FIRST, null_unspecified, LAST) int p;", Style);
+
+  // Reorder: put 'FIRST' and/or 'LAST' in the wrong spot.
+  verifyFormat("@property(class, LAST) int p;", "@property(LAST, class) int p;",
+               Style);
+  verifyFormat("@property(direct, LAST) int p;",
+               "@property(LAST, direct) int p;", Style);
+  verifyFormat("@property(atomic, LAST) int p;",
+               "@property(LAST, atomic) int p;", Style);
+  verifyFormat("@property(nonatomic, LAST) int p;",
+               "@property(LAST, nonatomic) int p;", Style);
+  verifyFormat("@property(assign, LAST) int p;",
+               "@property(LAST, assign) int p;", Style);
+  verifyFormat("@property(retain, LAST) int p;",
+               "@property(LAST, retain) int p;", Style);
+  verifyFormat("@property(strong, LAST) int p;",
+               "@property(LAST, strong) int p;", Style);
+  verifyFormat("@property(copy, LAST) int p;", "@property(LAST, copy) int p;",
+               Style);
+  verifyFormat("@property(weak, LAST) int p;", "@property(LAST, weak) int p;",
+               Style);
+  verifyFormat("@property(unsafe_unretained, LAST) int p;",
+               "@property(LAST, unsafe_unretained) int p;", Style);
+  verifyFormat("@property(readonly, LAST) int p;",
+               "@property(LAST, readonly) int p;", Style);
+  verifyFormat("@property(readwrite, LAST) int p;",
+               "@property(LAST, readwrite) int p;", Style);
+  verifyFormat("@property(getter, LAST) int p;",
+               "@property(LAST, getter) int p;", Style);
+  verifyFormat("@property(setter, LAST) int p;",
+               "@property(LAST, setter) int p;", Style);
+  verifyFormat("@property(nullable, LAST) int p;",
+               "@property(LAST, nullable) int p;", Style);
+  verifyFormat("@property(nonnull, LAST) int p;",
+               "@property(LAST, nonnull) int p;", Style);
+  verifyFormat("@property(null_resettable, LAST) int p;",
+               "@property(LAST, null_resettable) int p;", Style);
+  verifyFormat("@property(null_unspecified, LAST) int p;",
+               "@property(LAST, null_unspecified) int p;", Style);
+
+  verifyFormat("@property(FIRST, class) int p;",
+               "@property(class, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, direct) int p;",
+               "@property(direct, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, atomic) int p;",
+               "@property(atomic, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nonatomic) int p;",
+               "@property(nonatomic, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, assign) int p;",
+               "@property(assign, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, retain) int p;",
+               "@property(retain, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, strong) int p;",
+               "@property(strong, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, copy) int p;", "@property(copy, FIRST) int p;",
+               Style);
+  verifyFormat("@property(FIRST, weak) int p;", "@property(weak, FIRST) int p;",
+               Style);
+  verifyFormat("@property(FIRST, unsafe_unretained) int p;",
+               "@property(unsafe_unretained, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, readonly) int p;",
+               "@property(readonly, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, readwrite) int p;",
+               "@property(readwrite, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, getter) int p;",
+               "@property(getter, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, setter) int p;",
+               "@property(setter, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nullable) int p;",
+               "@property(nullable, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nonnull) int p;",
+               "@property(nonnull, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, null_resettable) int p;",
+               "@property(null_resettable, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, null_unspecified) int p;",
+               "@property(null_unspecified, FIRST) int p;", Style);
+
+  verifyFormat("@property(FIRST, class, LAST) int p;",
+               "@property(LAST, class, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, direct, LAST) int p;",
+               "@property(LAST, direct, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, atomic, LAST) int p;",
+               "@property(LAST, atomic, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nonatomic, LAST) int p;",
+               "@property(LAST, nonatomic, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, assign, LAST) int p;",
+               "@property(LAST, assign, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, retain, LAST) int p;",
+               "@property(LAST, retain, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, strong, LAST) int p;",
+               "@property(LAST, strong, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, copy, LAST) int p;",
+               "@property(LAST, copy, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, weak, LAST) int p;",
+               "@property(LAST, weak, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, unsafe_unretained, LAST) int p;",
+               "@property(LAST, unsafe_unretained, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, readonly, LAST) int p;",
+               "@property(LAST, readonly, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, readwrite, LAST) int p;",
+               "@property(LAST, readwrite, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, getter, LAST) int p;",
+               "@property(LAST, getter, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, setter, LAST) int p;",
+               "@property(LAST, setter, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nullable, LAST) int p;",
+               "@property(LAST, nullable, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, nonnull, LAST) int p;",
+               "@property(LAST, nonnull, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, null_resettable, LAST) int p;",
+               "@property(LAST, null_resettable, FIRST) int p;", Style);
+  verifyFormat("@property(FIRST, null_unspecified, LAST) int p;",
+               "@property(LAST, null_unspecified, FIRST) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesCommentsAroundAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b"};
+
+  // Zero attributes but comments.
+  verifyFormat("@property(/* 1 */) int p;", Style);
+  verifyFormat("@property(/* 1 */ /* 2 */) int p;", Style);
+
+  // One attribute with comments before or after.
+  verifyFormat("@property(/* 1 */ a) int p;", Style);
+  verifyFormat("@property(a /* 2 */) int p;", Style);
+  verifyFormat("@property(/* 1 */ a /* 2 */) int p;", Style);
+
+  // No reordering if comments are encountered anywhere.
+  // (Each case represents a reordering that would have happened
+  // without the comment.)
+  verifyFormat("@property(/* before */ b, a) int p;", Style);
+  verifyFormat("@property(b, /* between */ a) int p;", Style);
+  verifyFormat("@property(b, a /* after */) int p;", Style);
+}
+
+} // namespace
+} // namespace test
+} // namespace format
+} // namespace clang