Skip to content

[Diagnostics] Add -print-diagnostic-groups flag #76363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions include/swift/AST/DiagnosticEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "swift/AST/DeclNameLoc.h"
#include "swift/AST/DiagnosticConsumer.h"
#include "swift/AST/TypeLoc.h"
#include "swift/Basic/PrintDiagnosticNamesMode.h"
#include "swift/Basic/Statistic.h"
#include "swift/Basic/Version.h"
#include "swift/Basic/WarningAsErrorRule.h"
Expand Down Expand Up @@ -1065,6 +1066,13 @@ namespace swift {
/// diagnostic message.
std::unique_ptr<diag::LocalizationProducer> localization;

/// This allocator will retain diagnostic strings containing the
/// diagnostic's message and identifier as `message [id]` for the duration
/// of compiler invocation. This will be used when the frontend flags
/// `-debug-diagnostic-names` or `-print-diagnostic-groups` are used.
llvm::BumpPtrAllocator DiagnosticStringsAllocator;
llvm::StringSaver DiagnosticStringsSaver;

/// The number of open diagnostic transactions. Diagnostics are only
/// emitted once all transactions have closed.
unsigned TransactionCount = 0;
Expand All @@ -1074,9 +1082,11 @@ namespace swift {
/// input being compiled.
/// May be invalid.
SourceLoc bufferIndirectlyCausingDiagnostic;

/// Print diagnostic names after their messages
bool printDiagnosticNames = false;

/// When printing diagnostics, include either the diagnostic name
/// (diag::whatever) at the end or the associated diagnostic group.
PrintDiagnosticNamesMode printDiagnosticNamesMode =
PrintDiagnosticNamesMode::None;

/// Path to diagnostic documentation directory.
std::string diagnosticDocumentationPath = "";
Expand All @@ -1102,7 +1112,8 @@ namespace swift {
public:
explicit DiagnosticEngine(SourceManager &SourceMgr)
: SourceMgr(SourceMgr), ActiveDiagnostic(),
TransactionStrings(TransactionAllocator) {}
TransactionStrings(TransactionAllocator),
DiagnosticStringsSaver(DiagnosticStringsAllocator) {}

/// hadAnyError - return true if any *error* diagnostics have been emitted.
bool hadAnyError() const { return state.hadAnyError(); }
Expand Down Expand Up @@ -1144,11 +1155,11 @@ namespace swift {
void setWarningsAsErrorsRules(const std::vector<WarningAsErrorRule> &rules);

/// Whether to print diagnostic names after their messages
void setPrintDiagnosticNames(bool val) {
printDiagnosticNames = val;
void setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode val) {
printDiagnosticNamesMode = val;
}
bool getPrintDiagnosticNames() const {
return printDiagnosticNames;
PrintDiagnosticNamesMode getPrintDiagnosticNamesMode() const {
return printDiagnosticNamesMode;
}

void setDiagnosticDocumentationPath(std::string path) {
Expand All @@ -1169,8 +1180,7 @@ namespace swift {
void setLocalization(StringRef locale, StringRef path) {
assert(!locale.empty());
assert(!path.empty());
localization = diag::LocalizationProducer::producerFor(
locale, path, getPrintDiagnosticNames());
localization = diag::LocalizationProducer::producerFor(locale, path);
}

void ignoreDiagnostic(DiagID id) {
Expand Down Expand Up @@ -1426,8 +1436,9 @@ namespace swift {
public:
DiagnosticKind declaredDiagnosticKindFor(const DiagID id);

llvm::StringRef diagnosticStringFor(const DiagID id,
bool printDiagnosticNames);
llvm::StringRef
diagnosticStringFor(const DiagID id,
PrintDiagnosticNamesMode printDiagnosticNamesMode);

static llvm::StringRef diagnosticIDStringFor(const DiagID id);

Expand Down
8 changes: 5 additions & 3 deletions include/swift/Basic/DiagnosticOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#ifndef SWIFT_BASIC_DIAGNOSTICOPTIONS_H
#define SWIFT_BASIC_DIAGNOSTICOPTIONS_H

#include "swift/Basic/PrintDiagnosticNamesMode.h"
#include "swift/Basic/WarningAsErrorRule.h"
#include "llvm/ADT/Hashing.h"
#include <vector>
Expand Down Expand Up @@ -63,9 +64,10 @@ class DiagnosticOptions {
/// Rules for escalating warnings to errors
std::vector<WarningAsErrorRule> WarningsAsErrorsRules;

/// When printing diagnostics, include the diagnostic name (diag::whatever) at
/// the end.
bool PrintDiagnosticNames = false;
/// When printing diagnostics, include either the diagnostic name
/// (diag::whatever) at the end or the associated diagnostic group.
PrintDiagnosticNamesMode PrintDiagnosticNames =
PrintDiagnosticNamesMode::None;

/// If set to true, include educational notes in printed output if available.
/// Educational notes are documentation which supplement diagnostics.
Expand Down
34 changes: 34 additions & 0 deletions include/swift/Basic/PrintDiagnosticNamesMode.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//===--- PrintDiagnosticNamesMode.h -----------------------------*- C++ -*-===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

#ifndef SWIFT_BASIC_PRINTDIAGNOSTICNAMESMODE_H
#define SWIFT_BASIC_PRINTDIAGNOSTICNAMESMODE_H

namespace swift {

/// What diagnostic name will be printed alongside the diagnostic message.
enum class PrintDiagnosticNamesMode {
/// No diagnostic name will be printed.
None,

/// The identifier of a diagnostic (DiagID) will be used. Corresponds to the
/// `-debug-diagnostic-names` option in the frontend.
Identifier,

/// The associated group name (DiagGroupID) will be used. Corresponds to the
/// `-print-diagnostic-groups` option in the frontend.
Group
};

} // end namespace swift

#endif // SWIFT_BASIC_PRINTDIAGNOSTICNAMESMODE_H
22 changes: 4 additions & 18 deletions include/swift/Localization/LocalizationFormat.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,20 +157,9 @@ class SerializedLocalizationWriter {
};

class LocalizationProducer {
/// This allocator will retain localized diagnostic strings containing the
/// diagnostic's message and identifier as `message [id]` for the duration of
/// compiler invocation. This will be used when the frontend flag
/// `-debug-diagnostic-names` is used.
llvm::BumpPtrAllocator localizationAllocator;
llvm::StringSaver localizationSaver;
bool printDiagnosticNames;
LocalizationProducerState state = NotInitialized;

public:
LocalizationProducer(bool printDiagnosticNames = false)
: localizationSaver(localizationAllocator),
printDiagnosticNames(printDiagnosticNames) {}

/// If the message isn't available/localized in current context
/// return the fallback default message.
virtual llvm::StringRef getMessageOr(swift::DiagID id,
Expand All @@ -181,8 +170,7 @@ class LocalizationProducer {
/// `StringsLocalizationProducer` if the `.strings` file is available. If both
/// files aren't available returns a `nullptr`.
static std::unique_ptr<LocalizationProducer>
producerFor(llvm::StringRef locale, llvm::StringRef path,
bool printDiagnosticNames);
producerFor(llvm::StringRef locale, llvm::StringRef path);

virtual ~LocalizationProducer() {}

Expand All @@ -206,9 +194,8 @@ class StringsLocalizationProducer final : public LocalizationProducer {
std::vector<std::string> diagnostics;

public:
explicit StringsLocalizationProducer(llvm::StringRef filePath,
bool printDiagnosticNames = false)
: LocalizationProducer(printDiagnosticNames), filePath(filePath) {}
explicit StringsLocalizationProducer(llvm::StringRef filePath)
: LocalizationProducer(), filePath(filePath) {}

/// Iterate over all of the available (non-empty) translations
/// maintained by this producer, callback gets each translation
Expand All @@ -234,8 +221,7 @@ class SerializedLocalizationProducer final : public LocalizationProducer {

public:
explicit SerializedLocalizationProducer(
std::unique_ptr<llvm::MemoryBuffer> buffer,
bool printDiagnosticNames = false);
std::unique_ptr<llvm::MemoryBuffer> buffer);

protected:
bool initializeImpl() override;
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Option/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,9 @@ def no_color_diagnostics : Flag<["-"], "no-color-diagnostics">,
def debug_diagnostic_names : Flag<["-"], "debug-diagnostic-names">,
Flags<[FrontendOption, DoesNotAffectIncrementalBuild, HelpHidden]>,
HelpText<"Include diagnostic names when printing">;
def print_diagnostic_groups : Flag<["-"], "print-diagnostic-groups">,
Flags<[FrontendOption, DoesNotAffectIncrementalBuild, HelpHidden]>,
HelpText<"Include diagnostic groups in printed diagnostic output, if available">;
def print_educational_notes : Flag<["-"], "print-educational-notes">,
Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
HelpText<"Include educational notes in printed diagnostic output, if available">;
Expand Down
50 changes: 32 additions & 18 deletions lib/AST/DiagnosticEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,6 @@ static constexpr const char * const diagnosticStrings[] = {
"<not a diagnostic>",
};

static constexpr const char *const debugDiagnosticStrings[] = {
#define DIAG(KIND, ID, Group, Options, Text, Signature) Text " [" #ID "]",
#include "swift/AST/DiagnosticsAll.def"
"<not a diagnostic>",
};

static constexpr const char *const diagnosticIDStrings[] = {
#define DIAG(KIND, ID, Group, Options, Text, Signature) #ID,
#include "swift/AST/DiagnosticsAll.def"
Expand Down Expand Up @@ -1518,7 +1512,7 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {

return DiagnosticInfo(
diagnostic.getID(), loc, toDiagnosticKind(behavior),
diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNames()),
diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNamesMode()),
diagnostic.getArgs(), Category, getDefaultDiagnosticLoc(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Category here has a few hardcoded values. Should we provide the warning group name for Category when there is one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably could do it. But I'm sure we should. These categories are inferred from DiagnosticOptions.
The deprecation category seems to be interesting to look at. Maybe we should consider removing the DiagnosticOptions::Deprecation option and instead infer this category from checking if a diagnostic belongs to the deprecated group. So we don't have two ways to express the same thing.
And I don't know if we need the api-digester-breaking-change and no-usage categories expressed as groups.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the diagnostics that get each of these flags: the "deprecation" category does seem to line up fairly well with the "deprecated" group, the "no-usage" category could be useful as a general diagnostic group for "you did something and didn't use the result" warnings, and "api-digester-breaking-change" is a very specific group of separate diagnostics (mostly errors).

/*child note info*/ {}, diagnostic.getRanges(), fixIts,
diagnostic.isChildNote());
Expand Down Expand Up @@ -1683,18 +1677,38 @@ DiagnosticKind DiagnosticEngine::declaredDiagnosticKindFor(const DiagID id) {
return storedDiagnosticInfos[(unsigned)id].kind;
}

llvm::StringRef
DiagnosticEngine::diagnosticStringFor(const DiagID id,
bool printDiagnosticNames) {
auto defaultMessage = printDiagnosticNames
? debugDiagnosticStrings[(unsigned)id]
: diagnosticStrings[(unsigned)id];

if (auto producer = localization.get()) {
auto localizedMessage = producer->getMessageOr(id, defaultMessage);
return localizedMessage;
llvm::StringRef DiagnosticEngine::diagnosticStringFor(
const DiagID id, PrintDiagnosticNamesMode printDiagnosticNamesMode) {
llvm::StringRef message = diagnosticStrings[(unsigned)id];
if (auto localizationProducer = localization.get()) {
message = localizationProducer->getMessageOr(id, message);
}
auto formatMessageWithName = [&](StringRef message, StringRef name) {
const int additionalCharsLength = 3; // ' ', '[', ']'
std::string messageWithName;
messageWithName.reserve(message.size() + name.size() +
additionalCharsLength);
messageWithName += message;
messageWithName += " [";
messageWithName += name;
messageWithName += "]";
return DiagnosticStringsSaver.save(messageWithName);
};
switch (printDiagnosticNamesMode) {
case PrintDiagnosticNamesMode::None:
break;
case PrintDiagnosticNamesMode::Identifier:
message = formatMessageWithName(message, diagnosticIDStringFor(id));
break;
case PrintDiagnosticNamesMode::Group:
auto groupID = storedDiagnosticInfos[(unsigned)id].groupID;
if (groupID != DiagGroupID::no_group) {
message =
formatMessageWithName(message, getDiagGroupInfoByID(groupID).name);
}
break;
}
return defaultMessage;
return message;
}

llvm::StringRef
Expand Down
6 changes: 4 additions & 2 deletions lib/DriverTool/sil_opt_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -717,8 +717,10 @@ int sil_opt_main(ArrayRef<const char *> argv, void *MainAddr) {

Invocation.getLangOptions().BypassResilienceChecks =
options.BypassResilienceChecks;
Invocation.getDiagnosticOptions().PrintDiagnosticNames =
options.DebugDiagnosticNames;
if (options.DebugDiagnosticNames) {
Invocation.getDiagnosticOptions().PrintDiagnosticNames =
PrintDiagnosticNamesMode::Identifier;
}

for (auto &featureName : options.UpcomingFeatures) {
auto feature = getUpcomingFeature(featureName);
Expand Down
6 changes: 5 additions & 1 deletion lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2349,7 +2349,11 @@ static bool ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
}
}());
}
Opts.PrintDiagnosticNames |= Args.hasArg(OPT_debug_diagnostic_names);
if (Args.hasArg(OPT_debug_diagnostic_names)) {
Opts.PrintDiagnosticNames = PrintDiagnosticNamesMode::Identifier;
} else if (Args.hasArg(OPT_print_diagnostic_groups)) {
Opts.PrintDiagnosticNames = PrintDiagnosticNamesMode::Group;
}
Opts.PrintEducationalNotes |= Args.hasArg(OPT_print_educational_notes);
if (Arg *A = Args.getLastArg(OPT_diagnostic_documentation_path)) {
Opts.DiagnosticDocumentationPath = A->getValue();
Expand Down
5 changes: 2 additions & 3 deletions lib/Frontend/Frontend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -729,9 +729,8 @@ void CompilerInstance::setUpDiagnosticOptions() {
}
Diagnostics.setWarningsAsErrorsRules(
Invocation.getDiagnosticOptions().WarningsAsErrorsRules);
if (Invocation.getDiagnosticOptions().PrintDiagnosticNames) {
Diagnostics.setPrintDiagnosticNames(true);
}
Diagnostics.setPrintDiagnosticNamesMode(
Invocation.getDiagnosticOptions().PrintDiagnosticNames);
Diagnostics.setDiagnosticDocumentationPath(
Invocation.getDiagnosticOptions().DiagnosticDocumentationPath);
Diagnostics.setLanguageVersion(
Expand Down
2 changes: 1 addition & 1 deletion lib/IDE/CodeCompletionDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ bool CodeCompletionDiagnostics::getDiagnostics(
typename swift::detail::PassArgument<ArgTypes>::type... VArgs) {
DiagID id = ID.ID;
std::vector<DiagnosticArgument> DiagArgs{std::move(VArgs)...};
auto format = Engine.diagnosticStringFor(id, /*printDiagnosticNames=*/false);
auto format = Engine.diagnosticStringFor(id, PrintDiagnosticNamesMode::None);
DiagnosticEngine::formatDiagnosticText(Out, format, DiagArgs);
severity = getSeverity(Engine.declaredDiagnosticKindFor(id));

Expand Down
25 changes: 6 additions & 19 deletions lib/Localization/LocalizationFormat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,6 @@ enum LocalDiagID : uint32_t {
NumDiags
};

static constexpr const char *const diagnosticNameStrings[] = {
#define DIAG(KIND, ID, Group, Options, Text, Signature) " [" #ID "]",
#include "swift/AST/DiagnosticsAll.def"
"<not a diagnostic>",
};

} // namespace

namespace swift {
Expand Down Expand Up @@ -96,12 +90,6 @@ LocalizationProducer::getMessageOr(swift::DiagID id,
auto localizedMessage = getMessage(id);
if (localizedMessage.empty())
return defaultMessage;
if (printDiagnosticNames) {
llvm::StringRef diagnosticName(diagnosticNameStrings[(unsigned)id]);
auto localizedDebugDiagnosticMessage =
localizationSaver.save(localizedMessage.str() + diagnosticName.str());
return localizedDebugDiagnosticMessage;
}
return localizedMessage;
}

Expand All @@ -110,9 +98,8 @@ LocalizationProducerState LocalizationProducer::getState() const {
}

SerializedLocalizationProducer::SerializedLocalizationProducer(
std::unique_ptr<llvm::MemoryBuffer> buffer, bool printDiagnosticNames)
: LocalizationProducer(printDiagnosticNames), Buffer(std::move(buffer)) {
}
std::unique_ptr<llvm::MemoryBuffer> buffer)
: LocalizationProducer(), Buffer(std::move(buffer)) {}

bool SerializedLocalizationProducer::initializeImpl() {
auto base =
Expand All @@ -132,8 +119,8 @@ SerializedLocalizationProducer::getMessage(swift::DiagID id) const {
}

std::unique_ptr<LocalizationProducer>
LocalizationProducer::producerFor(llvm::StringRef locale, llvm::StringRef path,
bool printDiagnosticNames) {
LocalizationProducer::producerFor(llvm::StringRef locale,
llvm::StringRef path) {
llvm::SmallString<128> filePath(path);
llvm::sys::path::append(filePath, locale);
llvm::sys::path::replace_extension(filePath, ".db");
Expand All @@ -143,13 +130,13 @@ LocalizationProducer::producerFor(llvm::StringRef locale, llvm::StringRef path,
if (llvm::sys::fs::exists(filePath)) {
if (auto file = llvm::MemoryBuffer::getFile(filePath)) {
return std::make_unique<diag::SerializedLocalizationProducer>(
std::move(file.get()), printDiagnosticNames);
std::move(file.get()));
}
} else {
llvm::sys::path::replace_extension(filePath, ".strings");
if (llvm::sys::fs::exists(filePath)) {
return std::make_unique<diag::StringsLocalizationProducer>(
filePath.str(), printDiagnosticNames);
filePath.str());
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/PrintAsClang/ModuleContentsWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ class ModuleWriter {
// Emit a specific unavailable message when we know why a decl can't be
// exposed, or a generic message otherwise.
auto diagString = M.getASTContext().Diags.diagnosticStringFor(
diag.getID(), /*PrintDiagnosticNames=*/false);
diag.getID(), PrintDiagnosticNamesMode::None);
DiagnosticEngine::formatDiagnosticText(os, diagString, diag.getArgs(),
DiagnosticFormatOptions());
os << "\");\n";
Expand Down
Loading