Skip to content

[Serialization] Report modularization breaks as proper diagnostics #65713

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 4 commits into from
May 18, 2023
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
21 changes: 21 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,27 @@ ERROR(need_hermetic_seal_to_import_module,none,
"current compilation does not have -experimental-hermetic-seal-at-link",
(Identifier))

ERROR(modularization_issue_decl_moved,Fatal,
"reference to %select{top-level|type}0 %1 broken by a context change; "
"%1 was expected to be in %2, but now a candidate is found only in %3",
(bool, DeclName, Identifier, Identifier))
ERROR(modularization_issue_decl_type_changed,Fatal,
"reference to %select{top-level|type}0 %1 broken by a context change; "
"the details of %1 %select{from %2|}5 changed since building '%3'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this diagnostic in context in the test case made me feel like the details of was a bit vague. Would the declaration kind of make sense? It would be especially helpful if we could print what kind of declaration we expected it to be and what it actually is but that seems like a stretch goal for another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an improvement, I'll apply it in my next PR.

I took another look and we could print more details here. That diagnostics actually points out if there were candidate decls but all of them were filtered out. There's about a dozen reasons why a decl could be filtered out, most of them are simply to differentiate decls sharing a same name. However, we could modify that logic to collect the specific reasons that are triggered and show them in the diagnostics. I've seen that issue very rarely, about once a year, but when it does hit having more details could be critical. If time permits I'll come back to this diagnostic.

"%select{|, it was in %2 and is now found in %4}5",
(bool, DeclName, Identifier, StringRef, Identifier, bool))
ERROR(modularization_issue_decl_not_found,Fatal,
"reference to %select{top-level|type}0 %1 broken by a context change; "
"%1 is not found, it was expected to be in %2",
(bool, DeclName, Identifier))

NOTE(modularization_issue_side_effect_extension_error,none,
"could not deserialize extension",
())
NOTE(modularization_issue_side_effect_type_error,none,
"could not deserialize type for %0",
(DeclName))

ERROR(reserved_member_name,none,
"type member must not be named %0, since it would conflict with the"
" 'foo.%1' expression", (DeclName, StringRef))
Expand Down
154 changes: 126 additions & 28 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ const char InvalidRecordKindError::ID = '\0';
void InvalidRecordKindError::anchor() {}
const char UnsafeDeserializationError::ID = '\0';
void UnsafeDeserializationError::anchor() {}
const char ModularizationError::ID = '\0';
void ModularizationError::anchor() {}

static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error);

Expand All @@ -179,18 +181,108 @@ void ModuleFile::fatal(llvm::Error error) const {
Core->fatal(diagnoseFatal(std::move(error)));
}

SourceLoc ModuleFile::getSourceLoc() const {
auto &SourceMgr = getContext().Diags.SourceMgr;
auto filename = getModuleFilename();
auto bufferID = SourceMgr.getIDForBufferIdentifier(filename);
if (!bufferID)
bufferID = SourceMgr.addMemBufferCopy(StringRef(), filename);
return SourceMgr.getLocForBufferStart(*bufferID);
}

void
ModularizationError::diagnose(const ModuleFile *MF,
DiagnosticBehavior limit) const {
auto &ctx = MF->getContext();

auto diagnoseError = [&](Kind errorKind) {
switch (errorKind) {
case Kind::DeclMoved:
return ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_moved,
declIsType, name, expectedModuleName,
foundModuleName);
case Kind::DeclKindChanged:
return
ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_type_changed,
declIsType, name, expectedModuleName,
referencedFromModuleName, foundModuleName,
foundModuleName != expectedModuleName);
case Kind::DeclNotFound:
return ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_not_found,
declIsType, name, expectedModuleName);
}
llvm_unreachable("Unhandled ModularizationError::Kind in switch.");
};

auto inFlight = diagnoseError(errorKind);
inFlight.limitBehavior(limit);
inFlight.flush();

// We could pass along the `path` information through notes.
// However, for a top-level decl a path would just duplicate the
// expected module name and the decl name from the diagnostic.
}

void TypeError::diagnose(const ModuleFile *MF) const {
MF->getContext().Diags.diagnose(MF->getSourceLoc(),
diag::modularization_issue_side_effect_type_error,
name);
}

void ExtensionError::diagnose(const ModuleFile *MF) const {
MF->getContext().Diags.diagnose(MF->getSourceLoc(),
diag::modularization_issue_side_effect_extension_error);
}

llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const {
if (FileContext)
getContext().Diags.diagnose(SourceLoc(), diag::serialization_fatal,
Core->Name);

auto &ctx = getContext();
if (FileContext) {
if (ctx.LangOpts.EnableDeserializationRecovery) {
// Attempt to report relevant errors as diagnostics.
// At this time, only ModularizationErrors are reported directly. They
// can get here either directly or as underlying causes to a TypeError or
// and ExtensionError.
auto handleModularizationError =
[&](const ModularizationError &modularError) -> llvm::Error {
modularError.diagnose(this);
return llvm::Error::success();
};
error = llvm::handleErrors(std::move(error),
handleModularizationError,
[&](TypeError &typeError) -> llvm::Error {
if (typeError.diagnoseUnderlyingReason(handleModularizationError)) {
typeError.diagnose(this);
return llvm::Error::success();
}
return llvm::make_error<TypeError>(std::move(typeError));
},
[&](ExtensionError &extError) -> llvm::Error {
if (extError.diagnoseUnderlyingReason(handleModularizationError)) {
extError.diagnose(this);
return llvm::Error::success();
}
return llvm::make_error<ExtensionError>(std::move(extError));
});

// If no error is left, it was reported as a diagnostic. There's no
// need to crash.
if (!error)
return llvm::Error::success();
}

// General deserialization failure message.
ctx.Diags.diagnose(getSourceLoc(), diag::serialization_fatal, Core->Name);
}
// Unless in the debugger, crash. ModuleFileSharedCore::fatal() calls abort().
// This allows aggregation of crash logs for compiler development, but in a
// long-running process like LLDB this is undesirable. Only abort() if not in
// the debugger.
if (!getContext().LangOpts.DebuggerSupport)
if (!ctx.LangOpts.DebuggerSupport)
Core->fatal(std::move(error));

// Otherwise, augment the error with contextual information and pass it back.
// Otherwise, augment the error with contextual information at this point
// of failure and pass it back to be reported later.
std::string msg;
{
llvm::raw_string_ostream os(msg);
Expand Down Expand Up @@ -1780,18 +1872,21 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
// is mostly for compiler engineers to understand a likely solution at a
// quick glance.
SmallVector<char, 64> strScratch;
SmallVector<std::string, 2> notes;
auto declName = getXRefDeclNameForError();

auto errorKind = ModularizationError::Kind::DeclNotFound;
Identifier foundIn;
bool isType = false;

if (recordID == XREF_TYPE_PATH_PIECE ||
recordID == XREF_VALUE_PATH_PIECE) {
auto &ctx = getContext();
for (auto nameAndModule : ctx.getLoadedModules()) {
auto baseModule = nameAndModule.second;
auto otherModule = nameAndModule.second;

IdentifierID IID;
IdentifierID privateDiscriminator = 0;
TypeID TID = 0;
bool isType = (recordID == XREF_TYPE_PATH_PIECE);
isType = (recordID == XREF_TYPE_PATH_PIECE);
bool inProtocolExt = false;
bool importedFromClang = false;
bool isStatic = false;
Expand All @@ -1815,10 +1910,10 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {

values.clear();
if (privateDiscriminator) {
baseModule->lookupMember(values, baseModule, name,
otherModule->lookupMember(values, otherModule, name,
getIdentifier(privateDiscriminator));
} else {
baseModule->lookupQualified(baseModule, DeclNameRef(name),
otherModule->lookupQualified(otherModule, DeclNameRef(name),
NL_QualifiedDefault,
values);
}
Expand All @@ -1832,30 +1927,31 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
// Found a full match in a different module. It should be a different
// one because otherwise it would have succeeded on the first search.
// This is usually caused by the use of poorly modularized headers.
auto line = "There is a matching '" +
declName.getString(strScratch).str() +
"' in module '" +
std::string(nameAndModule.first.str()) +
"'. If this is imported from clang, please make sure " +
"the header is part of a single clang module.";
notes.emplace_back(line);
errorKind = ModularizationError::Kind::DeclMoved;
foundIn = otherModule->getName();
break;
} else if (hadAMatchBeforeFiltering) {
// Found a match that was filtered out. This may be from the same
// expected module if there's a type difference. This can be caused
// by the use of different Swift language versions between a library
// with serialized SIL and a client.
auto line = "'" +
declName.getString(strScratch).str() +
"' in module '" +
std::string(nameAndModule.first.str()) +
"' was filtered out.";
notes.emplace_back(line);
errorKind = ModularizationError::Kind::DeclKindChanged;
foundIn = otherModule->getName();
break;
}
}
}

return llvm::make_error<XRefError>("top-level value not found", pathTrace,
declName, notes);
auto declName = getXRefDeclNameForError();
auto expectedIn = baseModule->getName();
auto referencedFrom = getName();
return llvm::make_error<ModularizationError>(declName,
isType,
errorKind,
expectedIn,
referencedFrom,
foundIn,
pathTrace);
}

// Filters for values discovered in the remaining path pieces.
Expand Down Expand Up @@ -7256,7 +7352,8 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) {
// implementation-only import hiding types and decls.
// rdar://problem/60291019
if (error.isA<XRefNonLoadedModuleError>() ||
error.isA<UnsafeDeserializationError>()) {
error.isA<UnsafeDeserializationError>() ||
error.isA<ModularizationError>()) {
consumeError(std::move(error));
return llvm::Error::success();
}
Expand All @@ -7269,7 +7366,8 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) {
auto *TE = static_cast<TypeError*>(errorInfo.get());

if (TE->underlyingReasonIsA<XRefNonLoadedModuleError>() ||
TE->underlyingReasonIsA<UnsafeDeserializationError>()) {
TE->underlyingReasonIsA<UnsafeDeserializationError>() ||
TE->underlyingReasonIsA<ModularizationError>()) {
consumeError(std::move(errorInfo));
return llvm::Error::success();
}
Expand Down
Loading