[CodeCompletion] Add block placeholders when completing member access for Objective-C block property setters

[arphaman]

Summary

This patch depends on https://reviews.llvm.org/D25519.

This patch changes the way that Objective-C block properties are code complete: clang now suggests completion with an additional '=' and the block literal placeholder when providing member access completions for appropriate readwrite block properties. This patch uses a simple heuristic to determine if it's appropriate to suggest a setter completion for block properties: if we are completing member access that is a standalone statement, we provide setter completion. Otherwise we fallback to the default property completion.

The following example illustrates when the setter completion is triggered:

@interface Test : Obj
@property (readonly, nonatomic, copy) void (^onReadonly)(int *someParameter);
@end
@implementation Test
- foo {
  self.<COMPLETION> // will complete with ‘=‘ and block
}
- fooNot {
  // These will code complete normally:
  (self.<COMPLETION>)
  return self.<COMPLETION>
  [self foo: self.<COMPLETION>]
  if (self.<COMPLETION>) { }
}
@end

[akyrtzi]

What if the user just wants to invoke the block, this is as common or more, like:

self.onEventHandler(10)

The assign literal completion is useful but it should be an additional entry (with maybe lower priority) not replace the property completion.
BTW, it would be great if we had completion as if it was a function call, so 2 entries, 1 for block invocation and 1 for assigning literal. Absence the block invocation call it should be 1 for normal property completion and 1 for assigning literal.

[arphaman]

In D25520#568448, @akyrtzi wrote:

What if the user just wants to invoke the block, this is as common or more, like:

self.onEventHandler(10)

The assign literal completion is useful but it should be an additional entry (with maybe lower priority) not replace the property completion.
BTW, it would be great if we had completion as if it was a function call, so 2 entries, 1 for block invocation and 1 for assigning literal. Absence the block invocation call it should be 1 for normal property completion and 1 for assigning literal.

You're right, I forgot about the most common use of block properties. I will update the patch today.

It makes sense to complete block invocation as well, but that has to be done in a separate patch. I suppose that completion for block invocations should be provided for normal sub expressions as well then.

[arphaman]

The updated patch now treats the block property setter completion as a completion result that's additional to the default property completion result.

[arphaman]

