Skip to content

Wire up FileSpecificDiagnosticConsumer for serialized diagnostics #14900

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
Mar 3, 2018
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
70 changes: 70 additions & 0 deletions include/swift/AST/DiagnosticConsumer.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#ifndef SWIFT_BASIC_DIAGNOSTICCONSUMER_H
#define SWIFT_BASIC_DIAGNOSTICCONSUMER_H

#include "swift/Basic/LLVM.h"
#include "swift/Basic/SourceLoc.h"
#include "llvm/Support/SourceMgr.h"

Expand Down Expand Up @@ -111,6 +112,75 @@ class NullDiagnosticConsumer : public DiagnosticConsumer {
ArrayRef<DiagnosticArgument> FormatArgs,
const DiagnosticInfo &Info) override;
};

/// \brief DiagnosticConsumer that funnels diagnostics in certain files to
/// particular sub-consumers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the funneling needed? (I know I already commented about a bigger picture comment, which could help.)
Inheritance, or aggregation? Is it a special kind of DiagnosticConsumer that funnels, or is it an aggregation or ordinary DiagnosticConsumers, a multiplexer in front of a bunch of them? Just asking, don't know the best answer.

///
/// The intended use case for such a consumer is "batch mode" compilations,
/// where we want to record diagnostics for each file as if they were compiled
/// separately. This is important for incremental builds, so that if a file has
/// warnings but doesn't get recompiled in the next build, the warnings persist.
///
/// Diagnostics that are not in one of the special files are emitted into every
/// sub-consumer. This is necessary to deal with, for example, diagnostics in a
/// bridging header imported from Objective-C, which isn't really about the
/// current file.
class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
public:
/// A diagnostic consumer, along with the name of the buffer that it should
/// be associated with. An empty string means that a consumer is not
/// associated with any particular buffer, and should only receive diagnostics
/// that are not in any of the other consumers' files.
using ConsumerPair =
std::pair<std::string, std::unique_ptr<DiagnosticConsumer>>;

private:
/// All consumers owned by this FileSpecificDiagnosticConsumer.
const SmallVector<ConsumerPair, 4> SubConsumers;

using ConsumersOrderedByRangeEntry =
std::pair<CharSourceRange, DiagnosticConsumer *>;

/// The consumers owned by this FileSpecificDiagnosticConsumer, sorted by
/// the end locations of each file so that a lookup by position can be done
/// using binary search.
///
/// Generated and cached when the first diagnostic with a location is emitted.
/// This allows diagnostics to be emitted before files are actually opened,
/// as long as they don't have source locations.
///
/// \see #consumerForLocation
SmallVector<ConsumersOrderedByRangeEntry, 4> ConsumersOrderedByRange;

/// Indicates which consumer to send Note diagnostics too.
///
/// Notes are always considered attached to the error, warning, or remark
/// that was most recently emitted.
///
/// If null, Note diagnostics are sent to every consumer.
DiagnosticConsumer *ConsumerForSubsequentNotes = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice name!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I realized I was being too clever. I think at some point we're going to want to emit notes before a particular error or warning (which Clang already does in some cases, IIRC), and then we're going to need the notion of an explicit "diagnostic group", rather than the implicit thing we have now. But we can change that when it comes to that.


public:
/// Takes ownership of the DiagnosticConsumers specified in \p consumers.
///
/// There must not be two consumers for the same file (i.e., having the same
/// buffer name).
explicit FileSpecificDiagnosticConsumer(
SmallVectorImpl<ConsumerPair> &consumers);

void handleDiagnostic(SourceManager &SM, SourceLoc Loc,
DiagnosticKind Kind,
StringRef FormatString,
ArrayRef<DiagnosticArgument> FormatArgs,
const DiagnosticInfo &Info) override;

bool finishProcessing() override;

private:
void computeConsumersOrderedByRange(SourceManager &SM);
DiagnosticConsumer *consumerForLocation(SourceManager &SM,
SourceLoc loc) const;
};

} // end namespace swift

Expand Down
4 changes: 4 additions & 0 deletions include/swift/Frontend/InputFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ class InputFile {
std::string loadedModuleTracePath() const {
return getPrimarySpecificPaths().SupplementaryOutputs.LoadedModuleTracePath;
}
std::string serializedDiagnosticsPath() const {
return getPrimarySpecificPaths().SupplementaryOutputs
.SerializedDiagnosticsPath;
}
};
} // namespace swift

