[clang] Add a Clang plugin for Objective-C properties

[Asami Doi]

Hello people in the Clang mailing list,

Sylvain (sdefresne@) and I (asamidoi@) are working on cleaning up Objective-C's properties in Chrome for iOS [crbug].
We would like to show warning messages when a developer uses an `assign` attribute to a property pointing to an Objective-C object. Because, under the ARC environment, this property is treated as a `__unsafe_unretained` variable and it potentially becomes a dangling pointer.

```

@property (assign) NSSomeObject* object; // --> WARNING to suggest using strong/weak instead.

@property (assign) CXXSomeObject* object2; // --> OK because it's a C++ object. ARC doesn't manage C++ objects.

@property (assign) Bool b; // --> OK because it's a primitive type.
```

We considered updating AyeAye Analyzer to show warnings on Gerrit but the analyzer doesn't have the semantic information.

I know that the ideal solution would be updating the upstream clang. I've filed the issue to llvm-project and am waiting for the response.

Can we add a clang plugin until we update the upstream clang?
There is a WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/6495340

Let us know what you think!

Thank you,
Asami

[Nico Weber]

> I know that the ideal solution would be updating the upstream clang. I've filed the issue to llvm-project and am waiting for the response.

Have you an upstream PR with the warning that received pushback? If not, could we try implementing this upstream first?

To unsubscribe from this group and stop receiving emails from it, send an email to clang+un...@chromium.org.

[Nico Weber]

-bling-fundamentals, which apparently doesn't permit external messages.

Please don't mix internal and external lists :)

[Sylvain Defresne]

We have not yet tried to implement the warning directly in clang. We'll investigate doing it (we are both inexperienced with clang code base, and a plugin appeared easier to write).

-- Sylvain

[Asami Doi]

I was looking into the upstream clang and I've found the code that probably does what we want to do.

I'll investigate why we have never seen this warnings.

```

  // Check for assign on object types.
  if ((Attributes & ObjCPropertyAttribute::kind_assign) &&
      !(Attributes & ObjCPropertyAttribute::kind_unsafe_unretained) &&
      PropertyTy->isObjCRetainableType() &&
      !PropertyTy->isObjCARCImplicitlyUnretainedType()) {
    Diag(Loc, diag::warn_objc_property_assign_on_object); // message: 'assign' property of object type may become a dangling reference; consider using 'unsafe_unretained'
  }
```

https://github.com/llvm/llvm-project/blob/7bc16a0f63c3f31762b97e5855932b769c4d3cf5/clang/lib/Sema/SemaObjCProperty.cpp#L2654-L2660

[Nico Weber]

Nice find!

Looks like it's default-ignored:

 % rg ObjCPropertyAssignOnObjectType clang
clang/include/clang/Basic/DiagnosticGroups.td
653:def ObjCPropertyAssignOnObjectType : DiagGroup<"objc-property-assign-on-object-type">;

clang/include/clang/Basic/DiagnosticSemaKinds.td
1481:  InGroup<ObjCPropertyAssignOnObjectType>, DefaultIgnore;

And ObjCPropertyAssignOnObjectType isn't in Wall or Wextra (`rg ObjCPropertyAssignOnObjectType clang`)

It sounds like it's disabled intentionally: https://github.com/llvm/llvm-project/commit/52a503d4f333d

Explicitly passing -Wobjc-property-assign-on-object-type should turn it on, though.

[Sylvain Defresne]

Great find. Let's try enabling this for Chrome and fix the issues found.

-- Sylvain