[clang] consistently quote expressions in diagnostics

This changes the expression diagnostic printer to always add quotes,
and removes hardcoded quotes from the diagnostic format strings.

In some cases, a placeholder could be filled by either an expression
or as a string. In order to quote this consistently, a new modifier
was added, which can be used to quote strings.

One diagnostic was relying on unquoted expressions in order to
generate code suggestions. This diagnostic is converted to use fixit
hints instead.

Author: mizvekov

clang/docs/InternalsManual.rst

@@ -416,6 +416,17 @@ Description:
   and the modifier indexes in the substitution are re-numbered accordingly. The
   substituted text must itself be a valid format string before substitution.
 
+**"quoted" format**
+
+Example:
+  ``"expression %quoted0 evaluates to 0"``
+Class:
+  ``String``
+Description:
+  This is a simple formatter which adds quotes around the given string.
+  This is useful when the argument could be a string in some cases, but
+  another class in other cases, and it needs to be quoted consistently.
+
 .. _internals-producing-diag:
 
 Producing the Diagnostic

clang/docs/ReleaseNotes.rst

@@ -314,6 +314,7 @@ Improvements to Clang's diagnostics
 - Clang now respects the current language mode when printing expressions in
   diagnostics. This fixes a bunch of `bool` being printed as `_Bool`, and also
   a bunch of HLSL types being printed as their C++ equivalents.