Right now I gave the setter completion a flat priority bump of 3. Should something different be used? What do you think of the following possible priority heuristic: when completing blocks properties that return void the default property completion result should show up before the setter, otherwise the setter should show up before the property (we normally don't want to ignore the result of the call, right)?

[akyrtzi]

What do you think of the following possible priority heuristic

SGTM.

Changes LGTM. I'd also recommend that as a follow-up patch it would be great to extend the setter completion to variables as well (global variables, fields, ivars, etc.)

[akyrtzi]

Another recommendation for follow-up. When invoking completion on the right-hand side of the assignment it should provide a block literal completion with high priority. For example, when completing like this:

self.foo = <COMPLETE>

[arphaman]

In D25520#569389, @akyrtzi wrote:

What do you think of the following possible priority heuristic

SGTM.

Changes LGTM. I'd also recommend that as a follow-up patch it would be great to extend the setter completion to variables as well (global variables, fields, ivars, etc.)

Thanks, I will commit this as it is. I will post a follow-up patch that uses the priority heuristic that I described above, and will work on related patches that you mentioned as well.

cfe/trunk/include/clang/Parse/Parser.h

class Parser : public CodeCompletionHandler {
   ///
   /// This is necessary because Sema thinks we have left the container
   /// to parse the C-like decls, meaning Actions.getObjCDeclContext() will
   /// be NULL.
   bool ParsingInObjCContainer;
 
   bool SkipFunctionBodies;
 
+  /// The location of the expression statement that is being parsed right now.
+  /// Used to determine if an expression that is being parsed is a statement or
+  /// just a regular sub-expression.
+  SourceLocation ExprStatementTokLoc;
+
 public:
   Parser(Preprocessor &PP, Sema &Actions, bool SkipFunctionBodies);
   ~Parser() override;
 
   const LangOptions &getLangOpts() const { return PP.getLangOpts(); }
   const TargetInfo &getTargetInfo() const { return PP.getTargetInfo(); }
   Preprocessor &getPreprocessor() const { return PP; }
   Sema &getActions() const { return Actions; }

cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h

enum {
   /// "BOOL" is preferred.
   CCD_bool_in_ObjC = 1,
 
   /// \brief Adjustment for KVC code pattern priorities when it doesn't look
   /// like the
   CCD_ProbablyNotObjCCollection = 15,
 
   /// \brief An Objective-C method being used as a property.
-  CCD_MethodAsProperty = 2
+  CCD_MethodAsProperty = 2,
+
+  /// \brief An Objective-C block property completed as a setter with a
+  /// block placeholder.
+  CCD_BlockPropertySetter = 3
 };
 
 /// \brief Priority value factors by which we will divide or multiply the
 /// priority of a code-completion result.
 enum {
   /// \brief Divide by this factor when a code-completion result's type exactly
   /// matches the type we expect.
   CCF_ExactTypeMatch = 4,

cfe/trunk/include/clang/Sema/Sema.h

public:
   void CodeCompleteDeclSpec(Scope *S, DeclSpec &DS,
                             bool AllowNonIdentifiers,
                             bool AllowNestedNameSpecifiers);
 
   struct CodeCompleteExpressionData;
   void CodeCompleteExpression(Scope *S,
                               const CodeCompleteExpressionData &Data);
   void CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
-                                       SourceLocation OpLoc,
-                                       bool IsArrow);
+                                       SourceLocation OpLoc, bool IsArrow,
+                                       bool IsBaseExprStatement);
   void CodeCompletePostfixExpression(Scope *S, ExprResult LHS);
   void CodeCompleteTag(Scope *S, unsigned TagSpec);
   void CodeCompleteTypeQualifiers(DeclSpec &DS);
   void CodeCompleteBracketDeclarator(Scope *S);
   void CodeCompleteCase(Scope *S);
   void CodeCompleteCall(Scope *S, Expr *Fn, ArrayRef<Expr *> Args);
   void CodeCompleteConstructor(Scope *S, QualType Type, SourceLocation Loc,
                                ArrayRef<Expr *> Args);

cfe/trunk/lib/Parse/ParseExpr.cpp

case tok::period: {
                                        /*EnteringContext=*/false,
                                        &MayBePseudoDestructor);
         if (SS.isNotEmpty())
           ObjectType = nullptr;
       }
 
       if (Tok.is(tok::code_completion)) {
         // Code completion for a member access expression.
-        Actions.CodeCompleteMemberReferenceExpr(getCurScope(), LHS.get(),
-                                                OpLoc, OpKind == tok::arrow);
-        
+        Actions.CodeCompleteMemberReferenceExpr(
+            getCurScope(), LHS.get(), OpLoc, OpKind == tok::arrow,
+            ExprStatementTokLoc == LHS.get()->getLocStart());
         cutOffParsing();
         return ExprError();
       }
 
       if (MayBePseudoDestructor && !LHS.isInvalid()) {
         LHS = ParseCXXPseudoDestructor(LHS.get(), OpLoc, OpKind, SS, 
                                        ObjectType);
         break;

cfe/trunk/lib/Parse/ParseStmt.cpp

Retry:
   return Res;
 }
 
 /// \brief Parse an expression statement.
 StmtResult Parser::ParseExprStatement() {
   // If a case keyword is missing, this is where it should be inserted.
   Token OldToken = Tok;
 
+  ExprStatementTokLoc = Tok.getLocation();
+
   // expression[opt] ';'
   ExprResult Expr(ParseExpression());
   if (Expr.isInvalid()) {
     // If the expression is invalid, skip ahead to the next semicolon or '}'.
     // Not doing this opens us up to the possibility of infinite loops if
     // ParseExpression does not consume any tokens.
     SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
     if (Tok.is(tok::semi))

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

while (true) {
     }
     break;
   }
 }
 
 static std::string
 formatBlockPlaceholder(const PrintingPolicy &Policy, const NamedDecl *BlockDecl,
                        FunctionTypeLoc &Block, FunctionProtoTypeLoc &BlockProto,
+                       bool SuppressBlockName = false,
                        bool SuppressBlock = false,
                        Optional<ArrayRef<QualType>> ObjCSubsts = None);
 
 static std::string FormatFunctionParameter(const PrintingPolicy &Policy,
                                            const ParmVarDecl *Param,
                                            bool SuppressName = false,
                                            bool SuppressBlock = false,
                                Optional<ArrayRef<QualType>> ObjCSubsts = None) {
if (ObjCMethodParam) {
       Type.getAsStringInternal(Result, Policy);
     }
       
     return Result;
   }
     
   // We have the function prototype behind the block pointer type, as it was
   // written in the source.
