Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clangd] Show definition of underlying struct when hovering over a typedef #89570

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HighCommander4
Copy link
Collaborator

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 22, 2024

@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Fixes clangd/clangd#2020


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

4 Files Affected:

  • (modified) clang-tools-extra/clangd/Hover.cpp (+12)
  • (modified) clang-tools-extra/clangd/unittests/HoverTests.cpp (+28-5)
  • (modified) clang/include/clang/AST/PrettyPrinter.h (+9-1)
  • (modified) clang/lib/AST/DeclPrinter.cpp (+17-3)
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 06b949bc4a2b55..598c2a68b7bf16 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -69,6 +69,8 @@ namespace {
 PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
   Base.AnonymousTagLocations = false;
   Base.TerseOutput = true;
+  // Show struct fields and enum members.
+  Base.PrintTagTypeContents = true;
   Base.PolishForDeclaration = true;
   Base.ConstantsAsWritten = true;
   Base.SuppressTemplateArgsInCXXConstructors = true;
@@ -150,6 +152,16 @@ std::string printDefinition(const Decl *D, PrintingPolicy PP,
   std::string Definition;
   llvm::raw_string_ostream OS(Definition);
   D->print(OS, PP);
+  
+  // For a typedef to a TagDecl, also print the underlying TagDecl
+  if (const auto *TND = dyn_cast<TypedefNameDecl>(D)) {
+    if (const auto *TT = dyn_cast<TagType>(
+            TND->getUnderlyingType().getCanonicalType().getTypePtr())) {
+      OS << ";\n";
+      TT->getDecl()->print(OS, PP);
+    }
+  }
+
   OS.flush();
   return Definition;
 }
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 35db757b9c15b5..9657869b1b7641 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1645,7 +1645,16 @@ TEST(Hover, All) {
       {
           R"cpp(// Struct
             namespace ns1 {
-              struct MyClass {};
+              struct MyClass {
+                // Public fields shown in hover
+                int field1;
+                int field2;
+
+                // Methods and private fields not shown
+                void method();
+              private:
+                bool private_field;
+              };
             } // namespace ns1
             int main() {
               ns1::[[My^Class]]* Params;
@@ -1655,8 +1664,22 @@ TEST(Hover, All) {
             HI.Name = "MyClass";
             HI.Kind = index::SymbolKind::Struct;
             HI.NamespaceScope = "ns1::";
-            HI.Definition = "struct MyClass {}";
+            HI.Definition = "struct MyClass {\n  int field1;\n  int field2;\n}";
           }},
+          {
+            R"cpp(// Typedef to struct
+              struct Point { int x; int y; };
+              typedef Point TPoint;
+              [[TP^oint]] tp;
+            )cpp",
+            [](HoverInfo &HI) {
+              HI.Name = "TPoint";
+              HI.Kind = index::SymbolKind::TypeAlias;
+              HI.NamespaceScope = "";
+              HI.Type = "struct Point";
+              HI.Definition = "typedef Point TPoint;\nstruct Point {\n  int x;\n  int y;\n}";
+            }
+          },
       {
           R"cpp(// Class
             namespace ns1 {
@@ -1685,7 +1708,7 @@ TEST(Hover, All) {
             HI.Name = "MyUnion";
             HI.Kind = index::SymbolKind::Union;
             HI.NamespaceScope = "ns1::";
-            HI.Definition = "union MyUnion {}";
+            HI.Definition = "union MyUnion {\n  int x;\n  int y;\n}";
           }},
       {
           R"cpp(// Function definition via pointer
@@ -1880,7 +1903,7 @@ TEST(Hover, All) {
             HI.Name = "Foo";
             HI.Kind = index::SymbolKind::TypeAlias;
             HI.NamespaceScope = "";
-            HI.Definition = "typedef struct Bar Foo";
+            HI.Definition = "typedef struct Bar Foo;\nstruct Bar {}";
             HI.Type = "struct Bar";
             HI.Documentation = "Typedef with embedded definition";
           }},
@@ -2030,7 +2053,7 @@ TEST(Hover, All) {
             HI.Name = "Hello";
             HI.Kind = index::SymbolKind::Enum;
             HI.NamespaceScope = "";
-            HI.Definition = "enum Hello {}";
+            HI.Definition = "enum Hello { ONE, TWO, THREE }";
             HI.Documentation = "Enum declaration";
           }},
       {
diff --git a/clang/include/clang/AST/PrettyPrinter.h b/clang/include/clang/AST/PrettyPrinter.h
index da276e26049b00..6b8f1ae9143df3 100644
--- a/clang/include/clang/AST/PrettyPrinter.h
+++ b/clang/include/clang/AST/PrettyPrinter.h
@@ -70,7 +70,7 @@ struct PrintingPolicy {
         Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
         UseVoidForZeroParams(!LO.CPlusPlus),
         SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
-        PolishForDeclaration(false), Half(LO.Half),
+        PrintTagTypeContents(false), PolishForDeclaration(false), Half(LO.Half),
         MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
         MSVCFormatting(false), ConstantsAsWritten(false),
         SuppressImplicitBase(false), FullyQualifiedName(false),
@@ -252,6 +252,14 @@ struct PrintingPolicy {
   LLVM_PREFERRED_TYPE(bool)
   unsigned TerseOutput : 1;
 
+  /// Print the contents of tag (i.e. record and enum) types, even with
+  /// TerseOutput=true.
+  ///
+  /// For record types (structs/classes/unions), this only prints public
+  /// data members. For enum types, this prints enumerators.
+  /// Has no effect if TerseOutput=false (which prints all members).
+  unsigned PrintTagTypeContents : 1;
+
   /// When true, do certain refinement needed for producing proper declaration
   /// tag; such as, do not print attributes attached to the declaration.
   ///
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 43d221968ea3fb..3fe02fff687fe1 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Basic/Module.h"
+#include "clang/Basic/Specifiers.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 
@@ -464,8 +465,10 @@ void DeclPrinter::PrintConstructorInitializers(CXXConstructorDecl *CDecl,
 //----------------------------------------------------------------------------
 
 void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
-  if (Policy.TerseOutput)
-    return;
+  if (Policy.TerseOutput) {
+    if (!Policy.PrintTagTypeContents || !isa<TagDecl>(DC))
+      return;
+  }
 
   if (Indent)
     Indentation += Policy.Indentation;
@@ -474,6 +477,17 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
   for (DeclContext::decl_iterator D = DC->decls_begin(), DEnd = DC->decls_end();
        D != DEnd; ++D) {
 
+    // Print enum members and public struct fields when
+    // PrintTagTypeContents=true. Only applicable when TerseOutput=true since
+    // otherwise all members are printed.
+    if (Policy.TerseOutput) {
+      assert(Policy.PrintTagTypeContents);
+      if (!(isa<EnumConstantDecl>(*D) ||
+            (isa<FieldDecl>(*D) &&
+             dyn_cast<FieldDecl>(*D)->getAccess() == AS_public)))
+        continue;
+    }
+
     // Don't print ObjCIvarDecls, as they are printed when visiting the
     // containing ObjCInterfaceDecl.
     if (isa<ObjCIvarDecl>(*D))
@@ -1182,7 +1196,7 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
 
     // Print the class definition
     // FIXME: Doesn't print access specifiers, e.g., "public:"
-    if (Policy.TerseOutput) {
+    if (Policy.TerseOutput && !Policy.PrintTagTypeContents) {
       Out << " {}";
     } else {
       Out << " {\n";

@HighCommander4
Copy link
Collaborator Author

(This builds on #89557 and the commit view shows both commits. I still need to figure out how to do proper stacked PRs...)

@HighCommander4 HighCommander4 changed the title [clangd] Show struct members when hovering over a typedef [clangd] Show definition of underlying struct when hovering over a typedef Apr 25, 2024
@HighCommander4
Copy link
Collaborator Author

Updated patch with the following changes:

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 9617da88ab961145047076c45bb2bb1ac4513634 44aba390954c7b551ed7102e8e7b4209207c0d87 -- clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/unittests/HoverTests.cpp
View the diff from clang-format here.
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 582a311f35..fbef81f781 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1657,35 +1657,34 @@ TEST(Hover, All) {
             HI.NamespaceScope = "ns1::";
             HI.Definition = "struct MyClass {}";
           }},
-          {
-            R"cpp(// Typedef to struct
+      {
+          R"cpp(// Typedef to struct
               struct Point { int x; int y; };
               typedef Point TPoint;
               [[TP^oint]] tp;
             )cpp",
-            [](HoverInfo &HI) {
-              HI.Name = "TPoint";
-              HI.Kind = index::SymbolKind::TypeAlias;
-              HI.NamespaceScope = "";
-              HI.Type = "struct Point";
-              HI.Definition = "typedef Point TPoint;\nstruct Point {}";
-            }
-          },
-          {
-            R"cpp(// Two layers of typedef
+          [](HoverInfo &HI) {
+            HI.Name = "TPoint";
+            HI.Kind = index::SymbolKind::TypeAlias;
+            HI.NamespaceScope = "";
+            HI.Type = "struct Point";
+            HI.Definition = "typedef Point TPoint;\nstruct Point {}";
+          }},
+      {
+          R"cpp(// Two layers of typedef
               struct Point { int x; int y; };
               typedef Point TPoint;
               typedef TPoint TTPoint;
               [[TTP^oint]] tp;
             )cpp",
-            [](HoverInfo &HI) {
-              HI.Name = "TTPoint";
-              HI.Kind = index::SymbolKind::TypeAlias;
-              HI.NamespaceScope = "";
-              HI.Type = "struct Point";
-              HI.Definition = "typedef TPoint TTPoint;\ntypedef Point TPoint;\nstruct Point {}";
-            }
-          },
+          [](HoverInfo &HI) {
+            HI.Name = "TTPoint";
+            HI.Kind = index::SymbolKind::TypeAlias;
+            HI.NamespaceScope = "";
+            HI.Type = "struct Point";
+            HI.Definition = "typedef TPoint TTPoint;\ntypedef Point "
+                            "TPoint;\nstruct Point {}";
+          }},
       {
           R"cpp(// Class
             namespace ns1 {
@@ -3158,35 +3157,34 @@ TEST(Hover, CLanguage) {
     const char *const Code;
     const std::function<void(HoverInfo &)> ExpectedBuilder;
   } Cases[] = {
-    {
-      R"cpp(// Typedef to struct
+      {
+          R"cpp(// Typedef to struct
         struct Point { int x; int y; };
         typedef struct Point TPoint;
         [[TP^oint]] tp;
       )cpp",
-      [](HoverInfo &HI) {
-        HI.Name = "TPoint";
-        HI.Kind = index::SymbolKind::TypeAlias;
-        HI.NamespaceScope = "";
-        HI.Type = "struct Point";
-        HI.Definition = "typedef struct Point TPoint;\nstruct Point {}";
-      }
-    },
-    {
-      R"cpp(// Two layers of typedef
+          [](HoverInfo &HI) {
+            HI.Name = "TPoint";
+            HI.Kind = index::SymbolKind::TypeAlias;
+            HI.NamespaceScope = "";
+            HI.Type = "struct Point";
+            HI.Definition = "typedef struct Point TPoint;\nstruct Point {}";
+          }},
+      {
+          R"cpp(// Two layers of typedef
         struct Point { int x; int y; };
         typedef struct Point TPoint;
         typedef TPoint TTPoint;
         [[TTP^oint]] tp;
       )cpp",
-      [](HoverInfo &HI) {
-        HI.Name = "TTPoint";
-        HI.Kind = index::SymbolKind::TypeAlias;
-        HI.NamespaceScope = "";
-        HI.Type = "struct Point";
-        HI.Definition = "typedef TPoint TTPoint;\ntypedef struct Point TPoint;\nstruct Point {}";
-      }
-    },
+          [](HoverInfo &HI) {
+            HI.Name = "TTPoint";
+            HI.Kind = index::SymbolKind::TypeAlias;
+            HI.NamespaceScope = "";
+            HI.Type = "struct Point";
+            HI.Definition = "typedef TPoint TTPoint;\ntypedef struct Point "
+                            "TPoint;\nstruct Point {}";
+          }},
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Code);
@@ -4117,7 +4115,8 @@ TEST(Hover, Typedefs) {
 
   ASSERT_TRUE(H && H->Type);
   EXPECT_EQ(H->Type->Type, "int");
-  EXPECT_EQ(H->Definition, "using foo = type<true, int, double>;\nusing type = int");
+  EXPECT_EQ(H->Definition,
+            "using foo = type<true, int, double>;\nusing type = int");
 }
 
 TEST(Hover, EvaluateMacros) {

Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! One thought from me.


QualType Type = TND->getUnderlyingType();
while (PrintDeclForType(Type)) {
QualType Desugared = Type->getLocallyUnqualifiedSingleStepDesugaredType();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought: what do you think of resolving pointer types as well? I could envision cases where pointers are involved in an alias type e.g.

typedef struct _PROCESS_INFORMATION {
  HANDLE hProcess;
  HANDLE hThread;
  DWORD  dwProcessId;
  DWORD  dwThreadId;
} PROCESS_INFORMATION, *PPROCESS_INFORMATION, *LPPROCESS_INFORMATION;

(excerpted from PROCESS_INFORMATION)
People may want to peek at the definition of the struct while hovering over LPPROCESS_INFORMATION too.

@zyn0217
Copy link
Contributor

zyn0217 commented Apr 27, 2024

(I still need to figure out how to do proper stacked PRs...)

AFAIK, not possible unless our release manager installs e.g. such an application to the repository; but candidly, I have never used it before so I have no idea what kind of shape that would be.

This issue is also being tracked at #56636, in case you don't know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category clang-tools-extra clangd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show definition of underlying struct when hovering over a typedef
3 participants