Skip to content

Refactor conformance checker diagnostics #71041

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
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
8 changes: 4 additions & 4 deletions include/swift/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -1313,9 +1313,8 @@ class ASTContext final {
/// conformance itself, along with a bit indicating whether this diagnostic
/// produces an error.
struct DelayedConformanceDiag {
const ValueDecl *Requirement;
std::function<void()> Callback;
bool IsError;
std::function<void(NormalProtocolConformance *)> Callback;
};

/// Check whether current context has any errors associated with
Expand All @@ -1330,8 +1329,9 @@ class ASTContext final {

/// Add a delayed diagnostic produced while type-checking a
/// particular protocol conformance.
void addDelayedConformanceDiag(NormalProtocolConformance *conformance,
DelayedConformanceDiag fn);
void addDelayedConformanceDiag(
NormalProtocolConformance *conformance, bool isError,
std::function<void(NormalProtocolConformance *)> callback);

/// Retrieve the delayed-conformance diagnostic callbacks for the
/// given normal protocol conformance.
Expand Down
14 changes: 13 additions & 1 deletion include/swift/AST/AnyRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ struct AnyRequestVTable {
static void noteCycleStep(const void *ptr, DiagnosticEngine &diags) {
static_cast<const Request *>(ptr)->noteCycleStep(diags);
}
static SourceLoc getNearestLoc(const void *ptr) {
return static_cast<const Request *>(ptr)->getNearestLoc();
}
};

const uint64_t typeID;
Expand All @@ -66,6 +69,7 @@ struct AnyRequestVTable {
const std::function<void(const void *, llvm::raw_ostream &)> simpleDisplay;
const std::function<void(const void *, DiagnosticEngine &)> diagnoseCycle;
const std::function<void(const void *, DiagnosticEngine &)> noteCycleStep;
const std::function<SourceLoc(const void *)> getNearestLoc;

template <typename Request>
static const AnyRequestVTable *get() {
Expand All @@ -75,7 +79,8 @@ struct AnyRequestVTable {
&Impl<Request>::isEqual,
&Impl<Request>::simpleDisplay,
&Impl<Request>::diagnoseCycle,
&Impl<Request>::noteCycleStep
&Impl<Request>::noteCycleStep,
&Impl<Request>::getNearestLoc
};
return &vtable;
}
Expand Down Expand Up @@ -173,6 +178,11 @@ class AnyRequestBase {
getVTable()->noteCycleStep(getRawStorage(), diags);
}

/// Get the best source location describing the parameters to this request.
SourceLoc getNearestLoc() const {
return getVTable()->getNearestLoc(getRawStorage());
}

/// Compare two instances for equality.
friend bool operator==(const AnyRequestBase<Derived> &lhs,
const AnyRequestBase<Derived> &rhs) {
Expand Down Expand Up @@ -224,6 +234,8 @@ class AnyRequestBase {
/// - Cycle diagnostics operations:
/// void diagnoseCycle(DiagnosticEngine &diags) const;
/// void noteCycleStep(DiagnosticEngine &diags) const;
/// - Source location information:
/// SourceLoc getNearestLoc() const;
///
class ActiveRequest final : public AnyRequestBase<ActiveRequest> {
template <typename T>
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -5342,7 +5342,7 @@ ERROR(actor_protocol_illegal_inheritance,none,
"non-actor type %0 cannot conform to the %1 protocol",
(DeclName, DeclName))
ERROR(distributed_actor_conformance_missing_system_type,none,
"distributed actor %0 does not declare ActorSystem it can be used with.",
"distributed actor %0 does not declare ActorSystem it can be used with",
(DeclName))
NOTE(note_distributed_actor_system_can_be_defined_using_defaultdistributedactorsystem,none,
"you can provide a module-wide default actor system by declaring:\n"
Expand Down
5 changes: 5 additions & 0 deletions include/swift/AST/Evaluator.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ class Evaluator {
/// diagnostics through the given diagnostics engine.
Evaluator(DiagnosticEngine &diags, const LangOptions &opts);

/// For last-ditch diagnostics, get a good approximate source location for
/// the thing we're currently type checking by searching for a request whose
/// source location matches the predicate.
SourceLoc getInnermostSourceLoc(llvm::function_ref<bool(SourceLoc)> fn);

/// Emit GraphViz output visualizing the request graph.
void emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath);

Expand Down
2 changes: 2 additions & 0 deletions include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -6403,6 +6403,8 @@ void simple_display(llvm::raw_ostream &out, const DefaultArgumentExpr *expr);
void simple_display(llvm::raw_ostream &out, const Expr *expr);

SourceLoc extractNearestSourceLoc(const DefaultArgumentExpr *expr);
SourceLoc extractNearestSourceLoc(const MacroExpansionExpr *expr);
SourceLoc extractNearestSourceLoc(const ClosureExpr *expr);

} // end namespace swift

Expand Down
5 changes: 0 additions & 5 deletions include/swift/AST/ProtocolConformanceRef.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,6 @@ class ProtocolConformanceRef {
/// Create a canonical conformance from the current one.
ProtocolConformanceRef getCanonicalConformanceRef() const;

/// Get any additional requirements that are required for this conformance to
/// be satisfied, if they're possible to compute.
llvm::Optional<ArrayRef<Requirement>>
getConditionalRequirementsIfAvailable() const;

/// Get any additional requirements that are required for this conformance to
/// be satisfied.
ArrayRef<Requirement> getConditionalRequirements() const;
Expand Down
87 changes: 70 additions & 17 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,28 @@ template <> struct DenseMapInfo<OverrideSignatureKey> {
};
} // namespace llvm

namespace {

/// If the conformance is in a primary file, we might diagnose some failures
/// early via request evaluation, with all remaining failures diagnosed when
/// we completely force the conformance from typeCheckDecl(). To emit the
/// diagnostics together, we batch them up in the Diags vector.
///
/// If the conformance is in a secondary file, we instead just diagnose a
/// generic "T does not conform to P" error the first time we hit an error
/// via request evaluation. The detailed delayed conformance diagnostics
/// are discarded, since we'll emit them again when we compile the file as
/// a primary file.
struct DelayedConformanceDiags {
/// We set this if we've ever seen an error diagnostic here.
bool HadError = false;

/// The delayed conformance diagnostics that have not been emitted yet.
/// Never actually emitted for a secondary file.
std::vector<ASTContext::DelayedConformanceDiag> Diags;
};

}
struct ASTContext::Implementation {
Implementation();
~Implementation();
Expand Down Expand Up @@ -354,8 +376,7 @@ struct ASTContext::Implementation {

/// Map from normal protocol conformances to diagnostics that have
/// been delayed until the conformance is fully checked.
llvm::DenseMap<NormalProtocolConformance *,
std::vector<ASTContext::DelayedConformanceDiag>>
llvm::DenseMap<NormalProtocolConformance *, ::DelayedConformanceDiags>
DelayedConformanceDiags;

/// Map from normal protocol conformances to missing witnesses that have
Expand Down Expand Up @@ -1865,7 +1886,7 @@ void ASTContext::addCleanup(std::function<void(void)> cleanup) {
}

bool ASTContext::hadError() const {
return Diags.hadAnyError();
return Diags.hadAnyError() || hasDelayedConformanceErrors();
}

/// Retrieve the arena from which we should allocate storage for a type.
Expand Down Expand Up @@ -2733,25 +2754,18 @@ LazyIterableDeclContextData *ASTContext::getOrCreateLazyIterableContextData(
bool ASTContext::hasDelayedConformanceErrors(
NormalProtocolConformance const* conformance) const {

auto hasDelayedErrors = [](std::vector<DelayedConformanceDiag> const& diags) {
return std::any_of(diags.begin(), diags.end(),
[](ASTContext::DelayedConformanceDiag const& diag) {
return diag.IsError;
});
};

if (conformance) {
auto entry = getImpl().DelayedConformanceDiags.find(conformance);
if (entry != getImpl().DelayedConformanceDiags.end())
return hasDelayedErrors(entry->second);
return entry->second.HadError;

return false; // unknown conformance, so no delayed diags either.
}

// check all conformances for any delayed errors
for (const auto &entry : getImpl().DelayedConformanceDiags) {
auto const& diagnostics = entry.getSecond();
if (hasDelayedErrors(diagnostics))
if (diagnostics.HadError)
return true;
}

Expand All @@ -2761,9 +2775,49 @@ bool ASTContext::hasDelayedConformanceErrors(
MissingWitnessesBase::~MissingWitnessesBase() { }

void ASTContext::addDelayedConformanceDiag(
NormalProtocolConformance *conformance,
DelayedConformanceDiag fn) {
getImpl().DelayedConformanceDiags[conformance].push_back(std::move(fn));
NormalProtocolConformance *conformance, bool isError,
std::function<void(NormalProtocolConformance *)> callback) {
if (isError)
conformance->setInvalid();

auto &diagnostics = getImpl().DelayedConformanceDiags[conformance];

if (isError && !diagnostics.HadError) {
diagnostics.HadError = true;

auto *proto = conformance->getProtocol();
auto *dc = conformance->getDeclContext();
auto *sf = dc->getParentSourceFile();
auto *mod = sf->getParentModule();
assert(mod->isMainModule());

// If we have at least one primary file and the conformance is declared in a
// non-primary file, emit a fallback diagnostic.
if ((!sf->isPrimary() && !mod->getPrimarySourceFiles().empty()) ||
TypeCheckerOpts.EnableLazyTypecheck) {
auto complainLoc = evaluator.getInnermostSourceLoc([&](SourceLoc loc) {
if (loc.isInvalid())
return false;

auto *otherSF = mod->getSourceFileContainingLocation(loc);
if (otherSF == nullptr)
return false;

return otherSF->isPrimary();
});

if (complainLoc.isInvalid()) {
complainLoc = conformance->getLoc();
}

Diags.diagnose(complainLoc,
diag::type_does_not_conform,
dc->getSelfInterfaceType(),
proto->getDeclaredInterfaceType());
}
}

diagnostics.Diags.push_back({isError, callback});
}

void ASTContext::addDelayedMissingWitnesses(
Expand All @@ -2789,8 +2843,7 @@ ASTContext::takeDelayedConformanceDiags(NormalProtocolConformance const* cnfrm){
std::vector<ASTContext::DelayedConformanceDiag> result;
auto known = getImpl().DelayedConformanceDiags.find(cnfrm);
if (known != getImpl().DelayedConformanceDiags.end()) {
result = std::move(known->second);
getImpl().DelayedConformanceDiags.erase(known);
std::swap(result, known->second.Diags);
}
return result;
}
Expand Down
11 changes: 11 additions & 0 deletions lib/AST/Evaluator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ Evaluator::Evaluator(DiagnosticEngine &diags, const LangOptions &opts)
debugDumpCycles(opts.DebugDumpCycles),
recorder(opts.RecordRequestReferences) {}

SourceLoc Evaluator::getInnermostSourceLoc(
llvm::function_ref<bool(SourceLoc)> fn) {
for (auto request : llvm::reverse(activeRequests)) {
SourceLoc loc = request.getNearestLoc();
if (fn(loc))
return loc;
}

return SourceLoc();
}

bool Evaluator::checkDependency(const ActiveRequest &request) {
// Record this as an active request.
if (activeRequests.insert(request)) {
Expand Down
8 changes: 8 additions & 0 deletions lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2835,10 +2835,18 @@ void swift::simple_display(llvm::raw_ostream &out,
out << "expression";
}

SourceLoc swift::extractNearestSourceLoc(const ClosureExpr *expr) {
return expr->getLoc();
}

SourceLoc swift::extractNearestSourceLoc(const DefaultArgumentExpr *expr) {
return expr->getLoc();
}

SourceLoc swift::extractNearestSourceLoc(const MacroExpansionExpr *expr) {
return expr->getLoc();
}

// See swift/Basic/Statistic.h for declaration: this enables tracing Exprs, is
// defined here to avoid too much layering violation / circular linkage
// dependency.
Expand Down
6 changes: 0 additions & 6 deletions lib/AST/ProtocolConformance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -483,12 +483,6 @@ NormalProtocolConformance::getTypeWitnessAndDecl(AssociatedTypeDecl *assocType,
return { Type(), nullptr };
}

// If the conditional requirements aren't known, we can't properly run
// inference.
if (!getConditionalRequirementsIfAvailable()) {
return TypeWitnessAndDecl();
}

return evaluateOrDefault(
assocType->getASTContext().evaluator,
TypeWitnessRequest{const_cast<NormalProtocolConformance *>(this),
Expand Down
11 changes: 0 additions & 11 deletions lib/AST/ProtocolConformanceRef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,6 @@ ProtocolConformanceRef::getWitnessByName(Type type, DeclName name) const {
return getConcrete()->getWitnessDeclRef(requirement);
}

llvm::Optional<ArrayRef<Requirement>>
ProtocolConformanceRef::getConditionalRequirementsIfAvailable() const {
if (isConcrete())
return getConcrete()->getConditionalRequirementsIfAvailable();
else
// An abstract conformance is never conditional: any conditionality in the
// concrete types that will eventually pass through this at runtime is
// completely pre-checked and packaged up.
return ArrayRef<Requirement>();
}

ArrayRef<Requirement>
ProtocolConformanceRef::getConditionalRequirements() const {
if (isConcrete())
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7235,7 +7235,7 @@ void ConstraintSystem::maybeProduceFallbackDiagnostic(
// diagnostics already emitted or waiting to be emitted. Because they are
// a better indication of the problem.
ASTContext &ctx = getASTContext();
if (ctx.Diags.hadAnyError() || ctx.hasDelayedConformanceErrors())
if (ctx.hadError())
return;

ctx.Diags.diagnose(target.getLoc(), diag::failed_to_produce_diagnostic);
Expand Down
Loading