[clangd] Add a tweak for filling in enumerators of a switch statement.

[tdeo]

Summary

Add a tweak that populates an empty switch statement of an enumeration type with all of the enumerators of that type.

Before:

enum Color { RED, GREEN, BLUE };
void f(Color color) {
  switch (color) {}
}

After:

enum Color { RED, GREEN, BLUE };
void f(Color color) {
  switch (color) {
  case RED:
  case GREEN:
  case BLUE:
    break;
  }
}

[sammccall]

This is really nice!

We'd actually debated how to add such a feature, but had assumed it should be a "fix" rather than a "tweak" - I think this way is much better.

Obviously making it fill in the remaining enumerators in the general case would be great to have later. It looks like making prepare() make a cheap decision is the tricky part, but seems doable:

• bail out if there's a default or a gnu ... case
• bail out if there are at least as many cases as enumerators
• bail out if any case expression is value-dependent
• otherwise we should be able to find some cases to insert

(We're subject to AST error-recovery weirdness - e.g. CaseStmt with non-constants seem to just be deleted - but probably fine to ignore all these).

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp

97

while it's uncommon, there can be stuff other than cases in a switch stmt.
Maybe we simply want to bail out if the body is nonempty?

105

can we add a defensive nullcheck to getCond()?

code like switch() {} is the sort of thing that is often represented in AST as a null pointer.
AFAICT that doesn't happen today (we get no AST nodes at all instead) but I can see that being changed in future to improve recovery.

125

in the case where there's already stuff in the body (just not cases), it seems like RBracLoc-1 might be safer?

[tdeo]

• Check body is empty instead of having no case statements.
• Null check the return value from SwitchStmt::getCond.
• Insert replacement before the right bracket instead of after the left one, for future improvements.

[tdeo]

@sammccall, I don't have commit access, so if this revision has no issues, can you land it? Thanks.

[sammccall]

In D88383#2297735, @tdeo wrote:

@sammccall, I don't have commit access, so if this revision has no issues, can you land it? Thanks.

Absolutely, thanks a lot for the patch!

Any interest in extending to non-empty switches? Or better for someone else to pick that up?

[tdeo]

In D88383#2297778, @sammccall wrote:

In D88383#2297735, @tdeo wrote:

@sammccall, I don't have commit access, so if this revision has no issues, can you land it? Thanks.

Absolutely, thanks a lot for the patch!

Any interest in extending to non-empty switches? Or better for someone else to pick that up?

I'd like to give it a shot soon, but I wouldn't mind if someone else did it first.

clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt

add_clang_library(clangDaemonTweaks OBJECT
   DumpAST.cpp
   DefineInline.cpp
   DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp
   ExtractVariable.cpp
   ObjCLocalizeStringLiteral.cpp
+  PopulateSwitch.cpp
   RawStringLiteral.cpp
   RemoveUsingNamespace.cpp
   SwapIfBranches.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangDaemon

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp

+//===--- PopulateSwitch.cpp --------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Tweak that populates an empty switch statement of an enumeration type with
+// all of the enumerators of that type.
+//
+// Before:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+//     switch (color) {}
+//   }
+//
+// After:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+//     switch (color) {
+//     case RED:
+//     case GREEN:
+//     case BLUE:
+//       break;
+//     }
+//   }
+//
+//===----------------------------------------------------------------------===//
+
+#include "AST.h"
+#include "Selection.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include <string>
+
+namespace clang {
+namespace clangd {
+namespace {
+class PopulateSwitch : public Tweak {
+  const char *id() const override;
+  bool prepare(const Selection &Sel) override;
+  Expected<Effect> apply(const Selection &Sel) override;
+  std::string title() const override { return "Populate switch"; }
+  Intent intent() const override { return Refactor; }
+
+private:
+  ASTContext *ASTCtx = nullptr;
+  const DeclContext *DeclCtx = nullptr;
+  const SwitchStmt *Switch = nullptr;
+  const CompoundStmt *Body = nullptr;
+  const EnumDecl *EnumD = nullptr;
+};
+
+REGISTER_TWEAK(PopulateSwitch)
+
+bool PopulateSwitch::prepare(const Selection &Sel) {
+  ASTCtx = &Sel.AST->getASTContext();
+
+  const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
+  if (!CA)
+    return false;
+
+  const Stmt *CAStmt = CA->ASTNode.get<Stmt>();
+  if (!CAStmt)
+    return false;
+
+  // Go up a level if we see a compound statement.
+  // switch (value) {}
+  //                ^^
+  if (isa<CompoundStmt>(CAStmt)) {
+    CA = CA->Parent;
+    if (!CA)
+      return false;
+
+    CAStmt = CA->ASTNode.get<Stmt>();
+    if (!CAStmt)
+      return false;
+  }
+
+  DeclCtx = &CA->getDeclContext();
+  Switch = dyn_cast<SwitchStmt>(CAStmt);
+  if (!Switch)
+    return false;
+
+  Body = dyn_cast<CompoundStmt>(Switch->getBody());
+  if (!Body)
+    return false;
+
+  // Since we currently always insert all enumerators, don't suggest this tweak
+  // if the body is not empty.
+  if (!Body->body_empty())
+    return false;
+
+  const Expr *Cond = Switch->getCond();
+  if (!Cond)
+    return false;
+
+  // Ignore implicit casts, since enums implicitly cast to integer types.
+  Cond = Cond->IgnoreParenImpCasts();
+
+  const EnumType *EnumT = Cond->getType()->getAsAdjusted<EnumType>();
+  if (!EnumT)
+    return false;
+
+  EnumD = EnumT->getDecl();
+  if (!EnumD)
+    return false;
+
+  // If there aren't any enumerators, there's nothing to insert.
+  if (EnumD->enumerator_begin() == EnumD->enumerator_end())
+    return false;
+
+  return true;
+}
+
+Expected<Tweak::Effect> PopulateSwitch::apply(const Selection &Sel) {
+  const SourceManager &SM = ASTCtx->getSourceManager();
+  SourceLocation Loc = Body->getRBracLoc();
+
+  std::string Text;
+  for (EnumConstantDecl *Enumerator : EnumD->enumerators()) {
+    Text += "case ";
+    Text += getQualification(*ASTCtx, DeclCtx, Loc, EnumD);
+    if (EnumD->isScoped()) {
+      Text += EnumD->getName();
+      Text += "::";
+    }
+    Text += Enumerator->getName();
+    Text += ":";
+  }
+  Text += "break;";
+
+  return Effect::mainFileEdit(
+      SM, tooling::Replacements(tooling::Replacement(SM, Loc, 0, Text)));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/TweakTests.cpp

 };
 }
 })cpp";
       EXPECT_EQ(apply(SubCase, &EditedFiles), Case.ExpectedSource);
     }
   }
 }
 
