[AST] Fix printing tag decl groups in decl contexts

[jdenny]

Summary

For example, given:

struct T1 {
  struct T2 *p0;
};

-ast-print produced:

struct T1 {
  struct T2; 
  struct T2 *p0;
};

Compiling that produces a warning that the first struct T2 declaration
does not declare anything.

Details:

A tag decl group is one or more decls that share a type specifier that
is a tag decl (that is, a struct/union/class/enum decl). Within
functions, the parser builds such a tag decl group as part of a
DeclStmt. However, in decl contexts, such as file scope or a member
list, the parser does not group together the members of a tag decl
group. Previously, detection of tag decl groups during printing was
implemented but only if the tag decl was unnamed. Otherwise, as in
the above example, the members of the group did not print together and
so sometimes introduced warnings.

This patch extends detection of tag decl groups in decl contexts to
any tag decl that is recorded in the AST as not free-standing.

[jdenny]

Rebased onto a more recent master.

[jdenny]

Rebased. Ping.

[rsmith]

lib/AST/DeclPrinter.cpp

392–395

I would think the right thing to check here is that the ElaboratedType's OwnedTagDecl is Decls[0]. Currently, you also allow cases where it's merely a redeclaration of that same TagDecl, which will combine together declarations too often...

test/Misc/ast-print-record-decl.c

163–173

... such as here, for example. Because the two KW T declarations redeclare the same TagDecl, they will get merged by -ast-print.

[jdenny]

Made the suggested change. Thanks!

[rsmith]

Looks good, thanks.

It strikes me that this will still lead to inconsistencies. For example, I expect this:

struct A { struct B *a, *b; struct B *c, *d; };

... to print as:

struct A {
  struct B *a, *b;
  struct B *c;
  struct B *d;
};

... where the first two are joined because their type owns a declaration of struct B, and the second two are not joined because their type does not own a declaration (it just has a reference to the already-existing declaration of struct B). One (somewhat hacky) way to address this would be to compare the starting source locations of a sequence of DeclaratorDecls and group them if it's the same.

[jdenny]

In D45465#1098710, @rsmith wrote:

Looks good, thanks.

Thanks.

It strikes me that this will still lead to inconsistencies. For example, I expect this:

struct A { struct B *a, *b; struct B *c, *d; };

... to print as:

struct A {
  struct B *a, *b;
  struct B *c;
  struct B *d;
};

... where the first two are joined because their type owns a declaration of struct B, and the second two are not joined because their type does not own a declaration (it just has a reference to the already-existing declaration of struct B). One (somewhat hacky) way to address this would be to compare the starting source locations of a sequence of DeclaratorDecls and group them if it's the same.

While it would be nice to fix that, I'm not as concerned because, AFAICT, that doesn't ever change the semantics.

lib/AST/DeclPrinter.cpp

for (DeclContext::decl_iterator D = DC->decls_begin(), DEnd = DC->decls_end();
 
     // Don't print implicit specializations, as they are printed when visiting
     // corresponding templates.
     if (auto FD = dyn_cast<FunctionDecl>(*D))
       if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation &&
           !isa<ClassTemplateSpecializationDecl>(DC))
         continue;
 
-    // The next bits of code handles stuff like "struct {int x;} a,b"; we're
+    // The next bits of code handle stuff like "struct {int x;} a,b"; we're
     // forced to merge the declarations because there's no other way to
-    // refer to the struct in question.  This limited merging is safe without
-    // a bunch of other checks because it only merges declarations directly
-    // referring to the tag, not typedefs.
+    // refer to the struct in question.  When that struct is named instead, we
+    // also need to merge to avoid splitting off a stand-alone struct
+    // declaration that produces the warning ext_no_declarators in some
+    // contexts.
+    //
+    // This limited merging is safe without a bunch of other checks because it
+    // only merges declarations directly referring to the tag, not typedefs.
     //
     // Check whether the current declaration should be grouped with a previous
