Skip to content

[Serialization] Drop enums if a case payload can't be deserialized. #9720

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
5 changes: 3 additions & 2 deletions include/swift/Serialization/ModuleFormat.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const uint16_t VERSION_MAJOR = 0;
/// in source control, you should also update the comment to briefly
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
const uint16_t VERSION_MINOR = 345; // Last change: list of dependency types
const uint16_t VERSION_MINOR = 346; // Last change: dependency types for enums

using DeclID = PointerEmbeddedInt<unsigned, 31>;
using DeclIDField = BCFixed<31>;
Expand Down Expand Up @@ -802,7 +802,8 @@ namespace decls_block {
TypeIDField, // raw type
AccessibilityKindField, // accessibility
BCVBR<4>, // number of conformances
BCArray<TypeIDField> // inherited types
BCVBR<4>, // number of inherited types
BCArray<TypeIDField> // inherited types, followed by dependency types
// Trailed by the generic parameters (if any), the members record, and
// finally conformance info (if any).
>;
Expand Down
24 changes: 18 additions & 6 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3265,24 +3265,35 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
GenericEnvironmentID genericEnvID;
TypeID rawTypeID;
uint8_t rawAccessLevel;
unsigned numConformances;
ArrayRef<uint64_t> rawInheritedIDs;
unsigned numConformances, numInheritedTypes;
ArrayRef<uint64_t> rawInheritedAndDependencyIDs;

decls_block::EnumLayout::readRecord(scratch, nameID, contextID,
isImplicit, genericEnvID, rawTypeID,
rawAccessLevel,
numConformances, rawInheritedIDs);
numConformances, numInheritedTypes,
rawInheritedAndDependencyIDs);

auto DC = getDeclContext(contextID);
if (declOrOffset.isComplete())
return declOrOffset;

Identifier name = getIdentifier(nameID);
for (TypeID dependencyID :
rawInheritedAndDependencyIDs.slice(numInheritedTypes)) {
auto dependency = getTypeChecked(dependencyID);
if (!dependency) {
return llvm::make_error<TypeError>(
name, takeErrorInfo(dependency.takeError()));
}
}

auto genericParams = maybeReadGenericParams(DC);
if (declOrOffset.isComplete())
return declOrOffset;

auto theEnum = createDecl<EnumDecl>(SourceLoc(), getIdentifier(nameID),
SourceLoc(), None, genericParams, DC);
auto theEnum = createDecl<EnumDecl>(SourceLoc(), name, SourceLoc(), None,
genericParams, DC);

declOrOffset = theEnum;