+- Clang now consistently quotes expressions in diagnostics.
 - When printing types for diagnostics, clang now doesn't suppress the scopes of
   template arguments contained within nested names.
 - The ``-Wshift-bool`` warning has been added to warn about shifting a boolean. (#GH28334)

clang/include/clang/Basic/DiagnosticASTKinds.td

@@ -143,8 +143,8 @@ def note_constexpr_null_subobject : Note<
   "access array element of|perform pointer arithmetic on|"
   "access real component of|"
   "access imaginary component of}0 null pointer">;
-def note_constexpr_null_callee : Note<
-  "'%0' evaluates to a null function pointer">;
+def note_constexpr_null_callee
+    : Note<"%0 evaluates to a null function pointer">;
 def note_constexpr_function_param_value_unknown : Note<
   "function parameter %0 with unknown value cannot be used in a constant "
   "expression">;

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1639,7 +1639,7 @@ def note_omp_ctx_compatible_set_and_selector_for_property
            "'match(%2={%1(%0)})'">;
 def warn_omp_ctx_incompatible_score_for_property
     : Warning<"the context selector '%0' in the context set '%1' cannot have a "
-              "score ('%2'); score ignored">,
+              "score (%quoted2); score ignored">,
       InGroup<OpenMPClauses>;
 def warn_omp_more_one_device_type_clause
     : Warning<"more than one 'device_type' clause is specified">,

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -167,9 +167,10 @@ def err_expr_not_string_literal : Error<"expression is not a string literal">;
 def ext_predef_outside_function : Warning<
   "predefined identifier is only valid inside function">,
   InGroup<DiagGroup<"predefined-identifier-outside-function">>;
-def ext_init_from_predefined : ExtWarn<
-  "initializing an array from a '%0' predefined identifier is a Microsoft extension">,
-  InGroup<MicrosoftInitFromPredefined>;
+def ext_init_from_predefined
+    : ExtWarn<"initializing an array from a %0 predefined identifier is a "
+              "Microsoft extension">,
+      InGroup<MicrosoftInitFromPredefined>;
 def ext_string_literal_from_predefined : ExtWarn<
   "expansion of predefined identifier '%0' to a string literal is a Microsoft extension">,
   InGroup<MicrosoftStringLiteralFromPredefined>;
@@ -3021,14 +3022,14 @@ def note_substituted_constraint_expr_is_ill_formed : Note<
   "because substituted constraint expression is ill-formed%0">;
 def note_constraint_references_error
     : Note<"constraint depends on a previously diagnosed expression">;
-def note_atomic_constraint_evaluated_to_false : Note<
-  "%select{and|because}0 '%1' evaluated to false">;
-def note_concept_specialization_constraint_evaluated_to_false : Note<
-  "%select{and|because}0 '%1' evaluated to false">;
+def note_atomic_constraint_evaluated_to_false
+    : Note<"%select{and|because}0 %1 evaluated to false">;
+def note_concept_specialization_constraint_evaluated_to_false
+    : Note<"%select{and|because}0 %1 evaluated to false">;
 def note_single_arg_concept_specialization_constraint_evaluated_to_false : Note<
   "%select{and|because}0 %1 does not satisfy %2">;
-def note_atomic_constraint_evaluated_to_false_elaborated : Note<
-  "%select{and|because}0 '%1' (%2 %3 %4) evaluated to false">;
+def note_atomic_constraint_evaluated_to_false_elaborated
+    : Note<"%select{and|because}0 %1 (%2 %3 %4) evaluated to false">;
 def note_is_deducible_constraint_evaluated_to_false : Note<
   "cannot deduce template arguments for %0 from %1">;
 def err_constrained_virtual_method : Error<
@@ -3048,14 +3049,14 @@ def note_expr_requirement_expr_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_expr_requirement_expr_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
-def note_expr_requirement_noexcept_not_met : Note<
-  "%select{and|because}0 '%1' may throw an exception">;
+def note_expr_requirement_noexcept_not_met
+    : Note<"%select{and|because}0 %1 may throw an exception">;
 def note_expr_requirement_type_requirement_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_expr_requirement_type_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
-def note_expr_requirement_constraints_not_satisfied : Note<
-  "%select{and|because}0 type constraint '%1' was not satisfied:">;
+def note_expr_requirement_constraints_not_satisfied
+    : Note<"%select{and|because}0 type constraint %1 was not satisfied:">;
 def note_expr_requirement_constraints_not_satisfied_simple : Note<
   "%select{and|because}0 %1 does not satisfy %2:">;
 def note_type_requirement_substitution_error : Note<
@@ -4356,9 +4357,9 @@ def note_reference_is_return_value : Note<"%0 returns a reference">;
 
 def note_pointer_declared_here : Note<
   "pointer %0 declared here">;
-def warn_division_sizeof_ptr : Warning<
-  "'%0' will return the size of the pointer, not the array itself">,
-  InGroup<DiagGroup<"sizeof-pointer-div">>;
+def warn_division_sizeof_ptr
+    : Warning<"%0 will return the size of the pointer, not the array itself">,
+      InGroup<DiagGroup<"sizeof-pointer-div">>;
 def warn_division_sizeof_array : Warning<
   "expression does not compute the number of elements in this array; element "
   "type is %0, not %1">,
@@ -5581,8 +5582,9 @@ def note_function_member_spec_matched : Note<
 def err_template_recursion_depth_exceeded : Error<
   "recursive template instantiation exceeded maximum depth of %0">,
   DefaultFatal, NoSFINAE;
-def err_constraint_depends_on_self : Error<
-  "satisfaction of constraint '%0' depends on itself">, NoSFINAE;
+def err_constraint_depends_on_self
+    : Error<"satisfaction of constraint %0 depends on itself">,
+      NoSFINAE;
 def note_template_recursion_depth : Note<
   "use -ftemplate-depth=N to increase recursive template instantiation depth">;
 
@@ -7098,8 +7100,8 @@ def warn_precedence_bitwise_rel : Warning<
   InGroup<Parentheses>;
 def note_precedence_bitwise_first : Note<
   "place parentheses around the %0 expression to evaluate it first">;
-def note_precedence_silence : Note<
-  "place parentheses around the '%0' expression to silence this warning">;
+def note_precedence_silence : Note<"place parentheses around the %quoted0 "
+                                   "expression to silence this warning">;
 
 def warn_precedence_conditional : Warning<
   "operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
@@ -7117,9 +7119,11 @@ def warn_consecutive_comparison : Warning<
 def warn_enum_constant_in_bool_context : Warning<
   "converting the enum constant to a boolean">,
   InGroup<IntInBoolContext>, DefaultIgnore;
-def warn_left_shift_in_bool_context : Warning<
-  "converting the result of '<<' to a boolean; did you mean '(%0) != 0'?">,
-  InGroup<IntInBoolContext>, DefaultIgnore;
+def warn_left_shift_in_bool_context
+    : Warning<"converting the result of '<<' to a boolean; did you mean to "
+              "compare with '0'?">,
+      InGroup<IntInBoolContext>,
+      DefaultIgnore;
 def warn_logical_instead_of_bitwise : Warning<
   "use of logical '%0' with constant operand">,
   InGroup<DiagGroup<"constant-logical-operand">>;
@@ -10331,9 +10335,11 @@ def warn_dangling_pointer_assignment : Warning<
    "object backing %select{|the pointer }0%1 "
    "will be destroyed at the end of the full-expression">,
    InGroup<DanglingAssignment>;
-def warn_dangling_reference_captured : Warning<
-   "object whose reference is captured by '%0' will be destroyed at the end of "
-   "the full-expression">, InGroup<DanglingCapture>;
+def warn_dangling_reference_captured
+    : Warning<"object whose reference is captured by %0 will be destroyed at "
+              "the end of "
+              "the full-expression">,
+      InGroup<DanglingCapture>;
 def warn_dangling_reference_captured_by_unknown : Warning<
    "object whose reference is captured will be destroyed at the end of "
    "the full-expression">, InGroup<DanglingCapture>;
@@ -13038,15 +13044,15 @@ def note_acc_atomic_too_many_stmts
 def note_acc_atomic_expected_binop : Note<"expected binary operation on right "
                                           "hand side of assignment operator">;
 def note_acc_atomic_mismatch_operand
-    : Note<"left hand side of assignment operation('%0') must match one side "
-           "of the sub-operation on the right hand side('%1' and '%2')">;
+    : Note<"left hand side of assignment operation(%0) must match one side "
+           "of the sub-operation on the right hand side(%1 and %2)">;
 def note_acc_atomic_mismatch_compound_operand
     : Note<"variable %select{|in unary expression|on right hand side of "
            "assignment|on left hand side of assignment|on left hand side of "
-           "compound assignment|on left hand side of assignment}2('%3') must "
+           "compound assignment|on left hand side of assignment}2(%3) must "
            "match variable used %select{|in unary expression|on right hand "
            "side of assignment|<not possible>|on left hand side of compound "
-           "assignment|on left hand side of assignment}0('%1') from the first "
+           "assignment|on left hand side of assignment}0(%1) from the first "
            "statement">;
 def err_acc_declare_required_clauses
     : Error<"no valid clauses specified in OpenACC 'declare' directive">;
@@ -13066,9 +13072,9 @@ def err_acc_multiple_references
     : Error<"variable referenced in '%0' clause of OpenACC 'declare' directive "
             "was already referenced">;
 def err_acc_routine_not_func
-    : Error<"OpenACC routine name '%0' does not name a function">;
+    : Error<"OpenACC routine name %0 does not name a function">;
 def err_acc_routine_overload_set
-    : Error<"OpenACC routine name '%0' names a set of overloads">;
+    : Error<"OpenACC routine name %0 names a set of overloads">;
 def err_acc_magic_static_in_routine
     : Error<"function static variables are not permitted in functions to which "
             "an OpenACC 'routine' directive applies">;

clang/lib/AST/ASTDiagnostic.cpp

@@ -512,8 +512,6 @@ void clang::FormatASTNodeDiagnosticArgument(
       const Expr *E = reinterpret_cast<Expr *>(Val);
       assert(E && "Received null Expr!");
       E->printPretty(OS, /*Helper=*/nullptr, Context.getPrintingPolicy());
-      // FIXME: Include quotes when printing expressions.
-      NeedQuotes = false;
       break;
     }
   }

