[CodeComplete] Tweak completion for else.

[njames93]

Summary

If an if statement uses braces for its then block, suggest braces for the else and else if completion blocks, Otherwise don't suggest them.

[njames93]

I'm still unsure what is the best behaviour here.
Would suggesting both patterns, but sort them based on what the then branch uses be best
Example with:

if (...) {
  // Statements
}

Suggestions:

- else { // Statements }
- else if (...) { // Statements }
- else // Statement
- else if (...) // Statement

---

With:

if (...) 
  // Statement
}

Suggestions:

- else // Statement
- else if (...) // Statement
- else { // Statements }
- else if (...) { // Statements }

Or is it best keeping with the behaviour of this patch where only 1 suggestion appears based on whether braces are used or not??

[sammccall]

I like the behavior in this patch - I think multiple options is going to be distracting and crowd out other good results, and the heuristic seems pretty good.

This needs a test in e.g. llvm-project/clang/test/CodeCompletion/patterns.cpp.

clang/lib/Sema/SemaCodeComplete.cpp

5780

name could be clearer here: AddElseBodyPattern?

5789

Hmm, this is in clangd I guess, which ignores vertical space when rendering snippets.
Maybe that's a bug there, and clangd should include vertical space as space, but collapse consecutive ones?
I wonder what other consumers do.

If we do keep it (also fine) you could switch the order of the vertical and horizontal space, which suggests indentation. Up to you though.

5791

I'm not sure about this FIXME, I think it's pretty unlikely this will happen (this layer of clang is unlikely to retain enough info and know enough about style to do this, and if it's solved at another layer it's likely to be clang-format that doesn't require this hint)

5795

do you want to insert the semicolon after "statement"? (i.e. the semicolon is not part of the placeholder)
This seems likely to play better with e.g. clang-format, and should give more accurate diagnostics if the user hasn't filled in the placeholders yet.

(I don't see much precedent one way or the other, there's only "statements" in blocks... the for-loop snippet obviously inserts semicolons though)

[njames93]

Address comments and add tests.

[sammccall]

Nice, thanks for doing this!

clang/include/clang/Sema/Sema.h

QualType ProduceCtorInitMemberSignatureHelp(Scope *S, Decl *ConstructorDecl,
                                               SourceLocation OpenParLoc);
   void CodeCompleteInitializer(Scope *S, Decl *D);
   /// Trigger code completion for a record of \p BaseType. \p InitExprs are
   /// expressions in the initializer list seen so far and \p D is the current
   /// Designation being parsed.
   void CodeCompleteDesignator(const QualType BaseType,
                               llvm::ArrayRef<Expr *> InitExprs,
                               const Designation &D);
-  void CodeCompleteAfterIf(Scope *S);
+  void CodeCompleteAfterIf(Scope *S, bool IsBracedThen);
 
   void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS, bool EnteringContext,
                                bool IsUsingDeclaration, QualType BaseType,
                                QualType PreferredType);
   void CodeCompleteUsing(Scope *S);
   void CodeCompleteUsingDirective(Scope *S);
   void CodeCompleteNamespaceDecl(Scope *S);
   void CodeCompleteNamespaceAliasDecl(Scope *S);

clang/lib/Parse/ParseStmt.cpp

if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc,
                                 IsConstexpr ? Sema::ConditionKind::ConstexprIf
                                             : Sema::ConditionKind::Boolean))
     return StmtError();
 
   llvm::Optional<bool> ConstexprCondition;
   if (IsConstexpr)
     ConstexprCondition = Cond.getKnownValue();
 
+  bool IsBracedThen = Tok.is(tok::l_brace);
+
   // C99 6.8.4p3 - In C99, the body of the if statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.
   //
   // C++ 6.4p1:
   // The substatement in a selection-statement (each substatement, in the else
   // form of the if statement) implicitly defines a local scope.
   //
   // For C++ we create a scope for the condition and a new scope for
   // substatements because:
   // -When the 'then' scope exits, we want the condition declaration to still be
   //    active for the 'else' scope too.
   // -Sema will detect name clashes by considering declarations of a
   //    'ControlScope' as part of its direct subscope.
   // -If we wanted the condition and substatement to be in the same scope, we
   //    would have to notify ParseStatement not to create a new scope. It's
   //    simpler to let it create a new scope.
   //
-  ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace));
+  ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, IsBracedThen);
 
   MisleadingIndentationChecker MIChecker(*this, MSK_if, IfLoc);
 
   // Read the 'then' stmt.
   SourceLocation ThenStmtLoc = Tok.getLocation();
 
   SourceLocation InnerStatementTrailingElseLoc;
   StmtResult ThenStmt;
if (Tok.is(tok::kw_else)) {
     ElseStmt = ParseStatement();
 
     if (ElseStmt.isUsable())
       MIChecker.Check();
 
     // Pop the 'else' scope if needed.
     InnerScope.Exit();
   } else if (Tok.is(tok::code_completion)) {
-    Actions.CodeCompleteAfterIf(getCurScope());
+    Actions.CodeCompleteAfterIf(getCurScope(), IsBracedThen);
     cutOffParsing();
     return StmtError();
   } else if (InnerStatementTrailingElseLoc.isValid()) {
     Diag(InnerStatementTrailingElseLoc, diag::warn_dangling_else);
   }
 
   IfScope.Exit();
 