Expand Down
9 changes: 5 additions & 4 deletions include/swift/Frontend/SerializedDiagnosticConsumer.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,14 @@ namespace swift {
class DiagnosticConsumer;

namespace serialized_diagnostics {
/// \brief Create a DiagnosticConsumer that serializes diagnostics to a
/// file.
/// Create a DiagnosticConsumer that serializes diagnostics to a file, using
/// Clang's serialized diagnostics format.
///
/// \param serializedDiagnosticsPath the file path to write the diagnostics.
/// \param outputPath the file path to write the diagnostics to.
///
/// \returns A new diagnostic consumer that serializes diagnostics.
DiagnosticConsumer *createConsumer(llvm::StringRef serializedDiagnosticsPath);
std::unique_ptr<DiagnosticConsumer>
createConsumer(llvm::StringRef outputPath);
}
}

Expand Down
161 changes: 159 additions & 2 deletions lib/AST/DiagnosticConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,172 @@
//
//===----------------------------------------------------------------------===//

#define DEBUG_TYPE "swift-basic"
#define DEBUG_TYPE "swift-ast"
#include "swift/AST/DiagnosticConsumer.h"
#include "swift/AST/DiagnosticEngine.h"
#include "swift/Basic/Defer.h"
#include "swift/Basic/SourceManager.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
using namespace swift;

DiagnosticConsumer::~DiagnosticConsumer() { }
DiagnosticConsumer::~DiagnosticConsumer() = default;

llvm::SMLoc DiagnosticConsumer::getRawLoc(SourceLoc loc) {
return loc.Value;
}

LLVM_ATTRIBUTE_UNUSED
static bool hasDuplicateFileNames(
ArrayRef<FileSpecificDiagnosticConsumer::ConsumerPair> consumers) {
llvm::StringSet<> seenFiles;
for (const auto &consumerPair : consumers) {
if (consumerPair.first.empty()) {
// We can handle multiple consumers that aren't associated with any file,
// because they only collect diagnostics that aren't in any of the special
// files. This isn't an important use case to support, but also SmallSet
// doesn't handle empty strings anyway!
continue;
}

bool isUnique = seenFiles.insert(consumerPair.first).second;
if (!isUnique)
return true;
}
return false;
}

FileSpecificDiagnosticConsumer::FileSpecificDiagnosticConsumer(
SmallVectorImpl<ConsumerPair> &consumers)
: SubConsumers(std::move(consumers)) {
assert(!SubConsumers.empty() &&
"don't waste time handling diagnostics that will never get emitted");
assert(!hasDuplicateFileNames(SubConsumers) &&
"having multiple consumers for the same file is not implemented");
}

void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
SourceManager &SM) {
// Look up each file's source range and add it to the "map" (to be sorted).
for (const ConsumerPair &pair : SubConsumers) {
if (pair.first.empty())
continue;

Optional<unsigned> bufferID = SM.getIDForBufferIdentifier(pair.first);
assert(bufferID.hasValue() && "consumer registered for unknown file");
CharSourceRange range = SM.getRangeForBuffer(bufferID.getValue());
ConsumersOrderedByRange.emplace_back(range, pair.second.get());
}

// Sort the "map" by buffer /end/ location, for use with std::lower_bound
// later. (Sorting by start location would produce the same sort, since the
// ranges must not be overlapping, but since we need to check end locations
// later it's consistent to sort by that here.)
std::sort(ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(),
[](const ConsumersOrderedByRangeEntry &left,
const ConsumersOrderedByRangeEntry &right) -> bool {
auto compare = std::less<const char *>();
return compare(getRawLoc(left.first.getEnd()).getPointer(),
getRawLoc(right.first.getEnd()).getPointer());
});

// Check that the ranges are non-overlapping. If the files really are all
// distinct, this should be trivially true, but if it's ever not we might end
// up mis-filing diagnostics.
assert(ConsumersOrderedByRange.end() ==
std::adjacent_find(ConsumersOrderedByRange.begin(),
ConsumersOrderedByRange.end(),
[](const ConsumersOrderedByRangeEntry &left,
const ConsumersOrderedByRangeEntry &right) {
return left.first.overlaps(right.first);
}) &&
"overlapping ranges despite having distinct files");
}

