[clang] Implement ElaboratedType sugaring for types written bare

[mizvekov]

Summary

Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.

The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.

An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.

---

Troubleshooting list to deal with any breakage seen with this patch:

1. The most likely effect one would see by this patch is a change in how a type is printed. The type printer will, by design and default, print types as written. There are customization options there, but not that many, and they mainly apply to how to print a type that we somehow failed to track how it was written. This patch fixes a problem where we failed to distinguish between a type that was written without any elaborated-type qualifiers, such as a 'struct'/'class' tags and name spacifiers such as 'std::', and one that has been stripped of any 'metadata' that identifies such, the so called canonical types. Example: ` namespace foo { struct A {}; A a; }; ` If one were to print the type of foo::a, prior to this patch, this would result in foo::A. This is how the type printer would have, by default, printed the canonical type of A as well. As soon as you add any name qualifiers to A, the type printer would suddenly start accurately printing the type as written. This patch will make it print it accurately even when written without qualifiers, so we will just print A for the initial example, as the user did not really write that foo:: namespace qualifier.

2. This patch could expose a bug in some AST matcher. Matching types is harder to get right when there is sugar involved. For example, if you want to match a type against being a pointer to some type A, then you have to account for getting a type that is sugar for a pointer to A, or being a pointer to sugar to A, or both! Usually you would get the second part wrong, and this would work for a very simple test where you don't use any name qualifiers, but you would discover is broken when you do. The usual fix is to either use the matcher which strips sugar, which is annoying to use as for example if you match an N level pointer, you have to put N+1 such matchers in there, beginning to end and between all those levels. But in a lot of cases, if the property you want to match is present in the canonical type, it's easier and faster to just match on that... This goes with what is said in 1), if you want to match against the name of a type, and you want the name string to be something stable, perhaps matching on the name of the canonical type is the better choice.

3. This patch could expose a bug in how you get the source range of some TypeLoc. For some reason, a lot of code is using getLocalSourceRange(), which only looks at the given TypeLoc node. This patch introduces a new, and more common TypeLoc node which contains no source locations on itself. This is not an inovation here, and some other, more rare TypeLoc nodes could also have this property, but if you use getLocalSourceRange on them, it's not going to return any valid locations, because it doesn't have any. The right fix here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive into the inner TypeLoc to get the source range if it doesn't find it on the top level one. You can use getLocalSourceRange if you are really into micro-optimizations and you have some outside knowledge that the TypeLocs you are dealing with will always include some source location.

4. Exposed a bug somewhere in the use of the normal clang type class API, where you have some type, you want to see if that type is some particular kind, you try a dyn_cast such as dyn_cast<TypedefType> and that fails because now you have an ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match. Again, like 2), this would usually have been tested poorly with some simple tests with no qualifications, and would have been broken had there been any other kind of type sugar, be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType. The usual fix here is to use getAs instead of dyn_cast, which will look deeper into the type. Or use getAsAdjusted when dealing with TypeLocs. For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.

5. It could be a bug in this patch perhaps.

Let me know if you need any help!

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

..

[mizvekov]

clang/lib/Sema/SemaDecl.cpp

291

Sure. But even then, in the current implementation of the type printer, adding this node would not be a no-op, it would incorrectly change meaning as we would suppress printing the scope of not just the type under the Elaborated node, but for any children of that as well. So for example a template parameter which is sugar for a CXXRecordDecl would be printed with scope suppressed.

[mizvekov]

• Run libcxx CI.

[mizvekov]

rebase

[mizvekov]

• Avoid using any storage for an empty ElaboratedTYpeLoc.
  • But we still require pointer alignment for it, due to pre-existing bugs.

[mizvekov]

• fix IntrospectionTests.

[rsmith]

I've not looked at the test changes in any detail; please let me know if there's anything in there that deserves special attention.

clang-tools-extra/clang-change-namespace/ChangeNamespace.cpp

567–577 ↗