clang/lib/Sema/SemaCodeComplete.cpp

void Sema::CodeCompleteInitializer(Scope *S, Decl *D) {
   CodeCompleteExpressionData Data;
   Data.PreferredType = VD->getType();
   // Ignore VD to avoid completing the variable itself, e.g. in 'int foo = ^'.
   Data.IgnoreDecls.push_back(VD);
 
   CodeCompleteExpression(S, Data);
 }
 
-void Sema::CodeCompleteAfterIf(Scope *S) {
+void Sema::CodeCompleteAfterIf(Scope *S, bool IsBracedThen) {
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
                         CodeCompleter->getCodeCompletionTUInfo(),
                         mapCodeCompletionContext(*this, PCC_Statement));
   Results.setFilter(&ResultBuilder::IsOrdinaryName);
   Results.EnterNewScope();
 
   CodeCompletionDeclConsumer Consumer(Results, CurContext);
   LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
                      CodeCompleter->includeGlobals(),
                      CodeCompleter->loadExternal());
 
   AddOrdinaryNameResults(PCC_Statement, S, *this, Results);
 
   // "else" block
   CodeCompletionBuilder Builder(Results.getAllocator(),
                                 Results.getCodeCompletionTUInfo());
-  Builder.AddTypedTextChunk("else");
-  if (Results.includeCodePatterns()) {
+
+  auto AddElseBodyPattern = [&] {
+    if (IsBracedThen) {
       Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
       Builder.AddChunk(CodeCompletionString::CK_LeftBrace);
       Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
       Builder.AddPlaceholderChunk("statements");
       Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
       Builder.AddChunk(CodeCompletionString::CK_RightBrace);
+    } else {
+      Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
+      Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+      Builder.AddPlaceholderChunk("statement");
+      Builder.AddChunk(CodeCompletionString::CK_SemiColon);
     }
+  };
+  Builder.AddTypedTextChunk("else");
+  if (Results.includeCodePatterns())
+    AddElseBodyPattern();
   Results.AddResult(Builder.TakeString());
 
   // "else if" block
   Builder.AddTypedTextChunk("else if");
   Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
   Builder.AddChunk(CodeCompletionString::CK_LeftParen);
   if (getLangOpts().CPlusPlus)
     Builder.AddPlaceholderChunk("condition");
   else
     Builder.AddPlaceholderChunk("expression");
   Builder.AddChunk(CodeCompletionString::CK_RightParen);
   if (Results.includeCodePatterns()) {
-    Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
-    Builder.AddChunk(CodeCompletionString::CK_LeftBrace);
-    Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
-    Builder.AddPlaceholderChunk("statements");
-    Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
-    Builder.AddChunk(CodeCompletionString::CK_RightBrace);
+    AddElseBodyPattern();
   }
   Results.AddResult(Builder.TakeString());
 
   Results.ExitScope();
 
   if (S->getFnParent())
     AddPrettyFunctionResults(getLangOpts(), Results);
 

clang/test/CodeCompletion/patterns.cpp

 // RETURN-PTR-NOT: COMPLETION: Pattern : return;
 // RETURN-PTR: COMPLETION: Pattern : return <#expression#>;{{$}}
 // RETURN-PTR: COMPLETION: Pattern : return nullptr;
 
 // 'return nullptr' is not available before C++11.
 // RUN: %clang_cc1 -fsyntax-only -std=c++03 -code-completion-patterns -code-completion-at=%s:37:1 %s -o - | FileCheck -check-prefix=RETURN-PTR-STD03 %s
 // RUN: %clang_cc1 -fsyntax-only -std=c++03 -code-completion-patterns -code-completion-at=%s:41:1 %s -o - | FileCheck -check-prefix=RETURN-PTR-STD03 %s
 // RETURN-PTR-STD03-NOT: COMPLETION: Pattern : return nullptr;
+
+void something();
+
+void unbraced_if() {
+  if (true)
+    something();
+  // line 83
+}
+// RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:83:3 %s -o - | FileCheck -check-prefix=UNBRACED-IF %s
+// UNBRACED-IF: COMPLETION: Pattern : else
+// UNBRACED-IF-NEXT: <#statement#>;
+// UNBRACED-IF: COMPLETION: Pattern : else if (<#condition#>)
+// UNBRACED-IF-NEXT: <#statement#>;
+
+void braced_if() {
+  if (true) {
+    something();
+  }
+  // line 95
+}
+// RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:95:3 %s -o - | FileCheck -check-prefix=BRACED-IF %s
+// BRACED-IF: COMPLETION: Pattern : else {
+// BRACED-IF-NEXT: <#statements#>
+// BRACED-IF-NEXT: }
+// BRACED-IF: COMPLETION: Pattern : else if (<#condition#>) {
+// BRACED-IF-NEXT: <#statements#>
+// BRACED-IF-NEXT: }