We are seeing an assertion failure in one of Fuchsia's builders due to this:

Can you please attach the crash reproducer which clang should have generated? Thanks.

After this commit, the comment in clang/include/clang/AST/Decl.h for clang::TypeDecl::getTypeForDecl,

  // Low-level accessor. If you just want the type defined by this node,
  // check out ASTContext::getTypeDeclType or one of
  // ASTContext::getTypedefType, ASTContext::getRecordType, etc. if you
  // already know the specific kind of node this is.

isn't accurate any more, there's no more ASTContext::getRecordType.

Observation using clangd: In the following construct
...
The "arcana" field of the AST node of "Foo" inside the function reports the "QualType" as "Foo", whereas before it was "N::Foo". Is that expected?

It looks correct in that this is the as-written type.

Now I am not sure clangd wants to present the type that way, maybe they want to fully qualify it for printing instead? @kadircet @hokein

clangd doesn't really have a preference here, we just surface -ast-dump in a editor friendly format, for debugging purposes.

one can repro the situation with clang -xc++ -fsyntax-only -Xclang=-ast-dump a.cc

where a.cc is:

namespace N {
struct Foo {};
}  // namespace N
void f() {
  using namespace N;
  Foo foo;
  N::Foo x;
}

pre this change output looks like:

`-FunctionDecl 0x5621625282a0 <line:4:1, line:8:1> line:4:6 f 'void ()'
  `-CompoundStmt 0x562162528d10 <col:10, line:8:1>
    |-DeclStmt 0x5621625283e8 <line:5:3, col:20>
    | `-UsingDirectiveDecl 0x562162528390 <col:3, col:19> col:19 Namespace 0x562162528018 'N'
    |-DeclStmt 0x562162528b88 <line:6:3, col:10>
    | `-VarDecl 0x562162528440 <col:3, col:7> col:7 foo 'Foo':'N::Foo' callinit
    |   `-CXXConstructExpr 0x562162528b60 <col:7> 'Foo':'N::Foo' 'void () noexcept'
    `-DeclStmt 0x562162528cf8 <line:7:3, col:11>
      `-VarDecl 0x562162528c38 <col:3, col:10> col:10 x 'N::Foo' callinit
        `-CXXConstructExpr 0x562162528cd0 <col:10> 'N::Foo' 'void () noexcept'

emphasis on -VarDecl 0x562162528440 <col:3, col:7> col:7 foo 'Foo':'N::Foo' callinit

after thins change this is now reported as VarDecl 0x5652a7615860 <col:3, col:7> col:7 foo 'Foo' callinit

this was handled by https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L909-L926, I am not sure if change is intended (i think we should still be able to detect & desugar)

The new TraverseQualifier argument through the whole Traverse.*Type hierarchy ...

@mizvekov, anyway, it would be good to know if TraverseQualifier is a customization point for users who implement their own RecursiveASTVisitor subclasses, or the default argument value should always be true? There is an assertion inside TraverseQualifiedTypeLoc:

After this commit, the comment in clang/include/clang/AST/Decl.h for clang::TypeDecl::getTypeForDecl,
isn't accurate any more, there's no more ASTContext::getRecordType.

Thanks for reporting, I'll fix in a follow up patch.

Observation using clangd: In the following construct
this was handled by https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L909-L926, I am not sure if change is intended (i think we should still be able to detect & desugar)

