[clangd] IncludeCleaner: Complicated rules for enum usage

[kbobyrev]

Remove unwanted (though good) formatting changes.

[kbobyrev]

Add one more test.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

38

I don't understand this special case.
It seems you're trying to avoid requiring too many redecls of a referenced type. Why is this important, and different from e.g. Class?

93

This says what, rather than why.

Rather maybe:

Enums may be usefully forward-declared as *complete* types by specifying an underlying type.
In this case, the definition should see the declaration so they can be checked for compatibility.

96

D->isThisDeclarationADefinition() is more idiomatic for the first part

98

rather add(D) here?

[kbobyrev]

Resolve review comments.

clang-tools-extra/clangd/IncludeCleaner.cpp

38

Yes, this is for cases like

{
    "enum class Color;",
    "enum class Color {}; Color c;",
},

I think the problem here is that if we see the definition of the enum, it should be the "ground truth" for the usage, forward declarations are not really useful in this case. It's not much different from the class but I just wanted to handle it separately, not sure if it's OK to merge these two changes into one patch.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

38

The heuristic seems plausible, though there are counterexamples like:

"enum Color;"
"void foo(Color); enum Color {};"

My main argument would be that we have a general policy here: conservatively consider all redecls as used, rather than trying to work out which is best. It's not clear why enums are special enough that we should hack around them without rethinking the policy.

(I think VisitEnumDecl is different - our general policy is that decls *aren't* references to forward-decls, and in enums' case this is violates our meta-policy of "be conservative, consider everything that might be a use").

[kbobyrev]

Resolve review & offline comments.

clang-tools-extra/clangd/IncludeCleaner.cpp

38

This and our offline discussion makes total sense, I will delete it from the patch.

[kbobyrev]

Delete duplicate tests.

[thakis]

Follow-up commit https://github.com/llvm/llvm-project/commit/1c2e249f938c50e1b331a1f7adc83c0a381f3897 (which doesn't seem to have its own phab link) broke clangd tests everywhere, e.g. http://45.33.8.238/linux/59134/step_9.txt

Please take a look.

This is like the third clangd-caused build failure in the last 12 h or so h :/

[kbobyrev]

Yes, I'm aware and actively working on this one. I think I have the fix but
I'm trying to run all the tests (Clang etc) to make sure it doesn't break
anything else.

[sammccall]

Hey Kirill,

If you mean removing traversal of the enumdecl->enumtype in
RecursiveASTVisitor, that's a subtle change that potentially affects a
lot of callers.
We'd need to be able to revert it, so we shouldn't rely on it to get
the tests back to green.

It looks like there's a single assertion failure, can you just change
the assertion to assert the (bad) current behavior?
(Or revert the patches)

clang-tools-extra/clangd/IncludeCleaner.cpp

public:
 
   bool VisitMemberExpr(MemberExpr *ME) {
     add(ME->getMemberDecl());
     add(ME->getFoundDecl().getDecl());
     return true;
   }
 
   bool VisitTagType(TagType *TT) {
+    // For enumerations we will require only the definition if it's present and
+    // the underlying type is not specified.
+    if (TT->isEnumeralType()) {
+      const auto *Enum = llvm::dyn_cast<EnumDecl>(TT->getDecl());
+      assert(Enum);
+      if (!Enum->getIntegerTypeSourceInfo() && TT->getDecl()->getDefinition()) {
+        Result.insert(TT->getDecl()->getDefinition()->getLocation());
+        return true;
+      }
+    }
     add(TT->getDecl());
     return true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *CCE) {
     add(CCE->getConstructor());
     return true;
   }
public:
 
   bool VisitUsingDecl(UsingDecl *D) {
     for (const auto *Shadow : D->shadows()) {
       add(Shadow->getTargetDecl());
     }
     return true;
   }
 
+  // Require redeclarations only for definitions and only when the underlying
+  // type is specified.
+  bool VisitEnumDecl(EnumDecl *D) {
+    if (D != D->getDefinition() || !D->getIntegerTypeSourceInfo())
+      return false;
+    for (const auto *Redecl : D->redecls())
+      Result.insert(Redecl->getLocation());
+    return false;
+  }
+
 private:
   using Base = RecursiveASTVisitor<ReferencedLocationCrawler>;
 
   void add(const Decl *D) {
     if (!D || !isNew(D->getCanonicalDecl())) {
       return;
     }
     for (const Decl *Redecl : D->redecls()) {

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

TestCase Cases[] = {
           "enum ^Color { ^Red = 42, Green = 9000};",
           "int MyColor = Red;",
       },
       {
           "struct ^X { enum ^Language { ^CXX = 42, Python = 9000}; };",
           "int Lang = X::CXX;",
       },
       {
+          "enum class Color;",
+          "enum class Color {};",
+      },
+      {
+          "enum class ^Color : int;",
+          "enum class Color : int {};",
+      },
+      {
+          "enum class Color : int {};",
+          "enum class Color : int;",
+      },
+      {
+          "enum class Color;",
+          "enum class Color {}; Color c;",
+      },
+      {
+          "enum class ^Color : char;",
+          "Color c;",
+      },
+      {
+          "enum class ^Color : char {};",
+          "Color c;",
+      },
+      {
+          "enum class ^Color;",
+          "Color c;",
+      },
+      {
           // When a type is resolved via a using declaration, the
           // UsingShadowDecl is not referenced in the AST.
           // Compare to TypedefType, or DeclRefExpr::getFoundDecl().
           //                                 ^
           "namespace ns { class ^X; }; using ns::X;",
           "X *y;",
       }};
   for (const TestCase &T : Cases) {