[CodeComplete] Allow completing enum values within case statements, and insert 'case' as a fixit.

[sammccall]

Summary

There are a few alternative ways to expose this.
The most obvious is a RK_Pattern, but it seems to have some triggering problems.
Consider a completion "case Color::Red"

• we want the user input to match Red
• we *don't* want the user input to match Color::
• if "case" is displayed, users will expect to be able to match it.

It's not possible to satisfy all of these:

• [TypedText:case] [Text:Color::][TypedText:Red] has a bad getTypedText()
• [Text:case] [Text:Color::][TypedText:Red] shows "case" but you can't type it
• [TypedText:case Color::TypedText:Red] will match against the qualifier

(I suspect the third is the least bad here)

The option explored here is to provide a regular Declaration completion, but
with "case" inserted as a fix-it. This results in "case" being neither displayed
or matched, but inserted. (If the user types "case", the existing completions
kick in.)

Curious what you think. I'm biased as I'm working on an integration that
supports fixits but not patterns :-)

[ilya-biryukov]

Not displaying 'case' is actually confusing to my taste.
WDYT about allowing multiple "typed text" chunks or allowing to mark other chunks that are supposed be part of filter text?

[sammccall]

In D63098#1537601, @ilya-biryukov wrote:

Not displaying 'case' is actually confusing to my taste.
WDYT about allowing multiple "typed text" chunks or allowing to mark other chunks that are supposed be part of filter text?

Multiple typed-text chunks is not well supported - there's various APIs that assume there's only one/the first one is the "main" one.

[ilya-biryukov]

Notes from the offline discussion:

• Sam: supporting multiple typed text chunks is hard, many clients that expect only one. Objective-C already does that, though, but in a way that does not break most targets.
• Sam: supporting any other form of custom text for filtering is also hard, particularly in clangd that exposes a structured completion item with qualifiers, etc.
• me: feel strongly that we should show case at the start and match it.
• me: also feel strongly that the feature is useful. Having a single big "typed text" chunk that includes the qualifier and 'case' looks better than not having the feature.

Let me know if I missed or misinterpreted something.

[sammccall]

I'll give this a try. I expect it to feel quite glitchy when the enum type has a long name.

lib/Sema/SemaCodeComplete.cpp

 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/QualTypeNames.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Scope.h"
for (auto *Method : BR->methods()) {
             S.getPreprocessor(), S.getASTContext(), Builder,
             /*IncludeBriefComments=*/false, CCContext, Policy);
         Results.AddResult(CodeCompletionResult(CCS, Method, CCP_CodePattern));
       }
     }
   }
 }
 
+namespace {
+/// Information that allows to avoid completing redundant enumerators.
+struct CoveredEnumerators {
+  llvm::SmallPtrSet<EnumConstantDecl *, 8> Seen;
+  NestedNameSpecifier *SuggestedQualifier = nullptr;
+
+  // Determine which enumerators we have already seen in the switch statement.
+  // FIXME: Ideally, we would also be able to look *past* the code-completion
+  // token, in case we are code-completing in the middle of the switch and not
+  // at the end. However, we aren't able to do so at the moment.
+  static CoveredEnumerators gather(SwitchStmt *Switch) {
+    CoveredEnumerators Enumerators;
+    for (SwitchCase *SC = Switch->getSwitchCaseList(); SC;
+         SC = SC->getNextSwitchCase()) {
+      CaseStmt *Case = dyn_cast<CaseStmt>(SC);
+      if (!Case)
+        continue;
+
+      Expr *CaseVal = Case->getLHS()->IgnoreParenCasts();
+      if (auto *DRE = dyn_cast<DeclRefExpr>(CaseVal))
+        if (auto *Enumerator = dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
+          // We look into the AST of the case statement to determine which
+          // enumerator was named. Alternatively, we could compute the value of
+          // the integral constant expression, then compare it against the
+          // values of each enumerator. However, value-based approach would not
+          // work as well with C++ templates where enumerators declared within a
+          // template are type- and value-dependent.
+          Enumerators.Seen.insert(Enumerator);
+
+          // If this is a qualified-id, keep track of the nested-name-specifier
+          // so that we can reproduce it as part of code completion, e.g.,
+          //
+          //   switch (TagD.getKind()) {
+          //     case TagDecl::TK_enum:
+          //       break;
+          //     case XXX
+          //
+          // At the XXX, our completions are TagDecl::TK_union,
+          // TagDecl::TK_struct, and TagDecl::TK_class, rather than TK_union,
+          // TK_struct, and TK_class.
+          Enumerators.SuggestedQualifier = DRE->getQualifier();
+        }
+    }
+    return Enumerators;
+  }
+};
+} // namespace
+
+static void AddEnumerators(ResultBuilder &Results, ASTContext &Context,
+                           EnumDecl *Enum, DeclContext *CurContext,
+                           const CoveredEnumerators &Enumerators,
+                           llvm::Optional<FixItHint> FixIt = llvm::None) {
+  NestedNameSpecifier *Qualifier = Enumerators.SuggestedQualifier;
+  if (Context.getLangOpts().CPlusPlus && !Qualifier && Enumerators.Seen.empty()) {
+    // If there are no prior enumerators in C++, check whether we have to
+    // qualify the names of the enumerators that we suggest, because they
+    // may not be visible in this scope.
+    Qualifier = getRequiredQualification(Context, CurContext, Enum);
+  }
+
+  CodeCompletionBuilder Builder(Results.getAllocator(),
+                                Results.getCodeCompletionTUInfo());
+  Results.EnterNewScope();
+  for (auto *E : Enum->enumerators()) {
+    if (Enumerators.Seen.count(E))
+      continue;
+
+    CodeCompletionResult R(E, CCP_EnumInCase, Qualifier);
+    if (FixIt)
+      R.FixIts.push_back(*FixIt);
+    Results.AddResult(R, CurContext, nullptr, false);
+  }
+  Results.ExitScope();
+}
+
 /// Add language constructs that show up for "ordinary" names.
 static void AddOrdinaryNameResults(Sema::ParserCompletionContext CCC, Scope *S,
                                    Sema &SemaRef, ResultBuilder &Results) {
   CodeCompletionAllocator &Allocator = Results.getAllocator();
   CodeCompletionBuilder Builder(Allocator, Results.getCodeCompletionTUInfo());
 
   typedef CodeCompletionResult Result;
   switch (CCC) {
if (Results.includeCodePatterns()) {
       Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
       Builder.AddChunk(CodeCompletionString::CK_RightBrace);
       Results.AddResult(Result(Builder.TakeString()));
     }
 
     // Switch-specific statements.
     if (SemaRef.getCurFunction() &&
         !SemaRef.getCurFunction()->SwitchStack.empty()) {
+      EnumDecl *EnumDef = nullptr;
+      SwitchStmt *Switch =
+          SemaRef.getCurFunction()->SwitchStack.back().getPointer();
+      if (Switch->getCond()) {
+        QualType SwitchType = Switch->getCond()->IgnoreImplicit()->getType();
+        if (SwitchType->isEnumeralType())
+          EnumDef = SwitchType->castAs<EnumType>()->getDecl()->getDefinition();
+      }
+      if (EnumDef && SemaRef.CodeCompleter->includeFixIts()) {
+        // case <value>
+        AddEnumerators(
+            Results, SemaRef.Context, EnumDef, SemaRef.CurContext,
+            CoveredEnumerators::gather(Switch),
+            FixItHint::CreateInsertion(SemaRef.getPreprocessor()
+                                           .getCodeCompletionTokenRange()
+                                           .getBegin(),
+                                       "case "));
+      }
       // case expression:
       Builder.AddTypedTextChunk("case");
       Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace);
       Builder.AddPlaceholderChunk("expression");
       Builder.AddChunk(CodeCompletionString::CK_Colon);
       Results.AddResult(Result(Builder.TakeString()));
 
       // default:
struct Sema::CodeCompleteExpressionData {
 
   QualType PreferredType;
   bool IntegralConstantExpression;
   bool ObjCCollection;
   bool IsParenthesized;
   SmallVector<Decl *, 4> IgnoreDecls;
 };
 
-namespace {
-/// Information that allows to avoid completing redundant enumerators.
-struct CoveredEnumerators {
-  llvm::SmallPtrSet<EnumConstantDecl *, 8> Seen;
-  NestedNameSpecifier *SuggestedQualifier = nullptr;
-};
-} // namespace
-
-static void AddEnumerators(ResultBuilder &Results, ASTContext &Context,
-                           EnumDecl *Enum, DeclContext *CurContext,
-                           const CoveredEnumerators &Enumerators) {
-  NestedNameSpecifier *Qualifier = Enumerators.SuggestedQualifier;
-  if (Context.getLangOpts().CPlusPlus && !Qualifier && Enumerators.Seen.empty()) {
-    // If there are no prior enumerators in C++, check whether we have to
-    // qualify the names of the enumerators that we suggest, because they
-    // may not be visible in this scope.
-    Qualifier = getRequiredQualification(Context, CurContext, Enum);
-  }
-
-  Results.EnterNewScope();
-  for (auto *E : Enum->enumerators()) {
-    if (Enumerators.Seen.count(E))
-      continue;
-
-    CodeCompletionResult R(E, CCP_EnumInCase, Qualifier);
-    Results.AddResult(R, CurContext, nullptr, false);
-  }
-  Results.ExitScope();
-}
-
 /// Try to find a corresponding FunctionProtoType for function-like types (e.g.
 /// function pointers, std::function, etc).
 static const FunctionProtoType *TryDeconstructFunctionLike(QualType T) {
   assert(!T.isNull());
   // Try to extract first template argument from std::function<> and similar.
   // Note we only handle the sugared types, they closely match what users wrote.
   // We explicitly choose to not handle ClassTemplateSpecializationDecl.
   if (auto *Specialization = T->getAs<TemplateSpecializationType>()) {
void Sema::CodeCompleteCase(Scope *S) {
   }
 
   // Code-complete the cases of a switch statement over an enumeration type
   // by providing the list of
   EnumDecl *Enum = type->castAs<EnumType>()->getDecl();
   if (EnumDecl *Def = Enum->getDefinition())
     Enum = Def;
 
-  // Determine which enumerators we have already seen in the switch statement.
-  // FIXME: Ideally, we would also be able to look *past* the code-completion
-  // token, in case we are code-completing in the middle of the switch and not
-  // at the end. However, we aren't able to do so at the moment.
-  CoveredEnumerators Enumerators;
-  for (SwitchCase *SC = Switch->getSwitchCaseList(); SC;
-       SC = SC->getNextSwitchCase()) {
-    CaseStmt *Case = dyn_cast<CaseStmt>(SC);
-    if (!Case)
-      continue;
-
-    Expr *CaseVal = Case->getLHS()->IgnoreParenCasts();
-    if (auto *DRE = dyn_cast<DeclRefExpr>(CaseVal))
-      if (auto *Enumerator =
-              dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
-        // We look into the AST of the case statement to determine which
-        // enumerator was named. Alternatively, we could compute the value of
-        // the integral constant expression, then compare it against the
-        // values of each enumerator. However, value-based approach would not
-        // work as well with C++ templates where enumerators declared within a
-        // template are type- and value-dependent.
-        Enumerators.Seen.insert(Enumerator);
-
-        // If this is a qualified-id, keep track of the nested-name-specifier
-        // so that we can reproduce it as part of code completion, e.g.,
-        //
-        //   switch (TagD.getKind()) {
-        //     case TagDecl::TK_enum:
-        //       break;
-        //     case XXX
-        //
-        // At the XXX, our completions are TagDecl::TK_union,
-        // TagDecl::TK_struct, and TagDecl::TK_class, rather than TK_union,
-        // TK_struct, and TK_class.
-        Enumerators.SuggestedQualifier = DRE->getQualifier();
-      }
-  }
-
   // Add any enumerators that have not yet been mentioned.
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
                         CodeCompleter->getCodeCompletionTUInfo(),
                         CodeCompletionContext::CCC_Expression);
-  AddEnumerators(Results, Context, Enum, CurContext, Enumerators);
+  AddEnumerators(Results, Context, Enum, CurContext,
+                 CoveredEnumerators::gather(Switch));
 
   if (CodeCompleter->includeMacros()) {
     AddMacroResults(PP, Results, CodeCompleter->loadExternal(), false);
   }
   HandleCodeCompleteResults(this, CodeCompleter, Results.getCompletionContext(),
                             Results.data(), Results.size());
 }
 

test/CodeCompletion/enum-switch-case.cpp

 void test(enum N::Color color) {
   switch (color) {
   case N::Red:
     break;
     
   case N::Yellow:
     break;
     
-  case 
+  case N::Green:
     // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:8 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
     // CHECK-CC1: Blue : [#N::Color#]N::Blue
     // CHECK-CC1-NEXT: Green : [#N::Color#]N::Green
     // CHECK-CC1-NEXT: Indigo : [#N::Color#]N::Indigo
     // CHECK-CC1-NEXT: Orange : [#N::Color#]N::Orange
     // CHECK-CC1-NEXT: Violet : [#N::Color#]N::Violet
-    
+  V
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:28:4 %s -o - | FileCheck -check-prefix=CHECK-UNQUAL %s
+    // CHECK-UNQUAL: Violet : [#N::Color#]N::Violet (requires fix-it: {28:3-28:3} to "case ")