Expand All @@ -3301,7 +3312,8 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {

theEnum->computeType();

handleInherited(theEnum, rawInheritedIDs);
handleInherited(theEnum,
rawInheritedAndDependencyIDs.slice(0, numInheritedTypes));

theEnum->setMemberLoader(this, DeclTypeCursor.GetCurrentBitNo());
skipRecord(DeclTypeCursor, decls_block::MEMBERS);
Expand Down
9 changes: 8 additions & 1 deletion lib/Serialization/ModuleFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1330,7 +1330,14 @@ void ModuleFile::lookupValue(DeclName name,
auto iter = OperatorMethodDecls->find(name.getBaseName());
if (iter != OperatorMethodDecls->end()) {
for (auto item : *iter) {
auto VD = cast<ValueDecl>(getDecl(item.second));
Expected<Decl *> declOrError = getDeclChecked(item.second);
if (!declOrError) {
if (!getContext().LangOpts.EnableDeserializationRecovery)
fatal(declOrError.takeError());
llvm::consumeError(declOrError.takeError());
continue;
}
auto VD = cast<ValueDecl>(declOrError.get());
results.push_back(VD);
}
}
Expand Down
62 changes: 54 additions & 8 deletions lib/Serialization/Serialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2315,12 +2315,46 @@ void Serializer::writeForeignErrorConvention(const ForeignErrorConvention &fec){
resultTypeID);
}

static SmallVector<Type, 4> collectDependenciesFromType(CanType ty) {
llvm::SmallSetVector<Type, 4> result;
ty.visit([&](CanType next) {
if (auto *nominal = next->getAnyNominal())
result.insert(nominal->getDeclaredInterfaceType());
/// Returns true if the declaration of \p decl depends on \p problemContext
/// based on lexical nesting.
///
/// - \p decl is \p problemContext
/// - \p decl is declared within \p problemContext
/// - \p decl is declared in an extension of a type that depends on
/// \p problemContext
static bool contextDependsOn(const NominalTypeDecl *decl,
const DeclContext *problemContext) {
const DeclContext *dc = decl;
do {
if (dc == problemContext)
return true;
if (auto *extension = dyn_cast<ExtensionDecl>(dc))
dc = extension->getAsNominalTypeOrNominalTypeExtensionContext();
else
dc = dc->getParent();
} while (!dc->isModuleScopeContext());
return false;
}

static void collectDependenciesFromType(llvm::SmallSetVector<Type, 4> &seen,
Type ty,
const DeclContext *excluding) {
ty.visit([&](Type next) {
auto *nominal = next->getAnyNominal();
if (!nominal)
return;
// FIXME: The child context case is still important for enums. It's
// possible an enum element has a payload that references a type declaration
// nested within the enum that can't be imported (for whatever reason).
if (contextDependsOn(nominal, excluding))
return;
seen.insert(nominal->getDeclaredInterfaceType());
});
}

static SmallVector<Type, 4> collectDependenciesFromType(Type ty) {
llvm::SmallSetVector<Type, 4> result;
collectDependenciesFromType(result, ty, /*excluding*/nullptr);
return result.takeVector();
}

Expand Down Expand Up @@ -2668,9 +2702,20 @@ void Serializer::writeDecl(const Decl *D) {
ConformanceLookupKind::All,
nullptr, /*sorted=*/true);

SmallVector<TypeID, 4> inheritedTypes;
SmallVector<TypeID, 4> inheritedAndDependencyTypes;
for (auto inherited : theEnum->getInherited())
inheritedTypes.push_back(addTypeRef(inherited.getType()));
inheritedAndDependencyTypes.push_back(addTypeRef(inherited.getType()));

llvm::SmallSetVector<Type, 4> dependencyTypes;
for (const EnumElementDecl *nextElt : theEnum->getAllElements()) {
if (!nextElt->hasAssociatedValues())
continue;
collectDependenciesFromType(dependencyTypes,
nextElt->getArgumentInterfaceType(),
/*excluding*/theEnum);
}
for (Type ty : dependencyTypes)
inheritedAndDependencyTypes.push_back(addTypeRef(ty));

uint8_t rawAccessLevel =
getRawStableAccessibility(theEnum->getFormalAccess());
Expand All @@ -2685,7 +2730,8 @@ void Serializer::writeDecl(const Decl *D) {
addTypeRef(theEnum->getRawType()),
rawAccessLevel,
conformances.size(),
inheritedTypes);
theEnum->getInherited().size(),
inheritedAndDependencyTypes);

writeGenericParams(theEnum->getGenericParams());
writeMembers(theEnum->getMembers(), false);
Expand Down
94 changes: 94 additions & 0 deletions test/Serialization/Recovery/typedefs-in-enums.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// RUN: rm -rf %t && mkdir -p %t
// RUN: %target-swift-frontend -emit-sil -o - -emit-module-path %t/Lib.swiftmodule -module-name Lib -I %S/Inputs/custom-modules -disable-objc-attr-requires-foundation-module %s > /dev/null

// RUN: %target-swift-ide-test -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules | %FileCheck %s

// RUN: %target-swift-ide-test -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules -Xcc -DBAD | %FileCheck -check-prefix CHECK-RECOVERY %s

// RUN: %target-swift-frontend -typecheck -I %t -I %S/Inputs/custom-modules -Xcc -DBAD -DTEST %s -verify

#if TEST

import Typedefs
import Lib

func use(_: OkayEnum) {}
// FIXME: Better to import the enum and make it unavailable.
func use(_: BadEnum) {} // expected-error {{use of undeclared type 'BadEnum'}}

func test() {
_ = producesOkayEnum()
_ = producesBadEnum() // expected-error {{use of unresolved identifier 'producesBadEnum'}}

// Force a lookup of the ==
_ = Optional(OkayEnum.noPayload).map { $0 == .noPayload }
}

#else // TEST

import Typedefs

public enum BadEnum {
case noPayload
case perfectlyOkayPayload(Int)
case problematic(Any, WrappedInt)
case alsoOkay(Any, Any, Any)

static public func ==(a: BadEnum, b: BadEnum) -> Bool {
return false
}
}
// CHECK-LABEL: enum BadEnum {
// CHECK-RECOVERY-NOT: enum BadEnum

public enum OkayEnum {
case noPayload
case plainOldAlias(Any, UnwrappedInt)
case other(Int)

static public func ==(a: OkayEnum, b: OkayEnum) -> Bool {
return false
}
}
// CHECK-LABEL: enum OkayEnum {
// CHECK-NEXT: case noPayload
// CHECK-NEXT: case plainOldAlias(Any, UnwrappedInt)
// CHECK-NEXT: case other(Int)
// CHECK-NEXT: static func ==(a: OkayEnum, b: OkayEnum) -> Bool
// CHECK-NEXT: }
// CHECK-RECOVERY-LABEL: enum OkayEnum {
// CHECK-RECOVERY-NEXT: case noPayload
// CHECK-RECOVERY-NEXT: case plainOldAlias(Any, Int32)
// CHECK-RECOVERY-NEXT: case other(Int)
// CHECK-RECOVERY-NEXT: static func ==(a: OkayEnum, b: OkayEnum) -> Bool
// CHECK-RECOVERY-NEXT: }

public enum OkayEnumWithSelfRefs {
public struct Nested {}
indirect case selfRef(OkayEnumWithSelfRefs)
case nested(Nested)
}
// CHECK-LABEL: enum OkayEnumWithSelfRefs {
// CHECK-NEXT: struct Nested {
// CHECK-NEXT: init()
// CHECK-NEXT: }
// CHECK-NEXT: indirect case selfRef(OkayEnumWithSelfRefs)
// CHECK-NEXT: case nested(OkayEnumWithSelfRefs.Nested)
// CHECK-NEXT: }
// CHECK-RECOVERY-LABEL: enum OkayEnumWithSelfRefs {
// CHECK-RECOVERY-NEXT: struct Nested {
// CHECK-RECOVERY-NEXT: init()
// CHECK-RECOVERY-NEXT: }
// CHECK-RECOVERY-NEXT: indirect case selfRef(OkayEnumWithSelfRefs)
// CHECK-RECOVERY-NEXT: case nested(OkayEnumWithSelfRefs.Nested)
// CHECK-RECOVERY-NEXT: }

public func producesBadEnum() -> BadEnum { return .noPayload }
// CHECK-LABEL: func producesBadEnum() -> BadEnum
// CHECK-RECOVERY-NOT: func producesBadEnum() -> BadEnum

public func producesOkayEnum() -> OkayEnum { return .noPayload }
// CHECK-LABEL: func producesOkayEnum() -> OkayEnum
// CHECK-RECOVERY-LABEL: func producesOkayEnum() -> OkayEnum

#endif // TEST