This is an archive of the discontinued LLVM Phabricator instance.

[SemaObjC] Add a warning for @selector expressions that potentially refer to objc_direct methods
ClosedPublic

Authored by erik.pilkington on Jun 25 2020, 4:39 PM.

Details

Summary

As a heuristic, only warn if the selector matches a method declared in the current interface.

rdar://64621668

Diff Detail

Event Timeline

MadCoder added a comment.EditedJul 1 2020, 9:30 AM

So the risk with that one is that if someone had say the -didSomething direct selector and that some UIKit/Apple SDK API now adds this as a thing that you use with CFNotification center for example, where you _need_ dynamism, then the uses of the API would warn all the time when the internal to the client project -didSomething is in scope.

So we ideally need a way to silence this warning, so at the very least this needs to be a new -W flag (on by default (?)) so that callers that have an issue can use a _Pragma at the call site to ignore the error for when it's legal.

I would suggest something like -Wstrict-direct-dispatch or something.

MadCoder added inline comments.Jul 1 2020, 9:34 AM
clang/lib/Sema/SemaExprObjC.cpp
1230

missing line between the functions?

MadCoder added inline comments.Jul 1 2020, 9:37 AM
clang/test/SemaObjC/potentially-direct-selector.m
84

I think this might be too weak, if we add a warning then maybe what we want is a tri-state:

-Wstrict-direct-dispatch=none|self|all

because I can see cases where the heuristic here would be too weak

erik.pilkington marked 3 inline comments as done.

Add an off-by-default variant of this warning.

So the risk with that one is that if someone had say the -didSomething direct selector and that some UIKit/Apple SDK API now adds this as a thing that you use with CFNotification center for example, where you _need_ dynamism, then the uses of the API would warn all the time when the internal to the client project -didSomething is in scope.

So we ideally need a way to silence this warning, so at the very least this needs to be a new -W flag (on by default (?)) so that callers that have an issue can use a _Pragma at the call site to ignore the error for when it's legal.

The DiagGroup<"potentially-direct-selector"> creates a command line flag, so the pragma thing works. I couldn't think of a decent way of suppressing this without pragmas, but ISTM that this will have a low enough false-positive rate that it won't be a big problem.

I would suggest something like -Wstrict-direct-dispatch or something.

I kinda prefer -Wpotentially-direct-selector, since that seems to more closely correspond to what the compiler is complaining about. WDYT?

clang/test/SemaObjC/potentially-direct-selector.m
84

Sure - new patch adds an off-by-default "strict" variant, since I think that's a bit more common in clang then the -Wx=y approach.

I would suggest something like -Wstrict-direct-dispatch or something.

I kinda prefer -Wpotentially-direct-selector, since that seems to more closely correspond to what the compiler is complaining about. WDYT?

Yes, that is better, I wasn't dead-set on the name.

MadCoder accepted this revision.Jul 6 2020, 6:04 PM
This revision is now accepted and ready to land.Jul 6 2020, 6:04 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 10:30 AM