-  return formatBlockPlaceholder(Policy, Param, Block, BlockProto, SuppressBlock,
+  return formatBlockPlaceholder(Policy, Param, Block, BlockProto,
+                                /*SuppressBlockName=*/false, SuppressBlock,
                                 ObjCSubsts);
 }
 
 /// \brief Returns a placeholder string that corresponds to an Objective-C block
 /// declaration.
 ///
 /// \param BlockDecl A declaration with an Objective-C block type.
 ///
 /// \param Block The most relevant type location for that block type.
 ///
 /// \param SuppressBlockName Determines wether or not the name of the block
 /// declaration is included in the resulting string.
 static std::string
 formatBlockPlaceholder(const PrintingPolicy &Policy, const NamedDecl *BlockDecl,
                        FunctionTypeLoc &Block, FunctionProtoTypeLoc &BlockProto,
-                       bool SuppressBlock,
+                       bool SuppressBlockName, bool SuppressBlock,
                        Optional<ArrayRef<QualType>> ObjCSubsts) {
   std::string Result;
   QualType ResultType = Block.getTypePtr()->getReturnType();
   if (ObjCSubsts)
     ResultType =
         ResultType.substObjCTypeArgs(BlockDecl->getASTContext(), *ObjCSubsts,
                                      ObjCSubstitutionContext::Result);
   if (!ResultType->isVoidType() || SuppressBlock)
for (unsigned I = 0, N = Block.getNumParams(); I != N; ++I) {
         Params += ", ...";
     }
     Params += ")";
   }
 
   if (SuppressBlock) {
     // Format as a parameter.
     Result = Result + " (^";
-    if (BlockDecl->getIdentifier())
+    if (!SuppressBlockName && BlockDecl->getIdentifier())
       Result += BlockDecl->getIdentifier()->getName();
     Result += ")";
     Result += Params;
   } else {
     // Format as a block literal argument.
     Result = '^' + Result;
     Result += Params;
 
-    if (BlockDecl->getIdentifier())
+    if (!SuppressBlockName && BlockDecl->getIdentifier())
       Result += BlockDecl->getIdentifier()->getName();
   }
 
   return Result;
 }
 
 /// \brief Add function parameter chunks to the given code completion string.
 static void AddFunctionParameterChunks(Preprocessor &PP,
if (ObjCProtocolDecl *Protocol = dyn_cast<ObjCProtocolDecl>(Container)) {
     
     return Protocol;
   }
   return Container;
 }
 
 static void AddObjCProperties(const CodeCompletionContext &CCContext,
                               ObjCContainerDecl *Container,
-                              bool AllowCategories,
-                              bool AllowNullaryMethods,
+                              bool AllowCategories, bool AllowNullaryMethods,
                               DeclContext *CurContext,
                               AddedPropertiesSet &AddedProperties,
-                              ResultBuilder &Results) {
+                              ResultBuilder &Results,
+                              bool IsBaseExprStatement = false) {
   typedef CodeCompletionResult Result;
 
   // Retrieve the definition.
   Container = getContainerDef(Container);
   
   // Add properties in this container.
-  for (const auto *P : Container->instance_properties())
-    if (AddedProperties.insert(P->getIdentifier()).second)
-      Results.MaybeAddResult(Result(P, Results.getBasePriority(P), nullptr),
-                             CurContext);
+  for (const auto *P : Container->instance_properties()) {
+    if (!AddedProperties.insert(P->getIdentifier()).second)
+      continue;
+
 
+    // Provide additional block setter completion iff the base expression is a
+    // statement.
+    if (!P->isReadOnly() && IsBaseExprStatement &&
+        P->getType().getTypePtr()->isBlockPointerType()) {
+      FunctionTypeLoc BlockLoc;
+      FunctionProtoTypeLoc BlockProtoLoc;
+      findTypeLocationForBlockDecl(P->getTypeSourceInfo(), BlockLoc,
+                                   BlockProtoLoc);
+
+      // Provide block setter completion only when we are able to find
+      // the FunctionProtoTypeLoc with parameter names for the block.
+      if (BlockLoc) {
+        CodeCompletionBuilder Builder(Results.getAllocator(),
+                                      Results.getCodeCompletionTUInfo());
+        AddResultTypeChunk(Container->getASTContext(),
+                           getCompletionPrintingPolicy(Results.getSema()), P,
+                           CCContext.getBaseType(), Builder);
+        Builder.AddTypedTextChunk(
+            Results.getAllocator().CopyString(P->getName()));
+        Builder.AddChunk(CodeCompletionString::CK_Equal);
+
+        std::string PlaceholderStr = formatBlockPlaceholder(
+            getCompletionPrintingPolicy(Results.getSema()), P, BlockLoc,
+            BlockProtoLoc, /*SuppressBlockName=*/true);
+        // Add the placeholder string.
+        Builder.AddPlaceholderChunk(
+            Builder.getAllocator().CopyString(PlaceholderStr));
+
+        Results.MaybeAddResult(
+            Result(Builder.TakeString(), P,
+                   Results.getBasePriority(P) + CCD_BlockPropertySetter),
+            CurContext);
+      }
+    }
+  }
+
   // Add nullary methods
   if (AllowNullaryMethods) {
     ASTContext &Context = Container->getASTContext();
     PrintingPolicy Policy = getCompletionPrintingPolicy(Results.getSema());
     for (auto *M : Container->methods()) {
       if (M->getSelector().isUnarySelector())
         if (IdentifierInfo *Name = M->getSelector().getIdentifierInfoForSlot(0))
           if (AddedProperties.insert(Name).second) {
if (AllowNullaryMethods) {
     }
   }
     
   
   // Add properties in referenced protocols.
   if (ObjCProtocolDecl *Protocol = dyn_cast<ObjCProtocolDecl>(Container)) {
     for (auto *P : Protocol->protocols())
       AddObjCProperties(CCContext, P, AllowCategories, AllowNullaryMethods,
-                        CurContext, AddedProperties, Results);
+                        CurContext, AddedProperties, Results,
+                        IsBaseExprStatement);
   } else if (ObjCInterfaceDecl *IFace = dyn_cast<ObjCInterfaceDecl>(Container)){
     if (AllowCategories) {
       // Look through categories.
       for (auto *Cat : IFace->known_categories())
         AddObjCProperties(CCContext, Cat, AllowCategories, AllowNullaryMethods,
-                          CurContext, AddedProperties, Results);
+                          CurContext, AddedProperties, Results,
+                          IsBaseExprStatement);
     }
 
     // Look through protocols.
     for (auto *I : IFace->all_referenced_protocols())
       AddObjCProperties(CCContext, I, AllowCategories, AllowNullaryMethods,
-                        CurContext, AddedProperties, Results);
-    
+                        CurContext, AddedProperties, Results,
+                        IsBaseExprStatement);
     // Look in the superclass.
     if (IFace->getSuperClass())
       AddObjCProperties(CCContext, IFace->getSuperClass(), AllowCategories,
-                        AllowNullaryMethods, CurContext, 
-                        AddedProperties, Results);
+                        AllowNullaryMethods, CurContext, AddedProperties,
+                        Results, IsBaseExprStatement);
   } else if (const ObjCCategoryDecl *Category
                                     = dyn_cast<ObjCCategoryDecl>(Container)) {
     // Look through protocols.
     for (auto *P : Category->protocols())
       AddObjCProperties(CCContext, P, AllowCategories, AllowNullaryMethods,
-                        CurContext, AddedProperties, Results);
+                        CurContext, AddedProperties, Results,
+                        IsBaseExprStatement);
   }
 }
 
 void Sema::CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
