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

[erik.pilkington]

Summary

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

rdar://64621668

[MadCoder]

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]

clang/lib/Sema/SemaExprObjC.cpp

1230

missing line between the functions?

[MadCoder]

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

85

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]

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

[erik.pilkington]

In D82611#2125868, @MadCoder wrote:

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

85

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.

[MadCoder]

In D82611#2133521, @erik.pilkington wrote:

In D82611#2125868, @MadCoder wrote:

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.

clang/include/clang/Basic/DiagnosticGroups.td

def ObjCLiteralComparison : DiagGroup<"objc-literal-compare", [
   ]>;
 
 def ObjCSignedCharBool : DiagGroup<"objc-signed-char-bool",
   [ObjCSignedCharBoolImplicitIntConversion,
    ObjCSignedCharBoolImplicitFloatConversion,
    ObjCBoolConstantConversion,
    TautologicalObjCBoolCompare]>;
 
+def ObjCPotentiallyDirectSelector : DiagGroup<"potentially-direct-selector">;
+def ObjCStrictPotentiallyDirectSelector :
+  DiagGroup<"strict-potentially-direct-selector",
+            [ObjCPotentiallyDirectSelector]>;
+
 // Inline ASM warnings.
 def ASMOperandWidths : DiagGroup<"asm-operand-widths">;
 def ASM : DiagGroup<"asm", [
     ASMOperandWidths
   ]>;
 
 // OpenMP warnings.
 def SourceUsesOpenMP : DiagGroup<"source-uses-openmp">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_unimplemented_selector:  Warning<
   "no method with selector %0 is implemented in this translation unit">,
   InGroup<Selector>, DefaultIgnore;
 def warn_unimplemented_protocol_method : Warning<
   "method %0 in protocol %1 not implemented">, InGroup<Protocol>;
 def warn_multiple_selectors: Warning<
   "several methods with selector %0 of mismatched types are found "
   "for the @selector expression">,
   InGroup<SelectorTypeMismatch>, DefaultIgnore;
-def err_direct_selector_expression: Error<
+def err_direct_selector_expression : Error<
   "@selector expression formed with direct selector %0">;
+def warn_potentially_direct_selector_expression : Warning<
+  "@selector expression formed with potentially direct selector %0">,
+  InGroup<ObjCPotentiallyDirectSelector>;
+def warn_strict_potentially_direct_selector_expression : Warning<
+  warn_potentially_direct_selector_expression.Text>,
+  InGroup<ObjCStrictPotentiallyDirectSelector>, DefaultIgnore;
 
 def err_objc_kindof_nonobject : Error<
   "'__kindof' specifier cannot be applied to non-object type %0">;
 def err_objc_kindof_wrong_position : Error<
   "'__kindof' type specifier must precede the declarator">;
 
 def err_objc_method_unsupported_param_ret_type : Error<
   "%0 %select{parameter|return}1 type is unsupported; "

clang/lib/Sema/SemaExprObjC.cpp

for (Sema::GlobalMethodPool::iterator b = S.MethodPool.begin(),
 
     // second, class methods
     ObjCMethodList &ClsMethList = b->second.second;
     if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
                                                       Method, ClsMethList) || Warned)
       return;
   }
 }
 