-    // unnamed struct.
+    // non-free-standing tag declaration.
     QualType CurDeclType = getDeclType(*D);
     if (!Decls.empty() && !CurDeclType.isNull()) {
       QualType BaseType = GetBaseType(CurDeclType);
-      if (!BaseType.isNull() && isa<ElaboratedType>(BaseType))
-        BaseType = cast<ElaboratedType>(BaseType)->getNamedType();
-      if (!BaseType.isNull() && isa<TagType>(BaseType) &&
-          cast<TagType>(BaseType)->getDecl() == Decls[0]) {
+      if (!BaseType.isNull() && isa<ElaboratedType>(BaseType) &&
+          cast<ElaboratedType>(BaseType)->getOwnedTagDecl() == Decls[0]) {
         Decls.push_back(*D);
         continue;
       }
     }
 
     // If we have a merged group waiting to be handled, handle it now.
     if (!Decls.empty())
       ProcessDeclGroup(Decls);
 
-    // If the current declaration is an unnamed tag type, save it
+    // 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)->getIdentifier()) {
+    if (isa<TagDecl>(*D) && !cast<TagDecl>(*D)->isFreeStanding()) {
       Decls.push_back(*D);
       continue;
     }
 
     if (isa<AccessSpecDecl>(*D)) {
       Indentation -= Policy.Indentation;
       this->Indent();
       Print(D->getAccess());

test/Misc/ast-print-enum-decl.c

void declsOnly() {
   enum T *p2;
 
   // PRINT-NEXT: enum T *p3;
   enum __attribute__((deprecated(""))) T *p3;
 
   // PRINT-NEXT: enum T *p4;
   enum T *p4;
 }
+
+// Check that tag decl groups stay together in decl contexts.
+
+// PRINT-LABEL: enum DeclGroupAtFileScope {
+// PRINT-NEXT:    DeclGroupAtFileScope0
+// PRINT-NEXT:  } *DeclGroupAtFileScopePtr;
+enum DeclGroupAtFileScope { DeclGroupAtFileScope0 } *DeclGroupAtFileScopePtr;
+
+// PRINT-LABEL: struct DeclGroupInMemberList
+struct DeclGroupInMemberList {
+  // PRINT-NEXT:  enum T1 {
+  // PRINT-NEXT:    T10
+  // PRINT-NEXT:  } *p0;
+  enum T1 { T10 } *p0;
+  // PRINT-NEXT:  enum T2 {
+  // PRINT-NEXT:    T20
+  // PRINT-NEXT:  } *p1, *p2;
+  enum T2 { T20 } *p1, *p2;
+  // PRINT-NEXT: };
+};

test/Misc/ast-print-record-decl.c

 // Check struct:
 //
 //   First check compiling and printing of this file.
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm \
 //   RUN:        -DKW=struct -DBASES= -o - %s \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print -DKW=struct -DBASES= %s > %t.c
-//   RUN: FileCheck --check-prefixes=CHECK,PRINT -DKW=struct -DBASES= %s \
-//   RUN:           --input-file %t.c
+//   RUN: FileCheck --check-prefixes=CHECK,PRINT,PRINT-C -DKW=struct -DBASES= \
+//   RUN:           %s --input-file %t.c
 //
 //   Now check compiling and printing of the printed file.
 //
 //   RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.c
 //   RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.c
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm -o - %t.c \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print %t.c \
-//   RUN: | FileCheck --check-prefixes=CHECK,PRINT -DKW=struct -DBASES= %s
+//   RUN: | FileCheck --check-prefixes=CHECK,PRINT,PRINT-C -DKW=struct \
+//   RUN:             -DBASES= %s
 
 // Repeat for union:
 //
 //   First check compiling and printing of this file.
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm \
 //   RUN:        -DKW=union -DBASES= -o - %s \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print -DKW=union -DBASES= %s > %t.c
-//   RUN: FileCheck --check-prefixes=CHECK,PRINT -DKW=union -DBASES= %s \
-//   RUN:           --input-file %t.c
+//   RUN: FileCheck --check-prefixes=CHECK,PRINT,PRINT-C -DKW=union -DBASES= \
+//   RUN:           %s --input-file %t.c
 //
 //   Now check compiling and printing of the printed file.
 //
 //   RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.c
 //   RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.c
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm -o - %t.c \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print %t.c \
-//   RUN: | FileCheck --check-prefixes=CHECK,PRINT -DKW=union -DBASES= %s
+//   RUN: | FileCheck --check-prefixes=CHECK,PRINT,PRINT-C -DKW=union \
+//   RUN:             -DBASES= %s
 
 // Repeat for C++ (BASES helps ensure we're printing as C++ not as C):
 //
 //   First check compiling and printing of this file.
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm \
 //   RUN:        -DKW=struct -DBASES=' : B' -o - -xc++ %s \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print -DKW=struct -DBASES=' : B' -xc++ %s \
 //   RUN: > %t.cpp
-//   RUN: FileCheck --check-prefixes=CHECK,PRINT,CXX -DKW=struct \
+//   RUN: FileCheck --check-prefixes=CHECK,PRINT,PRINT-CXX -DKW=struct \
 //   RUN:           -DBASES=' : B' %s --input-file %t.cpp
 //
 //   Now check compiling and printing of the printed file.
 //
 //   RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.cpp
 //   RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.cpp
 //
 //   RUN: %clang -target x86_64-linux -Xclang -verify -S -emit-llvm -o - %t.cpp \
 //   RUN: | FileCheck --check-prefixes=CHECK,LLVM %s
 //
 //   RUN: %clang_cc1 -verify -ast-print %t.cpp \
-//   RUN: | FileCheck --check-prefixes=CHECK,PRINT,CXX -DKW=struct \
+//   RUN: | FileCheck --check-prefixes=CHECK,PRINT,PRINT-CXX -DKW=struct \
 //   RUN:             -DBASES=' : B' %s
 
 // END.
 
 #ifndef KW
 # error KW undefined
 # define KW struct // help syntax checkers
 #endif

 
 // CHECK-LABEL: defSelfRef
 void defSelfRef() {
   // PRINT-NEXT: [[KW]] __attribute__((deprecated(""))) T *p0;
   // expected-warning@+2 {{'T' is deprecated}}
   // expected-note@+1 2 {{'T' has been explicitly marked deprecated here}}
   KW __attribute__((deprecated(""))) T *p0;
 
-  // PRINT-NEXT: [[KW]] __attribute__((aligned(16))) T[[BASES]] {
-  // PRINT-NEXT:   int i;
-  // PRINT-NEXT:   [[KW]] T *p2;
-  // PRINT-NEXT: } *p1;
-  KW __attribute__((aligned(16))) T BASES { // expected-warning {{'T' is deprecated}}
+  // PRINT-NEXT:  [[KW]] __attribute__((aligned(64))) T[[BASES]] {
+  // PRINT-NEXT:    int i;
+  // PRINT-NEXT:    [[KW]] T *p2;
+  // PRINT-NEXT:    [[KW]] __attribute__((may_alias)) T *p3;
+  // PRINT-NEXT:    [[KW]] T *p4;
+  // PRINT-NEXT:  } *p1;
+  KW __attribute__((aligned(64))) T BASES { // expected-warning {{'T' is deprecated}}
     int i;
     KW T *p2;
+    // FIXME: For C++, T at p3 loses aligned and deprecated, perhaps because
+    // that RecordDecl isn't in the same redecl list.  Perhaps the redecl lists
+    // are split here but not in C due to the different scoping rules in C++
+    // classes.
+    KW __attribute__((may_alias)) T *p3;
+    KW T *p4;
   } *p1;
 
-  // LLVM: store i64 16
+  // LLVM: store i64 64
   long s0 = sizeof *p0;
-  // LLVM-NEXT: store i64 16
+  // LLVM-NEXT: store i64 64
   long s1 = sizeof *p1;
-  // LLVM-NEXT: store i64 16
+  // LLVM-NEXT: store i64 64
   long s2 = sizeof *p0->p2;
-  // LLVM-NEXT: store i64 16
-  long s3 = sizeof *p1->p2;
-  // LLVM-NEXT: store i64 16
-  long s4 = sizeof *p1->p2->p2;
+  // LLVM-NEXT: store i64 64
+  long s3 = sizeof *p1->p3;
+  // LLVM-NEXT: store i64 64
+  long s4 = sizeof *p1->p4->p2;
 }
 
 // CHECK-LABEL: declsOnly
 void declsOnly() {
   // PRINT-NEXT: [[KW]] T *p0;
   KW T *p0;
 
   // PRINT-NEXT: [[KW]] __attribute__((may_alias)) T *p1;
void inInit() {
   KW T1 { int i; };
   // PRINT-NEXT: [[KW]] T2 {
   // PRINT-NEXT:   long i;
   // PRINT-NEXT: } t2 = {sizeof([[KW]] T1)};
   KW T2 { long i; } t2 = {sizeof(KW T1)};
 }
 
 #ifdef __cplusplus
-// CXX-LABEL: inMemberPtr
+// PRINT-CXX-LABEL: inMemberPtr
 void inMemberPtr() {
-  // CXX-NEXT: [[KW]] T1 {
-  // CXX-NEXT:   int i;
-  // CXX-NEXT: };
+  // PRINT-CXX-NEXT: [[KW]] T1 {
+  // PRINT-CXX-NEXT:   int i;
+  // PRINT-CXX-NEXT: };
   KW T1 { int i; };
-  // CXX-NEXT: [[KW]] T2 {
-  // CXX-NEXT: } T1::*p;
+  // PRINT-CXX-NEXT: [[KW]] T2 {
+  // PRINT-CXX-NEXT: } T1::*p;
   KW T2 {} T1::*p;
 }
 #endif
+
+// Check that tag decl groups stay together in decl contexts.
+
+// PRINT-LABEL: DeclGroupAtFileScope {
+// PRINT-NEXT:    int i;
+// PRINT-NEXT:  } *DeclGroupAtFileScopePtr;
+KW DeclGroupAtFileScope { int i; } *DeclGroupAtFileScopePtr;
+
+// PRINT-LABEL: DeclGroupInMemberList {
+KW DeclGroupInMemberList {
+  // PRINT-NEXT:  struct  T1 {
+  // PRINT-NEXT:    int i;
+  // PRINT-NEXT:  } t1;
+  struct T1 { int i; } t1;
+  // PRINT-NEXT:  union T2 {
+  // PRINT-NEXT:    int i;
+  // PRINT-NEXT:  } *t20, t21[2];
+  union T2 { int i; } *t20, t21[2];
+  // PRINT-NEXT:  enum T3 {
+  // PRINT-NEXT:    T30
+  // PRINT-NEXT:  } t30;
+  enum T3 { T30 } t30;
+  // PRINT-NEXT: };
+};
+
+// A tag decl group in the tag decl's own member list is exercised in
+// defSelfRef above.

test/Sema/ast-print.c

 }
 
 // CHECK-LABEL: enum __attribute__((deprecated(""))) EnumWithAttributes {
 enum EnumWithAttributes { // expected-warning {{'EnumWithAttributes' is deprecated}}
   // CHECK-NEXT: EnumWithAttributesFoo __attribute__((deprecated(""))),
   EnumWithAttributesFoo __attribute__((deprecated)),
   // CHECK-NEXT: EnumWithAttributesBar __attribute__((unavailable(""))) = 50
   EnumWithAttributesBar __attribute__((unavailable)) = 50
-  // CHECK-NEXT: };
-  // CHECK-NEXT: enum EnumWithAttributes *EnumWithAttributesPtr;
+  // CHECK-NEXT: } *EnumWithAttributesPtr;
 } __attribute__((deprecated)) *EnumWithAttributesPtr; // expected-note {{'EnumWithAttributes' has been explicitly marked deprecated here}}
 
 // FIXME: If enum is forward-declared at file scope, attributes are lost.
 // CHECK-LABEL: enum EnumWithAttributes2 *EnumWithAttributes2Ptr;
 // expected-warning@+2 {{'EnumWithAttributes2' is deprecated}}
 // expected-note@+1 {{'EnumWithAttributes2' has been explicitly marked deprecated here}}
 enum __attribute__((deprecated)) EnumWithAttributes2 *EnumWithAttributes2Ptr;
 

test/SemaCXX/ast-print.cpp

 }
 }
 
 namespace {
 // CHECK: struct {{\[\[gnu::visibility\(\"hidden\"\)\]\]}} S;
 struct [[gnu::visibility("hidden")]] S;
 }
 
-// CHECK: struct CXXFunctionalCastExprPrint fce = CXXFunctionalCastExprPrint{};
+// CHECK:      struct CXXFunctionalCastExprPrint {
+// CHECK-NEXT: } fce = CXXFunctionalCastExprPrint{};
 struct CXXFunctionalCastExprPrint {} fce = CXXFunctionalCastExprPrint{};
 
-// CHECK: struct CXXTemporaryObjectExprPrint toe = CXXTemporaryObjectExprPrint{};
+// CHECK:      struct CXXTemporaryObjectExprPrint {
+// CHECK-NEXT:   CXXTemporaryObjectExprPrint();
+// CHECK-NEXT: } toe = CXXTemporaryObjectExprPrint{};
 struct CXXTemporaryObjectExprPrint { CXXTemporaryObjectExprPrint(); } toe = CXXTemporaryObjectExprPrint{};
 
 namespace PR24872 {
 // CHECK: template <typename T> struct Foo : T {
 // CHECK: using T::operator-;
 template <typename T> struct Foo : T {
   using T::operator-;
 };