clang/lib/Basic/Diagnostic.cpp

@@ -1144,20 +1144,25 @@ FormatDiagnostic(const char *DiagStr, const char *DiagEnd,
 
     switch (Kind) {
     // ---- STRINGS ----
-    case DiagnosticsEngine::ak_std_string: {
-      const std::string &S = getArgStdStr(ArgNo);
-      assert(ModifierLen == 0 && "No modifiers for strings yet");
-      EscapeStringForDiagnostic(S, OutStr);
-      break;
-    }
+    case DiagnosticsEngine::ak_std_string:
     case DiagnosticsEngine::ak_c_string: {
-      const char *S = getArgCStr(ArgNo);
-      assert(ModifierLen == 0 && "No modifiers for strings yet");
-
-      // Don't crash if get passed a null pointer by accident.
-      if (!S)
-        S = "(null)";
+      StringRef S = [&]() -> StringRef {
+        if (Kind == DiagnosticsEngine::ak_std_string)
+          return getArgStdStr(ArgNo);
+        const char *SZ = getArgCStr(ArgNo);
+        // Don't crash if get passed a null pointer by accident.
+        return SZ ? SZ : "(null)";
+      }();
+      bool Quoted = false;
+      if (ModifierIs(Modifier, ModifierLen, "quoted")) {
+        Quoted = true;
+        OutStr.push_back('\'');
+      } else {
+        assert(ModifierLen == 0 && "unknown modifier for string");
+      }
       EscapeStringForDiagnostic(S, OutStr);
+      if (Quoted)
+        OutStr.push_back('\'');
       break;
     }
     // ---- INTEGERS ----

clang/lib/Sema/SemaChecking.cpp

@@ -11685,7 +11685,10 @@ static void DiagnoseIntInBoolContext(Sema &S, Expr *E) {
         S.Diag(ExprLoc, diag::warn_left_shift_always)
             << (Result.Val.getInt() != 0);
       else if (E->getType()->isSignedIntegerType())
-        S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;
+        S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context)
+            << FixItHint::CreateInsertion(E->getBeginLoc(), "(")
+            << FixItHint::CreateInsertion(S.getLocForEndOfToken(E->getEndLoc()),
+                                          ") != 0");
     }
   }
 