-                                           SourceLocation OpLoc,
-                                           bool IsArrow) {
+                                           SourceLocation OpLoc, bool IsArrow,
+                                           bool IsBaseExprStatement) {
   if (!Base || !CodeCompleter)
     return;
   
   ExprResult ConvertedBase = PerformMemberExprBaseConversion(Base, IsArrow);
   if (ConvertedBase.isInvalid())
     return;
   Base = ConvertedBase.get();
 
if (const RecordType *Record = BaseType->getAs<RecordType>()) {
     AddedPropertiesSet AddedProperties;
 
     if (const ObjCObjectPointerType *ObjCPtr =
             BaseType->getAsObjCInterfacePointerType()) {
       // Add property results based on our interface.
       assert(ObjCPtr && "Non-NULL pointer guaranteed above!");
       AddObjCProperties(CCContext, ObjCPtr->getInterfaceDecl(), true,
                         /*AllowNullaryMethods=*/true, CurContext,
-                        AddedProperties, Results);
+                        AddedProperties, Results, IsBaseExprStatement);
     }
 
     // Add properties from the protocols in a qualified interface.
     for (auto *I : BaseType->getAs<ObjCObjectPointerType>()->quals())
       AddObjCProperties(CCContext, I, true, /*AllowNullaryMethods=*/true,
-                        CurContext, AddedProperties, Results);
+                        CurContext, AddedProperties, Results,
+                        IsBaseExprStatement);
   } else if ((IsArrow && BaseType->isObjCObjectPointerType()) ||
              (!IsArrow && BaseType->isObjCObjectType())) {
     // Objective-C instance variable access.
     ObjCInterfaceDecl *Class = nullptr;
     if (const ObjCObjectPointerType *ObjCPtr
                                     = BaseType->getAs<ObjCObjectPointerType>())
       Class = ObjCPtr->getInterfaceDecl();
     else

cfe/trunk/test/Index/complete-block-property-assignment.m

+// Note: the run lines follow their respective tests, since line/column
+// matter in this test.
+
+// rdar://28481726
+
+void func(int x);
+typedef int Foo;
+typedef void (^FooBlock)(Foo *someParameter);
+
+@interface Obj
+@property (readwrite, nonatomic, copy) void (^onAction)(Obj *object);
+@property (readwrite, nonatomic) int foo;
+@end
+
+@interface Test : Obj
+@property (readwrite, nonatomic, copy) FooBlock onEventHandler;
+@property (readonly, nonatomic, copy) void (^onReadonly)(int *someParameter);
+@property (readonly, nonatomic, strong) Obj *obj;
+@end
+
+@implementation Test
+
+#define SELFY self
+
+- (void)test {
+  self.foo = 2;
+  [self takeInt: 2]; self.foo = 2;
+  /* Comment */ self.foo = 2;
+  SELFY.foo = 2
+}
+
+// RUN: c-index-test -code-completion-at=%s:26:8 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// RUN: c-index-test -code-completion-at=%s:27:27 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// RUN: c-index-test -code-completion-at=%s:28:22 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// RUN: c-index-test -code-completion-at=%s:29:9 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// CHECK-CC1: ObjCPropertyDecl:{ResultType int}{TypedText foo} (35)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType Obj *}{TypedText obj} (35)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType void (^)(Obj *)}{TypedText onAction} (35)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType void (^)(Obj *)}{TypedText onAction}{Equal  = }{Placeholder ^(Obj *object)} (38)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType FooBlock}{TypedText onEventHandler} (35)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType FooBlock}{TypedText onEventHandler}{Equal  = }{Placeholder ^(Foo *someParameter)} (38)
+// CHECK-CC1-NEXT: ObjCPropertyDecl:{ResultType void (^)(int *)}{TypedText onReadonly} (35)
+
+- (void) takeInt:(int)x { }
+
+- (int) testFailures {
+  (self.foo);
+  int x = self.foo;
+  [self takeInt: self.foo];
+  if (self.foo) {
+    func(self.foo);
+  }
+  return self.foo;
+}
+
+// RUN: c-index-test -code-completion-at=%s:47:9 %s | FileCheck -check-prefix=CHECK-NO %s
+// RUN: c-index-test -code-completion-at=%s:48:16 %s | FileCheck -check-prefix=CHECK-NO %s
+// RUN: c-index-test -code-completion-at=%s:49:23 %s | FileCheck -check-prefix=CHECK-NO %s
+// RUN: c-index-test -code-completion-at=%s:50:12 %s | FileCheck -check-prefix=CHECK-NO %s
+// RUN: c-index-test -code-completion-at=%s:51:15 %s | FileCheck -check-prefix=CHECK-NO %s
+// RUN: c-index-test -code-completion-at=%s:53:15 %s | FileCheck -check-prefix=CHECK-NO %s
+// CHECK-NO: ObjCPropertyDecl:{ResultType int}{TypedText foo} (35)
+// CHECK-NO-NEXT: ObjCPropertyDecl:{ResultType Obj *}{TypedText obj} (35)
+// CHECK-NO-NEXT: ObjCPropertyDecl:{ResultType void (^)(Obj *)}{TypedText onAction} (35)
+// CHECK-NO-NEXT: ObjCPropertyDecl:{ResultType FooBlock}{TypedText onEventHandler} (35)
+// CHECK-NO-NEXT: ObjCPropertyDecl:{ResultType void (^)(int *)}{TypedText onReadonly} (35)
+
+@end