[PATCH] D45463: [AST] Print correct tag decl for tag specifier

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.


================
Comment at: lib/AST/DeclPrinter.cpp:180-181
     if (isFirst) {
-      if(TD)
-        SubPolicy.IncludeTagDefinition = true;
+      if (TD)
+        SubPolicy.TagSpecifierAs = TD;
       SubPolicy.SuppressSpecifiers = false;
----------------
rsmith wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > jdenny wrote:
> > > > rsmith wrote:
> > > > > Instead of the changes in this patch, can you address your use case by just changing the `if` here to `if (TD && TD->isThisDeclarationADefinition())`?
> > > > It sounds like that would fix the case of duplicated definitions.
> > > > 
> > > > It doesn't address the case of dropped or relocated attributes.  For example, -ast-print drops the following attribute and thus the deprecation warning:
> > > > 
> > > > ```
> > > > void fn() {
> > > >   struct T *p0;
> > > >   struct __attribute__((deprecated)) T *p1;
> > > > }
> > > > ```
> > > > 
> > > > I don't know how clang *should* accept and interpret attributes in such cases.  However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.
> > > > I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.
> > > 
> > > This has been a *very* long-standing issue with -ast-print that I'd love to see fixed. The AST printer does not print attributes in the correct locations except by accident, and it oftentimes prints with the wrong syntax, or is missing from places where it should be printed.
> > > 
> > > Long story short, I agree that this behavior is wrong and needs to be fixed, but it's also going to take a bit of work to get it right.
> > > it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.
> > 
> > Historically we've viewed `-ast-print` as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix. However, one of the important and intended uses of the `-ast-print` mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.
> > 
> > > It doesn't address the case of dropped or relocated attributes.
> > 
> > Hmm. A big part of the problem here, in my view, is that when we come to print a `TagType`, we call `TagType::getDecl`, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type. We could easily fix that (by adding an accessor to reach `TagType::decl` rather than calling `TagType::getDecl()`), and perhaps *that* plus the current tracking of whether we're printing the tag declaration that is owned by the current `DeclGroup` would be enough to get correct displays here.
> > 
> > Another part of the problem is that `TagType` does not track whether it's referring to its "own" `TagDecl` (ie, one created when parsing that type) or some pre-existing one, and that affects whether and how attributes should be printed. That's why we need the `IncludeTagDefinition` flag in the first place; it's our attempt to recreate knowledge about whether the `TagType` owns its `TagDecl`. We could consider instead tracking a flag on `TagType` sugar nodes to model that, which would let us get rid of that flag.
> > Hmm. A big part of the problem here, in my view, is that when we come to print a TagType, we call TagType::getDecl, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type.
> 
> Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the `ElaboratedType` node could track that for us.
> Historically we've viewed -ast-print as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix. 

I've heard that, but it seemed to me that what's good for the latter is likely good for the former.

Still, I don't want to waste time on patches that won't be accepted because they're harder to justify purely for debugging purposes.  Is there a better place to explore pretty printing valid source code from a clang AST?

> However, one of the important and intended uses of the -ast-print mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

I haven't considered cases where diagnostics are broken by this issue.  For diagnostics, is it important to print the exact set of attributes on a particular declaration rather than to print, say, the entire list of attributes from all declarations of the tag?  The latter appears to be easier to implement.

> Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the ElaboratedType node could track that for us.

My approach was to constrain my changes to the -ast-print implementation as much as possible without changing the AST in order to avoid breaking something else.  I found that I could fix a big problem (printing the right tag decl at each specifier) just using the AST as it's already constructed.  DeclGroup gives us the info we need.  That info just isn't in the most convenient place, as I think you're pointing out.  (Also keep in mind that there's a child patch that fixes decl group discovery in some contexts not exercised by this patch.)

If you feel that ElaboratedType is a better way to go, I'll look into it and get back to you later.


https://reviews.llvm.org/D45463