DiagnosticConsumer *
FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
SourceLoc loc) const {
// If there's only one consumer, we'll use it no matter what, because...
// - ...all diagnostics within the file will go to that consumer.
// - ...all diagnostics not within the file will not be claimed by any
// consumer, and so will go to all (one) consumers.
if (SubConsumers.size() == 1)
return SubConsumers.front().second.get();

// Diagnostics with invalid locations always go to every consumer.
if (loc.isInvalid())
return nullptr;

// This map is generated on first use and cached, to allow the
// FileSpecificDiagnosticConsumer to be set up before the source files are
// actually loaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great comment!

if (ConsumersOrderedByRange.empty()) {
auto *mutableThis = const_cast<FileSpecificDiagnosticConsumer*>(this);
mutableThis->computeConsumersOrderedByRange(SM);
}

// This std::lower_bound call is doing a binary search for the first range
// that /might/ contain 'loc'. Specifically, since the ranges are sorted
// by end location, it's looking for the first range where the end location
// is greater than or equal to 'loc'.
auto possiblyContainingRangeIter =
std::lower_bound(ConsumersOrderedByRange.begin(),
ConsumersOrderedByRange.end(),
loc,
[](const ConsumersOrderedByRangeEntry &entry,
SourceLoc loc) -> bool {
auto compare = std::less<const char *>();
return compare(getRawLoc(entry.first.getEnd()).getPointer(),
getRawLoc(loc).getPointer());
});

if (possiblyContainingRangeIter != ConsumersOrderedByRange.end() &&
possiblyContainingRangeIter->first.contains(loc)) {
return possiblyContainingRangeIter->second;
}

return nullptr;
}

void FileSpecificDiagnosticConsumer::handleDiagnostic(
SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
const DiagnosticInfo &Info) {

DiagnosticConsumer *specificConsumer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would factor the switch out into a function that returned the specificConsumer (or null). The specificConsumer variable becomes local to the first three cases that way, so it's less code to trace through in a reader's head.

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 still don't think that's worth it, especially since there's an update to ConsumerForSubsequentNotes. Lots of small functions are also hard to reason about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there's a bit of Smalltalk-oidal transformative experience going on on my part here. Here's how I would push back: In this case, it would be a function called findSpecificConsumerIfAny and I could just read the name and know what's going on in the caller. Without the separate function, I need to read the body of the switch to see that it is setting the variable, and then also read further to see that the variable is never changed again. (Swift is way better:)

I realize that the uninitialized variable followed by conditional code that sets it is a common idiom in C(++), but my other beef is that the compiler does nothing to guarantee it is not modified below. When functions get longer, I always have to grep for the variable to ensure it doesn't get modified in the middle.

This particular case is not that bad, but I feel that there is something like "the death of a thousand cuts", where enough not-too-tough cases have a cumulative effect on the effort required to dive into an unfamiliar code base.

switch (Kind) {
case DiagnosticKind::Error:
case DiagnosticKind::Warning:
case DiagnosticKind::Remark:
specificConsumer = consumerForLocation(SM, Loc);
ConsumerForSubsequentNotes = specificConsumer;
break;
case DiagnosticKind::Note:
specificConsumer = ConsumerForSubsequentNotes;
break;
}

if (specificConsumer) {
specificConsumer->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs,
Info);
} else {
for (auto &subConsumer : SubConsumers) {
subConsumer.second->handleDiagnostic(SM, Loc, Kind, FormatString,
FormatArgs, Info);
}
}
}

bool FileSpecificDiagnosticConsumer::finishProcessing() {
// Deliberately don't use std::any_of here because we don't want early-exit
// behavior.
bool hadError = false;
for (auto &subConsumer : SubConsumers)
hadError |= subConsumer.second->finishProcessing();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen this idiom before in our code, but I worry about the bool/int conflation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's always safe with C++ bool, so I don't feel too bad about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

return hadError;
}

void NullDiagnosticConsumer::handleDiagnostic(
SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
Expand Down
4 changes: 0 additions & 4 deletions lib/Frontend/PrintingDiagnosticConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ namespace {
};
} // end anonymous namespace

llvm::SMLoc DiagnosticConsumer::getRawLoc(SourceLoc loc) {
return loc.Value;
}

void PrintingDiagnosticConsumer::handleDiagnostic(
SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
Expand Down
7 changes: 4 additions & 3 deletions lib/Frontend/SerializedDiagnosticConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,10 @@ class SerializedDiagnosticConsumer : public DiagnosticConsumer {
};
} // end anonymous namespace

namespace swift { namespace serialized_diagnostics {
DiagnosticConsumer *createConsumer(StringRef serializedDiagnosticsPath) {
return new SerializedDiagnosticConsumer(serializedDiagnosticsPath);
namespace swift {
namespace serialized_diagnostics {
std::unique_ptr<DiagnosticConsumer> createConsumer(StringRef outputPath) {
return llvm::make_unique<SerializedDiagnosticConsumer>(outputPath);
}
} // namespace serialized_diagnostics
} // namespace swift
Expand Down
Loading