Standardize Property Declaration Style in Core Classes

[Adlai-Holler]

EDIT I've proposed getting rid of explicit atomic further down in the comments and given my rationale.

OK folks this one won't be fun but let's bang it out.

I propose this as the property declaration style for Texture:

• class, if applicable
• Nullability first, if applicable.
  • Include nullable on weak properties even though it's implied. Nullability and storage class are separate concerns.
• Atomicity.
  • Even if the accessor has specific threading restrictions such as "first access must be on main," it may still be atomic.
  • Even if the object that is accessed doesn't provide an atomic or thread-safe interface (UIView), it may still be atomic.
  • Atomic may be synthesized, C11 atomics, or locked.
• Storage class.
  • Do not include default ARC storage classes – strong for objects, copy for blocks, and assign for primitives.
• Writeability
  • Never include readwrite.
• Custom getter/setter names.

So (nullable, atomic, readonly) is good. (atomic, readwrite, assign) is bad.

Other good examples:
✅ (nullable, atomic) id
✅ (atomic) NSInteger
✅ (nonatomic, copy) NSString *
✅ (atomic, readonly) NSInteger

Nonexamples:
🚫 (atomic, nullable) id
🚫 id
🚫 (nonatomic, assign, readwrite) NSInteger

Thoughts from folks? @garrettmoon @nguyenhuy @maicki @appleguy ?

[appleguy]

@Adlai-Holler thanks for taking the initiative to propose and fix up the project!

I'm curious if others have any style feedback; I haven't given it too much thought, but honestly am happy with anything consistent. So, you have my vote on this if others agree with the style (we should discuss any small tweaks if folks think of them).

There is one thing, though, which is entirely about semantics and not style: how we can achieve consistency and safety in handling transactionalized / locked properties. As of now, this is locking, since atomic is insufficient for correct behavior in our primary use cases.

We agree that atomic properties are fast (per your research / proof of this), which means we can use them anywhere they are useful. However, we need to carefully consider how to safely use them. It has become clear in a number of cases that using atomics is insufficient for even fairly trivial usages, where any other locked data (at all) is involved.

In such cases, not only the usage of the atomic needs to be locked anyway, but also the setter method for the atomic needs the lock...

For this reason, I'd suggest we do atomic in a separate pass at the same time as a quick writeup / documentation of the threading rules (which we already have a number of established). We'd need to equip engineers with some limited / brief guidance on how atomics can be safely used. In particular though, I don't think there is value in atomic-by-default, because all meaningful objects (including ASDisplayNode, ASTextNode, and ASImageNode) need to continue locking the setters, getters, and usages of all properties (atomic or not).

Now, if the properties are implemented with custom getters and setters in order to put in the locking, I suppose the nonatomic vs atomic makes no functional difference? If that's correct, then it doesn't really matter — I hesitate a bit because atomic won't indicate whether it is object-locked or just property-locked. I wish we could solve that ambiguity somehow, but using atomic seems fine as long as we are not relying on the property-level atomicity unless we're certain it's safe.

Thanks again for working on this kind of stuff. It's makes a real difference for the project.

[Adlai-Holler]

Now, if the properties are implemented with custom getters and setters in order to put in the locking, I suppose the nonatomic vs atomic makes no functional difference? If that's correct, then it doesn't really matter — I hesitate a bit because atomic won't indicate whether it is object-locked or just property-locked. I wish we could solve that ambiguity somehow, but using atomic seems fine as long as we are not relying on the property-level atomicity unless we're certain it's safe.

That's exactly the case. Atomic is necessary but not sufficient, and it only affects compilation if you use synthesized accessors. We should document any extra requirements or guarantees about a property, but it's simply incorrect to declare it nonatomic.

For instance, CALayer's properties are all declared atomic (implicitly, since they're not declared nonatomic) and they are all guarded by the same spinlock. "Objective-C atomics" aren't involved.

[Adlai-Holler]

Actually as I go through all these, I think I want to reverse course and leave out atomic explicitly.

There is a warning available for using the implicit atomic attribute, but it's off by default for new projects currently.

Since virtually all of our properties are going to be atomic, having the extra word all over the place is noise. And it's great to call out the nonatomic properties by having longer declarations IMO.

So it would be like:

@property NSInteger myInt;
@property (nonatomic) NSInteger myThreadUnsafeIntegerThatReliesOnUIKit
@property (nullable, weak) id delegate;
@property (nullable, copy) UIColor *backgroundColor;

Less is more.

[nguyenhuy]

Thanks for taking this on, @Adlai-Holler. Consistency is indeed crucial.

On atomicity, I'd agree to leave out atomic because:

1. It's consistent with platform convention.
2. The majority of properties are atomic (either via auto synthesis, locking or queuing is implementation details). So if atomic is required, virtually every property will need either atomic or nonatomic. And that might cause nonatomic to be overlooked.

With this new convention, I think it's important to weed out all nonatomic properties that are in fact thread-safe internally. That's just incorrect and can cause confusions.

And we should update our Coding Guidelines accordingly.

[garrettmoon]

I'm also in on all of these changes. As @appleguy says they bring up questions about how best to indicate what kind of thread safety mechanisms are being used internally, but all your changes should simply move us closer to this goal. Thanks @Adlai-Holler !

[Adlai-Holler]

As for how to document threading requirements/guarantees beyond atomic/nonatomic, I think we could make empty macros like:

#define AS_MAIN_ONLY
#define AS_THREAD_SAFE
#define AS_OBJECT_LOCKED

// Copying from the style of `NSString.UTF8String`

@property (nonatomic) NSInteger mainThreadInt AS_MAIN_ONLY;

@property (nullable, copy) NSArray<NSNumber *> *pointSizeScaleFactors AS_OBJECT_LOCKED;

- (void)setNeedsLayout AS_THREAD_SAFE;

//
// or copying from the style of `os/log.h`
//

AS_MAIN_ONLY
@property (nonatomic) NSInteger mainThreadInt;

AS_OBJECT_LOCKED
@property (nullable, copy) NSArray<NSNumber *> *pointSizeScaleFactors;

AS_THREAD_SAFE
- (void)setNeedsLayout;

Takes? @garrettmoon @nguyenhuy @appleguy

[Adlai-Holler]

The remaining license header warnings are false positives. We use a different license header for YYText.

[garrettmoon]

I like it, though it doesn't help when reading calls to these methods… I wish there was a way to do that too without pollutting the name of the method too much.

[ghost]

	4 Warnings
⚠️	This is a big PR, please consider splitting it up to ease code review.
⚠️	Please ensure license is correct for ASTextDebugOption.h: // // ASTextDebugOption.h // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) through the present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //
⚠️	Please ensure license is correct for ASTextRunDelegate.h: // // ASTextRunDelegate.h // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) through the present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //
⚠️	Please ensure license is correct for NSAttributedString+ASText.h: // // NSAttributedString+ASText.h // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) through the present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //

Generated by 🚫 Danger