[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

151

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

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.
+    // Handle RecordDecls separately: classes and structs are often
+    // forward-declared and we don't want to include every header that
+    // forward-declares the symbol because they're often unrelated. Also, if
+    // definition is available, we can safely skip forward declarations because
+    // they have no effect. The only exception is symbols that require their
+    // original declaration because of the scope specifier, e.g. nested
+    // classes:
+    //
+    // Foo.h:
+    // class Foo { class Bar; }
+    //
+    // Foo.cpp
+    // class Foo::Bar {};
+    //
+    // Here, Foo::Bar definition will require the original declaration inside
+    // Foo, but it will be available because we will still mark Foo definition
+    // as used.
     if (const auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
-      if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
-        if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
+      const auto *Definition = RD->getDefinition();
+      if (Definition) {
+        Result.insert(Definition->getLocation());
+        return;
+      }
+      // If we see a declaration in the mainfile, skip all non-definition
+      // decls.
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation())) {
+        if (Definition)
           Result.insert(Definition->getLocation());
         return;
       }
     }
     for (const Decl *Redecl : D->redecls())
       Result.insert(Redecl->getLocation());
   }
 

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 {}; }",
+          "namespace ns { struct X; struct ^X {}; }",
           "using ns::X;",
       },
       {
           "namespace ns { struct X; struct X {}; }",
           "using namespace ns;",
       },
       {
           "struct ^A {}; using B = A; using ^C = B;",
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
       {