-static void HelperToDiagnoseDirectSelectorsExpr(Sema &S, SourceLocation AtLoc,
-                                                Selector Sel,
+static ObjCMethodDecl *LookupDirectMethodInMethodList(Sema &S, Selector Sel,
                                                       ObjCMethodList &MethList,
-                                                bool &onlyDirect) {
+                                                      bool &onlyDirect,
+                                                      bool &anyDirect) {
+  (void)Sel;
   ObjCMethodList *M = &MethList;
-  for (M = M->getNext(); M; M = M->getNext()) {
+  ObjCMethodDecl *DirectMethod = nullptr;
+  for (; M; M = M->getNext()) {
     ObjCMethodDecl *Method = M->getMethod();
-    if (Method->getSelector() != Sel)
+    if (!Method)
       continue;
-    if (!Method->isDirectMethod())
+    assert(Method->getSelector() == Sel && "Method with wrong selector in method list");
+    if (Method->isDirectMethod()) {
+      anyDirect = true;
+      DirectMethod = Method;
+    } else
       onlyDirect = false;
   }
+
+  return DirectMethod;
 }
 
-static void DiagnoseDirectSelectorsExpr(Sema &S, SourceLocation AtLoc,
-                                        Selector Sel, bool &onlyDirect) {
-  for (Sema::GlobalMethodPool::iterator b = S.MethodPool.begin(),
-       e = S.MethodPool.end(); b != e; b++) {
-    // first, instance methods
-    ObjCMethodList &InstMethList = b->second.first;
-    HelperToDiagnoseDirectSelectorsExpr(S, AtLoc, Sel, InstMethList,
-                                        onlyDirect);
+// Search the global pool for (potentially) direct methods matching the given
+// selector. If a non-direct method is found, set \param onlyDirect to false. If
+// a direct method is found, set \param anyDirect to true. Returns a direct
+// method, if any.
+static ObjCMethodDecl *LookupDirectMethodInGlobalPool(Sema &S, Selector Sel,
+                                                      bool &onlyDirect,
+                                                      bool &anyDirect) {
+  auto Iter = S.MethodPool.find(Sel);
+  if (Iter == S.MethodPool.end())
+    return nullptr;
 
-    // second, class methods
-    ObjCMethodList &ClsMethList = b->second.second;
-    HelperToDiagnoseDirectSelectorsExpr(S, AtLoc, Sel, ClsMethList, onlyDirect);
-  }
+  ObjCMethodDecl *DirectInstance = LookupDirectMethodInMethodList(
+      S, Sel, Iter->second.first, onlyDirect, anyDirect);
+  ObjCMethodDecl *DirectClass = LookupDirectMethodInMethodList(
+      S, Sel, Iter->second.second, onlyDirect, anyDirect);
+
+  return DirectInstance ? DirectInstance : DirectClass;
+
+static ObjCMethodDecl *findMethodInCurrentClass(Sema &S, Selector Sel) {
+  auto *CurMD = S.getCurMethodDecl();
+  if (!CurMD)
+    return nullptr;
+  ObjCInterfaceDecl *IFace = CurMD->getClassInterface();
+
+  // The language enforce that only one direct method is present in a given
+  // class, so we just need to find one method in the current class to know
+  // whether Sel is potentially direct in this context.
+  if (ObjCMethodDecl *MD = IFace->lookupMethod(Sel, /*isInstance=*/true))
+    return MD;
+  if (ObjCMethodDecl *MD = IFace->lookupPrivateMethod(Sel, /*isInstance=*/true))
+    return MD;
+  if (ObjCMethodDecl *MD = IFace->lookupMethod(Sel, /*isInstance=*/false))
+    return MD;
+  if (ObjCMethodDecl *MD = IFace->lookupPrivateMethod(Sel, /*isInstance=*/false))
+    return MD;
+
+  return nullptr;
 }
 
 ExprResult Sema::ParseObjCSelectorExpression(Selector Sel,
                                              SourceLocation AtLoc,
                                              SourceLocation SelLoc,
                                              SourceLocation LParenLoc,
                                              SourceLocation RParenLoc,
                                              bool WarnMultipleSelectors) {
if (const ObjCMethodDecl *OM = SelectorsForTypoCorrection(Sel)) {
                                 RParenLoc.getLocWithOffset(-1));
       Diag(SelLoc, diag::warn_undeclared_selector_with_typo)
         << Sel << MatchedSel
         << FixItHint::CreateReplacement(SelectorRange, MatchedSel.getAsString());
 
     } else
         Diag(SelLoc, diag::warn_undeclared_selector) << Sel;
   } else {
-    bool onlyDirect = Method->isDirectMethod();
-    DiagnoseDirectSelectorsExpr(*this, AtLoc, Sel, onlyDirect);
     DiagnoseMismatchedSelectors(*this, AtLoc, Method, LParenLoc, RParenLoc,
                                 WarnMultipleSelectors);
+
+    bool onlyDirect = true;
+    bool anyDirect = false;
+    ObjCMethodDecl *GlobalDirectMethod =
+        LookupDirectMethodInGlobalPool(*this, Sel, onlyDirect, anyDirect);
+
     if (onlyDirect) {
       Diag(AtLoc, diag::err_direct_selector_expression)
           << Method->getSelector();
       Diag(Method->getLocation(), diag::note_direct_method_declared_at)
           << Method->getDeclName();
+    } else if (anyDirect) {
+      // If we saw any direct methods, see if we see a direct member of the
+      // current class. If so, the @selector will likely be used to refer to
+      // this direct method.
+      ObjCMethodDecl *LikelyTargetMethod = findMethodInCurrentClass(*this, Sel);
+      if (LikelyTargetMethod && LikelyTargetMethod->isDirectMethod()) {
+        Diag(AtLoc, diag::warn_potentially_direct_selector_expression) << Sel;
+        Diag(LikelyTargetMethod->getLocation(),
+             diag::note_direct_method_declared_at)
+            << LikelyTargetMethod->getDeclName();
+      } else if (!LikelyTargetMethod) {
+        // Otherwise, emit the "strict" variant of this diagnostic, unless
+        // LikelyTargetMethod is non-direct.
+        Diag(AtLoc, diag::warn_strict_potentially_direct_selector_expression)
+            << Sel;
+        Diag(GlobalDirectMethod->getLocation(),
+             diag::note_direct_method_declared_at)
+            << GlobalDirectMethod->getDeclName();
+      }
     }
   }
 
   if (Method &&
       Method->getImplementationControl() != ObjCMethodDecl::Optional &&
       !getSourceManager().isInSystemHeader(Method->getLocation()))
     ReferencedSelectors.insert(std::make_pair(Sel, AtLoc));
 

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

+// RUN: %clang_cc1 %s -Wpotentially-direct-selector -verify
+// RUN: %clang_cc1 %s -Wstrict-potentially-direct-selector -verify=expected,strict
+
+#define NS_DIRECT __attribute__((objc_direct))
+
+__attribute__((objc_root_class))
+@interface Dummies
+-(void)inBase;
+-(void)inBaseImpl;
+-(void)inBaseCat;
+-(void)inBaseCatImpl;
+-(void)inDerived;
+-(void)inDerivedImpl;
+-(void)inDerivedCat;
+-(void)inDerivedCatImpl;
++(void)inBaseClass;
++(void)inDerivedClass;
++(void)inDerivedCatClass;
+@end
+
+__attribute__((objc_root_class))
+@interface Base
+-(void)inBase NS_DIRECT; // expected-note + {{direct method}}
++(void)inBaseClass NS_DIRECT;  // expected-note + {{direct method}}
+@end
+
+@implementation Base
+-(void)inBaseImpl NS_DIRECT { // expected-note + {{direct method}}
+}
+-(void)inBase {}
++(void)inBaseClass {}
+@end
+
+@interface Base (Cat)
+-(void)inBaseCat NS_DIRECT; // expected-note + {{direct method}}
+@end
+
+@implementation Base (Cat)
+-(void)inBaseCatImpl NS_DIRECT { // expected-note + {{direct method}}
+}
+-(void)inBaseCat {}
+@end
+
+@interface Derived : Base
+-(void)inDerived NS_DIRECT; // expected-note + {{direct method}}
++(void)inDerivedClass NS_DIRECT;  // expected-note + {{direct method}}
+@end
+
+@implementation Derived
+-(void)inDerivedImpl NS_DIRECT { // expected-note + {{direct method}}
+}
+-(void)inDerived {}
++(void)inDerivedClass {}
+@end
+
+@interface Derived (Cat)
+-(void)inDerivedCat NS_DIRECT; // expected-note + {{direct method}}
++(void)inDerivedCatClass NS_DIRECT; // expected-note + {{direct method}}
+@end
+
+@implementation Derived (Cat)
+-(void)inDerivedCatImpl NS_DIRECT { // expected-note + {{direct method}}
+}
+-(void)inDerivedCat {}
++(void)inDerivedCatClass {}
+
+-(void)test1 {
+  (void)@selector(inBase); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCat); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCatImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerived); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCat); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedClass); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseClass); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatClass); // expected-warning{{@selector expression formed with potentially direct selector}}
+}
+@end
+
+void test2() {
+  (void)@selector(inBase); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCat); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCatImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerived); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCat); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+}
+
+@interface OnlyBase : Base @end
+@implementation OnlyBase
+-(void)test3 {
+  (void)@selector(inBase); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCat); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCatImpl); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerived); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCat); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseClass); // expected-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+}
+@end
+
+__attribute__((objc_root_class))
+@interface Unrelated @end
+@implementation Unrelated
+-(void)test4 {
+  (void)@selector(inBase); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCat); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseCatImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerived); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCat); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatImpl); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inBaseClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+  (void)@selector(inDerivedCatClass); // strict-warning{{@selector expression formed with potentially direct selector}}
+}
+@end
+
+@implementation Dummies
+-(void)inBase {}
+-(void)inBaseImpl {}
+-(void)inBaseCat {}
+-(void)inBaseCatImpl {}
+-(void)inDerived {}
+-(void)inDerivedImpl {}
+-(void)inDerivedCat {}
+-(void)inDerivedCatImpl {}
++(void)inBaseClass {}
++(void)inDerivedClass {}
++(void)inDerivedCatClass {}
+
+-(void)test5 {
+  (void)@selector(inBase);
+  (void)@selector(inBaseImpl);
+  (void)@selector(inBaseCat);
+  (void)@selector(inBaseCatImpl);
+  (void)@selector(inDerived);
+  (void)@selector(inDerivedImpl);
+  (void)@selector(inDerivedCat);
+  (void)@selector(inDerivedCatImpl);
+  (void)@selector(inDerivedClass);
+  (void)@selector(inBaseClass);
+  (void)@selector(inDerivedCatClass);
+}
+@end