Skip to content

[AST] DiagnosticEngine: retain strings used in tentative diagnostic arguments #26342

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 3 commits into from
Jul 25, 2019
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
27 changes: 25 additions & 2 deletions include/swift/AST/DiagnosticEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include "swift/AST/TypeLoc.h"
#include "swift/AST/DeclNameLoc.h"
#include "swift/AST/DiagnosticConsumer.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Allocator.h"

namespace swift {
class Decl;
Expand Down Expand Up @@ -323,6 +325,8 @@ namespace swift {
SourceLoc Loc;
const Decl *Decl = nullptr;

friend DiagnosticEngine;

public:
// All constructors are intentionally implicit.
template<typename ...ArgTypes>
Expand Down Expand Up @@ -564,6 +568,12 @@ namespace swift {
/// results that we can point to on the command line.
llvm::DenseMap<const Decl *, SourceLoc> PrettyPrintedDeclarations;

llvm::BumpPtrAllocator TransactionAllocator;
/// A set of all strings involved in current transactional chain.
/// This is required because diagnostics are not directly emitted
/// but rather stored until all transactions complete.
llvm::StringMap<char, llvm::BumpPtrAllocator &> TransactionStrings;

/// The number of open diagnostic transactions. Diagnostics are only
/// emitted once all transactions have closed.
unsigned TransactionCount = 0;
Expand All @@ -579,8 +589,8 @@ namespace swift {

public:
explicit DiagnosticEngine(SourceManager &SourceMgr)
: SourceMgr(SourceMgr), ActiveDiagnostic() {
}
: SourceMgr(SourceMgr), ActiveDiagnostic(),
TransactionStrings(TransactionAllocator) {}

/// hadAnyError - return true if any *error* diagnostics have been emitted.
bool hadAnyError() const { return state.hadAnyError(); }
Expand Down Expand Up @@ -797,6 +807,11 @@ namespace swift {
DiagnosticFormatOptions FormatOpts = DiagnosticFormatOptions());

private:
/// Called when tentative diagnostic is about to be flushed,
/// to apply any required transformations e.g. copy string arguments
/// to extend their lifetime.
void onTentativeDiagnosticFlush(Diagnostic &diagnostic);

/// Flush the active diagnostic.
void flushActiveDiagnostic();

Expand Down Expand Up @@ -856,6 +871,9 @@ namespace swift {
bool IsOpen = true;

public:
DiagnosticTransaction(const DiagnosticTransaction &) = delete;
DiagnosticTransaction &operator=(const DiagnosticTransaction &) = delete;

explicit DiagnosticTransaction(DiagnosticEngine &engine)
: Engine(engine),
PrevDiagnostics(Engine.TentativeDiagnostics.size()),
Expand All @@ -870,6 +888,11 @@ namespace swift {
if (IsOpen) {
commit();
}

if (Depth == 0) {
Engine.TransactionStrings.clear();
Engine.TransactionAllocator.Reset();
}
}

/// Abort and close this transaction and erase all diagnostics
Expand Down
15 changes: 15 additions & 0 deletions lib/AST/DiagnosticEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ void DiagnosticEngine::flushActiveDiagnostic() {
if (TransactionCount == 0) {
emitDiagnostic(*ActiveDiagnostic);
} else {
onTentativeDiagnosticFlush(*ActiveDiagnostic);
TentativeDiagnostics.emplace_back(std::move(*ActiveDiagnostic));
}
ActiveDiagnostic.reset();
Expand Down Expand Up @@ -940,3 +941,17 @@ BufferIndirectlyCausingDiagnosticRAII::BufferIndirectlyCausingDiagnosticRAII(
if (loc.isValid())
Diags.setBufferIndirectlyCausingDiagnosticToInput(loc);
}

void DiagnosticEngine::onTentativeDiagnosticFlush(Diagnostic &diagnostic) {
for (auto &argument : diagnostic.Args) {
if (argument.getKind() != DiagnosticArgumentKind::String)
continue;

auto content = argument.getAsString();
if (content.empty())
continue;

auto I = TransactionStrings.insert(std::make_pair(content, char())).first;
argument = DiagnosticArgument(StringRef(I->getKeyData()));
}
}
4 changes: 2 additions & 2 deletions lib/Sema/DerivedConformanceCodable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,7 @@ ValueDecl *DerivedConformance::deriveEncodable(ValueDecl *requirement) {
// diagnostics, then potentially collect notes. If we succeed in
// synthesizing Encodable, we can cancel the transaction and get rid of the
// fake failures.
auto diagnosticTransaction = DiagnosticTransaction(TC.Context.Diags);
DiagnosticTransaction diagnosticTransaction(TC.Context.Diags);
TC.diagnose(ConformanceDecl, diag::type_does_not_conform,
Nominal->getDeclaredType(), getProtocolType());
TC.diagnose(requirement, diag::no_witnesses, diag::RequirementKind::Func,
Expand Down Expand Up @@ -1215,7 +1215,7 @@ ValueDecl *DerivedConformance::deriveDecodable(ValueDecl *requirement) {
// diagnostics produced by canSynthesize and deriveDecodable_init to produce
// them in the right order -- see the comment in deriveEncodable for
// background on this transaction.
auto diagnosticTransaction = DiagnosticTransaction(TC.Context.Diags);
DiagnosticTransaction diagnosticTransaction(TC.Context.Diags);
TC.diagnose(ConformanceDecl->getLoc(), diag::type_does_not_conform,
Nominal->getDeclaredType(), getProtocolType());
TC.diagnose(requirement, diag::no_witnesses,
Expand Down