[AST] Fix -ast-print for _Bool when have diagnostics

[jdenny]

Summary

For example, given:

#define bool _Bool
_Bool i;
void fn() { 1; }

-ast-print produced:

tmp.c:3:13: warning: expression result unused
void fn() { 1; }
            ^
bool i;
void fn() {
    1;
}

That fails to compile because bool is undefined.

Details:

Diagnostics print _Bool as bool when the latter is defined as the
former. However, diagnostics were altering the printing policy for
-ast-print as well. The printed source was then invalid because the
preprocessor eats the bool definition.

Problematic diagnostics included suppressed warnings (e.g., add
-Wno-unused-value to the above example), including those that are
suppressed by default.

This patch fixes this bug and cleans up some related comments.

[jdenny]

Rebased onto a more recent master.

[jdenny]

Rebased onto a more recent master.

[jdenny]

Rebased.

[jdenny]

Rebased. Added example to summary. Ping.

[aaron.ballman]

This approach generally looks good to me, but I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.

[jdenny]

In D45093#1087652, @aaron.ballman wrote:

This approach generally looks good to me, but I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.

Thanks for your comment. Give the discussion in D45463, it seems I need a more precise understanding of the purpose of -ast-print before I write any more fixes like these.

[rsmith]

If you want to force a particular printing policy to be used for -ast-print, I think it would be better to change the print call in lib/Frontend/ASTConsumers.cpp to pass your desired printing policy, rather than changing other components to prevent them from changing the ASTContext's default printing policy.

(For what it's worth, I think we should also try to change the way we print diagnostics so that Sema is asked for a printing policy when it's needed rather than it setting global ASTContext state each time we consider printing a diagnostic. That way we could also take the SourceLocation information into account to figure out whether bool is suitably #defined at the point of the diagnostic rather than checking whether it's defined at the current end-of-preprocessing state. But that's not really relevant for this patch, except that generally I think we should be moving away from stashing a global PrintingPolicy on the ASTContext.)

I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.

Given the discussion in D45463, it seems I need a more precise understanding of the purpose of -ast-print before I write any more fixes like these.

As things stand, I'd generally consider -ast-print to be a "best-effort" feature: we don't guarantee that it produces valid code, but we would prefer that it does. I do not think we yet have a clear argument that it's worth accepting costs elsewhere in order to solely improve -ast-print, but fortunately most improvements to -ast-print also improve our diagnostic quality, the usability of our AST in tooling scenarios, or some other property that we do care about, so this issue seldom arises. (For example, in the context of D45463, information on whether a declaration owns a TagDecl is directly useful to tooling (for instance, refactoring tools care how a multi-declarator declaration was written so that they can properly rewrite it), as well as being useful for -ast-print fidelity. So it would make sense to consider how to better represent this information in the AST so that all the interested users have access to it.)

However... Clang is an open-source meritocracy. As such, the goals of the Clang project are a synthesized amalgam of the goals of the Clang contributors. If you want to take ownership of the AST printer and, for instance, make it always generate valid code, we can discuss the technical merits of that (use cases, benefit to you and to other users, maintenance costs, etc) even if the original purpose of the flag did not extend that far.

[jdenny]

In D45093#1090292, @rsmith wrote:

If you want to force a particular printing policy to be used for -ast-print, I think it would be better to change the print call in lib/Frontend/ASTConsumers.cpp to pass your desired printing policy, rather than changing other components to prevent them from changing the ASTContext's default printing policy.

Thanks. I'll look into that.

(For what it's worth, I think we should also try to change the way we print diagnostics so that Sema is asked for a printing policy when it's needed rather than it setting global ASTContext state each time we consider printing a diagnostic. That way we could also take the SourceLocation information into account to figure out whether bool is suitably #defined at the point of the diagnostic rather than checking whether it's defined at the current end-of-preprocessing state. But that's not really relevant for this patch, except that generally I think we should be moving away from stashing a global PrintingPolicy on the ASTContext.)

I'll keep that in mind in case I have more work in that area.

I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.

Given the discussion in D45463, it seems I need a more precise understanding of the purpose of -ast-print before I write any more fixes like these.

As things stand, I'd generally consider -ast-print to be a "best-effort" feature: we don't guarantee that it produces valid code, but we would prefer that it does. I do not think we yet have a clear argument that it's worth accepting costs elsewhere in order to solely improve -ast-print, but fortunately most improvements to -ast-print also improve our diagnostic quality, the usability of our AST in tooling scenarios, or some other property that we do care about, so this issue seldom arises. (For example, in the context of D45463, information on whether a declaration owns a TagDecl is directly useful to tooling (for instance, refactoring tools care how a multi-declarator declaration was written so that they can properly rewrite it), as well as being useful for -ast-print fidelity. So it would make sense to consider how to better represent this information in the AST so that all the interested users have access to it.)

Thanks. That helps.

However... Clang is an open-source meritocracy. As such, the goals of the Clang project are a synthesized amalgam of the goals of the Clang contributors. If you want to take ownership of the AST printer and, for instance, make it always generate valid code, we can discuss the technical merits of that (use cases, benefit to you and to other users, maintenance costs, etc) even if the original purpose of the flag did not extend that far.

For the moment, I'll just fix issues as I encounter them. If I decide -ast-print fidelity is vital to my work in the long term, or if my fixes introduce problematic costs so that our goals for -ast-print then conflict, then we can have that larger discussion.

I'm not sure I'm experienced enough to take ownership of -ast-print just yet. However, if there are as many fidelity issues as you've suggested, I'll probably get there eventually.

[jdenny]

Made the suggested changes.

[aaron.ballman]

LGTM!

include/clang/Sema/Sema.h

public:
   void checkSpecializationVisibility(SourceLocation Loc, NamedDecl *Spec);
 
   /// We've found a use of a template specialization that would select a
   /// partial specialization. Check that the partial specialization is visible,
   /// and diagnose if not.
   void checkPartialSpecializationVisibility(SourceLocation Loc,
                                             NamedDecl *Spec);
 
-  /// Retrieve a suitable printing policy.
+  /// Retrieve a suitable printing policy for diagnostics.
   PrintingPolicy getPrintingPolicy() const {
     return getPrintingPolicy(Context, PP);
   }
 
-  /// Retrieve a suitable printing policy.
+  /// Retrieve a suitable printing policy for diagnostics.
   static PrintingPolicy getPrintingPolicy(const ASTContext &Ctx,
                                           const Preprocessor &PP);
 
   /// Scope actions.
   void ActOnPopScope(SourceLocation Loc, Scope *S);
   void ActOnTranslationUnitScope(Scope *S);
 
   Decl *ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,

lib/Frontend/ASTConsumers.cpp

void print(Decl *D) {
         if (DeclContext *DC = dyn_cast<DeclContext>(D)) {
           if (DC == DC->getPrimaryContext())
             DC->dumpLookups(Out, OutputKind != None, OutputKind == DumpFull);
           else
             Out << "Lookup map is in primary DeclContext "
                 << DC->getPrimaryContext() << "\n";
         } else
           Out << "Not a DeclContext\n";
-      } else if (OutputKind == Print)
-        D->print(Out, /*Indentation=*/0, /*PrintInstantiation=*/true);
-      else if (OutputKind != None)
+      } else if (OutputKind == Print) {
+        PrintingPolicy Policy(D->getASTContext().getLangOpts());
+        D->print(Out, Policy, /*Indentation=*/0, /*PrintInstantiation=*/true);
+      } else if (OutputKind != None)
         D->dump(Out, OutputKind == DumpFull);
     }
 
     raw_ostream &Out;
     std::unique_ptr<raw_ostream> OwnedOut;
 
     /// How to output individual declarations.
     Kind OutputKind;