+TWEAK_TEST(PopulateSwitch);
+TEST_F(PopulateSwitchTest, Test) {
+  struct Case {
+    CodeContext Context;
+    llvm::StringRef TestSource;
+    llvm::StringRef ExpectedSource;
+  };
+
+  Case Cases[]{
+      {
+          // No enumerators
+          Function,
+          R""(enum Enum {}; ^switch ((Enum)0) {})"",
+          "unavailable",
+      },
+      {
+          // Existing enumerators in switch
+          Function,
+          R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+          "unavailable",
+      },
+      {
+          // Body not CompoundStmt
+          Function,
+          R""(enum Enum {A}; ^switch (A);)"",
+          "unavailable",
+      },
+      {
+          // Selection on switch token
+          Function,
+          R""(enum Enum {A}; ^switch (A) {})"",
+          R""(enum Enum {A}; switch (A) {case A:break;})"",
+      },
+      {
+          // Selection on switch condition
+          Function,
+          R""(enum Enum {A}; switch (^A) {})"",
+          R""(enum Enum {A}; switch (A) {case A:break;})"",
+      },
+      {
+          // Selection in switch body
+          Function,
+          R""(enum Enum {A}; switch (A) {^})"",
+          R""(enum Enum {A}; switch (A) {case A:break;})"",
+      },
+      {
+          // Scoped enumeration
+          Function,
+          R""(enum class Enum {A}; ^switch (Enum::A) {})"",
+          R""(enum class Enum {A}; switch (Enum::A) {case Enum::A:break;})"",
+      },
+      {
+          // Scoped enumeration with multiple enumerators
+          Function,
+          R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
+          R""(enum class Enum {A,B}; )""
+          R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+      },
+      {
+          // Scoped enumerations in namespace
+          File,
+          R""(
+            namespace ns { enum class Enum {A}; }
+            void function() { ^switch (ns::Enum::A) {} }
+          )"",
+          R""(
+            namespace ns { enum class Enum {A}; }
+            void function() { switch (ns::Enum::A) {case ns::Enum::A:break;} }
+          )"",
+      },
+      {
+          // Unscoped enumerations in namespace
+          File,
+          R""(
+            namespace ns { enum Enum {A}; }
+            void function() { ^switch (ns::A) {} }
+          )"",
+          R""(
+            namespace ns { enum Enum {A}; }
+            void function() { switch (ns::A) {case ns::A:break;} }
+          )"",
+      },
+  };
+
+  for (const auto &Case : Cases) {
+    Context = Case.Context;
+    EXPECT_EQ(apply(Case.TestSource), Case.ExpectedSource);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang