From d057811655d8de3900748bba03d0c7ebcb6fafe3 Mon Sep 17 00:00:00 2001 From: Matheus Izvekov Date: Mon, 7 Apr 2025 23:19:32 -0300 Subject: [PATCH] [clang] fix diagnostic printing of expressions ignoring LangOpts (#134693) Currently when printing a template argument of expression type, the expression is converted immediately into a string to be sent to the diagnostic engine, unsing a fake LangOpts. This makes the expression printing look incorrect for the current language, besides being inneficient, as we don't actually need to print the expression if the diagnostic would be ignored. This fixes a nastiness with the TemplateArgument constructor for expressions being implicit, and all current users just passing an expression to a diagnostic were implicitly going through the template argument path. The expressions are also being printed unquoted. This will be fixed in a subsequent patch, as the test churn is much larger. --- .../clang-tidy/ClangTidyDiagnosticConsumer.cpp | 3 +++ clang/docs/ReleaseNotes.rst | 3 +++ clang/include/clang/AST/Expr.h | 8 ++++++++ clang/include/clang/AST/TemplateBase.h | 2 +- clang/include/clang/Basic/Diagnostic.h | 5 ++++- clang/lib/AST/ASTDiagnostic.cpp | 8 ++++++++ clang/lib/AST/TemplateBase.cpp | 16 +++------------- clang/lib/Basic/Diagnostic.cpp | 1 + clang/lib/Sema/SemaTemplateDeduction.cpp | 8 ++++---- clang/lib/Sema/SemaTemplateVariadic.cpp | 2 +- clang/lib/Sema/TreeTransform.h | 6 +++--- clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 10 +++++----- .../instantiate-expanded-type-constraint.cpp | 4 ++-- .../SemaTemplate/instantiate-requires-expr.cpp | 4 ++-- .../trailing-return-short-circuit.cpp | 4 ++-- .../cpp17_iterator_concepts.verify.cpp | 2 +- 16 files changed, 51 insertions(+), 35 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 71e852545203..abd6d7b4cd60 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -22,6 +22,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" #include "clang/AST/Attr.h" +#include "clang/AST/Expr.h" #include "clang/Basic/CharInfo.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticOptions.h" @@ -528,6 +529,8 @@ void ClangTidyDiagnosticConsumer::forwardDiagnostic(const Diagnostic &Info) { case clang::DiagnosticsEngine::ak_addrspace: Builder << static_cast(Info.getRawArg(Index)); break; + case clang::DiagnosticsEngine::ak_expr: + Builder << reinterpret_cast(Info.getRawArg(Index)); } } } diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index f8f4dfbafb4f..e67118352256 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -280,6 +280,9 @@ Improvements to Clang's diagnostics - Clang now better preserves the sugared types of pointers to member. - Clang now better preserves the presence of the template keyword with dependent prefixes. +- 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. - 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) diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index dedbff5944af..20f70863a05b 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -7379,6 +7379,14 @@ private: friend class ASTStmtWriter; }; +/// Insertion operator for diagnostics. This allows sending +/// Expr into a diagnostic with <<. +inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, + const Expr *E) { + DB.AddTaggedVal(reinterpret_cast(E), DiagnosticsEngine::ak_expr); + return DB; +} + } // end namespace clang #endif // LLVM_CLANG_AST_EXPR_H diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h index a800a16fc3e7..bea624eb0494 100644 --- a/clang/include/clang/AST/TemplateBase.h +++ b/clang/include/clang/AST/TemplateBase.h @@ -262,7 +262,7 @@ public: /// This form of template argument only occurs in template argument /// lists used for dependent types and for expression; it will not /// occur in a non-dependent, canonical template argument list. - TemplateArgument(Expr *E, bool IsDefaulted = false) { + explicit TemplateArgument(Expr *E, bool IsDefaulted = false) { TypeOrValue.Kind = Expression; TypeOrValue.IsDefaulted = IsDefaulted; TypeOrValue.V = reinterpret_cast(E); diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index 848acce3c4f1..19524856a9bb 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -284,7 +284,10 @@ public: ak_qualtype_pair, /// Attr * - ak_attr + ak_attr, + + /// Expr * + ak_expr, }; /// Represents on argument value, which is a union discriminated diff --git a/clang/lib/AST/ASTDiagnostic.cpp b/clang/lib/AST/ASTDiagnostic.cpp index b4e7360e126f..ccfef9c7ae36 100644 --- a/clang/lib/AST/ASTDiagnostic.cpp +++ b/clang/lib/AST/ASTDiagnostic.cpp @@ -508,6 +508,14 @@ void clang::FormatASTNodeDiagnosticArgument( NeedQuotes = false; break; } + case DiagnosticsEngine::ak_expr: { + const Expr *E = reinterpret_cast(Val); + assert(E && "Received null Expr!"); + E->printPretty(OS, /*Helper=*/nullptr, Context.getPrintingPolicy()); + // FIXME: Include quotes when printing expressions. + NeedQuotes = false; + break; + } } if (NeedQuotes) { diff --git a/clang/lib/AST/TemplateBase.cpp b/clang/lib/AST/TemplateBase.cpp index 0be0a83b7010..42da4e6ca996 100644 --- a/clang/lib/AST/TemplateBase.cpp +++ b/clang/lib/AST/TemplateBase.cpp @@ -478,7 +478,7 @@ TemplateArgument TemplateArgument::getPackExpansionPattern() const { return getAsType()->castAs()->getPattern(); case Expression: - return cast(getAsExpr())->getPattern(); + return TemplateArgument(cast(getAsExpr())->getPattern()); case TemplateExpansion: return TemplateArgument(getAsTemplateOrTemplatePattern()); @@ -654,18 +654,8 @@ static const T &DiagTemplateArg(const T &DB, const TemplateArgument &Arg) { case TemplateArgument::TemplateExpansion: return DB << Arg.getAsTemplateOrTemplatePattern() << "..."; - case TemplateArgument::Expression: { - // This shouldn't actually ever happen, so it's okay that we're - // regurgitating an expression here. - // FIXME: We're guessing at LangOptions! - SmallString<32> Str; - llvm::raw_svector_ostream OS(Str); - LangOptions LangOpts; - LangOpts.CPlusPlus = true; - PrintingPolicy Policy(LangOpts); - Arg.getAsExpr()->printPretty(OS, nullptr, Policy); - return DB << OS.str(); - } + case TemplateArgument::Expression: + return DB << Arg.getAsExpr(); case TemplateArgument::Pack: { // FIXME: We're guessing at LangOptions! diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp index 9e2f13413564..4b4a85aaccf8 100644 --- a/clang/lib/Basic/Diagnostic.cpp +++ b/clang/lib/Basic/Diagnostic.cpp @@ -1247,6 +1247,7 @@ FormatDiagnostic(const char *DiagStr, const char *DiagEnd, case DiagnosticsEngine::ak_nestednamespec: case DiagnosticsEngine::ak_declcontext: case DiagnosticsEngine::ak_attr: + case DiagnosticsEngine::ak_expr: getDiags()->ConvertArgToString(Kind, getRawArg(ArgNo), StringRef(Modifier, ModifierLen), StringRef(Argument, ArgumentLen), diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp index 200168087136..6236bce74343 100644 --- a/clang/lib/Sema/SemaTemplateDeduction.cpp +++ b/clang/lib/Sema/SemaTemplateDeduction.cpp @@ -493,8 +493,8 @@ DeduceNullPtrTemplateArgument(Sema &S, TemplateParameterList *TemplateParams, : CK_NullToPointer) .get(); return DeduceNonTypeTemplateArgument( - S, TemplateParams, NTTP, DeducedTemplateArgument(Value), Value->getType(), - Info, PartialOrdering, Deduced, HasDeducedAnyParam); + S, TemplateParams, NTTP, TemplateArgument(Value), Value->getType(), Info, + PartialOrdering, Deduced, HasDeducedAnyParam); } /// Deduce the value of the given non-type template parameter @@ -508,8 +508,8 @@ DeduceNonTypeTemplateArgument(Sema &S, TemplateParameterList *TemplateParams, SmallVectorImpl &Deduced, bool *HasDeducedAnyParam) { return DeduceNonTypeTemplateArgument( - S, TemplateParams, NTTP, DeducedTemplateArgument(Value), Value->getType(), - Info, PartialOrdering, Deduced, HasDeducedAnyParam); + S, TemplateParams, NTTP, TemplateArgument(Value), Value->getType(), Info, + PartialOrdering, Deduced, HasDeducedAnyParam); } /// Deduce the value of the given non-type template parameter diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp index 3d4a245eb8bd..3040a30454b0 100644 --- a/clang/lib/Sema/SemaTemplateVariadic.cpp +++ b/clang/lib/Sema/SemaTemplateVariadic.cpp @@ -1286,7 +1286,7 @@ TemplateArgumentLoc Sema::getTemplateArgumentPackExpansionPattern( Expr *Pattern = Expansion->getPattern(); Ellipsis = Expansion->getEllipsisLoc(); NumExpansions = Expansion->getNumExpansions(); - return TemplateArgumentLoc(Pattern, Pattern); + return TemplateArgumentLoc(TemplateArgument(Pattern), Pattern); } case TemplateArgument::TemplateExpansion: diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index 1e126a887533..13762dc485c3 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -3981,7 +3981,7 @@ public: if (Result.isInvalid()) return TemplateArgumentLoc(); - return TemplateArgumentLoc(Result.get(), Result.get()); + return TemplateArgumentLoc(TemplateArgument(Result.get()), Result.get()); } case TemplateArgument::Template: @@ -16131,8 +16131,8 @@ TreeTransform::TransformSizeOfPackExpr(SizeOfPackExpr *E) { E->getPackLoc()); if (DRE.isInvalid()) return ExprError(); - ArgStorage = new (getSema().Context) - PackExpansionExpr(DRE.get(), E->getPackLoc(), std::nullopt); + ArgStorage = TemplateArgument(new (getSema().Context) PackExpansionExpr( + DRE.get(), E->getPackLoc(), std::nullopt)); } PackArgs = ArgStorage; } diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl index 941e0a975d5f..34930d896368 100644 --- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl +++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl @@ -60,13 +60,13 @@ RWBuffer > r9; // arrays not allowed // expected-error@+3 {{constraints not satisfied for class template 'RWBuffer'}} // expected-note@*:* {{because 'half[4]' does not satisfy '__is_typed_resource_element_compatible'}} -// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(__fp16[4])' evaluated to false}} +// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(half[4])' evaluated to false}} RWBuffer r10; typedef vector int8; // expected-error@+3 {{constraints not satisfied for class template 'RWBuffer'}} // expected-note@*:* {{because 'vector' (vector of 8 'int' values) does not satisfy '__is_typed_resource_element_compatible'}} -// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(int __attribute__((ext_vector_type(8))))' evaluated to false}} +// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(vector)' evaluated to false}} RWBuffer r11; typedef int MyInt; @@ -74,12 +74,12 @@ RWBuffer r12; // expected-error@+3 {{constraints not satisfied for class template 'RWBuffer'}} // expected-note@*:* {{because 'bool' does not satisfy '__is_typed_resource_element_compatible'}} -// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(_Bool)' evaluated to false}} +// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(bool)' evaluated to false}} RWBuffer r13; // expected-error@+3 {{constraints not satisfied for class template 'RWBuffer'}} // expected-note@*:* {{because 'vector' (vector of 2 'bool' values) does not satisfy '__is_typed_resource_element_compatible'}} -// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(_Bool __attribute__((ext_vector_type(2))))' evaluated to false}} +// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(vector)' evaluated to false}} RWBuffer> r14; enum numbers { one, two, three }; @@ -91,7 +91,7 @@ RWBuffer r15; // expected-error@+3 {{constraints not satisfied for class template 'RWBuffer'}} // expected-note@*:* {{because 'vector' (vector of 3 'double' values) does not satisfy '__is_typed_resource_element_compatible'}} -// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(double __attribute__((ext_vector_type(3))))' evaluated to false}} +// expected-note@*:* {{because '__builtin_hlsl_is_typed_resource_element_compatible(vector)' evaluated to false}} RWBuffer r16; diff --git a/clang/test/SemaTemplate/instantiate-expanded-type-constraint.cpp b/clang/test/SemaTemplate/instantiate-expanded-type-constraint.cpp index 73fef87b9782..3edf24398295 100644 --- a/clang/test/SemaTemplate/instantiate-expanded-type-constraint.cpp +++ b/clang/test/SemaTemplate/instantiate-expanded-type-constraint.cpp @@ -8,7 +8,7 @@ constexpr bool is_same_v = true; template concept same_as = is_same_v; -// expected-note@-1{{because 'is_same_v' evaluated to false}} +// expected-note@-1{{because 'is_same_v' evaluated to false}} template concept either = (is_same_v || ...); @@ -16,7 +16,7 @@ concept either = (is_same_v || ...); template struct T { template... Us> - // expected-note@-1{{because 'same_as' evaluated to false}} + // expected-note@-1{{because 'same_as' evaluated to false}} static void foo(Us... u, int x) { }; // expected-note@-1{{candidate template ignored: deduced too few arguments}} // expected-note@-2{{candidate template ignored: constraints not satisfied}} diff --git a/clang/test/SemaTemplate/instantiate-requires-expr.cpp b/clang/test/SemaTemplate/instantiate-requires-expr.cpp index ab5fac1f9e63..47689b93db50 100644 --- a/clang/test/SemaTemplate/instantiate-requires-expr.cpp +++ b/clang/test/SemaTemplate/instantiate-requires-expr.cpp @@ -72,8 +72,8 @@ namespace type_requirement { template requires false_v; }> - // expected-note@-1 {{because 'false_v::template temp >; }>' evaluated to false}} - // expected-note@-2 {{because 'false_v::template temp >; }>' evaluated to false}} + // expected-note@-1 {{because 'false_v::template temp>; }>' evaluated to false}} + // expected-note@-2 {{because 'false_v::template temp>; }>' evaluated to false}} struct r2 {}; using r2i1 = r2>; // expected-error{{constraints not satisfied for class template 'r2' [with T = type_requirement::contains_template]}} diff --git a/clang/test/SemaTemplate/trailing-return-short-circuit.cpp b/clang/test/SemaTemplate/trailing-return-short-circuit.cpp index 0d1c9b52b0e8..4ef7888d23dc 100644 --- a/clang/test/SemaTemplate/trailing-return-short-circuit.cpp +++ b/clang/test/SemaTemplate/trailing-return-short-circuit.cpp @@ -39,13 +39,13 @@ void usage() { Foo(true); // expected-error@-1{{no matching function for call to 'Foo'}} // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}} - // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}} + // expected-note@#FOO_REQ{{because 'sizeof(bool) > 2' (1 > 2) evaluated to false}} // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}} TrailingReturn(true); // expected-error@-1{{no matching function for call to 'TrailingReturn'}} // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}} - // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}} + // expected-note@#TRAILING_REQ{{because 'sizeof(bool) > 2' (1 > 2) evaluated to false}} // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}} // Fails the 1st check, fails 2nd because ::value is false. diff --git a/libcxx/test/libcxx/algorithms/cpp17_iterator_concepts.verify.cpp b/libcxx/test/libcxx/algorithms/cpp17_iterator_concepts.verify.cpp index 99f67c8a795a..c4462b26f5c9 100644 --- a/libcxx/test/libcxx/algorithms/cpp17_iterator_concepts.verify.cpp +++ b/libcxx/test/libcxx/algorithms/cpp17_iterator_concepts.verify.cpp @@ -173,7 +173,7 @@ void check_bidirectional_iterator_requirements() { _LIBCPP_REQUIRE_CPP17_BIDIRECTIONAL_ITERATOR(missing_postdecrement, ""); // expected-error {{static assertion failed}} // expected-note@*:* {{cannot decrement value of type 'missing_postdecrement'}} _LIBCPP_REQUIRE_CPP17_BIDIRECTIONAL_ITERATOR(not_returning_iter_reference, ""); // expected-error {{static assertion failed}} - // expected-note@*:* {{because type constraint 'same_as >' was not satisfied}} + // expected-note-re@*:* {{because type constraint 'same_as{{ ?}}>' was not satisfied}} // clang-format on }