clang/test/AST/ByteCode/literals.cpp

@@ -920,7 +920,7 @@ namespace CompoundLiterals {
   // null pointer suggests we're doing something odd during constant expression
   // evaluation: I think it's still taking 'x' as being null from the call to
   // f3() rather than tracking the assignment happening in the VLA.
-  constexpr int f3(int *x, int (*y)[*(x=(int[]){1,2,3})]) { // both-warning {{object backing the pointer x will be destroyed at the end of the full-expression}}
+  constexpr int f3(int *x, int (*y)[*(x=(int[]){1,2,3})]) { // both-warning {{object backing the pointer 'x' will be destroyed at the end of the full-expression}}
     return x[0]; // both-note {{read of dereferenced null pointer is not allowed in a constant expression}}
   }
   constexpr int h = f3(0,0); // both-error {{constexpr variable 'h' must be initialized by a constant expression}} \

clang/test/CodeGenCXX/sections.cpp

@@ -18,19 +18,19 @@ int D = 1;
 #pragma data_seg(".data")
 int a = 1;
 extern const Mutable mutable_custom_section;
-const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
+const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
 extern const Normal normal_custom_section;
 const Normal normal_custom_section;
 struct NonTrivialDtor {
   ~NonTrivialDtor();
 };
 extern const NonTrivialDtor non_trivial_dtor_custom_section;
-const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
+const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
 struct NonTrivialCtor {
   NonTrivialCtor();
 };
 extern const NonTrivialCtor non_trivial_ctor_custom_section;
-const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
+const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
 #pragma data_seg(push, label, ".data2")
 extern const int b;
 const int b = 1;