Fix the double space and double attribute printing of the final keyword. #88600

vgvassilev · 2024-04-13T06:42:42Z

Fixes #56517.

cc: @kimgr

llvmbot · 2024-04-13T06:43:11Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Vassil Vassilev (vgvassilev)

Changes

Fixes #56517.

cc: @kimgr

Full diff: https://github.com/llvm/llvm-project/pull/88600.diff

4 Files Affected:

(modified) clang/lib/AST/DeclPrinter.cpp (+18-13)
(modified) clang/test/SemaCXX/cxx11-attr-print.cpp (+5)
(modified) clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl (+2-2)
(modified) clang/utils/TableGen/ClangAttrEmitter.cpp (+1-1)

diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..c758f792b04a59 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
     void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> Args,
                                 const TemplateParameterList *Params);
     enum class AttrPosAsWritten { Default = 0, Left, Right };
-    void
+    bool
     prettyPrintAttributes(const Decl *D,
                           AttrPosAsWritten Pos = AttrPosAsWritten::Default);
     void prettyPrintPragmas(Decl *D);
@@ -252,10 +252,12 @@ static DeclPrinter::AttrPosAsWritten getPosAsWritten(const Attr *A,
   return DeclPrinter::AttrPosAsWritten::Right;
 }
 
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
                                         AttrPosAsWritten Pos /*=Default*/) {
+  bool hasPrinted = false;
   if (Policy.PolishForDeclaration)
-    return;
+    return hasPrinted;
 
   if (D->hasAttrs()) {
     const AttrVec &Attrs = D->getAttrs();
@@ -275,6 +277,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
           if (Pos != AttrPosAsWritten::Left)
             Out << ' ';
           A->printPretty(Out, Policy);
+          hasPrinted = true;
           if (Pos == AttrPosAsWritten::Left)
             Out << ' ';
         }
@@ -282,6 +285,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
       }
     }
   }
+  return hasPrinted;
 }
 
 void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1064,14 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
   // FIXME: add printing of pragma attributes if required.
   if (!Policy.SuppressSpecifiers && D->isModulePrivate())
     Out << "__module_private__ ";
-  Out << D->getKindName();
 
-  prettyPrintAttributes(D);
+  Out << D->getKindName() << ' ';
+
+  // FIXME: Move before printing the decl kind to match the behavior of the
+  // attribute printing for variables and function where they are printed first.
+  prettyPrintAttributes(D, AttrPosAsWritten::Left);
 
   if (D->getIdentifier()) {
-    Out << ' ';
     if (auto *NNS = D->getQualifier())
       NNS->print(Out, Policy);
     Out << *D;
@@ -1082,11 +1088,8 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
     }
   }
 
