Issue 1092 in include-what-you-use: Breaking change on Clang mainline

0 views
Skip to first unread message

notifi...@include-what-you-use.org

unread,
Aug 6, 2022, 6:13:42 AMAug 6
to include-wh...@googlegroups.com
New issue 1092 by kimgr: Breaking change on Clang mainline
https://github.com/include-what-you-use/include-what-you-use/issues/1092

[The patch eventually committed as 15f3cd6bfc670b](https://github.com/llvm/llvm-project/commit/15f3cd6bfc670ba6106184a903eb04be059e5977) breaks IWYU in several different ways. I haven't isolated all of them yet, so I'll use this issue to keep a log of what's broken, and potential fixes.

Original review and some discussion around dealing with breakage: https://reviews.llvm.org/D112374.

This landed after the branch of llvm-15, so IWYU users can still build IWYU mainline with the llvm-14 or llvm-15 branches.


notifi...@include-what-you-use.org

unread,
Aug 15, 2022, 2:44:52 PMAug 15
to include-wh...@googlegroups.com
Comment #1 on issue 1092 by kimgr: Breaking change on Clang mainline
https://github.com/include-what-you-use/include-what-you-use/issues/1092

I just want to briefly describe what's happened here, and why it's taking so long to fix.

The Clang-side change modified the construction of the AST so that every `Type`-derived node in the tree is wrapped in an `ElaboratedType`. Elaborated type nodes were previously only inserted for namespace-qualified types (e.g. `ns::foo::Bar`) or elaborated type specifiers (e.g. `struct Foo` or `class Bar`).

The rationale is that an `ElaboratedType` with specifier `ETK_None` is a clearer indication that the type lacks an elaborated type specifier than just the absence of `ElaboratedType`, and type printing for Clang diagnostics has been improved significantly after this change (i.e. types are printed as they were written in source code, not accidentally elaborated or not).

Because `ElaboratedType` nodes were previously uncommon, we had ad-hoc handling of them where they might show up, and only some cases where we explicitly look at them to control use policy (e.g. the parameter in `void foo(class const Bar&);` counts as a forward-declaration in and of itself).

For individual nodes, this is not so much of a problem. But IWYU does much of its AST work looking at the _shape of subtrees_ in the AST, and the presence of a new `ElaboratedType` node breaks a lot of assumptions.

`ElaboratedType` like a few other AST node types, is colloquially called "sugar", and can mostly be ignored.

I'm making inventory to try and understand what the different cases are, and see if there's a systematic way to solve each one. Here's what I have so far:

* Getting a `Type*` from somewhere (e.g. an expression) and checking if it `isa<SomeType>` or can be `dyn_cast<SomeOtherType>` is no longer viable. It seems the safe way to look through sugar for cases like this is to do `auto x = type->getAs<SomeType>();`
* Given two `Type*`s, we can't generally compare them for identity without first stripping off sugar, using e.g. `t1->getUnqualifiedDesugaredType() == t2->getUnqualifiedDesugaredType()` or maybe using IWYU's `GetCanonicalType`: `GetCanonicalType(t1) == GetCanonicalType(t2)`
* Since the AST has changed shape, we can't generally assume that the `ASTNode::*Parent*` methods will do what we expect -- e.g. `ast_node->IsA<TypedefType> && ast_node->ParentIsA<TypedefDecl>` won't hold if the current AST node is wrapped in `ElaboratedType`.
* IWYU's resugar machinery is heavily based on `Type*` identity, so much of our template instantiation handling is currently confused.
* We need to pay attention not to desugar _too much_; sometimes we need to carefully unravel sugar wrapping to report a use of both a typedef and the underlying type, for example.

I have a couple of fixups that are strict improvements, but fixes for the remaining issues seem to cause oscillation where fixing one testcase breaks another. More information as I have it.


notifi...@include-what-you-use.org

unread,
Aug 16, 2022, 12:12:52 PMAug 16
to include-wh...@googlegroups.com
Comment #1 on issue 1092 by kimgr: Breaking change on Clang mainline
https://github.com/include-what-you-use/include-what-you-use/issues/1092

I just want to briefly describe what's happened here, and why it's taking so long to fix.

The Clang-side change modified the construction of the AST so that every `Type`-derived node in the tree is wrapped in an `ElaboratedType`. Elaborated type nodes were previously only inserted for namespace-qualified types (e.g. `ns::foo::Bar`) or elaborated type specifiers (e.g. `struct Foo` or `class Bar`).

The rationale is that an `ElaboratedType` with specifier `ETK_None` is a clearer indication that the type lacks an elaborated type specifier than just the absence of `ElaboratedType`, and type printing for Clang diagnostics has been improved significantly after this change (i.e. types are printed as they were written in source code, not accidentally elaborated or not).

Because `ElaboratedType` nodes were previously uncommon, we had ad-hoc handling of them where they might show up, and only some cases where we explicitly look at them to control use policy (e.g. the parameter in `void foo(class Bar*);` counts as a forward-declaration in and of itself).
Reply all
Reply to author
Forward
0 new messages