[libcxx-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

Wed Jul 13 16:12:42 PDT 2022

mizvekov added a comment.

In D112374#3648624 <https://reviews.llvm.org/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 usually 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 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112374/new/

https://reviews.llvm.org/D112374