(On Diff #389352)

Can we keep at least the second check here?

clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.h

28 ↗

(On Diff #389352)

If this is an externally-visible option that might be in someone's config file, can we keep it meaning the same thing? Eg, start with a NestingLevel of 1, not 0, so it counts the number of name components rather than the number of ::s.

clang/include/clang/AST/TypeLoc.h

2255–2257 ↗

(On Diff #389352)

Instead of using a LocalData type of void and adding llvm::SizeOf etc, how about using a local data type of ElaboratedLocInfo and overriding getLocalDataSize to sometimes return 0 if you don't actually want to store local data? AdjustedTypeLoc does something similar already.

clang/lib/AST/FormatString.cpp

979–984

Optional, but I think this is a bit clearer. (You'd need to re-add the return false; after the loop too.)

clang/lib/CodeGen/CGCall.cpp

2790 ↗

(On Diff #389352)

Hm, this and the checks for this attribute in CGExprScalar.cpp aren't fully fixed: if we don't find the attribute on a typedef we should look inside that typedef to see if its target is another typedef. See TypeHasMayAlias for an example of code doing it correctly. Might make sense to land that as a separate change since it's a pre-existing bug?

clang/lib/CodeGen/CodeGenFunction.cpp

2210 ↗

(On Diff #389352)

Hm, this seems to already be reachable for an ElaboratedType, even without this change:

void f(int n) {
  using T = int[n];
  struct A {
    using U = T;
  };
  A::U au;
}

... but I think that's actually a bug. Bad things are going to happen if the members of A mention U.

Filed that as https://github.com/llvm/llvm-project/issues/55686.

clang/lib/Sema/SemaTemplate.cpp

3989

I think in the case we have a constructor or destructor name, we should pass in an empty scope specifier here, so that pretty-printing Namespace::Class::Class() {} produces just that instead of Namespace::Class::Namespace::Class) {}. (The first name specifier comes from printing the declaration's qualified name, and the second one would come from printing the constructor type.)

llvm/include/llvm/Support/SizeOf.h

20–23 ↗

(On Diff #389352)

I think it's too surprising to call this simply SizeOf, especially given that under https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0146r1.html we might eventually see C++ defining sizeof(void) as some non-zero value.

25–32 ↗

(On Diff #389352)

There's already a (different!) llvm::AlignOf in llvm/Support/AlignOf.h.

[mizvekov]

In D112374#3535949, @rsmith wrote:

I've not looked at the test changes in any detail; please let me know if there's anything in there that deserves special attention.

The new (since last rebase) change in clang-tools-extra/test/clang-tidy/checkers/bugprone/shared-ptr-array-mismatch.cpp might be worth a look.
I think this is a fix, and the pre-existing code missed it by pure accident.

I am not sure there are any scenarios where we might want to avoid suggesting this fix in dependent contexts, if there were we should throw a FIXME in there.

There are a couple of minor significant changes since the last diff, might be worth an extra look, but mainly some minor conflicts with the introduction of UsingType by @sammccall .

llvm/include/llvm/Support/SizeOf.h

20–23 ↗

(On Diff #389352)

The author merely suggests the two issues are independent :)

If this paper ever gets followed up on, leaving sizeof(void) != 0 would be a riot!

25–32 ↗

(On Diff #389352)

Well that is the one which is surprising to simply call AlignOf =)

[rsmith]

LGTM

[nikic]

FYI this change had a noticeable compile-time impact (http://llvm-compile-time-tracker.com/compare.php?from=ee88c0cf09969ba44307068797e12533b94768a6&to=bdc6974f92304f4ed542241b9b89ba58ba6b20aa&stat=instructions), is that expected? Largest regressions are in kimwitu++, where error.cc and gutil.cc regress by 2%.

[mizvekov]

In D112374#3647530, @nikic wrote:

FYI this change had a noticeable compile-time impact (http://llvm-compile-time-tracker.com/compare.php?from=ee88c0cf09969ba44307068797e12533b94768a6&to=bdc6974f92304f4ed542241b9b89ba58ba6b20aa&stat=instructions), is that expected? Largest regressions are in kimwitu++, where error.cc and gutil.cc regress by 2%.

It was expected that there would be some amount of compile time impact.

How do you think these are representative / translate to real world impact?
Any opinions on trade vs benefit?

The impact this patch causes on a TU would be slightly lower, basically equivalent to simply using name qualifications whenever possible.

So if you had a program like this:

struct A {};

A a;

The impact this patch causes should be smaller than simply rewriting it to:

struct A {};

struct A a;

or

struct A {};

::A a;

Where qualifications already exist or are more prevalent, I would expect the impact to be none or lesser.

In other words, I would not expect to make worst cases significantly worse.

So I would have suspected this not to be of much impact in scenarios where compilation time is already critical, like complex programs that use qualifications more heavily, or where most of the time is spent instantiating templates for example.

[JDevlieghere]

This breaks all the LLDB tests that import the std module:

import-std-module/array.TestArrayFromStdModule.py
import-std-module/deque-basic.TestDequeFromStdModule.py
import-std-module/deque-dbg-info-content.TestDbgInfoContentDequeFromStdModule.py
import-std-module/forward_list.TestForwardListFromStdModule.py
import-std-module/forward_list-dbg-info-content.TestDbgInfoContentForwardListFromStdModule.py
import-std-module/list.TestListFromStdModule.py
import-std-module/list-dbg-info-content.TestDbgInfoContentListFromStdModule.py
import-std-module/queue.TestQueueFromStdModule.py
import-std-module/stack.TestStackFromStdModule.py
import-std-module/vector.TestVectorFromStdModule.py
import-std-module/vector-bool.TestVectorBoolFromStdModule.py
import-std-module/vector-dbg-info-content.TestDbgInfoContentVectorFromStdModule.py
import-std-module/vector-of-vectors.TestVectorOfVectorsFromStdModule.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45301/

Given that the bot has been red for 14 hours I went ahead and reverted this change. Please keep an eye on this bot when relanding.

[kimgr]

This patch also broke IWYU, not exactly sure how or why yet. We run around the AST quite a bit, so structural changes like this often bite us.

Can you expand on what happened here? Before/after kind-of thing? Thanks!

[mizvekov]

In D112374#3648624, @JDevlieghere wrote:

This breaks all the LLDB tests that import the std module:

Given that the bot has been red for 14 hours I went ahead and reverted this change. Please keep an eye on this bot when relanding.

OK, this seems like the simple breakage where the test expected the type to be printed fully qualified, just because it was written without any qualifications.

I am not sure what lldb really wants here.
I think usually the two sane choices you want are:

1. Always print the type as written, which this patch makes it easier and consistent.
2. Always print the types fully qualified as if ignoring the existence of ElaboratedType nodes, which the type printer never really supported, but you would usually get this effect if the type was canonicalized, as that would remove all sugar.

So if lldb wants 1), that's great this patch fixes a bug in lldb and we just need to change the test expectation.
If it wants 2, then we just exposed a bug in lldb, because this would never have worked consistently. If libc++ devs had written any name qualifiers in that size_type typedef, then the type printer would suddenly switch to printing the type as-written.

As a side note, it's unfortunate that this test is not supported on Windows, as that is what I primarily develop clang on. In fact, the lldb test suite is poorly supported there, as running the whole thing says half the tests are unsupported, and creates an unholly mess on my desktop with a bunch of frozen terminal windows created!

@JDevlieghere can you identify what lldb wants here, 1, 2 or some other option I missed? Or otherwise subscribe someone who can?

In D112374#3649525, @kimgr wrote:

This patch also broke IWYU, not exactly sure how or why yet. We run around the AST quite a bit, so structural changes like this often bite us.

Can you expand on what happened here? Before/after kind-of thing? Thanks!

Thanks for the report!

So this patch makes (name-qualifiable) types that come from the parser consistently carry an ElaboratedType node, which otherwise would be absent when the type represents some parsed entity which was written without any name qualifiers.

So if you take a look at the many fixes around in this patch, and without any further idea from what is going on at IWYU, then the churn this patch causes is usually:

1. What I explained to @JDevlieghere above, a change in how the type printer prints a type.
2. Exposed a bug in some AST matcher. Matching types is harder to get right when there is sugar involved. For example, if you want to match a type against being a pointer to some type A, then you have to account for getting a type that is sugar for a pointer to A, or being a pointer to sugar to A, or both! Usually you would get the second part wrong, and this would work for a very simple test where you don't use any name qualifiers when you write A, but you would discover is broken when someone reports that it doesn't work when you do. The usual fix is to either use the matcher which strips sugar, which is annoying to use as for example if you match an N level pointer, you have to put N+1 such matchers in there, beginning to end and between all those levels. But in a lot of cases, if the property you want to match is present in the canonical type, it's easier and faster to just match on that...
3. Exposed a bug in how you get the source range of some TypeLoc. For some reason, a lot of code is using getLocalSourceRange(), which only looks at the given TypeLoc node. This patch introduces a new, and more common TypeLoc node which contains no source locations on itself. This is not new and some other, more rare TypeLoc nodes could also have this property, but if you use getLocalSourceRange on them, it's not going to return any valid locations, because it doesn't have any. The right fix here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive into the inner TypeLoc to get the source range if it doesn't find it on the top level one. You can use getLocalSourceRange if you are really into micro-optimizations and you have some outside knowledge that the TypeLocs you are dealing with will always include some source location.
4. Exposed a bug somewhere in the use of the normal clang type class API, where you have some type, you want to see if that type is some particular kind, you try a dyn_cast such as dyn_cast<TypedefType> and that fails because now you have an ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match. Again, like 2 this would usually have been tested poorly with some simple tests with no qualifications, and would have been broken had there been any other kind of sugar to what you wanted at all, be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType. The usual fix here is to use getAs instead of dyn_cast, which will look deeper into the type. Or use getAsAdjusted when dealing with TypeLocs. For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.
5. Some thing else. Would be happy to help debug if we can get some more information.

[glandium]

FWIW, this change also broke this check in Firefox's clang plugin: https://searchfox.org/mozilla-central/rev/0d11f3660945ce35c49501bb44bc4f82bb2b503c/build/clang-plugin/NoPrincipalGetURI.cpp

[mizvekov]

In D112374#3650749, @glandium wrote:

FWIW, this change also broke this check in Firefox's clang plugin: https://searchfox.org/mozilla-central/rev/0d11f3660945ce35c49501bb44bc4f82bb2b503c/build/clang-plugin/NoPrincipalGetURI.cpp

Thanks for the report!

I think this is a combination of 1) and 2) from the post above (https://reviews.llvm.org/D112374#3650056).

I believe that hasType(asString()) is supposed to match the type as-written. The fact that it did not so for unqualified names is what this patch is fixing.

Can you confirm that this NoPrincipalGetURI matcher would, in clang version without this patch here, fail to match if you had rewritten the GetUri method to use a qualified name? Something like this:

From:

nsIPrincipal *getURI();

To:

::nsIPrincipal *getURI();

Ie qualify with the global namespace or something equivalent that works in the Firefox codebase?

I believe that matching to the canonical type would be the simplest fix for that bug:

On mozilla-central/build/clang-plugin/NoPrincipalGetURI.cpp change from:

anyOf(on(hasType(asString("class nsIPrincipal *"))),
      on(hasType(asString("class nsIPrincipal")))),

To:

anyOf(on(hasType(hasCanonicalType(asString("class nsIPrincipal *")))),
      on(hasType(hasCanonicalType(asString("class nsIPrincipal"))))),

Ie nest the asString matcher inside a hasCanonicalType matcher.

[mizvekov]

@JDevlieghere I spent a lot of time trying to get this test running on my machine to no avail. I think lldb build and test setup is quite convoluted, fragile and antiquated. It uses many deprecated CMake features, It fails to properly link to system libraries it needs like librt. And I just kept patching these problems up and more kept coming. At some point I just gave up.

And even this test itself requires the whole libcxx it seems, which is another difficult thing to get building outside of CI.

I think pre-commit CI is essential here.

So I would be happy to help sort this lldb bug, but you have to help me help you.

I think reverting it with no prior notification was unreasonable, and landing this back as it was, no changes, is reasonable.

Do you agree? Please let me know soon otherwise.

[glandium]

In D112374#3651218, @mizvekov wrote:

Can you confirm that this NoPrincipalGetURI matcher would, in clang version without this patch here, fail to match if you had rewritten the GetUri method to use a qualified name?

I can confirm it's the case, and your fix works. Thanks.

[chapuni]

clang/lib/Sema/TypeLocBuilder.cpp

159 ↗

(On Diff #444856)

It causes a warning with -Asserts. May be rolled back.

[JDevlieghere]

In D112374#3653702, @mizvekov wrote:

@JDevlieghere I spent a lot of time trying to get this test running on my machine to no avail. I think lldb build and test setup is quite convoluted, fragile and antiquated. It uses many deprecated CMake features, It fails to properly link to system libraries it needs like librt. And I just kept patching these problems up and more kept coming. At some point I just gave up.

I'm sorry to hear you're having trouble building LLDB. The LLDB website has quite an elaborate guide with instructions in how to build LLDB: https://lldb.llvm.org/resources/build.html, including specific instructions on Windows. Windows is not my main platform, but I've successfully built LLDB there in the past following those instructions. I'm not sure what you feel is "convoluted, fragile and antiquated" about our build and test setup, as it's fairly similar to the rest of LLVM. I'd be happy to hear your suggestions on how we could improve things.

And even this test itself requires the whole libcxx it seems, which is another difficult thing to get building outside of CI.

I think pre-commit CI is essential here.

So I would be happy to help sort this lldb bug, but you have to help me help you.

I'm happy to help out. I personally don't know if we should go with (1) or (2), both sound reasonable in their own way. I'd like @teemperor, who's the author of the feature and the affected tests, to weigh in.

I think reverting it with no prior notification was unreasonable, and landing this back as it was, no changes, is reasonable.

Do you agree? Please let me know soon otherwise.

I don't. I think reverting your change was well within the guidelines outlined by LLVM's patch reversion policy: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

Additionally, I think you could've given me a little bit more time to catch up on the discussion here. The code review policy and practices (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) recommend pinging every few days to once per week depending on how urgent the patch is.

By relanding, you broke the bots again (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45354/#showFailuresLink) and I'm forced to revert this change a second time. Please refrain from landing this again until we've settled on a way forward.

[chrish_ericsson_atx]

In D112374#3653967, @JDevlieghere wrote:

I don't. I think reverting your change was well within the guidelines outlined by LLVM's patch reversion policy: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

Additionally, I think you could've given me a little bit more time to catch up on the discussion here. The code review policy and practices (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) recommend pinging every few days to once per week depending on how urgent the patch is.

By relanding, you broke the bots again (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45354/#showFailuresLink) and I'm forced to revert this change a second time. Please refrain from landing this again until we've settled on a way forward.

I agree with @JDevlieghere here. In general, it's reasonable that the patch was reverted when it was found to break other things in the LLVM project, and reasonable to expect the original author's cooperation in resolving that breakage before the patch gets reapplied--especially when the patch is as large and far-reaching as this one is.

As an aside, this also patch breaks one of our internal tidy checkers. Likely the fix we'll need for that checker is simple, but it'll take a bit of research for me to understand what needs to happen in our internal code. Thanks for your patience and willingness to help your colleagues understand and adapt to the change you've authored.

[kimgr]

It's a little confusing, because it now looks like _every_ Type in the AST is wrapped in an ElaboratedTypeLoc + ElaboratedType. IWYU's debug AST dump shows this (excerpt):

tests/cxx/sizeof_reference.cc:51:8: (1) [ VarDecl ] size_t s2                                                                                                                                                      
tests/cxx/sizeof_reference.cc:51:1: (2) [ ElaboratedTypeLoc ] size_t                                                                                                                                               
tests/cxx/sizeof_reference.cc:51:1: (2) [ ElaboratedType ] size_t                                                                                                                                                  
tests/cxx/sizeof_reference.cc:51:1: (3) [ TypedefTypeLoc ] size_t                                                                                                                                                  
tests/cxx/sizeof_reference.cc:51:1: (3) [ TypedefType ] size_t                                                                                                                                                     
Marked full-info use of decl size_t (from /home/kimgr/code/iwyu/out/main/lib/clang/15.0.0/include/stddef.h:46:23) at tests/cxx/sizeof_reference.cc:51:1
tests/cxx/sizeof_reference.cc:51:13: (2) [ UnaryExprOrTypeTraitExpr ] UnaryExprOrTypeTraitExpr 0x5589fd2a4230 'unsigned long' sizeof 'IndirectTemplateStruct<IndirectClass> &'                                     
                                                                                                                                                                                                                   
(For type IndirectTemplateStruct<IndirectClass>):                                                                                                                                                                  
Marked full-info use of decl IndirectTemplateStruct (from tests/cxx/sizeof_reference.cc:18:30) at tests/cxx/sizeof_reference.cc:51:20                                                                              
tests/cxx/sizeof_reference.cc:51:20: (3) [ LValueReferenceTypeLoc ] IndirectTemplateStruct<IndirectClass> &                                                                                                        
tests/cxx/sizeof_reference.cc:51:20: (3) [ LValueReferenceType ] IndirectTemplateStruct<IndirectClass> &                                                                                                           
tests/cxx/sizeof_reference.cc:51:20: (4) [ ElaboratedTypeLoc ] IndirectTemplateStruct<IndirectClass>                                                                                                               
tests/cxx/sizeof_reference.cc:51:20: (4) [ ElaboratedType ] IndirectTemplateStruct<IndirectClass>                                                                                                                  
tests/cxx/sizeof_reference.cc:51:20: (5) [ TemplateSpecializationTypeLoc ] IndirectTemplateStruct<IndirectClass>                                                                                                   
tests/cxx/sizeof_reference.cc:51:20: (5) [ TemplateSpecializationType ] IndirectTemplateStruct<IndirectClass>                                                                                                      
Marked fwd-decl use of decl IndirectTemplateStruct (from tests/cxx/sizeof_reference.cc:18:30) at tests/cxx/sizeof_reference.cc:51:20                                                                               
tests/cxx/sizeof_reference.cc:51:20: (6, fwd decl) [ TemplateName ] IndirectTemplateStruct                                                                                                                         
tests/cxx/sizeof_reference.cc:51:43: (6, fwd decl) [ TemplateArgumentLoc ] <IndirectClass>                                                                                                                         
tests/cxx/sizeof_reference.cc:51:43: (7, fwd decl) [ ElaboratedTypeLoc ] IndirectClass                                                                                                                             
tests/cxx/sizeof_reference.cc:51:43: (7, fwd decl) [ ElaboratedType ] IndirectClass                                                                                                                                
tests/cxx/sizeof_reference.cc:51:43: (8, fwd decl) [ RecordTypeLoc ] class IndirectClass                                                                                                                           
tests/cxx/sizeof_reference.cc:51:43: (8, fwd decl) [ RecordType ] class IndirectClass                                                                                                                              
Marked fwd-decl use of decl IndirectClass (from tests/cxx/indirect.h:18:7) at tests/cxx/sizeof_reference.cc:51:43

for this line of code:

size_t s2 = sizeof(IndirectTemplateStruct<IndirectClass>&);

I'm not sure I understand why the elaborated type nodes are necessary even when they don't seem to add any additional information?

[mizvekov]

In D112374#3653967, @JDevlieghere wrote:

I'm sorry to hear you're having trouble building LLDB. The LLDB website has quite an elaborate guide with instructions in how to build LLDB: https://lldb.llvm.org/resources/build.html, including specific instructions on Windows.

The instructions exist, doesn't mean they work or that they are a fair burden on the developers of the other projects.

The LLDB build / test system is made of the same 'stuff' as the rest of LLVM sure, but it does a lot more 'questionable' things which makes it look more hazardous.
For example, opening hundreds of frozen terminal windows or creating paths in the build directory which contain unsubstituted variables.
So it seems to me that for me to be comfortable working with this, I would have to adjust my workflow to build LLVM in a sandbox instead.

If we tried polling other clang devs, we might find that standard practice is that we are not really even trying to build and run LLDB tests locally anymore.

libcxx devs have a CI pre-commit system which only runs when a path in their sub-project is touched. I think this would be reasonable start for LLDB.

Lastly, my main concern is that by keeping this patch off, even if we don't suspect a problem in it, this will create a very long tail. The users affected don't know about it yet, and they will keep coming
with a delay one by one as we re-land and revert.

I'm happy to help out. I personally don't know if we should go with (1) or (2), both sound reasonable in their own way. I'd like @teemperor, who's the author of the feature and the affected tests, to weigh in.

I think, but can't confirm, that this is just a case of (1) and for what is being tested we don't really care how the return type of those size() methods is written.
I would like some way to test that the functionality is not really broken, we just changed that test expectation, but alas I can't.

I don't. I think reverting your change was well within the guidelines outlined by LLVM's patch reversion policy: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

Additionally, I think you could've given me a little bit more time to catch up on the discussion here. The code review policy and practices (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) recommend pinging every few days to once per week depending on how urgent the patch is.

I think, given the size of this patch and the other points I made, we could have simply fixed those issues post-commit, if I had received any prior notification.

In D112374#3657332, @kimgr wrote:

It's a little confusing, because it now looks like _every_ Type in the AST is wrapped in an ElaboratedTypeLoc + ElaboratedType. IWYU's debug AST dump shows this (excerpt):
I'm not sure I understand why the elaborated type nodes are necessary even when they don't seem to add any additional information?

It's the difference in knowing the type was written without any tag or nested-name specifier, and having a type that you are not sure how it was written.

When we are dealing with a type which we are not sure, we would like to print it fully qualified, with a synthetic nested name specifier computed from it's DC, because otherwise it could be confusing as the type could come from somewhere very distant from the context we are printing the type from. We would not want to assume that a type which has been desugared was written how it's desugared state would seem to imply.

FWIW, in the state of affairs we leave clang after this patch, I don't think it's worth keeping a separate ElaboratedType anymore, we might as well fuse it's functionality into the type nodes which could be wrapped in it. Taking care to optimize storage when not used otherwise, I think we can recoup the performance lost in this patch, perhaps even end in a better state overall.

But I think doing these two steps in one go would not be sensibly incremental. We have in this patch here a very simple core change, which is very unlikely to have bugs in itself, but creates enormous test churn.

The second step of eliminating ElaboratedType could be a less simple core change with almost zero test churn, which makes it less risky that it would introduce a bug that escapes review.

[kimgr]

It's the difference in knowing the type was written without any tag or nested-name specifier, and having a type that you are not sure how it was written.

When we are dealing with a type which we are not sure, we would like to print it fully qualified, with a synthetic nested name specifier computed from it's DC,
because otherwise it could be confusing as the type could come from somewhere very distant from the context we are printing the type from. We would not
want to assume that a type which has been desugared was written how it's desugared state would seem to imply.

I'm coming at this from pretty far away, so there's very likely lots of details that I'm overlooking. But it seems to me the mainline had only had an ElaboratedType node if there was elaboration, and not otherwise. And that makes a lot more sense to me than having 2 ElaboratedType* nodes _for every type in the AST_, just to express that "hey, by the way, this type had no elaboration".

FWIW, in the state of affairs we leave clang after this patch, I don't think it's worth keeping a separate ElaboratedType anymore, we might as
well fuse it's functionality into the type nodes which could be wrapped in it. Taking care to optimize storage when not used otherwise, I think
we can recoup the performance lost in this patch, perhaps even end in a better state overall.

But I think doing these two steps in one go would not be sensibly incremental. We have in this patch here a very simple core change, which
is very unlikely to have bugs in itself, but creates enormous test churn.

The second step of eliminating ElaboratedType could be a less simple core change with almost zero test churn, which makes it less risky that
it would introduce a bug that escapes review.

That sounds good at face value, but if you're planning to remove these nodes again, that would create enormous churn for out-of-tree tools to re-adjust to the new shape of the tree.

I can't say what the best solution is, but this patch generates quite a lot of work for me, and I would really hope that catching up with the new AST does not generate even more work down the line.

[mizvekov]

In D112374#3657472, @kimgr wrote:

I'm coming at this from pretty far away, so there's very likely lots of details that I'm overlooking. But it seems to me the mainline had only had an ElaboratedType node if there was elaboration, and not otherwise. And that makes a lot more sense to me than having 2 ElaboratedType* nodes _for every type in the AST_, just to express that "hey, by the way, this type had no elaboration".

There are no 2 ElaboratedType nodes, there is only one. If you are seeing something like an ElaboratedType wrapping directly over another ElaboratedType, that would seem to be a bug.

To the second point, it's a problem of representation. Having no elaboration is not the same thing as having no information about elaboration, so we better not represent both things with the same state.

That sounds good at face value, but if you're planning to remove these nodes again, that would create enormous churn for out-of-tree tools to re-adjust to the new shape of the tree.

I can't say what the best solution is, but this patch generates quite a lot of work for me, and I would really hope that catching up with the new AST does not generate even more work down the line.

That part I don't understand why. Before this patch, clang can produce a bunch of type nodes wrapped in an ElTy, or not. After this patch, we add ElTys in more cases, but the basic situation remains the same.

Why IWYU would even care about ElaboratedTypes at all? I would have expected a git grep ElaboratedType on IWYU sources to have no matches.

Can you elaborate on that?

In general, I would not expect external tools to care about the shape of the AST. I would expect the type API would be used in a way where we ignore a type sugar node we have no reason to acknowledge.
Ie you query if some type is a (possible sugar to) X, and you would either get X or nothing. The type sugar over it would just be skipped over and you would have no reason to know what was in there or what shape it had.

Of course that was not what happened in practice. A lot of code used dyn_cast where it should have used getAs. Just look at all the fixes in this patch for examples.
And fixing that, besides making that code compatible with this patch, also fixed other bugs where it would not properly ignore other pre-existing type sugar.

If IWYU has unit tests that test too much clang implementation details, that would generate unneeded burden on both sides. Is it for example doing AST dump tests and expecting exact outputs?
Not even the clang test suite does that too much.
Is it to compensate for any perceived lack of testing on mainline side?

[mizvekov]

In D112374#3657472, @kimgr wrote:

I can't say what the best solution is, but this patch generates quite a lot of work for me, and I would really hope that catching up with the new AST does not generate even more work down the line.

Okay, I checked out IWYU and I see why you need to look at ElaboratedType in some cases. And that also answers a lot of my previous questions.

Some type nodes were before rarely ever elaborated, but will have an ElaboratedType over them consistently now.
Searching IWYU source code, some cases where dyn_cast is used in some of them:

iwyu.cc:

    // If we're a constructor, we also need to construct the entire class,
    // even typedefs that aren't used at construct time. Try compiling
    //    template<class T> struct C { typedef typename T::a t; };
    //    class S; int main() { C<S> c; }
    if (isa<CXXConstructorDecl>(fn_decl)) {
      CHECK_(parent_type && "How can a constructor have no parent?");
      parent_type = RemoveElaboration(parent_type);
      if (!TraverseDataAndTypeMembersOfClassHelper(
              dyn_cast<TemplateSpecializationType>(parent_type)))
        return false;
    }
    return true;
`

if (const auto* enum_type = dyn_cast<EnumType>(type))
  return !CanBeOpaqueDeclared(enum_type);

[mizvekov]

@kimgr One other general comment.

The way this function is implemented is quite error prone:

static const NamedDecl* TypeToDeclImpl(const Type* type, bool as_written) {
  // Get past all the 'class' and 'struct' prefixes, and namespaces.
  type = RemoveElaboration(type);

  // Read past SubstTemplateTypeParmType (this can happen if a
  // template function returns the tpl-arg type: e.g. for
  // 'T MyFn<T>() {...}; MyFn<X>.a', the type of MyFn<X> will be a Subst.
  type = RemoveSubstTemplateTypeParm(type);

  CHECK_(!isa<ObjCObjectType>(type) && "IWYU doesn't support Objective-C");

Ie the beginning is being too explicit, testing for very specific sugar type nodes kinds, in a very specific order, just to skip over them.

That makes it very fragile against clang changes.

You can instead just use getAs to step over them in a generic fashion.

I don't think this one gets broken by this MR, but I am very confident it will get broken by another patch I have.

[kimgr]

In D112374#3657640, @mizvekov wrote:

In D112374#3657472, @kimgr wrote:

I'm coming at this from pretty far away, so there's very likely lots of details that I'm overlooking. But it seems to me the mainline had only had an ElaboratedType node if there was elaboration, and not otherwise. And that makes a lot more sense to me than having 2 ElaboratedType* nodes _for every type in the AST_, just to express that "hey, by the way, this type had no elaboration".

There are no 2 ElaboratedType nodes, there is only one. If you are seeing something like an ElaboratedType wrapping directly over another ElaboratedType, that would seem to be a bug.

I meant the ElaboratedTypeLoc + ElaboratedType, but yeah, those are parallel, not nested.

That sounds good at face value, but if you're planning to remove these nodes again, that would create enormous churn for out-of-tree tools to re-adjust to the new shape of the tree.

I can't say what the best solution is, but this patch generates quite a lot of work for me, and I would really hope that catching up with the new AST does not generate even more work down the line.

That part I don't understand why. Before this patch, clang can produce a bunch of type nodes wrapped in an ElTy, or not. After this patch, we add ElTys in more cases, but the basic situation remains the same.

Why IWYU would even care about ElaboratedTypes at all? I would have expected a git grep ElaboratedType on IWYU sources to have no matches.

Can you elaborate on that?

Haha. Pun intended? :-)

In general, I would not expect external tools to care about the shape of the AST. I would expect the type API would be used in a way where we ignore a type sugar node we have no reason to acknowledge.
Ie you query if some type is a (possible sugar to) X, and you would either get X or nothing. The type sugar over it would just be skipped over and you would have no reason to know what was in there or what shape it had.

Of course that was not what happened in practice. A lot of code used dyn_cast where it should have used getAs. Just look at all the fixes in this patch for examples.
And fixing that, besides making that code compatible with this patch, also fixed other bugs where it would not properly ignore other pre-existing type sugar.

As you noticed, it's not our tests that care about the AST, it's the tool itself. IWYU has been around since 2010-11, so there's probably lots of code in there to work around bugs and idiosyncrasies in the Clang AST that have since been fixed. I've inherited the project, so I don't have much information on how or why the implementation ended up the way it did.

Anyway, thanks for the heads-up about getAs vs dyn_cast, that seems like an easy transformation for us to do. Though I'm not sure where -- everywhere a Type* is processed?

Also, I suspect we have a few open-ended searches where we're looking for the first desugared type in the tree, but I guess that's where getCanonicalType would be used?

Thanks!

[mizvekov]

In D112374#3658059, @kimgr wrote:

Haha. Pun intended? :-)

Yes :-)

As you noticed, it's not our tests that care about the AST, it's the tool itself. IWYU has been around since 2010-11, so there's probably lots of code in there to work around bugs and idiosyncrasies in the Clang AST that have since been fixed. I've inherited the project, so I don't have much information on how or why the implementation ended up the way it did.

Anyway, thanks for the heads-up about getAs vs dyn_cast, that seems like an easy transformation for us to do. Though I'm not sure where -- everywhere a Type* is processed?

Also, I suspect we have a few open-ended searches where we're looking for the first desugared type in the tree, but I guess that's where getCanonicalType would be used?

I think for IWYU getCanonicalType could be problematic. It would be fine for quickly testing what kind of node you have under all that sugar and such, but if for example you try to get a Decl represented by some canonical type, you would likely get a canonical decl, but it seems to me that would not be useful because you might need to know the exact (re)-declaration used, which has source location information and you could pin down to a specific file.

There is getDesugaredType, which will just pull off all top level sugar without really canonicalizing the whole thing.

If instead you want to search down a type for the first thing of interest, then you can in that case have a main switch case on the type class, or even perhaps keep an if else chain of dyn_casts, but on your default or else case you could just test that the current type node is sugar with getSingleStepDesugaredType on it, see if you get a different result and try again with it in that case.
That way this mechanism does not get poisoned by clang introducing some new type sugar, which could not even be relevant to IWYU.

Thanks!

Thank you for your patience as well, and sorry for the trouble!

[mizvekov]

clang/lib/Sema/TypeLocBuilder.cpp

159 ↗

(On Diff #444856)

Thanks! fixed in latest rebase.

[mizvekov]

@JDevlieghere @teemperor ping.

Sorry for the urgency here, but I have a lot of work built on top of this patch.

And this patch fixes a bunch of bugs that other people are unaware and duplicating effort, see github issues.

[nikic]

Given that LLVM 15 branches off in one week, maybe it would be better to wait for that before relanding the change, as it seems to have significant impact on plugins?

[mizvekov]

In D112374#3659597, @nikic wrote:

Given that LLVM 15 branches off in one week, maybe it would be better to wait for that before relanding the change, as it seems to have significant impact on plugins?

I think it would make sense to try LLVM15.

So this change in this MR is in effect a sanitizer for some incorrect use of a bunch of APIs, it turns a bunch of bugs from something that needs a specific set of circumstances to happen, into problems that happen reliably.

And those incorrect uses, for all examples I have seen so far, and there are many, are pretty easy to fix.

[mizvekov]

@JDevlieghere @teemperor ping

Can you please verify that everything works with LLDB, we just changed how those types are printed?

[sammccall]

In D112374#3657640, @mizvekov wrote:

In general, I would not expect external tools to care about the shape of the AST. I would expect the type API would be used in a way where we ignore a type sugar node we have no reason to acknowledge.
Ie you query if some type is a (possible sugar to) X, and you would either get X or nothing. The type sugar over it would just be skipped over and you would have no reason to know what was in there or what shape it had.

I'm afraid your expectations are wrong, and not by a little bit :-)

I totally agree this is the best way to the use the AST: understanding what you want to depend on and what groups of differences (e.g. sugar) to ignore, and writing code that expresses that intent.

However this is empirically not how lots of downstream (and plenty of in-tree) code is written, because:

• it requires a deep understanding of the "philosophy" of the AST to understand where extensions are possible in future
• many people write AST traversals using matchers, which constrains and obscures exactly what's being matched
• many people write AST trawling code based on AST dumps of examples, rather than a first-principles approach
• it is difficult and tedious to test
• people are often only as careful as they're incentivized to be

I've seen plenty of (useful) out-of-tree tidy checks written by people fuzzy on the difference between a Type and a TypeLoc, or what sugar is. Clang makes it (almost) easy to write tools but hard to write robust tools.

All of this is to say I like this change & appreciate how willing you are to help out-of-tree tools (which is best-effort), but I expect a lot of churn downstream. (And LLVM has a clear policy that that's OK).

(BTW, last time I landed such a change, investigating the LLDB tests was indeed the most difficult part, and I'm not even on windows. Running a linux VM of some sort might be your best bet, unfortunately)

[mizvekov]

In D112374#3665249, @sammccall wrote:

I've seen plenty of (useful) out-of-tree tidy checks written by people fuzzy on the difference between a Type and a TypeLoc, or what sugar is. Clang makes it (almost) easy to write tools but hard to write robust tools.

I agree.

All of this is to say I like this change & appreciate how willing you are to help out-of-tree tools (which is best-effort), but I expect a lot of churn downstream. (And LLVM has a clear policy that that's OK).

Thanks!

(BTW, last time I landed such a change, investigating the LLDB tests was indeed the most difficult part, and I'm not even on windows. Running a linux VM of some sort might be your best bet, unfortunately)

Yeah I finally managed to build and run the tests on WSL2 running debian testing. The exact configuration used by the lldb-bot seems not to be supported there anymore.

I added the needed changes there, it was really only a change in expectation on the type printer.
These tests are so easy to break not only because they depend exactly on how clang prints types, but they also depend on how libc++ devs write internal implementation details.

If anyone wants to take a look at the new changes to lldb tests, be my guest. Otherwise I will try to land this again soon. It might well be that we figure out some other in-tree user is affected, but I'd rather do that sooner than later.

[JDevlieghere]

In D112374#3665989, @mizvekov wrote:

If anyone wants to take a look at the new changes to lldb tests, be my guest. Otherwise I will try to land this again soon. It might well be that we figure out some other in-tree user is affected, but I'd rather do that sooner than later.

Please allow @teemperor some time to take a look at the llvm changes before landing this.

[teemperor]

import-std-module test changes look good to me, thanks for fixing that up.

And yes, ideally the tests should never use any libc++ internal names (and LLDB should never print them for those tests). So I think not having those in the here is a step in the right direction.

[kimgr]

From a purely personal perspective, I'd prefer if this landed after the branch for llvm-15.

We try to co-release IWYU shortly after LLVM/Clang are released, to get a public API-compatible release out there. So it would be really nice if we didn't have to spend time working around AST changes while the clock is ticking to get a release out the door.

That said, I know we're just an out-of-tree project and the LLVM project as such doesn't make any promises (and shouldn't have to!). I just thought I'd raise the concern to see if anybody shares it.

[mizvekov]

In D112374#3666531, @kimgr wrote:

From a purely personal perspective, I'd prefer if this landed after the branch for llvm-15.

We try to co-release IWYU shortly after LLVM/Clang are released, to get a public API-compatible release out there. So it would be really nice if we didn't have to spend time working around AST changes while the clock is ticking to get a release out the door.

That said, I know we're just an out-of-tree project and the LLVM project as such doesn't make any promises (and shouldn't have to!). I just thought I'd raise the concern to see if anybody shares it.

So I checked out and played a bit with IWYU sources yesterday. I saw that my previous suggestions only fixes one of the six test breakages.

Yeah its a bit messy, I totally understand now why you seem a bit desperate about this haha.

Since I am not one to met around in the torture business, I concur to hold.

[hans]

Here's an example where I think this regressed a Clang diagnostic. Consider:

template <typename T> struct Template { Template(int x) {} };

struct S1 {
  struct Foo;
  typedef Template<Foo> Typedef;
};

struct S2 {
  struct Foo;
  typedef Template<Foo> Typedef;
};

typedef S1::Typedef Bar;
Bar f;

before this change, Clang would say:

/tmp/a.cc:14:5: error: no matching constructor for initialization of 'Bar' (aka 'Template<S1::Foo>')
Bar f;
    ^

however, after this change it says:

/tmp/a.cc:14:5: error: no matching constructor for initialization of 'Bar' (aka 'Template<Foo>')
Bar f;
    ^

The problem is that just based on Template<Foo> it's not clear whether it's S1::Foo or S2::Foo that's referred to.

[mizvekov]

In D112374#3684722, @hans wrote:

The problem is that just based on Template<Foo> it's not clear whether it's S1::Foo or S2::Foo that's referred to.

The logic in the diagnostic here ended up doing a silent single step desugar through a typedef in the 'aka'.

I think the danger of doing that as you exemplify is that, if you don't know what context the printed type belongs to, then it's hard to make sense of the meaning of what was written.

If we have to step through a sugar node which carries context, like type aliases / using types and such, perhaps we should go straight for the canonical type instead.

[mizvekov]

In D112374#3684722, @hans wrote:

Here's an example where I think this regressed a Clang diagnostic. Consider:

Consider this simple extension of this example, to show how this general problem already existed: https://godbolt.org/z/n6nGhejTc

template <typename T> struct Template { Template(int x) {} };

struct S1 {
  struct Baz {
    struct Foo;
  };
  typedef Template<Baz::Foo> Typedef;
};

struct S2 {
  struct Baz {
    struct Foo;
  };
  typedef Template<Baz::Foo> Typedef;
};

typedef S1::Typedef Bar;
Bar f;

Prints: error: no matching constructor for initialization of 'Bar' (aka 'Template<Baz::Foo>')

You still don't know which Foo this refers to, because you don't know which Baz it is either.

This patch fixed the inconsistency where we printed the bare Foo with the synthetic nested name, but printed Baz::Foo as written.

[rtrieu]

clang/lib/AST/QualTypeNames.cpp

455 ↗

(On Diff #447960)

Moving this down here means when the ElaboratedType is stripped off, its Qualifers aren't preserved in the underlying type. rGfb7fa27f92ca has a fix to reattach the discarded Qualifiers.

[mizvekov]

clang/lib/AST/QualTypeNames.cpp

455 ↗

(On Diff #447960)

Thanks!

[alexfh]

One more problem related to this patch: it changes the behavior of PRETTY_FUNCTION: https://gcc.godbolt.org/z/Mvnj9j74E. There may be dependencies upon the specific format of expansions of this macro: tests, log processing tools, libraries like ctti, and probably other things. It would be better to retain the format of function names expanded from this macro.

[mizvekov]

In D112374#3716944, @alexfh wrote:

One more problem related to this patch: it changes the behavior of PRETTY_FUNCTION: https://gcc.godbolt.org/z/Mvnj9j74E. There may be dependencies upon the specific format of expansions of this macro: tests, log processing tools, libraries like ctti, and probably other things. It would be better to retain the format of function names expanded from this macro.

Well it looks to me that is a bug fix.

We even match GCC now: https://gcc.godbolt.org/z/WT93WdE7e

Ie we are printing the function type as-written correctly now.

[ilya-biryukov]

In D112374#3716982, @mizvekov wrote:

We even match GCC now: https://gcc.godbolt.org/z/WT93WdE7e

Ie we are printing the function type as-written correctly now.

We don't match GCC in other cases. GCC seems to always print the type name without qualifiers, clang used to always print with qualifiers, but will now print what was typed in the code, see https://gcc.godbolt.org/z/jznncGboM

[mizvekov]

In D112374#3718417, @ilya-biryukov wrote:

We don't match GCC in other cases. GCC seems to always print the type name without qualifiers, clang used to always print with qualifiers, but will now print what was typed in the code, see https://gcc.godbolt.org/z/jznncGboM

Well, Clang always tried printing them as typed in the code, except for this bug that we fixed here.

It would fail to represent a name written without both qualifiers and elaboration. If it had either or both, it would print the name as-written.
Example: https://gcc.godbolt.org/z/3xv4xxYf1

So, consider that relying on __PRETTY_FUNCTION__ output was pretty unreliable from the get go, as you already had different results across compilers.

Also, consider these other points:

• This patch had a lot of test churn, but no test churn on __PRETTY_FUNCTION__ tests, so I think this means that this was pretty much untested on Clang's part.
• The implementation relies on reconstructing the function signature as-written from the AST. So any improvement in that area would cause changes, or extra complexity to keep the old limitations around behind switches.
• There seems to be no attempt to enforce the stability of this result at the architectural level in Clang. It relies directly on the type printer, which is used all around in many different scenarios, and you can see that folks here just make changes to the type printer for cosmetic reasons, with little vetting and no concern for stability.

Otherwise, the type printer is customizable with Policies, so you could think of adding a new one to customize this behavior. But even then, I think the old Clang behavior was too weird to make an option for it.
What would we even call such a policy, something like InventFullNameQualifierIfNeitherQualifiedOrElaborated?

We could make a separate type printer system for such a macro, stripping the function signature of any decoration and using a stable policy, and create all the test cases for it so that we don't regress accidentally.
We could leave the current macro as is, printing the signature as written, and add this new one behind a new macro, so that the user gets to choose between __PRETTY_FUNCTION__ and __NAKED_FUNCTION__? 🤔

[ilya-biryukov]

I agree that the change in behaviour is reasonable and have no objections to it. The code should not rely on particular output of __PRETTY_FUNCTION__.
I just wanted to point out that we still don't match GCC in other cases, not that is was a worthwhile goal to chase.

[davrec]

clang/include/clang/AST/Type.h

5398–5405

This documentation needs to be updated given the changes in this patch. Suggest you remove the first paragraph and flesh out the second paragraph, e.g.

/// A sugar type used to keep track of a type name as written in the
/// source code, including any tag keywords (e.g. struct S) and/or any 
/// nested-name-specifiers (e.g. N::M::type).  Note it will even be created
/// for types written *without* tag words or nested-name-specifiers, to
/// properly represent their absence in the written code.

[Abramo-Bagnara]

These changes introduce a subtle bug in template instantiation of newly added ElaboratedType around TypedefType:

bool refersInstantiatedDecl(QualType T) in SemaTemplateInstantiate.cpp does not handle this ElaboratedType correctly then inhibiting the proper instantiation of TypedefType whose declaration has been subject to instantiation.

[mizvekov]

In D112374#3792825, @Abramo-Bagnara wrote:

These changes introduce a subtle bug in template instantiation of newly added ElaboratedType around TypedefType:

bool refersInstantiatedDecl(QualType T) in SemaTemplateInstantiate.cpp does not handle this ElaboratedType correctly then inhibiting the proper instantiation of TypedefType whose declaration has been subject to instantiation.

I can't find that function, either in that file or anywhere else within clang sub-project.

Even more amazingly, a google search for that function name yields 0 hits.

Can you post a repro of the problem?

[Abramo-Bagnara]

I have to doubly apologize:

1. my reference to refersInstantiatedDecl is completely wrong and I have been mislead by an old patch on my machine.
2. the problem despite being very real is independent of your changes

If you are still interested this is the repro followed by the explaination:

abramo@igor:/tmp$ cat a.c
template <typename>
void p() {
  typedef int x;
  sizeof(x);
}

int main() {
  p<int>();
}
abramo@igor:/tmp$ clang++-16 -cc1 -ast-dump -xc++ a.c
a.c:4:3: warning: expression result unused [-Wunused-value]
  sizeof(x);
  ^~~~~~~~~
a.c:4:3: warning: expression result unused [-Wunused-value]
  sizeof(x);
  ^~~~~~~~~
a.c:8:3: note: in instantiation of function template specialization 'p<int>' requested here
  p<int>();
  ^
TranslationUnitDecl 0x55df356fc8c8 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x55df356fd130 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x55df356fce90 '__int128'
|-TypedefDecl 0x55df356fd1a0 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x55df356fceb0 'unsigned __int128'
|-TypedefDecl 0x55df356fd518 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x55df356fd290 '__NSConstantString_tag'
|   `-CXXRecord 0x55df356fd1f8 '__NSConstantString_tag'
|-TypedefDecl 0x55df356fd5b0 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x55df356fd570 'char *'
|   `-BuiltinType 0x55df356fc970 'char'
|-TypedefDecl 0x55df35742aa8 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list '__va_list_tag[1]'
| `-ConstantArrayType 0x55df35742a50 '__va_list_tag[1]' 1 
|   `-RecordType 0x55df356fd6a0 '__va_list_tag'
|     `-CXXRecord 0x55df356fd608 '__va_list_tag'
|-FunctionTemplateDecl 0x55df35742ca8 <a.c:1:1, line:5:1> line:2:6 p
| |-TemplateTypeParmDecl 0x55df35742b00 <line:1:11> col:19 typename depth 0 index 0
| |-FunctionDecl 0x55df35742c08 <line:2:1, line:5:1> line:2:6 p 'void ()'
| | `-CompoundStmt 0x55df35742ed0 <col:10, line:5:1>
| |   |-DeclStmt 0x55df35742e30 <line:3:3, col:16>
| |   | `-TypedefDecl 0x55df35742dd8 <col:3, col:15> col:15 referenced x 'int'
| |   |   `-BuiltinType 0x55df356fc9d0 'int'
| |   `-UnaryExprOrTypeTraitExpr 0x55df35742eb0 <line:4:3, col:11> 'unsigned long' sizeof 'x':'int'
| `-FunctionDecl 0x55df357430e8 <line:2:1, line:5:1> line:2:6 used p 'void ()'
|   |-TemplateArgument type 'int'
|   | `-BuiltinType 0x55df356fc9d0 'int'
|   `-CompoundStmt 0x55df35743328 <col:10, line:5:1>
|     |-DeclStmt 0x55df35743310 <line:3:3, col:16>
|     | `-TypedefDecl 0x55df357432b8 <col:3, col:15> col:15 referenced x 'int'
|     |   `-BuiltinType 0x55df356fc9d0 'int'
|     `-UnaryExprOrTypeTraitExpr 0x55df35742eb0 <line:4:3, col:11> 'unsigned long' sizeof 'x':'int'
`-FunctionDecl 0x55df35742f40 <line:7:1, line:9:1> line:7:5 main 'int ()'
  `-CompoundStmt 0x55df357432a0 <col:12, line:9:1>
    `-CallExpr 0x55df35743280 <line:8:3, col:10> 'void'
      `-ImplicitCastExpr 0x55df35743268 <col:3, col:8> 'void (*)()' <FunctionToPointerDecay>
        `-DeclRefExpr 0x55df357431e0 <col:3, col:8> 'void ()' lvalue Function 0x55df357430e8 'p' 'void ()' (FunctionTemplate 0x55df35742ca8 'p')
2 warnings generated.
abramo@igor:/tmp$

If you compare the UnaryExprOrTypeTraitExpr in the template and the one in the instantation you will see that they have wrongly the same pointer.

This is due to the fact that the two inner types are not distinct despite referring two distinct TypedefDecl.

The bug is in bool TemplateInstantiator::AlreadyTransformed(QualType T) where it is returned true also for types that when instantiated would refer distinct (but not dependent) declarations.

[ldionne]

(sorry, no idea why it says "this revision is now accepted and ready to land on my behalf, I was just removing the libc++ review group to clear our review queue)

clang/include/clang/AST/Type.h

public:
   static StringRef getTagTypeKindName(TagTypeKind Kind) {
     return getKeywordName(getKeywordForTagTypeKind(Kind));
   }
 
   class CannotCastToThisType {};
   static CannotCastToThisType classof(const Type *);
 };
 
 /// Represents a type that was referred to using an elaborated type
 /// keyword, e.g., struct S, or via a qualified name, e.g., N::M::type,
 /// or both.
 ///
 /// This type is used to keep track of a type name as written in the
 /// source code, including tag keywords and any nested-name-specifiers.
 /// The type itself is always "sugar", used to express what was written
 /// in the source code but containing no additional semantic information.
 class ElaboratedType final
     : public TypeWithKeyword,
       public llvm::FoldingSetNode,
       private llvm::TrailingObjects<ElaboratedType, TagDecl *> {
   friend class ASTContext; // ASTContext creates these
   friend TrailingObjects;
 
   /// The nested name specifier containing the qualifier.
ElaboratedType(ElaboratedTypeKeyword Keyword, NestedNameSpecifier *NNS,
                                        toTypeDependence(NNS->getDependence()))
                                  : TypeDependence::None)),
         NNS(NNS), NamedType(NamedType) {
     ElaboratedTypeBits.HasOwnedTagDecl = false;
     if (OwnedTagDecl) {
       ElaboratedTypeBits.HasOwnedTagDecl = true;
       *getTrailingObjects<TagDecl *>() = OwnedTagDecl;
     }
-    assert(!(Keyword == ETK_None && NNS == nullptr) &&
-           "ElaboratedType cannot have elaborated type keyword "
-           "and name qualifier both null.");
   }
 
 public:
   /// Retrieve the qualification on this type.
   NestedNameSpecifier *getQualifier() const { return NNS; }
 
   /// Retrieve the type named by the qualified-id.
   QualType getNamedType() const { return NamedType; }

clang/lib/AST/FormatString.cpp

if (CS.isAnyIntArg() || CS.getKind() == ConversionSpecifier::nArg) {
     }
   }
 
   return None;
 }
 
 bool FormatSpecifier::namedTypeToLengthModifier(QualType QT,
                                                 LengthModifier &LM) {
-  assert(isa<TypedefType>(QT) && "Expected a TypedefType");
-  const TypedefNameDecl *Typedef = cast<TypedefType>(QT)->getDecl();
+  for (const TypedefNameDecl *Typedef;; QT = Typedef->getUnderlyingType()) {
+    const auto *TT = QT->getAs<TypedefType>();
+    if (!TT)
+      return false;
+    Typedef = TT->getDecl();
 
-  for (;;) {
     const IdentifierInfo *Identifier = Typedef->getIdentifier();
     if (Identifier->getName() == "size_t") {
       LM.setKind(LengthModifier::AsSizeT);
       return true;
     } else if (Identifier->getName() == "ssize_t") {
       // Not C99, but common in Unix.
       LM.setKind(LengthModifier::AsSizeT);
       return true;
     } else if (Identifier->getName() == "intmax_t") {
       LM.setKind(LengthModifier::AsIntMax);
       return true;
     } else if (Identifier->getName() == "uintmax_t") {
       LM.setKind(LengthModifier::AsIntMax);
       return true;
     } else if (Identifier->getName() == "ptrdiff_t") {
       LM.setKind(LengthModifier::AsPtrDiff);
       return true;
     }
-
-    QualType T = Typedef->getUnderlyingType();
-    if (!isa<TypedefType>(T))
-      break;
-
-    Typedef = cast<TypedefType>(T)->getDecl();
   }
-  return false;
 }

clang/lib/Sema/SemaDecl.cpp

static ParsedType recoverFromTypeInKnownDependentBase(Sema &S,
   TypeLocBuilder Builder;
   DependentNameTypeLoc DepTL = Builder.push<DependentNameTypeLoc>(T);
   DepTL.setNameLoc(NameLoc);
   DepTL.setElaboratedKeywordLoc(SourceLocation());
   DepTL.setQualifierLoc(SS.getWithLocInContext(Context));
   return S.CreateParsedType(T, Builder.getTypeSourceInfo(Context, T));
 }
 
+/// Build a ParsedType for a simple-type-specifier with a nested-name-specifier.
+static ParsedType buildNestedType(Sema &S, const CXXScopeSpec *SS, QualType T,
+                                  SourceLocation NameLoc,
+                                  bool WantNontrivialTypeSourceInfo = true) {
+  switch (T->getTypeClass()) {
+  case Type::DeducedTemplateSpecialization:
+  case Type::Enum:
+  case Type::InjectedClassName:
+  case Type::Record:
+  case Type::Typedef:
+  case Type::UnresolvedUsing:
+    break;
+  case Type::ObjCTypeParam:
+  case Type::TemplateTypeParm:
+    return ParsedType::make(T);
+  default:
+    llvm_unreachable("Unexpected Type Class");
+  }
+
+  if (!SS || !SS->isNotEmpty())
+    return ParsedType::make(
+        S.Context.getElaboratedType(ETK_None, nullptr, T, nullptr));
+
+  QualType ElTy = S.getElaboratedType(ETK_None, *SS, T);
+  if (!WantNontrivialTypeSourceInfo)
+    return ParsedType::make(ElTy);
+
+  TypeLocBuilder Builder;
+  Builder.pushTypeSpec(T).setNameLoc(NameLoc);
+  ElaboratedTypeLoc ElabTL = Builder.push<ElaboratedTypeLoc>(ElTy);
+  ElabTL.setElaboratedKeywordLoc(SourceLocation());
+  ElabTL.setQualifierLoc(SS->getWithLocInContext(S.Context));
+  return S.CreateParsedType(ElTy, Builder.getTypeSourceInfo(S.Context, ElTy));
+}
+
 /// If the identifier refers to a type name within this scope,
 /// return the declaration of that type.
 ///
 /// This routine performs ordinary name lookup of the identifier II
 /// within the given scope, with optional C++ scope specifier SS, to
 /// determine whether the name refers to a type. If so, returns an
 /// opaque pointer (actually a QualType) corresponding to that
 /// type. Otherwise, returns NULL.
ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
   }
 
   if (T.isNull()) {
     // If it's not plausibly a type, suppress diagnostics.
     Result.suppressDiagnostics();
     return nullptr;
   }
 
-  // NOTE: avoid constructing an ElaboratedType(Loc) if this is a
-  // constructor or destructor name (in such a case, the scope specifier
-  // will be attached to the enclosing Expr or Decl node).
-  if (SS && SS->isNotEmpty() && !IsCtorOrDtorName &&
-      !isa<ObjCInterfaceDecl, UnresolvedUsingIfExistsDecl>(IIDecl)) {
-    if (WantNontrivialTypeSourceInfo) {
-      // Construct a type with type-source information.
-      TypeLocBuilder Builder;
-      Builder.pushTypeSpec(T).setNameLoc(NameLoc);
-
-      T = getElaboratedType(ETK_None, *SS, T);
-      ElaboratedTypeLoc ElabTL = Builder.push<ElaboratedTypeLoc>(T);
-      ElabTL.setElaboratedKeywordLoc(SourceLocation());
-      ElabTL.setQualifierLoc(SS->getWithLocInContext(Context));
-      return CreateParsedType(T, Builder.getTypeSourceInfo(Context, T));
-    } else {
-      T = getElaboratedType(ETK_None, *SS, T);
-    }
-  }
-
+  if (isa<ObjCInterfaceDecl, UnresolvedUsingIfExistsDecl>(IIDecl))
     return ParsedType::make(T);
+  return buildNestedType(*this, SS, T, NameLoc, WantNontrivialTypeSourceInfo);
 }
 
 // Builds a fake NNS for the given decl context.
 static NestedNameSpecifier *
 synthesizeCurrentNestedNameSpecifier(ASTContext &Context, DeclContext *DC) {
   for (;; DC = DC->getLookupParent()) {
     DC = DC->getPrimaryContext();
     auto *ND = dyn_cast<NamespaceDecl>(DC);
if (TagDecl *Tag = R.getAsSingle<TagDecl>()) {
     Result.clear(Sema::LookupTagName);
     SemaRef.LookupParsedName(Result, S, &SS);
     return true;
   }
 
   return false;
 }
 
-/// Build a ParsedType for a simple-type-specifier with a nested-name-specifier.
-static ParsedType buildNestedType(Sema &S, CXXScopeSpec &SS,
-                                  QualType T, SourceLocation NameLoc) {
-  ASTContext &Context = S.Context;
-
-  TypeLocBuilder Builder;
-  Builder.pushTypeSpec(T).setNameLoc(NameLoc);
-
-  T = S.getElaboratedType(ETK_None, SS, T);
-  ElaboratedTypeLoc ElabTL = Builder.push<ElaboratedTypeLoc>(T);
-  ElabTL.setElaboratedKeywordLoc(SourceLocation());
-  ElabTL.setQualifierLoc(SS.getWithLocInContext(Context));
-  return S.CreateParsedType(T, Builder.getTypeSourceInfo(Context, T));
-}
-
 Sema::NameClassification Sema::ClassifyName(Scope *S, CXXScopeSpec &SS,
                                             IdentifierInfo *&Name,
                                             SourceLocation NameLoc,
                                             const Token &NextToken,
                                             CorrectionCandidateCallback *CCC) {
   DeclarationNameInfo NameInfo(Name, NameLoc);
   ObjCMethodDecl *CurMethod = getCurMethodDecl();
 
if (getLangOpts().CPlusPlus && NextToken.is(tok::less) &&
     return IsVarTemplate ? NameClassification::VarTemplate(Template)
                          : NameClassification::TypeTemplate(Template);
   }
 
   NamedDecl *FirstDecl = (*Result.begin())->getUnderlyingDecl();
   if (TypeDecl *Type = dyn_cast<TypeDecl>(FirstDecl)) {
     DiagnoseUseOfDecl(Type, NameLoc);
     MarkAnyDeclReferenced(Type->getLocation(), Type, /*OdrUse=*/false);
-    QualType T = Context.getTypeDeclType(Type);
-    if (SS.isNotEmpty())
-      return buildNestedType(*this, SS, T, NameLoc);
-    return ParsedType::make(T);
+    return buildNestedType(*this, &SS, Context.getTypeDeclType(Type), NameLoc);
   }
 
   ObjCInterfaceDecl *Class = dyn_cast<ObjCInterfaceDecl>(FirstDecl);
   if (!Class) {
     // FIXME: It's unfortunate that we don't have a Type node for handling this.
     if (ObjCCompatibleAliasDecl *Alias =
             dyn_cast<ObjCCompatibleAliasDecl>(FirstDecl))
       Class = Alias->getClassInterface();
Corrected:
   // seems likely a type is wanted instead of the non-type that was found.
   bool NextIsOp = NextToken.isOneOf(tok::amp, tok::star);
   if ((NextToken.is(tok::identifier) ||
        (NextIsOp &&
         FirstDecl->getUnderlyingDecl()->isFunctionOrFunctionTemplate())) &&
       isTagTypeWithMissingTag(*this, Result, S, SS, Name, NameLoc)) {
     TypeDecl *Type = Result.getAsSingle<TypeDecl>();
     DiagnoseUseOfDecl(Type, NameLoc);
-    QualType T = Context.getTypeDeclType(Type);
-    if (SS.isNotEmpty())
-      return buildNestedType(*this, SS, T, NameLoc);
-    return ParsedType::make(T);
+    return buildNestedType(*this, &SS, Context.getTypeDeclType(Type), NameLoc);
   }
 
   // If we already know which single declaration is referenced, just annotate
   // that declaration directly. Defer resolving even non-overloaded class
   // member accesses, as we need to defer certain access checks until we know
   // the context.
   bool ADL = UseArgumentDependentLookup(SS, Result, NextToken.is(tok::l_paren));
   if (Result.isSingleResult() && !ADL && !FirstDecl->isCXXClassMember())

clang/lib/Sema/SemaDeclCXX.cpp

if (!TyD) {
       if (!TyD && BaseType.isNull()) {
         Diag(IdLoc, diag::err_mem_init_not_member_or_class)
           << MemberOrBase << SourceRange(IdLoc,Init->getSourceRange().getEnd());
         return true;
       }
     }
 
     if (BaseType.isNull()) {
-      BaseType = Context.getTypeDeclType(TyD);
+      BaseType = getElaboratedType(ETK_None, SS, Context.getTypeDeclType(TyD));
       MarkAnyDeclReferenced(TyD->getLocation(), TyD, /*OdrUse=*/false);
       if (SS.isSet()) {
-        BaseType = Context.getElaboratedType(ETK_None, SS.getScopeRep(),
-                                             BaseType);
         TInfo = Context.CreateTypeSourceInfo(BaseType);
         ElaboratedTypeLoc TL = TInfo->getTypeLoc().castAs<ElaboratedTypeLoc>();
         TL.getNamedTypeLoc().castAs<TypeSpecTypeLoc>().setNameLoc(IdLoc);
         TL.setElaboratedKeywordLoc(SourceLocation());
         TL.setQualifierLoc(SS.getWithLocInContext(Context));
       }
     }
   }
return new (Context) CXXCtorInitializer(Context, TInfo, InitRange.getBegin(),
                                           DelegationInit.getAs<Expr>(),
                                           InitRange.getEnd());
 }
 
 MemInitResult
 Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
                            Expr *Init, CXXRecordDecl *ClassDecl,
                            SourceLocation EllipsisLoc) {
-  SourceLocation BaseLoc
-    = BaseTInfo->getTypeLoc().getLocalSourceRange().getBegin();
+  SourceLocation BaseLoc = BaseTInfo->getTypeLoc().getBeginLoc();
 
   if (!BaseType->isDependentType() && !BaseType->isRecordType())
     return Diag(BaseLoc, diag::err_base_init_does_not_name_class)
-             << BaseType << BaseTInfo->getTypeLoc().getLocalSourceRange();
+           << BaseType << BaseTInfo->getTypeLoc().getSourceRange();
 
   // C++ [class.base.init]p2:
   //   [...] Unless the mem-initializer-id names a nonstatic data
   //   member of the constructor's class or a direct or virtual base
   //   of that class, the mem-initializer is ill-formed. A
   //   mem-initializer-list can initialize a base class using any
   //   name that denotes that base class type.
 
if (!DirectBaseSpec && !VirtualBaseSpec) {
       // one of those types will resolve to the same type as
       // BaseType. Therefore, just treat this as a dependent base
       // class initialization.  FIXME: Should we try to check the
       // initialization anyway? It seems odd.
       if (ClassDecl->hasAnyDependentBases())
         Dependent = true;
       else
         return Diag(BaseLoc, diag::err_not_direct_base_or_virtual)
                << BaseType << Context.getTypeDeclType(ClassDecl)
-          << BaseTInfo->getTypeLoc().getLocalSourceRange();
+               << BaseTInfo->getTypeLoc().getSourceRange();
     }
   }
 
   if (Dependent) {
     DiscardCleanupsInEvaluationContext();
 
     return new (Context) CXXCtorInitializer(Context, BaseTInfo,
                                             /*IsVirtual=*/false,
for (const DeclaratorChunk &Chunk : llvm::reverse(D.type_objects())) {
     // Check that the return type is written as a specialization of
     // the template specified as the deduction-guide's name.
     ParsedType TrailingReturnType = Chunk.Fun.getTrailingReturnType();
     TypeSourceInfo *TSI = nullptr;
     QualType RetTy = GetTypeFromParser(TrailingReturnType, &TSI);
     assert(TSI && "deduction guide has valid type but invalid return type?");
     bool AcceptableReturnType = false;
     bool MightInstantiateToSpecialization = false;
-    if (auto RetTST =
-            TSI->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
+    TypeLoc TL = TSI->getTypeLoc();
+    // FIXME: in the future we can just require to see an ElaboratedType here.
+    if (auto ETL = TL.getAs<ElaboratedTypeLoc>())
+      TL = ETL.getNamedTypeLoc();
+
+    if (auto RetTST = TL.getAs<TemplateSpecializationTypeLoc>()) {
       TemplateName SpecifiedName = RetTST.getTypePtr()->getTemplateName();
       bool TemplateMatches =
           Context.hasSameTemplateName(SpecifiedName, GuidedTemplate);
       if (SpecifiedName.getKind() == TemplateName::Template && TemplateMatches)
         AcceptableReturnType = true;
       else {
         // This could still instantiate to the right type, unless we know it
         // names the wrong class template.
         auto *TD = SpecifiedName.getAsTemplateDecl();

clang/lib/Sema/SemaTemplate.cpp

TemplateSpecializationTypeLoc SpecTL
     = TLB.push<TemplateSpecializationTypeLoc>(Result);
   SpecTL.setTemplateKeywordLoc(TemplateKWLoc);
   SpecTL.setTemplateNameLoc(TemplateIILoc);
   SpecTL.setLAngleLoc(LAngleLoc);
   SpecTL.setRAngleLoc(RAngleLoc);
   for (unsigned i = 0, e = SpecTL.getNumArgs(); i != e; ++i)
     SpecTL.setArgLocInfo(i, TemplateArgs[i].getLocInfo());
 
-  // NOTE: avoid constructing an ElaboratedTypeLoc if this is a
-  // constructor or destructor name (in such a case, the scope specifier
-  // will be attached to the enclosing Decl or Expr node).
-  if (SS.isNotEmpty() && !IsCtorOrDtorName) {
-    // Create an elaborated-type-specifier containing the nested-name-specifier.
-    Result = Context.getElaboratedType(ETK_None, SS.getScopeRep(), Result);
-    ElaboratedTypeLoc ElabTL = TLB.push<ElaboratedTypeLoc>(Result);
-    ElabTL.setElaboratedKeywordLoc(SourceLocation());
-    ElabTL.setQualifierLoc(SS.getWithLocInContext(Context));
-  }
-
+  Result = getElaboratedType(ETK_None, SS, Result);
+  // FIXME: avoid constructing typeloc where SS is emtpy.
   return CreateParsedType(Result, TLB.getTypeSourceInfo(Context, Result));
 }
 
 TypeResult Sema::ActOnTagTemplateIdType(TagUseKind TUK,
                                         TypeSpecifierType TagSpec,
                                         SourceLocation TagLoc,
                                         CXXScopeSpec &SS,
                                         SourceLocation TemplateKWLoc,
                                         TemplateTy TemplateD,
                                         SourceLocation TemplateLoc,