[clang] [clang][AST] Fix printing `TagDecl`s. (PR #69971)

[via cfe-commits]

https://github.com/isuckatcs created https://github.com/llvm/llvm-project/pull/69971

https://github.com/llvm/llvm-project/commit/ae7c9443559ac420a6f401b7a24eb2fcea8ba3e8 `[AST] Fix printing tag decl groups in decl contexts` introduced a change that made sure, that implicitly created `TagDecl`s are not printed by `-ast-print`. This change however also introduced a side-effect, where non-compiling code is printed or the printer crashes.

An example of the non compiling code being printed:
```c++
struct A {
  int x;
};

struct Class {
  struct obj *(*next)(struct A *q);
};
```
```c++
// Result of -ast-print
struct A {
    int x;
};
struct Class {
    struct obj *(*next)(struct A {
        int x;
    } *);
};
```

An example of a crash:
```c++
struct Class {
  struct obj *(*next)(struct Class *q);
};
```

The issue is casued by how an undefined `struct obj` is modeled.
```c++
struct Class {
  struct foo *p1;
};
```
```
`-CXXRecordDecl ... struct Class definition
  |-...
  |-CXXRecordDecl ... implicit struct Class
  |-CXXRecordDecl ... parent &TranslationUnitDecl ... struct foo
  `-FieldDecl ... p1 'struct foo *'
```
```
`-RecordDecl ... struct Class definition
  |-RecordDecl ... parent &TranslationUnitDecl ... struct foo
  `-FieldDecl ... p1 'struct foo *'
```
In `C++` a new `CXXRecordDecl`, while in `C` a new `RecordDecl` node is inserted, with their parent set to the `TranslationUnitDecl`. This inserted record is not spelled in the source code and we want to avoid treating it as a part of the `DeclContext` too, unless it has a definition in which case we want to print the definition.

```c++
struct Class {
  struct foo {
    int x;
  } *p1;
};
```
```
`-CXXRecordDecl ... struct Class definition
  |-...
  |-CXXRecordDecl ... struct foo definition
  | |-...
  `-FieldDecl ... p1 'struct foo *'
```
```
`-RecordDecl ... struct Class definition
  |-RecordDecl ... parent &TranslationUnitDecl struct foo definition
  | `-...
  `-FieldDecl ... p1 'struct foo *'
```
Note that in `C` the parent of the definition is still set to the `TranslationUnitDecl`, while
in `C++` a nested `CXXRecordDecl` is created.


Fixes #69350

>From f4023d4d5d6a6f35ef2315f1b1ce7dd35f24e901 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 23 Oct 2023 22:46:47 +0200
Subject: [PATCH] fix

---
 clang/lib/AST/DeclPrinter.cpp    | 19 ++++++++++++++++++-
 clang/test/Sema/ast-print.c      | 20 ++++++++++++++++++++
 clang/test/SemaCXX/ast-print.cpp | 25 +++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index daa219b43c4e6cf..e14933374340666 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -519,7 +519,24 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
     // If the current declaration is not a free standing declaration, save it
     // so we can merge it with the subsequent declaration(s) using it.
     if (isa<TagDecl>(*D) && !cast<TagDecl>(*D)->isFreeStanding()) {
-      Decls.push_back(*D);
+
+      // Here we try to filter out the implicitly inserted tag declarations.
+      // E.g.:
+      //   struct Class {
+      //     struct foo *p1;
+      //   ;
+      //
+      //   CXXRecordDecl ... Class ...
+      //   |-CXXRecordDecl ... parent ... struct foo <-- ignore this
+      //   `-FieldDecl ... p1 'struct foo *'
+      //
+      // If `struct foo` has a definition, it will be modeled as a nested record
+      // inside `Class`, so the first condition will be true, unless we are in
+      // C, where `struct foo` will be a child of the translation unit node. In
+      // both cases if there is a definition, we want to print it.
+      if (cast<TagDecl>(*D)->getParent() == DC ||
+          cast<TagDecl>(*D)->getDefinition())
+        Decls.push_back(*D);
       continue;
     }
 
diff --git a/clang/test/Sema/ast-print.c b/clang/test/Sema/ast-print.c
index 2ba5ca34b513474..bb0990e020d49ce 100644
--- a/clang/test/Sema/ast-print.c
+++ b/clang/test/Sema/ast-print.c
@@ -104,3 +104,23 @@ void EnumWithAttributes3Fn(void) {
   // move to this use of the variable.
   void *p = EnumWithAttributes3Ptr;
 }
+
+// CHECK:      struct Recursive {
+// CHECK-NEXT:     struct obj *(*next)(struct Recursive *);
+// CHECK-NEXT: };
+struct Recursive {
+  struct obj *(*next)(struct Recursive *q);
+};
+
+// CHECK:      struct A {
+// CHECK-NEXT:     int x;
+// CHECK-NEXT: };
+// CHECK-NEXT: struct Foo {
+// CHECK-NEXT:     struct obj *(*next)(struct A *);
+// CHECK-NEXT: ;
+struct A {
+  int x;
+};
+struct Foo {
+  struct obj *(*next)(struct A *q);
+};
diff --git a/clang/test/SemaCXX/ast-print.cpp b/clang/test/SemaCXX/ast-print.cpp
index 2cb1ec440b6bb37..c71626ad96fecff 100644
--- a/clang/test/SemaCXX/ast-print.cpp
+++ b/clang/test/SemaCXX/ast-print.cpp
@@ -243,3 +243,28 @@ struct S {
 constexpr auto var = T::X;
 //CHECK: constexpr auto var = T::X;
 }
+
+namespace RecursiveClass {
+// CHECK:      struct Recursive {
+// CHECK-NEXT:     struct obj *(*next)(struct Recursive *);
+// CHECK-NEXT: };
+struct Recursive {
+  struct obj *(*next)(struct Recursive *q);
+};
+}
+
+namespace ClassParameter {
+// CHECK:      struct A {
+// CHECK-NEXT:     int x;
+// CHECK-NEXT: };
+// CHECK-NEXT: struct Foo {
+// CHECK-NEXT:     struct obj *(*next)(struct A *);
+// CHECK-NEXT: ;
+struct A {
+  int x;
+};
+
+struct Foo {
+  struct obj *(*next)(struct A *q);
+};
+}