Skip to content

[AST] Only dump desugared type when visibly different #65214

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 26, 2023
Merged

[AST] Only dump desugared type when visibly different #65214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/docs/HowToSetupToolingForLLVM.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ Examples:
clang::ASTConsumer *newASTConsumer() (CompoundStmt 0x44da290 </home/alexfh/local/llvm/tools/clang/tools/clang-check/ClangCheck.cpp:64:40, line:72:3>
(IfStmt 0x44d97c8 <line:65:5, line:66:45>
<<<NULL>>>
(ImplicitCastExpr 0x44d96d0 <line:65:9> '_Bool':'_Bool' <UserDefinedConversion>
(ImplicitCastExpr 0x44d96d0 <line:65:9> '_Bool' <UserDefinedConversion>
...
$ clang-check tools/clang/tools/clang-check/ClangCheck.cpp -ast-print -ast-dump-filter ActionFactory::newASTConsumer
Processing: tools/clang/tools/clang-check/ClangCheck.cpp.
Expand Down
22 changes: 22 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,28 @@ ABI Changes in This Version
- Following the SystemV ABI for x86-64, ``__int128`` arguments will no longer
be split between a register and a stack slot.

AST Dumping Potentially Breaking Changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fantastic release note!!

----------------------------------------
- When dumping a sugared type, Clang will no longer print the desugared type if
its textual representation is the same as the sugared one. This applies to
both text dumps of the form ``'foo':'foo'`` which will now be dumped as just
``'foo'``, and JSON dumps of the form:

.. code-block:: json

"type": {
"qualType": "foo",
"desugaredQualType": "foo"
}

which will now be dumped as just:

.. code-block:: json

"type": {
"qualType": "foo"
}

What's New in Clang |release|?
==============================
Some of the major new features and improvements to Clang are listed
Expand Down
10 changes: 7 additions & 3 deletions clang/lib/AST/JSONNodeDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,12 +315,16 @@ std::string JSONNodeDumper::createPointerRepresentation(const void *Ptr) {

llvm::json::Object JSONNodeDumper::createQualType(QualType QT, bool Desugar) {
SplitQualType SQT = QT.split();
llvm::json::Object Ret{{"qualType", QualType::getAsString(SQT, PrintPolicy)}};
std::string SQTS = QualType::getAsString(SQT, PrintPolicy);
llvm::json::Object Ret{{"qualType", SQTS}};

if (Desugar && !QT.isNull()) {
SplitQualType DSQT = QT.getSplitDesugaredType();
if (DSQT != SQT)
Ret["desugaredQualType"] = QualType::getAsString(DSQT, PrintPolicy);
if (DSQT != SQT) {
std::string DSQTS = QualType::getAsString(DSQT, PrintPolicy);
if (DSQTS != SQTS)
Ret["desugaredQualType"] = DSQTS;
}
Comment on lines +323 to +327
Copy link
Contributor

Choose a reason for hiding this comment

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

You could make the argument that it make sense for json to have both.
Not having it slightly complicate the logic of consumer code.

I think we need to decide to either:

  • keep it
  • document the change in a ReleaseNotes entry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the issue for consumer code? It already needs to handle non-sugar types. And it's not like the type in the JSON is a structured representation of the QualType, it's just a textual dump, so you'd have to go and re-parse it to do anything with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The aim with the original code was "if the desugared type and the given qual type are the same thing, only print one if we've been asked to print desugared", and this keeps that same aim, so I think this is reasonable.

That said, I agree with @cor3ntin that this needs a release note because there is a change in behavior that may break downstream consumers who expect the desugaredQualType field that's no longer going to show up for them. I don't expect it to be significantly disruptive in practice, but we should explicitly communicate that sort of change to users.

if (const auto *TT = QT->getAs<TypedefType>())
Ret["typeAliasDeclId"] = createPointerRepresentation(TT->getDecl());
}
Expand Down
13 changes: 9 additions & 4 deletions clang/lib/AST/TextNodeDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,13 +692,18 @@ void TextNodeDumper::dumpBareType(QualType T, bool Desugar) {
ColorScope Color(OS, ShowColors, TypeColor);

SplitQualType T_split = T.split();
OS << "'" << QualType::getAsString(T_split, PrintPolicy) << "'";
std::string T_str = QualType::getAsString(T_split, PrintPolicy);
OS << "'" << T_str << "'";

if (Desugar && !T.isNull()) {
// If the type is sugared, also dump a (shallow) desugared type.
// If the type is sugared, also dump a (shallow) desugared type when
// it is visibly different.
SplitQualType D_split = T.getSplitDesugaredType();
if (T_split != D_split)
OS << ":'" << QualType::getAsString(D_split, PrintPolicy) << "'";
if (T_split != D_split) {
std::string D_str = QualType::getAsString(D_split, PrintPolicy);
if (T_str != D_str)
OS << ":'" << QualType::getAsString(D_split, PrintPolicy) << "'";
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions clang/test/AST/HLSL/this-reference-template.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ void main() {
// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} <line:8:3, line:10:3> line:8:5 used getFirst 'int ()' implicit_instantiation implicit-inline
// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}} <col:16, line:10:3>
// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}} <line:9:4, col:16>
// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int':'int' <LValueToRValue>
// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int':'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int' <LValueToRValue>
// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:11> 'Pair<int, float>' lvalue this
// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} <line:12:3, line:14:3> line:12:5 used getSecond 'float ()' implicit_instantiation implicit-inline
// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}} <col:17, line:14:3>
// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}} <line:13:5, col:12>
// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} <col:12> 'float':'float' <LValueToRValue>
// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:12> 'float':'float' lvalue .Second 0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} <col:12> 'float' <LValueToRValue>
// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:12> 'float' lvalue .Second 0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:12> 'Pair<int, float>' lvalue implicit this
10 changes: 5 additions & 5 deletions clang/test/AST/ast-dump-APValue-anon-union.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,23 @@ union U1 {

void Test() {
constexpr S0 s0{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s0 'const S0':'const S0' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s0 'const S0' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | `-field: Union .i Int 42

constexpr U0 u0a{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0a 'const U0':'const U0' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0a 'const U0' constexpr listinit
// CHECK-NEXT: | |-value: Union None

constexpr U0 u0b{3.1415f};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0b 'const U0':'const U0' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0b 'const U0' constexpr listinit
// CHECK-NEXT: | |-value: Union .U0::(anonymous union at {{.*}}) Union .f Float 3.141500e+00

constexpr U1 u1a{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1a 'const U1':'const U1' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1a 'const U1' constexpr listinit
// CHECK-NEXT: | |-value: Union .U1::(anonymous union at {{.*}}) Union .f Float 0.000000e+00

constexpr U1 u1b{3.1415f};
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1b 'const U1':'const U1' constexpr listinit
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1b 'const U1' constexpr listinit
// CHECK-NEXT: |-value: Union .U1::(anonymous union at {{.*}}) Union .f Float 3.141500e+00
}
12 changes: 6 additions & 6 deletions clang/test/AST/ast-dump-APValue-struct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,25 @@ struct S5 : S4 {

void Test() {
constexpr S0 s0{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s0 'const S0':'const S0' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s0 'const S0' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | `-fields: Int 0, Union .j Int 0

constexpr S1 s1{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s1 'const S1':'const S1' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s1 'const S1' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | |-field: Int 0
// CHECK-NEXT: | | `-field: Union .s
// CHECK-NEXT: | | `-Struct
// CHECK-NEXT: | | `-field: Int 0

constexpr S2 s2{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s2 'const S2':'const S2' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s2 'const S2' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | `-fields: Int 0, Union .u Union .j Int 0

constexpr S3 s3{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s3 'const S3':'const S3' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s3 'const S3' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | |-field: Int 0
// CHECK-NEXT: | | `-field: Union .u
Expand All @@ -87,7 +87,7 @@ void Test() {
// CHECK-NEXT: | | `-field: Int 0

constexpr S4 s4{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s4 'const S4':'const S4' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s4 'const S4' constexpr listinit
// CHECK-NEXT: | |-value: Struct
// CHECK-NEXT: | | |-base: Struct
// CHECK-NEXT: | | | `-fields: Int 0, Union .j Int 0
Expand All @@ -96,7 +96,7 @@ void Test() {
// CHECK-NEXT: | | `-fields: Int 4, Int 5, Int 6

constexpr S5 s5{};
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s5 'const S5':'const S5' constexpr listinit
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s5 'const S5' constexpr listinit
// CHECK-NEXT: |-value: Struct
// CHECK-NEXT: | |-base: Struct
// CHECK-NEXT: | | |-base: Struct
Expand Down
10 changes: 5 additions & 5 deletions clang/test/AST/ast-dump-APValue-union.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,25 @@ union U3 {

void Test() {
constexpr U0 u0{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0 'const U0':'const U0' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u0 'const U0' constexpr listinit
// CHECK-NEXT: | |-value: Union .i Int 42

constexpr U1 u1{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1 'const U1':'const U1' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u1 'const U1' constexpr listinit
// CHECK-NEXT: | |-value: Union .uinner Union .f Float 3.141500e+00

constexpr U2 u2{};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u2 'const U2':'const U2' constexpr listinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u2 'const U2' constexpr listinit
// CHECK-NEXT: | |-value: Union .uinner
// CHECK-NEXT: | | `-Union .arr
// CHECK-NEXT: | | `-Array size=2
// CHECK-NEXT: | | `-elements: Int 1, Int 2

constexpr U3 u3a = {.f = 3.1415};
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u3a 'const U3':'const U3' constexpr cinit
// CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u3a 'const U3' constexpr cinit
// CHECK-NEXT: | |-value: Union .f Float 3.141500e+00

constexpr U3 u3b = {.uinner = {}};
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u3b 'const U3':'const U3' constexpr cinit
// CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} u3b 'const U3' constexpr cinit
// CHECK-NEXT: |-value: Union .uinner Union .d Float 3.141500e+00
}
12 changes: 6 additions & 6 deletions clang/test/AST/ast-dump-attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,17 @@ struct C { char a[16]; };
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct my_union
// CHECK: CXXRecordDecl {{.*}} implicit struct my_union
// CHECK: FieldDecl {{.*}} buffer 'char[1024]'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::A':'TestAligns::A'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::B':'TestAligns::B'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::C':'TestAligns::C'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::A'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::B'
// CHECK-NEXT: AlignedAttr {{.*}} alignas 'TestAligns::C'
my_union<A, B, C> my_union_val;

// CHECK: ClassTemplateSpecializationDecl {{.*}} struct my_union2
// CHECK: CXXRecordDecl {{.*}} implicit struct my_union2
// CHECK: FieldDecl {{.*}} buffer 'char[1024]'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::A':'TestAligns::A'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::B':'TestAligns::B'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::C':'TestAligns::C'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::A'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::B'
// CHECK-NEXT: AlignedAttr {{.*}} _Alignas 'TestAligns::C'
my_union2<A, B, C> my_union2_val;

} // namespace TestAligns
Expand Down
4 changes: 0 additions & 4 deletions clang/test/AST/ast-dump-decl-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1351,7 +1351,6 @@ void testParmVarDecl(int TestParmVarDecl);
// CHECK-NEXT: "isUsed": true,
// CHECK-NEXT: "name": "x",
// CHECK-NEXT: "type": {
// CHECK-NEXT: "desugaredQualType": "enum Enum",
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, this is an example of why we need the release note; the information didn't change, it flat-out disappeared which could catch some folks by surprise.

// CHECK-NEXT: "qualType": "enum Enum"
// CHECK-NEXT: }
// CHECK-NEXT: },
Expand Down Expand Up @@ -1424,7 +1423,6 @@ void testParmVarDecl(int TestParmVarDecl);
// CHECK-NEXT: }
// CHECK-NEXT: },
// CHECK-NEXT: "type": {
// CHECK-NEXT: "desugaredQualType": "enum Enum",
// CHECK-NEXT: "qualType": "enum Enum"
// CHECK-NEXT: },
// CHECK-NEXT: "valueCategory": "prvalue",
Expand All @@ -1446,7 +1444,6 @@ void testParmVarDecl(int TestParmVarDecl);
// CHECK-NEXT: }
// CHECK-NEXT: },
// CHECK-NEXT: "type": {
// CHECK-NEXT: "desugaredQualType": "enum Enum",
// CHECK-NEXT: "qualType": "enum Enum"
// CHECK-NEXT: },
// CHECK-NEXT: "valueCategory": "lvalue",
Expand All @@ -1455,7 +1452,6 @@ void testParmVarDecl(int TestParmVarDecl);
// CHECK-NEXT: "kind": "ParmVarDecl",
// CHECK-NEXT: "name": "x",
// CHECK-NEXT: "type": {
// CHECK-NEXT: "desugaredQualType": "enum Enum",
// CHECK-NEXT: "qualType": "enum Enum"
// CHECK-NEXT: }
// CHECK-NEXT: }
Expand Down
1 change: 0 additions & 1 deletion clang/test/AST/ast-dump-decl-json.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,6 @@ void f(void) {
// CHECK-NEXT: },
// CHECK-NEXT: "name": "T",
// CHECK-NEXT: "type": {
// CHECK-NEXT: "desugaredQualType": "id",
// CHECK-NEXT: "qualType": "id",
// CHECK-NEXT: "typeAliasDeclId": "0x{{.*}}"
// CHECK-NEXT: }
Expand Down
Loading