Right, the difference here is that before this patch, if you print a fully desugared tag type (that's what the 'aka' in the diagnostic does), the type is printed fully qualified. That's because the name qualifiers were carried in the 'ElaboratedType' sugar node, and we used to handle this situation specially: we assumed a bare tag type only occurred because of canonicalization.

With this patch the tag types themselves carry the name qualifier, so just removing the top level sugar doesn't make them lose name qualifier information.

We still fully qualify canonical tag types when printing them though, but that now happens because a canonical tag type is a special value which is not identical to any other tag type for the same declaration.

We may still want to fully qualify types for the 'aka' diagnostic, but I deferred that for a follow up patch in order to limit the amount of test churn.

For clangd, I assume the same thing is happening here, for this arcana" field, you are fully desugaring the type before printing it.

@mizvekov, anyway, it would be good to know if TraverseQualifier is a customization point for users who implement their own RecursiveASTVisitor subclasses, or the default argument value should always be true?

The TraverseQualifier flag means, 'should I traverse NestedNameSpecifiers?'

If you are simply customizing one of the Traverse* methods which take it, but still delegating to the base class Traverse method, you should just forward that flag along to the base method.

If you are implementing your own Traverse method for a type from scratch, then if this flag is false, you should not traverse the nested name specifier for that type.

There is an assertion inside TraverseQualifiedTypeLoc:

llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h

Line 1282 in 72b53cd

assert(TraverseQualifier &&

However, I don't see where in the world QualifiedTypeLoc is created...

QualifiedTypeLoc is created implicitly for QualTypes which have top level qualifiers, and they represent the source locations for type qualifiers (not name qualifiers, different thing.)

This assert here means that a top level type qualifier can't appear within a nested name specifier (but it could appear for example in the underlying type for a typedef).

The C++ grammar doesn't allow you to write const / volatile in a way which applies to an entity in the name qualifier itself.

While such a NestedNameSpecifier could still be produced as part of template argument deduction, we simply remove the top level qualifiers in that case, as the NestedNameSpecifier AST node itself still can't represent it.

We are seeing an assertion failure in one of Fuchsia's builders due to this:

Can you please attach the crash reproducer which clang should have generated? Thanks.

https://storage.googleapis.com/fuchsia-artifacts/builds/8707018386865583409/build-debug/clang-crashreports/zircon-startup-bad996.tar

@mizvekov, thanks for the clarifying! In fact, it means that at least TraverseTypeLoc should not, in general, be given false as TraverseQualifier argument because (it looks like) nothing inside RecursiveASTVisitor changes it to true before TraverseQualifiedTypeLoc call. Note that it may be called not only from the NNS traversal.

@mizvekov, thanks for the clarifying! In fact, it means that at least TraverseTypeLoc should not, in general, be given false as TraverseQualifier argument because (it looks like) nothing inside RecursiveASTVisitor changes it to true before TraverseQualifiedTypeLoc call. Note that it may be called not only from the NNS traversal.

Yeah, it may only given false in the implementation of TraverseNestedNameSpecifier, which for out-of-tree users is only relevant if you are overriding that. If you mistakenly passed true instead, the name qualifiers would be traversed twice.

I mean that I cannot do this:

struct MyVisitor : RecursiveASTVisitor<MyVisitor> {
  using Base = RecursiveASTVisitor<MyVisitor>;

  bool TraverseTypeLoc(TypeLoc TL, bool TraverseQualifier = false) {
    return Base::TraverseTypeLoc(TL, TraverseQualifier);
  }
};

Calling MyVisitor::TraverseTypeLoc on e.g. const int would cause the assertion failure.

We are seeing an assertion failure in one of Fuchsia's builders due to this:

Can you please attach the crash reproducer which clang should have generated? Thanks.

https://storage.googleapis.com/fuchsia-artifacts/builds/8707018386865583409/build-debug/clang-crashreports/zircon-startup-bad996.tar

Minimized this using C-Vise to 620 bytes: https://pastebin.com/yegBAfpe

Minimized this using C-Vise to 620 bytes: https://pastebin.com/yegBAfpe

Thanks, that has been fixed here: #153344

The following fails as well:

struct MyVisitor : RecursiveASTVisitor<MyVisitor> {
} visitor;
visitor.TraverseTypeLoc(typeloc, false);

if the given typeloc is const int.

So you insist that TraverseQualifier is not a customization point, and users should keep the default argument of both TraverseType and TraverseTypeLoc untouched? It is sad a little because IWYU needs to avoid NNS traversal in some places. Previously, it was achieved just by desugaring ElaboratedTypeLoc. Now, I tried to do that by means of passing false as the TraverseQualifier argument, and it even seems to work because IWYU AST visitor transforms QualifiedTypeLoc into UnqualTypeLoc, and the assertion is not reached. However, I may be missing some of consequences, or if it is just "not a documented way", then we need to implement our own flag to block the traversal inside an own overload of TraverseNestedNameSpecifier...

If you want to avoid traversal of NNSes, why don't you just override TraverseNestedNameSpecifier{,Loc} with no-ops?
I think that's the intended way to do it.

They should be avoided not always but in certain circumstances, so there should be a new flag field in the visitor class. Not many new code, TBH, but using TraverseQualifier was so tempting...

We can make it work, we could move the assert elsewhere.

The assert doesn't hurt us, as I've said. I just asked if it is a "documented way" of working with the new visitor interface. If it is not to be avoided, then we will probably just proceed with the TraverseQualifier manipulation.

It would be hard for us to document this as an intended interface, but leave that assert in the way, even if you found yourself a way to avoid it. This wouldn't look very polished.

@kimgr, what do you think?