[clangd] Improve PopulateSwitch tweak

[dgoldman]

Summary

• Support enums in C and ObjC as their AST representations differ slightly.

• Add support for typedef'ed enums.

[sammccall]

Thanks!

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

178–179

"we need a storage kind" doesn't seem quite right, rather "we need a stored value"

clang-tools-extra/clangd/unittests/tweaks/PopulateSwitchTests.cpp

210

There's nothing ObjC-specific about these tests, I don't think we should duplicate them between C and ObjC.

(Unlike C vs C++, it's very rare for plain-c code to have different semantics in ObjC)

[dgoldman]

Minor fixes for review

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

bool PopulateSwitch::prepare(const Selection &Sel) {
   DeclCtx = &CA->getDeclContext();
 
   // Examine the condition of the switch statement to see if it's an enum.
   const Expr *Cond = Switch->getCond();
   if (!Cond)
     return false;
   // Ignore implicit casts, since enums implicitly cast to integer types.
   Cond = Cond->IgnoreParenImpCasts();
-  EnumT = Cond->getType()->getAsAdjusted<EnumType>();
+  // Get the canonical type to handle typedefs.
+  EnumT = Cond->getType().getCanonicalType()->getAsAdjusted<EnumType>();
   if (!EnumT)
     return false;
   EnumD = EnumT->getDecl();
   if (!EnumD || EnumD->isDependentType())
     return false;
 
   // Finally, check which cases exist and which are covered.
   // We trigger if there are any values in the enum that aren't covered by the
if (isa<DefaultStmt>(CaseList))
       return false;
 
     const CaseStmt *CS = cast<CaseStmt>(CaseList);
 
     // GNU range cases are rare, we don't support them.
     if (CS->caseStmtIsGNURange())
       return false;
 
+    // Support for direct references to enum constants. This is required to
+    // support C and ObjC which don't contain values in their ConstantExprs.
+    // The general way to get the value of a case is EvaluateAsRValue, but we'd
+    // rather not deal with that in case the AST is broken.
+    if (auto *DRE = dyn_cast<DeclRefExpr>(CS->getLHS()->IgnoreParenCasts())) {
+      if (auto *Enumerator = dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
+        auto Iter = ExpectedCases.find(Normalize(Enumerator->getInitVal()));
+        if (Iter != ExpectedCases.end())
+          Iter->second.setCovered();
+        continue;
+      }
+    }
+
+    // ConstantExprs with values are expected for C++, otherwise the storage
+    // kind will be None.
+
     // Case expression is not a constant expression or is value-dependent,
     // so we may not be able to work out which cases are covered.
     const ConstantExpr *CE = dyn_cast<ConstantExpr>(CS->getLHS());
     if (!CE || CE->isValueDependent())
       return false;
 
-    // Unsure if this case could ever come up, but prevents an unreachable
-    // executing in getResultAsAPSInt.
+    // We need a stored value in order to continue; currently both C and ObjC
+    // enums won't have one.
     if (CE->getResultStorageKind() == ConstantExpr::RSK_None)
       return false;
     auto Iter = ExpectedCases.find(Normalize(CE->getResultAsAPSInt()));
     if (Iter != ExpectedCases.end())
       Iter->second.setCovered();
   }
 
   return !llvm::all_of(ExpectedCases,

clang-tools-extra/clangd/unittests/tweaks/PopulateSwitchTests.cpp

 
 TWEAK_TEST(PopulateSwitch);
 
 TEST_F(PopulateSwitchTest, Test) {
   struct Case {
     CodeContext Context;
     llvm::StringRef TestSource;
     llvm::StringRef ExpectedSource;
+    llvm::StringRef FileName = "TestTU.cpp";
   };
 
   Case Cases[]{
       {
           // No enumerators
           Function,
           R""(enum Enum {}; ^switch ((Enum)0) {})"",
           "unavailable",
Case Cases[]{
           "unavailable",
       },
       {
           // Enum is dependent type
           File,
           R""(template<typename T> void f() {enum Enum {A}; ^switch (A) {}})"",
           "unavailable",
       },
+      {// C: Only filling in missing enumerators
+       Function,
+       R""(
+            enum CEnum {A,B,C};
+            enum CEnum val = A;
+            ^switch (val) {case B:break;}
+          )"",
+       R""(
+            enum CEnum {A,B,C};
+            enum CEnum val = A;
+            switch (val) {case B:break;case A:case C:break;}
+          )"",
+       "TestTU.c"},
+      {// C: Only filling in missing enumerators w/ typedefs
+       Function,
+       R""(
+            typedef unsigned long UInteger;
+            enum ControlState : UInteger;
+            typedef enum ControlState ControlState;
+            enum ControlState : UInteger {A,B,C};
+            ControlState controlState = A;
+            switch (^controlState) {case A:break;}
+          )"",
+       R""(
+            typedef unsigned long UInteger;
+            enum ControlState : UInteger;
+            typedef enum ControlState ControlState;
+            enum ControlState : UInteger {A,B,C};
+            ControlState controlState = A;
+            switch (controlState) {case A:break;case B:case C:break;}
+          )"",
+       "TestTU.c"},
   };
 
   for (const auto &Case : Cases) {
     Context = Case.Context;
+    FileName = Case.FileName;
     EXPECT_EQ(apply(Case.TestSource), Case.ExpectedSource);
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang