diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 81c4839089825..5ee91ebbf3e65 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -878,7 +878,7 @@ ERROR(modularization_issue_decl_moved,Fatal, (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'" + "the declaration kind of %1 %select{from %2|}5 changed since building '%3'" "%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, diff --git a/include/swift/Basic/LangOptions.h b/include/swift/Basic/LangOptions.h index 7e2402e145833..fb26c7e859a9f 100644 --- a/include/swift/Basic/LangOptions.h +++ b/include/swift/Basic/LangOptions.h @@ -238,6 +238,9 @@ namespace swift { /// Emit a remark after loading a module. bool EnableModuleLoadingRemarks = false; + /// Emit remarks about contextual inconsistencies in loaded modules. + bool EnableModuleRecoveryRemarks = false; + /// Emit a remark when indexing a system module. bool EnableIndexingSystemModuleRemarks = false; diff --git a/include/swift/Option/Options.td b/include/swift/Option/Options.td index b8e134e4f81b5..eb6cfca1dc49d 100644 --- a/include/swift/Option/Options.td +++ b/include/swift/Option/Options.td @@ -388,7 +388,10 @@ def emit_cross_import_remarks : Flag<["-"], "Rcross-import">, def remark_loading_module : Flag<["-"], "Rmodule-loading">, Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>, - HelpText<"Emit a remark and file path of each loaded module">; + HelpText<"Emit remarks about loaded module">; +def remark_module_recovery : Flag<["-"], "Rmodule-recovery">, + Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>, + HelpText<"Emit remarks about contextual inconsistencies in loaded modules">; def remark_indexing_system_module : Flag<["-"], "Rindexing-system-module">, Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>, diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp index 6856112b062bc..0956d71598574 100644 --- a/lib/Frontend/CompilerInvocation.cpp +++ b/lib/Frontend/CompilerInvocation.cpp @@ -929,6 +929,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args, Opts.EnableCrossImportRemarks = Args.hasArg(OPT_emit_cross_import_remarks); Opts.EnableModuleLoadingRemarks = Args.hasArg(OPT_remark_loading_module); + Opts.EnableModuleRecoveryRemarks = Args.hasArg(OPT_remark_module_recovery); Opts.EnableIndexingSystemModuleRemarks = Args.hasArg(OPT_remark_indexing_system_module); diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp index e14225ae81cff..1d3175e791da4 100644 --- a/lib/Serialization/Deserialization.cpp +++ b/lib/Serialization/Deserialization.cpp @@ -160,8 +160,6 @@ void UnsafeDeserializationError::anchor() {} const char ModularizationError::ID = '\0'; void ModularizationError::anchor() {} -static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error); - /// Skips a single record in the bitstream. /// /// Destroys the stream position if the next entry is not a record. @@ -234,21 +232,15 @@ void ExtensionError::diagnose(const ModuleFile *MF) const { diag::modularization_issue_side_effect_extension_error); } -llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const { - - 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), +llvm::Error +ModuleFile::diagnoseModularizationError(llvm::Error error, + DiagnosticBehavior limit) const { + auto handleModularizationError = + [&](const ModularizationError &modularError) -> llvm::Error { + modularError.diagnose(this, limit); + return llvm::Error::success(); + }; + llvm::Error outError = llvm::handleErrors(std::move(error), handleModularizationError, [&](TypeError &typeError) -> llvm::Error { if (typeError.diagnoseUnderlyingReason(handleModularizationError)) { @@ -265,6 +257,16 @@ llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const { return llvm::make_error(std::move(extError)); }); + return outError; +} + +llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const { + + auto &ctx = getContext(); + if (FileContext) { + if (ctx.LangOpts.EnableDeserializationRecovery) { + error = diagnoseModularizationError(std::move(error)); + // If no error is left, it was reported as a diagnostic. There's no // need to crash. if (!error) @@ -1460,7 +1462,7 @@ ModuleFile::getSubstitutionMapChecked(serialization::SubstitutionMapID id) { for (auto typeID : replacementTypeIDs) { auto typeOrError = getTypeChecked(typeID); if (!typeOrError) { - consumeError(typeOrError.takeError()); + diagnoseAndConsumeError(typeOrError.takeError()); continue; } replacementTypes.push_back(typeOrError.get()); @@ -1715,7 +1717,7 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { if (maybeType.errorIsA()) return maybeType.takeError(); // FIXME: Don't throw away the inner error's information. - consumeError(maybeType.takeError()); + diagnoseAndConsumeError(maybeType.takeError()); return llvm::make_error("couldn't decode type", pathTrace, name); } @@ -2085,7 +2087,7 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { return maybeType.takeError(); // FIXME: Don't throw away the inner error's information. - consumeError(maybeType.takeError()); + diagnoseAndConsumeError(maybeType.takeError()); return llvm::make_error("couldn't decode type", pathTrace, memberName); } @@ -2924,7 +2926,7 @@ class DeclDeserializer { auto maybeType = MF.getTypeChecked(typeID); if (!maybeType) { - llvm::consumeError(maybeType.takeError()); + MF.diagnoseAndConsumeError(maybeType.takeError()); continue; } inheritedTypes.push_back( @@ -3250,10 +3252,10 @@ class DeclDeserializer { return overriddenOrError.takeError(); } else if (MF.allowCompilerErrors()) { // Drop overriding relationship when allowing errors. - llvm::consumeError(overriddenOrError.takeError()); + MF.diagnoseAndConsumeError(overriddenOrError.takeError()); overridden = nullptr; } else { - llvm::consumeError(overriddenOrError.takeError()); + MF.diagnoseAndConsumeError(overriddenOrError.takeError()); if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) { return llvm::make_error(name, errorFlags, numVTableEntries); @@ -3397,7 +3399,7 @@ class DeclDeserializer { if (overridden.errorIsA()) return overridden.takeError(); - llvm::consumeError(overridden.takeError()); + MF.diagnoseAndConsumeError(overridden.takeError()); return llvm::make_error( name, getErrorFlags(), numVTableEntries); @@ -3509,7 +3511,7 @@ class DeclDeserializer { // FIXME: This is actually wrong. We can't just drop stored properties // willy-nilly if the struct is @frozen. - consumeError(backingDecl.takeError()); + MF.diagnoseAndConsumeError(backingDecl.takeError()); return var; } @@ -3726,7 +3728,7 @@ class DeclDeserializer { overridden = overriddenOrError.get(); } else { if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) { - llvm::consumeError(overriddenOrError.takeError()); + MF.diagnoseAndConsumeError(overriddenOrError.takeError()); return llvm::make_error(name, errorFlags, numVTableEntries); } @@ -3734,7 +3736,7 @@ class DeclDeserializer { if (overriddenOrError.errorIsA()) return overriddenOrError.takeError(); - llvm::consumeError(overriddenOrError.takeError()); + MF.diagnoseAndConsumeError(overriddenOrError.takeError()); overridden = nullptr; } @@ -3999,7 +4001,7 @@ class DeclDeserializer { if (!subMapOrError) { // If the underlying type references internal details, ignore it. auto unconsumedError = - consumeErrorIfXRefNonLoadedModule(subMapOrError.takeError()); + MF.consumeExpectedError(subMapOrError.takeError()); if (unconsumedError) return std::move(unconsumedError); } else { @@ -4056,7 +4058,7 @@ class DeclDeserializer { return pattern.takeError(); // Silently drop the pattern... - llvm::consumeError(pattern.takeError()); + MF.diagnoseAndConsumeError(pattern.takeError()); // ...but continue to read any further patterns we're expecting. continue; } @@ -4574,7 +4576,7 @@ class DeclDeserializer { // Pass through deserialization errors. if (overridden.errorIsA()) return overridden.takeError(); - llvm::consumeError(overridden.takeError()); + MF.diagnoseAndConsumeError(overridden.takeError()); DeclDeserializationError::Flags errorFlags; return llvm::make_error( @@ -5098,7 +5100,7 @@ llvm::Error DeclDeserializer::deserializeCustomAttrs() { // is safe to drop when it can't be deserialized. // rdar://problem/56599179. When allowing errors we're doing a best // effort to create a module, so ignore in that case as well. - consumeError(deserialized.takeError()); + MF.diagnoseAndConsumeError(deserialized.takeError()); } else return deserialized.takeError(); } else if (!deserialized.get() && MF.allowCompilerErrors()) { @@ -6030,7 +6032,7 @@ Expected DESERIALIZE_TYPE(NAME_ALIAS_TYPE)( // We're going to recover by falling back to the underlying type, so // just ignore the error. - llvm::consumeError(aliasOrError.takeError()); + MF.diagnoseAndConsumeError(aliasOrError.takeError()); } if (!alias || !alias->getDeclaredInterfaceType()->isEqual(underlyingType)) { @@ -7267,13 +7269,16 @@ Decl *handleErrorAndSupplyMissingProtoMember(ASTContext &context, return suppliedMissingMember; } -Decl *handleErrorAndSupplyMissingMiscMember(llvm::Error &&error) { - llvm::consumeError(std::move(error)); +Decl * +ModuleFile::handleErrorAndSupplyMissingMiscMember(llvm::Error &&error) const { + diagnoseAndConsumeError(std::move(error)); return nullptr; } -Decl *handleErrorAndSupplyMissingMember(ASTContext &context, Decl *container, - llvm::Error &&error) { +Decl * +ModuleFile::handleErrorAndSupplyMissingMember(ASTContext &context, + Decl *container, + llvm::Error &&error) const { // Drop the member if it had a problem. // FIXME: Handle overridable members in class extensions too, someday. if (auto *containingClass = dyn_cast(container)) { @@ -7347,14 +7352,14 @@ void ModuleFile::loadAllMembers(Decl *container, uint64_t contextData) { } } -static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) { +llvm::Error ModuleFile::consumeExpectedError(llvm::Error &&error) { // Missing module errors are most likely caused by an // implementation-only import hiding types and decls. // rdar://problem/60291019 if (error.isA() || error.isA() || error.isA()) { - consumeError(std::move(error)); + diagnoseAndConsumeError(std::move(error)); return llvm::Error::success(); } @@ -7368,7 +7373,7 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) { if (TE->underlyingReasonIsA() || TE->underlyingReasonIsA() || TE->underlyingReasonIsA()) { - consumeError(std::move(errorInfo)); + diagnoseAndConsumeError(std::move(errorInfo)); return llvm::Error::success(); } @@ -7378,6 +7383,19 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) { return std::move(error); } +void ModuleFile::diagnoseAndConsumeError(llvm::Error error) const { + auto &ctx = getContext(); + if (ctx.LangOpts.EnableModuleRecoveryRemarks) { + error = diagnoseModularizationError(std::move(error), + DiagnosticBehavior::Remark); + // If error was already diagnosed it was also consumed. + if (!error) + return; + } + + consumeError(std::move(error)); +} + namespace { class LazyConformanceLoaderInfo final : llvm::TrailingObjects(*thirdOrError); } else if (getContext().LangOpts.EnableDeserializationRecovery) { third = nullptr; - consumeError(thirdOrError.takeError()); + diagnoseAndConsumeError(thirdOrError.takeError()); } else { fatal(thirdOrError.takeError()); } @@ -7655,7 +7673,7 @@ void ModuleFile::finishNormalConformance(NormalProtocolConformance *conformance, if (deserializedReq) { req = cast_or_null(*deserializedReq); } else if (getContext().LangOpts.EnableDeserializationRecovery) { - consumeError(deserializedReq.takeError()); + diagnoseAndConsumeError(deserializedReq.takeError()); req = nullptr; needToFillInOpaqueValueWitnesses = true; } else { @@ -7672,7 +7690,7 @@ void ModuleFile::finishNormalConformance(NormalProtocolConformance *conformance, // In that case, we want the conformance to still be available, but // we can't make use of the relationship to the underlying decl. } else if (getContext().LangOpts.EnableDeserializationRecovery) { - consumeError(deserializedWitness.takeError()); + diagnoseAndConsumeError(deserializedWitness.takeError()); isOpaque = true; witness = nullptr; } else { @@ -7705,7 +7723,7 @@ void ModuleFile::finishNormalConformance(NormalProtocolConformance *conformance, if (witnessSubstitutions.errorIsA() || witnessSubstitutions.errorIsA() || allowCompilerErrors()) { - consumeError(witnessSubstitutions.takeError()); + diagnoseAndConsumeError(witnessSubstitutions.takeError()); isOpaque = true; } else diff --git a/lib/Serialization/ModuleFile.cpp b/lib/Serialization/ModuleFile.cpp index 44f82f17846be..4461863f0bfe1 100644 --- a/lib/Serialization/ModuleFile.cpp +++ b/lib/Serialization/ModuleFile.cpp @@ -328,7 +328,7 @@ void ModuleFile::lookupValue(DeclName name, if (!declOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(declOrError.takeError()); - llvm::consumeError(declOrError.takeError()); + diagnoseAndConsumeError(declOrError.takeError()); continue; } auto VD = cast(declOrError.get()); @@ -347,7 +347,7 @@ void ModuleFile::lookupValue(DeclName name, if (!declOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(declOrError.takeError()); - llvm::consumeError(declOrError.takeError()); + diagnoseAndConsumeError(declOrError.takeError()); continue; } auto VD = cast(declOrError.get()); @@ -589,7 +589,7 @@ void ModuleFile::lookupVisibleDecls(ImportPath::Access accessPath, if (!declOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(declOrError.takeError()); - llvm::consumeError(declOrError.takeError()); + diagnoseAndConsumeError(declOrError.takeError()); return; } consumer.foundDecl(cast(declOrError.get()), @@ -638,7 +638,7 @@ void ModuleFile::loadExtensions(NominalTypeDecl *nominal) { if (!declOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(declOrError.takeError()); - llvm::consumeError(declOrError.takeError()); + diagnoseAndConsumeError(declOrError.takeError()); } } } else { @@ -651,7 +651,7 @@ void ModuleFile::loadExtensions(NominalTypeDecl *nominal) { if (!declOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(declOrError.takeError()); - llvm::consumeError(declOrError.takeError()); + diagnoseAndConsumeError(declOrError.takeError()); } } } @@ -708,7 +708,7 @@ void ModuleFile::loadDerivativeFunctionConfigurations( if (!derivativeGenSigOrError) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(derivativeGenSigOrError.takeError()); - llvm::consumeError(derivativeGenSigOrError.takeError()); + diagnoseAndConsumeError(derivativeGenSigOrError.takeError()); } auto derivativeGenSig = derivativeGenSigOrError.get(); // NOTE(TF-1038): Result indices are currently unsupported in derivative @@ -882,7 +882,7 @@ void ModuleFile::lookupClassMembers(ImportPath::Access accessPath, if (!decl) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(decl.takeError()); - llvm::consumeError(decl.takeError()); + diagnoseAndConsumeError(decl.takeError()); continue; } @@ -905,7 +905,7 @@ void ModuleFile::lookupClassMembers(ImportPath::Access accessPath, if (!decl) { if (!getContext().LangOpts.EnableDeserializationRecovery) fatal(decl.takeError()); - llvm::consumeError(decl.takeError()); + diagnoseAndConsumeError(decl.takeError()); continue; } diff --git a/lib/Serialization/ModuleFile.h b/lib/Serialization/ModuleFile.h index aabc132c150d2..392cca8f83de2 100644 --- a/lib/Serialization/ModuleFile.h +++ b/lib/Serialization/ModuleFile.h @@ -341,6 +341,12 @@ class ModuleFile template T *createDecl(Args &&... args); + Decl *handleErrorAndSupplyMissingMiscMember(llvm::Error &&error) const; + + Decl *handleErrorAndSupplyMissingMember(ASTContext &context, + Decl *container, + llvm::Error &&error) const; + public: /// Change the status of the current module. Status error(Status issue) { @@ -404,6 +410,23 @@ class ModuleFile return llvm::consumeError(diagnoseFatal(msg)); } + /// Consume errors that are usually safe to ignore because they + /// are expected to support language features or caused by project + /// misconfigurations. + /// + /// If the error is handled, success is returned, otherwise the original + /// error is returned. + llvm::Error consumeExpectedError(llvm::Error &&error); + + /// Report project errors as remarks if desired, otherwise drop the error + /// silently. + void diagnoseAndConsumeError(llvm::Error error) const; + + /// Report modularization error, either directly or indirectly if it + /// caused a \c TypeError or \c ExtensionError. Returns any unhandled errors. + llvm::Error + diagnoseModularizationError(llvm::Error error, + DiagnosticBehavior limit = DiagnosticBehavior::Fatal) const; /// Report an unexpected format error that could happen only from a /// memory-level inconsistency. Please prefer passing an error to diff --git a/test/Serialization/Recovery/module-recovery-remarks.swift b/test/Serialization/Recovery/module-recovery-remarks.swift new file mode 100644 index 0000000000000..d5b05300e3434 --- /dev/null +++ b/test/Serialization/Recovery/module-recovery-remarks.swift @@ -0,0 +1,49 @@ +// RUN: %empty-directory(%t) +// RUN: %empty-directory(%t/sdk) +// RUN: split-file %s %t + +/// Compile two library modules A and A_related, and a middle library LibWithXRef with a reference to a type in A. +// RUN: %target-swift-frontend %t/LibOriginal.swift -emit-module-path %t/A.swiftmodule -module-name A -I %t +// RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A_related.swiftmodule -module-name A_related +// RUN: %target-swift-frontend %t/LibWithXRef.swift -emit-module-path %t/sdk/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t -swift-version 5 -enable-library-evolution + +/// Move MyType from A to A_related, triggering most notes. +// RUN: %target-swift-frontend %t/EmptyOverlay.swift -emit-module-path %t/A.swiftmodule -module-name A -I %t +// RUN: %target-swift-frontend %t/LibOriginal.swift -emit-module-path %t/A_related.swiftmodule -module-name A_related -I %t +// RUN: not %target-swift-frontend -c -O %t/Client.swift -I %t -I %t/sdk -Rmodule-recovery -sdk %t/sdk -swift-version 4 2>&1 \ +// RUN: | %FileCheck --check-prefixes CHECK-MOVED %s + +/// Main error downgraded to a remark. +// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related' +//--- module.modulemap +module A { + header "A.h" +} + +//--- A.h +void foo() {} + +//--- Empty.swift + +//--- EmptyOverlay.swift +@_exported import A + +//--- LibOriginal.swift +@_exported import A + +public struct MyType { + public init() {} +} + +//--- LibWithXRef.swift +import A +import A_related + +public func foo() -> MyType { + fatalError() +} + +//--- Client.swift +import LibWithXRef + +foo() diff --git a/test/Serialization/modularization-error.swift b/test/Serialization/modularization-error.swift index 2c0ae39b05bec..c391bf5f61b4b 100644 --- a/test/Serialization/modularization-error.swift +++ b/test/Serialization/modularization-error.swift @@ -19,14 +19,14 @@ // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/B.swiftmodule -module-name B // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \ // RUN: | %FileCheck --check-prefixes CHECK,CHECK-KIND-CHANGED %s -// CHECK-KIND-CHANGED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the details of 'MyType' from 'A' changed since building 'LibWithXRef' +// CHECK-KIND-CHANGED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' from 'A' changed since building 'LibWithXRef' /// Change MyType into a function and move it. // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A.swiftmodule -module-name A // RUN: %target-swift-frontend %t/LibTypeChanged.swift -emit-module-path %t/B.swiftmodule -module-name B // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \ // RUN: | %FileCheck --check-prefixes CHECK,CHECK-KIND-CHANGED-AND-MOVED %s -// CHECK-KIND-CHANGED-AND-MOVED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the details of 'MyType' changed since building 'LibWithXRef', it was in 'A' and is now found in 'B' +// CHECK-KIND-CHANGED-AND-MOVED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' changed since building 'LibWithXRef', it was in 'A' and is now found in 'B' /// Remove MyType from all imported modules. // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A.swiftmodule -module-name A