Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes
ClosedPublic

Authored by jaredgrubb on May 7 2023, 9:02 PM.

Details

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`).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This comment was removed by jaredgrubb.

@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.

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

Should I open a GitHub issue for this patch?

Should I open a GitHub issue for this patch?

That'd be great.

See also D153228.

clang/include/clang/Format/Format.h
2998

And reflow the comment.

clang/lib/Format/Format.cpp
3488–3492

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

3536
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 added inline comments.Oct 29 2023, 11:37 PM
clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
105

Oops! Please ignore.

owenpan added inline comments.Oct 30 2023, 2:09 PM
clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
185

Why not InPPDirective?

jaredgrubb marked 28 inline comments as done.

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 updated this revision to Diff 558010.Nov 5 2023, 1:34 PM

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

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 added a comment.EditedTue, Nov 14, 11:59 PM

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

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.

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.)

Thank you for your patience!

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

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 marked 7 inline comments as done.Sat, Nov 18, 1:07 PM
jaredgrubb added inline comments.
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 marked 3 inline comments as done.

Update to address all the review comments.

owenpan accepted this revision.Wed, Nov 22, 3:42 AM

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. 🙂

This revision is now accepted and ready to land.Wed, Nov 22, 3:42 AM
jaredgrubb marked 11 inline comments as done.Thu, Nov 23, 9:23 AM
jaredgrubb added inline comments.
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 marked 4 inline comments as done.

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 :)

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!

This revision was landed with ongoing or failed builds.Fri, Dec 1, 5:41 PM
This revision was automatically updated to reflect the committed changes.