lib/Sema/Sema.cpp

SourceLocation Sema::getLocForEndOfToken(SourceLocation Loc, unsigned Offset) {
   return Lexer::getLocForEndOfToken(Loc, Offset, SourceMgr, LangOpts);
 }
 
 ModuleLoader &Sema::getModuleLoader() const { return PP.getModuleLoader(); }
 
 PrintingPolicy Sema::getPrintingPolicy(const ASTContext &Context,
                                        const Preprocessor &PP) {
   PrintingPolicy Policy = Context.getPrintingPolicy();
-  // Our printing policy is copied over the ASTContext printing policy whenever
-  // a diagnostic is emitted, so recompute it.
+  // In diagnostics, we print _Bool as bool if the latter is defined as the
+  // former.
   Policy.Bool = Context.getLangOpts().Bool;
   if (!Policy.Bool) {
     if (const MacroInfo *BoolMacro = PP.getMacroInfo(Context.getBoolName())) {
       Policy.Bool = BoolMacro->isObjectLike() &&
                     BoolMacro->getNumTokens() == 1 &&
                     BoolMacro->getReplacementToken(0).is(tok::kw__Bool);
     }
   }
case DiagnosticIDs::SFINAE_Suppress:
 
       // Suppress this diagnostic.
       Diags.setLastDiagnosticIgnored();
       Diags.Clear();
       return;
     }
   }
 
-  // Set up the context's printing policy based on our current state.
+  // Copy the diagnostic printing policy over the ASTContext printing policy.
+  // TODO: Stop doing that.  See: https://reviews.llvm.org/D45093#1090292
   Context.setPrintingPolicy(getPrintingPolicy());
 
   // Emit the diagnostic.
   if (!Diags.EmitCurrentDiagnostic())
     return;
 
   // If this is not a note, and we're in a template instantiation
   // that is different from the last template instantiation where

test/Misc/ast-print-bool.c

+// RUN: %clang_cc1 -verify -ast-print %s -xc -DDEF_BOOL_CBOOL \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-CBOOL,CBOOL
+//
+// RUN: %clang_cc1 -verify -ast-print %s -xc -DDEF_BOOL_CBOOL -DDIAG \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-CBOOL,CBOOL
+//
+// RUN: %clang_cc1 -verify -ast-print %s -xc -DDEF_BOOL_INT \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-INT,CBOOL
+//
+// RUN: %clang_cc1 -verify -ast-print %s -xc -DDEF_BOOL_INT -DDIAG \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-INT,CBOOL
+//
+// RUN: %clang_cc1 -verify -ast-print %s -xc++ \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-BOOL
+//
+// RUN: %clang_cc1 -verify -ast-print %s -xc++ -DDIAG \
+// RUN: | FileCheck %s --check-prefixes=BOOL-AS-BOOL
+
+#if DEF_BOOL_CBOOL
+# define bool _Bool
+#elif DEF_BOOL_INT
+# define bool int
+#endif
+
+// BOOL-AS-CBOOL: _Bool i;
+// BOOL-AS-INT:   int i;
+// BOOL-AS-BOOL:  bool i;
+bool i;
+
+#ifndef __cplusplus
+// CBOOL: _Bool j;
+_Bool j;
+#endif
+
+// Induce a diagnostic (and verify we actually managed to do so), which used to
+// permanently alter the -ast-print printing policy for _Bool.  How bool is
+// defined by the preprocessor is examined only once per compilation, when the
+// diagnostic is emitted, and it used to affect the entirety of -ast-print, so
+// test only one definition of bool per compilation.
+#if DIAG
+void fn() { 1; } // expected-warning {{expression result unused}}
+#else
+// expected-no-diagnostics
+#endif