[-Wcompletion-handler] Extend list of detected conventions

[vsavchenko]

Summary

Update convention detection to accomodate changes from:
https://github.com/DougGregor/swift-evolution/blob/concurrency-objc/proposals/NNNN-concurrency-objc.md#asynchronous-completion-handler-methods

[NoQ]

Looks all clear.

In that document there's also a clause that the conventional completion handler discovered through its suffix has to be the last parameter. I don't immediately see that respective check in the code. Do we have it?

[vsavchenko]

In D98251#2615049, @NoQ wrote:

Looks all clear.

In that document there's also a clause that the conventional completion handler discovered through its suffix has to be the last parameter. I don't immediately see that respective check in the code. Do we have it?

You are right on that part, I'll try to incorporate that check into the current (a bit tangled) logic.

clang/lib/Analysis/CalledOnceCheck.cpp

 namespace {
 static constexpr unsigned EXPECTED_MAX_NUMBER_OF_PARAMS = 2;
 template <class T>
 using ParamSizedVector = llvm::SmallVector<T, EXPECTED_MAX_NUMBER_OF_PARAMS>;
 static constexpr unsigned EXPECTED_NUMBER_OF_BASIC_BLOCKS = 8;
 template <class T>
 using CFGSizedVector = llvm::SmallVector<T, EXPECTED_NUMBER_OF_BASIC_BLOCKS>;
 constexpr llvm::StringLiteral CONVENTIONAL_NAMES[] = {
-    "completionHandler", "completion", "withCompletionHandler"};
+    "completionHandler", "completion",      "withCompletionHandler",
+    "withCompletion",    "completionBlock", "withCompletionBlock",
+    "replyTo",           "reply",           "withReplyTo"};
 constexpr llvm::StringLiteral CONVENTIONAL_SUFFIXES[] = {
-    "WithCompletionHandler", "WithCompletion"};
+    "WithCompletionHandler", "WithCompletion", "WithCompletionBlock",
+    "WithReplyTo", "WithReply"};
 constexpr llvm::StringLiteral CONVENTIONAL_CONDITIONS[] = {
     "error", "cancel", "shouldCall", "done", "OK", "success"};
 
 class ParameterStatus {
 public:
   // Status kind is basically the main part of parameter's status.
   // The kind represents our knowledge (so far) about a tracked parameter
   // in the context of this analysis.
if (!isConventional(PieceType)) {
       return false;
     }
 
     if (MethodSelector.getNumArgs() == 1) {
       assert(PieceIndex == 0);
       return hasConventionalSuffix(MethodSelector.getNameForSlot(0));
     }
 
-    return isConventional(MethodSelector.getNameForSlot(PieceIndex));
+    llvm::StringRef PieceName = MethodSelector.getNameForSlot(PieceIndex);
+    return isConventional(PieceName) || hasConventionalSuffix(PieceName);
   }
 
   bool shouldBeCalledOnce(const ParmVarDecl *Parameter) const {
     return isExplicitlyMarked(Parameter) ||
            (CheckConventionalParameters &&
-            isConventional(Parameter->getName()) &&
+            (isConventional(Parameter->getName()) ||
+             hasConventionalSuffix(Parameter->getName())) &&
             isConventional(Parameter->getType()));
   }
 
   bool shouldBeCalledOnce(const DeclContext *ParamContext,
                           const ParmVarDecl *Param) {
     unsigned ParamIndex = Param->getFunctionScopeIndex();
     if (const auto *Function = dyn_cast<FunctionDecl>(ParamContext)) {
       return shouldBeCalledOnce(Function, ParamIndex);

clang/test/SemaObjC/warn-called-once.m

 
 - (void)testWithCompletion:(void (^)(void))callback {
   if ([self condition]) {
     // expected-warning@-1{{completion handler is never called when taking false branch}}
     callback();
   }
 }
 
+- (void)test:(int)cond fooWithReplyTo:(void (^)(void))handler {
+  if (cond) {
+    // expected-warning@-1{{completion handler is never called when taking false branch}}
+    handler();
+  }
+}
+
+- (void)test:(int)cond with:(void (^)(void))fooWithCompletionBlock {
+  if (cond) {
+    // expected-warning@-1{{completion handler is never called when taking false branch}}
+    fooWithCompletionBlock();
+  }
+}
+
 - (void)completion_handler_wrong_type:(int (^)(void))completionHandler {
   // We don't want to consider completion handlers with non-void return types.
   if ([self condition]) {
     // no-warning
     completionHandler();
   }
 }