-  if (D->hasDefinition()) {
-    if (D->hasAttr<FinalAttr>()) {
-      Out << " final";
-    }
-  }
+  if (prettyPrintAttributes(D, AttrPosAsWritten::Right))
+    Out << ' ';
 
   if (D->isCompleteDefinition()) {
     // Print the base classes
@@ -1114,10 +1117,12 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
 
     // Print the class definition
     // FIXME: Doesn't print access specifiers, e.g., "public:"
+    //if (!D->getIdentifier())
+      Out << ' ';
     if (Policy.TerseOutput) {
-      Out << " {}";
+      Out << "{}";
     } else {
-      Out << " {\n";
+      Out << "{\n";
       VisitDeclContext(D);
       Indent() << "}";
     }
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S<int>;
 
 // CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
 using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template <typename T> class FinalTemplate final {};
+// CHECK: template <typename T> class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index 7e79ae3bf005fc..b1a15c43191829 100644
--- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
+++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
@@ -6,11 +6,11 @@ typedef vector<float, 3> float3;
 RWBuffer<float3> Buffer;
 
 // expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer}}
 RWBuffer BufferErr1;
 
 // expected-error@+2 {{too few template arguments for class template 'RWBuffer'}}
-// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer}}
 RWBuffer<> BufferErr2;
 
 [numthreads(1,1,1)]
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index 6c56f99f503df4..765cbbf3b04bcf 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1608,7 +1608,7 @@ writePrettyPrintFunction(const Record &R,
       Prefix = "[";
       Suffix = "]";
     } else if (Variety == "Keyword") {
-      Prefix = " ";
+      Prefix = "";
       Suffix = "";
     } else if (Variety == "Pragma") {
       Prefix = "#pragma ";

vgvassilev · 2024-04-13T06:46:04Z

clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl

@@ -6,11 +6,11 @@ typedef vector<float, 3> float3;
 RWBuffer<float3> Buffer;

 // expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer}}


We remove the final keyword here as it is modeled as an attribute and HLSL calls FinalAttr::CreateImplicit to constrain the users from inheriting from the builtin classes. We do not print implicit attributes. This used to work because we double printed final once as an attribute and once as part of the regular printing of CXXRecordDecls...

kimgr · 2024-04-13T09:45:44Z

Thanks!

Could you add this DeclPrinterTest unittest for regression?

TEST(DeclPrinter, TestTemplateFinal) {
  ASSERT_TRUE(PrintedDeclCXX11Matches(
    "template<typename T>"
    "class FinalTemplate final {};",
    classTemplateDecl(hasName("FinalTemplate")).bind("id"),
    "template <typename T> class FinalTemplate final {}"));
}

This is my expectation for correct output, but the current branch seems to disagree -- now there's two spaces between final and {}. Not sure if that's a problem. IWYU has enough post-processing that it doesn't break us either way.

EDIT: Hmm, I see your -ast-print test does not have double space, so I guess it has something to do with the printing policy. Here's what I've seen so far:

With .PolishForDeclaration=true, there are NO final specifiers (which is what we want to produce forward decls in IWYU)
In the DeclPrinter unittests I guess the default policy applies; they print ... X final {} with double space after final
In your end-to-end test, there are no double spaces, so I guess the compiler sets up a printing policy that masks it?

kimgr · 2024-04-13T12:04:02Z

With .PolishForDeclaration=true, there are NO final specifiers (which is what we want to produce forward decls in IWYU)

This is actually a regression in this PR, and it breaks the clangd test added here: 9f57b65 (the patch that originally led to double finals).

EDIT: It looks like prettyPrintAttributes is only intended for semantic attributes, whereas clangd wanted to preserve as-written final keyword. So I wonder if this change needs to be tweaked a little: https://github.com/llvm/llvm-project/pull/88600/files#diff-81d69bc555945d6582a758e0c094ff870cbc38697501ad7415694ee30c567dbfL1085

vgvassilev · 2024-04-13T13:32:36Z

With .PolishForDeclaration=true, there are NO final specifiers (which is what we want to produce forward decls in IWYU)

This is actually a regression in this PR, and it breaks the clangd test added here: 9f57b65 (the patch that originally led to double finals).

EDIT: It looks like prettyPrintAttributes is only intended for semantic attributes, whereas clangd wanted to preserve as-written final keyword. So I wonder if this change needs to be tweaked a little: https://github.com/llvm/llvm-project/pull/88600/files#diff-81d69bc555945d6582a758e0c094ff870cbc38697501ad7415694ee30c567dbfL1085

I've added a fix for your example. Can you provide a test case for the clangd use-case?

kimgr · 2024-04-13T14:02:44Z

Thank you.

I believe this should cover both cases, added some attempt at rationale in comments:

diff --git a/clang/unittests/AST/DeclPrinterTest.cpp b/clang/unittests/AST/DeclPrinterTest.cpp
index f2b027a25621..8691a6c38f16 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -86,16 +86,13 @@ PrintedDeclCXX98Matches(StringRef Code, const DeclarationMatcher &NodeMatch,
                             ExpectedPrinted, "input.cc");
 }
 
-::testing::AssertionResult PrintedDeclCXX11Matches(
-                                  StringRef Code,
-                                  const DeclarationMatcher &NodeMatch,
-                                  StringRef ExpectedPrinted) {
+::testing::AssertionResult
+PrintedDeclCXX11Matches(StringRef Code, const DeclarationMatcher &NodeMatch,
+                        StringRef ExpectedPrinted,
+                        PrintingPolicyAdjuster PolicyModifier = nullptr) {
   std::vector<std::string> Args(1, "-std=c++11");
-  return PrintedDeclMatches(Code,
-                            Args,
-                            NodeMatch,
-                            ExpectedPrinted,
-                            "input.cc");
+  return PrintedDeclMatches(Code, Args, NodeMatch, ExpectedPrinted, "input.cc",
+                            PolicyModifier);
 }
 
 ::testing::AssertionResult PrintedDeclCXX11nonMSCMatches(
@@ -1556,3 +1553,25 @@ TEST(DeclPrinter, VarDeclWithInitializer) {
       PrintedDeclCXX17Matches("void foo() {int arr[42]; for(int a : arr);}",
                               namedDecl(hasName("a")).bind("id"), "int a"));
 }
+
+TEST(DeclPrinter, TestTemplateFinal) {
+  // By default we should print 'final' keyword whether class is implicitly or
+  // explicitly marked final.
+  ASSERT_TRUE(PrintedDeclCXX11Matches(
+      "template<typename T>\n"
+      "class FinalTemplate final {};",
+      classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+      "template <typename T> class FinalTemplate final {}"));
+}
+
+TEST(DeclPrinter, TestTemplateFinalWithPolishForDecl) {
+  // clangd relies on the 'final' keyword being printed when
+  // PolishForDeclaration is enabled, so make sure it is even if implicit attrs
+  // are disabled.
+  ASSERT_TRUE(PrintedDeclCXX11Matches(
+      "template<typename T>\n"
+      "class FinalTemplate final {};",
+      classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+      "template <typename T> class FinalTemplate final {}",
+      [](PrintingPolicy &Policy) { Policy.PolishForDeclaration = true; }));
+}

vgvassilev · 2024-04-13T14:05:48Z

IIUC, the PolishForDeclaration option is supposed to When true, do certain refinement needed for producing proper declaration tag; such as, do not print attributes attached to the declaration. . If the intent is to produce a forward declaration the final keyword cannot be attached to a forward declaration. So I am not sure what's the "right" fix here...

vgvassilev · 2024-04-13T14:38:44Z

I've put a fix that fixes the cases that you mentioned...

Fixes llvm#56517.

kimgr · 2024-04-13T15:21:09Z

If the intent is to produce a forward declaration the final keyword cannot be attached to a forward declaration. So I am not sure what's the "right" fix here...

I don't believe that's the intent of DeclPrinter or PolishForDeclaration --

Clangd uses PolishForDeclaration to print an informational "hover text" for the declaration (and I guess that's why they want to include final -- it's something that's good for an interactive user to know about the decl)
IWYU uses PolishForDeclaration to get a valid declaration, and then does some very hacky heuristic post-processing to turn it into a forward-decl.

Sorry for stirring confusion into this, my primary focus is IWYU, but I remember that the original double-final came from a change intended for Clangd.

Hopefully this helps clarify.

kimgr · 2024-04-13T15:24:02Z

clang/lib/AST/DeclPrinter.cpp


  if (D->hasAttrs()) {
    const AttrVec &Attrs = D->getAttrs();
    for (auto *A : Attrs) {
      if (A->isInherited() || A->isImplicit())
        continue;
+      // Don't strip out the keyword attributes, they aren't regular attributes.
+      if (Policy.PolishForDeclaration && !A->isKeywordAttribute())


Nice, I was about to suggest something like this, but didn't know isKeywordAttribute existed.

vgvassilev · 2024-04-13T15:26:58Z

IWYU uses PolishForDeclaration to get a valid declaration, and then does some very hacky heuristic post-processing to turn it into a forward-decl.

The ROOT usecase of the Cling interpreter has similar infrastructure and it works at scale couple of million lines of scientific codes: https://github.com/root-project/root/blob/master/interpreter/cling/lib/Interpreter/ForwardDeclPrinter.cpp. There are some tests that one can look at: https://github.com/root-project/root/tree/master/interpreter/cling/test/Autoloading

I am wondering if that'd be interesting and if so, maybe we can share it between both projects via upstreaming to Clang...

kimgr · 2024-04-13T15:47:17Z

Current PR passes all my tests, both Clang (ninja ASTTests), Clangd (ninja ClangdTests) and IWYU end-to-end tests -- thanks!

kimgr · 2024-04-13T15:56:13Z

I am wondering if that'd be interesting and if so, maybe we can share it between both projects via upstreaming to Clang...

That sounds fantastic, but mostly because I don't have anything to offer, and everything to benefit :)

I was just thinking about adding a .ForwardDeclaration policy to DeclPrinter this morning -- the current PolishForDeclaration output is so close. But it seemed weird to add a mode for which IWYU (a non-LLVM project) was the only user.

I suspect your ForwardDeclarePrinter is more principled. I guess it might be hard to coordinate forward-decl style between users, but ultimately that's just printing policy (or chaining libFormat, or something).

Let's maybe continue this discussion somewhere else?

erichkeane

2 nits, else LGTM. Once you and @kimgr are in agreement with what to do next (and have looked into my nits), feel free to commit.

erichkeane · 2024-04-15T13:59:24Z

clang/lib/AST/DeclPrinter.cpp


  if (D->hasAttrs()) {
    const AttrVec &Attrs = D->getAttrs();
    for (auto *A : Attrs) {
      if (A->isInherited() || A->isImplicit())
        continue;
+      // Don't strip out the keyword attributes, they aren't regular attributes.


Suggested change

// Don't strip out the keyword attributes, they aren't regular attributes.

// Don't print out the keyword attributes, they aren't regular attributes.

??

We should word it as Print out the keyword attributes, they aren't regular attributes., I think.

erichkeane · 2024-04-15T14:01:47Z

clang/lib/AST/DeclPrinter.cpp

          if (Pos == AttrPosAsWritten::Left)
            Out << ' ';
        }
        break;
      }
    }
  }
+  return hasPrinted;


see Out->tell() here, it is perhaps less error-prone here (that is, store the uint64_t value above, and see if it changed).

I was thinking to use something like that but I believe the boolean flag is more readable. I'd like to keep it the way it is.

kimgr · 2024-04-18T06:59:43Z

@vgvassilev Thank you!

vgvassilev requested review from AaronBallman and erichkeane April 13, 2024 06:42

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Apr 13, 2024

vgvassilev mentioned this pull request Apr 13, 2024

Rework the printing of attributes #87281

Merged

vgvassilev force-pushed the fix-attr-print-final branch from fe26228 to b43c52e Compare April 13, 2024 06:44

vgvassilev commented Apr 13, 2024

View reviewed changes

vgvassilev force-pushed the fix-attr-print-final branch from b43c52e to bcd04db Compare April 13, 2024 13:32

vgvassilev force-pushed the fix-attr-print-final branch from bcd04db to c68344d Compare April 13, 2024 14:38

vgvassilev force-pushed the fix-attr-print-final branch from c68344d to cb3da95 Compare April 13, 2024 14:50

Fix the double space and double attribute printing of the final keyword.

9b2bb90

Fixes llvm#56517.

vgvassilev force-pushed the fix-attr-print-final branch from cb3da95 to 9b2bb90 Compare April 13, 2024 14:50

kimgr reviewed Apr 13, 2024

View reviewed changes

erichkeane approved these changes Apr 15, 2024

View reviewed changes

Address more review comments

2a54307

vgvassilev merged commit 71c0784 into llvm:main Apr 18, 2024
4 checks passed

vgvassilev deleted the fix-attr-print-final branch April 18, 2024 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the double space and double attribute printing of the final keyword. #88600

Fix the double space and double attribute printing of the final keyword. #88600

vgvassilev commented Apr 13, 2024

llvmbot commented Apr 13, 2024 •

edited

vgvassilev Apr 13, 2024

kimgr commented Apr 13, 2024 •

edited

kimgr commented Apr 13, 2024 •

edited

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

vgvassilev commented Apr 13, 2024

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

kimgr Apr 13, 2024

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

kimgr commented Apr 13, 2024

erichkeane left a comment

erichkeane Apr 15, 2024

vgvassilev Apr 15, 2024

erichkeane Apr 15, 2024

vgvassilev Apr 17, 2024

kimgr commented Apr 18, 2024

	// Don't strip out the keyword attributes, they aren't regular attributes.
	// Don't print out the keyword attributes, they aren't regular attributes.

Fix the double space and double attribute printing of the final keyword. #88600

Fix the double space and double attribute printing of the final keyword. #88600

Conversation

vgvassilev commented Apr 13, 2024

llvmbot commented Apr 13, 2024 • edited

vgvassilev Apr 13, 2024

Choose a reason for hiding this comment

kimgr commented Apr 13, 2024 • edited

kimgr commented Apr 13, 2024 • edited

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

vgvassilev commented Apr 13, 2024

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

kimgr Apr 13, 2024

Choose a reason for hiding this comment

vgvassilev commented Apr 13, 2024

kimgr commented Apr 13, 2024

kimgr commented Apr 13, 2024

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Apr 15, 2024

Choose a reason for hiding this comment

vgvassilev Apr 15, 2024

Choose a reason for hiding this comment

erichkeane Apr 15, 2024

Choose a reason for hiding this comment

vgvassilev Apr 17, 2024

Choose a reason for hiding this comment

kimgr commented Apr 18, 2024

llvmbot commented Apr 13, 2024 •

edited

kimgr commented Apr 13, 2024 •

edited

kimgr commented Apr 13, 2024 •

edited