[clangd] IncludeCleaner: Do not require forward declarations of RecordDecls when definition is available

[kbobyrev]

Summary

This makes IncludeCleaner more useful in the presense of a large number of
forward declarations. If the definition is already in the Translation Unit and
visible to the Main File, forward declarations have no effect.

The original patch D112707 was split in two: D114864 and this one.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

140

if we made it here, we already know definition wasn't visible in the current TU.

maybe just something like:

// Special case RecordDecls, as it is common for them to be forward declared multiple times.
// The most common two cases are:
// - definition available in TU, only mark that one as usage. rest is likely to be unnecessary. this might result in false positives when an internal definition is visible.
// - there's a forward declaration in the main file, no need for other redecls.
if (auto *RD = ...) {
  if (auto *Def) {
    insert that;
    return;
  }
  if (mostRecentInMainFile) {
    return;
  }
}

If we really want to keep a note around nested decls, we might say something like:
Forward decls/definitions of nested recorddecls can only be nested inside other recorddecls, and the definition for outer recorddecl has to be visible, and we always preserve definitions.

But I don't think all the additional information provides much value, rather creates confusion and forces one to think more about the details.

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

66–67

you mind changing these into functions as well?

[kbobyrev]

Resolve review comments.

[kadircet]

thanks!

clang-tools-extra/clangd/IncludeCleaner.cpp

127

i know it was me who didn't capitalize, but can you capitalize the first words :D
s/rest/Rest
s/this/This (line below)

[kbobyrev]

In D114949#3169074, @kadircet wrote:

thanks!

Aww, sorry, I capitalized/reformatted parts of them but missed the rest :D

[kbobyrev]

Fix typos.

[kbobyrev]

Improve wording.

clang-tools-extra/clangd/IncludeCleaner.cpp

public:
   }
 
 private:
   using Base = RecursiveASTVisitor<ReferencedLocationCrawler>;
 
   void add(const Decl *D) {
     if (!D || !isNew(D->getCanonicalDecl()))
       return;
-    // If we see a declaration in the mainfile, skip all non-definition decls.
-    // We only do this for classes because forward declarations are common and
-    // we don't want to include every header that forward-declares the symbol
-    // because they're often unrelated.
+    // Special case RecordDecls, as it is common for them to be forward
+    // declared multiple times. The most common cases are:
+    // - Definition available in TU, only mark that one as usage. The rest is
+    //   likely to be unnecessary. This might result in false positives when an
+    //   internal definition is visible.
+    // - There's a forward declaration in the main file, no need for other
+    //   redecls.
     if (const auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
-      if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
-        if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
-          Result.insert(Definition->getLocation());
+      if (const auto *Definition = RD->getDefinition()) {
         return;
       }
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation()))
+        return;
     }
     for (const Decl *Redecl : D->redecls())
       Result.insert(Redecl->getLocation());
   }
 
   bool isNew(const void *P) { return P && Visited.insert(P).second; }
 
   ReferencedLocations &Result;
   llvm::DenseSet<const void *> Visited;
   const SourceManager &SM;

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

TestCase Cases[] = {
           "int ^x();",
           "int y = x();",
       },
       // RecordDecl
       {
           "class ^X;",
           "X *y;",
       },
-      // FIXME(kirillbobyrev): When definition is available, we don't need to
-      // mark forward declarations as used.
+      // When definition is available, we don't need to mark forward
+      // declarations as used.
       {
-          "class ^X {}; class ^X;",
+          "class ^X {}; class X;",
           "X *y;",
       },
       // We already have forward declaration in the main file, the other
       // non-definition declarations are not needed.
       {
           "class ^X {}; class X;",
           "class X; X *y;",
       },
+      // Nested class definition can occur outside of the parent class
+      // definition. Bar declaration should be visible to its definition but
+      // it will always be because we will mark Foo definition as used.
+      {
+          "class ^Foo { class Bar; };",
+          "class Foo::Bar {};",
+      },
       // TypedefType and UsingDecls
       {
           "using ^Integer = int;",
           "Integer x;",
       },
       {
-          "namespace ns { struct ^X; struct ^X {}; }",
-          "using ns::X;",
+          "namespace ns { void ^foo(); void ^foo() {} }",
+          "using ns::foo;",
       },
       {
-          "namespace ns { struct X; struct X {}; }",
+          "namespace ns { void foo(); void foo() {}; }",
           "using namespace ns;",
       },
       {
           "struct ^A {}; using B = A; using ^C = B;",
           "C a;",
       },
       {
           "typedef bool ^Y; template <typename T> struct ^X {};",
TestCase Cases[] = {
       },
       // Expr (type is traversed)
       {
           "class ^X{}; X ^foo();",
           "auto bar() { return foo(); }",
       },
       // Redecls
       {
-          "class ^X; class ^X{}; class ^X;",
-          "X *y;",
+          "void ^foo(); void ^foo() {} void ^foo();",
+          "void bar() { foo(); }",
       },
       // Constructor
       {
           "struct ^X { ^X(int) {} int ^foo(); };",
           "auto x = X(42); auto y = x.foo();",
       },
       // Function
       {