Skip to content

Commit 5659f48

Browse files
jrose-applejrose-apple
authored
Merge pull request #8698 from jrose-apple/deserialization-recovery-for-subscripts-and-vars
Deserialization recovery for subscripts and properties with missing bases
2 parents ea48ab5 + d0a9ec5 commit 5659f48

File tree

4 files changed

+192
-41
lines changed

4 files changed

+192
-41
lines changed

‎include/swift/Serialization/ModuleFile.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -503,9 +503,7 @@ class ModuleFile : public LazyMemberLoader {
503503
bool readCommentBlock(llvm::BitstreamCursor &cursor);
504504

505505
/// Recursively reads a pattern from \c DeclTypeCursor.
506-
///
507-
/// If the record at the cursor is not a pattern, returns null.
508-
Pattern *maybeReadPattern(DeclContext *owningDC);
506+
llvm::Expected<Pattern *> readPattern(DeclContext *owningDC);
509507

510508
ParameterList *readParameterList();
511509

‎lib/Serialization/Deserialization.cpp

Lines changed: 97 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -459,15 +459,29 @@ ParameterList *ModuleFile::readParameterList() {
459459
return ParameterList::create(getContext(), params);
460460
}
461461

462-
Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
462+
Expected<Pattern *> ModuleFile::readPattern(DeclContext *owningDC) {
463+
// Currently, the only case in which this function can fail (return an error)
464+
// is when reading a pattern for a single variable declaration.
465+
463466
using namespace decls_block;
464467

468+
auto readPatternUnchecked = [this](DeclContext *owningDC) -> Pattern * {
469+
Expected<Pattern *> deserialized = readPattern(owningDC);
470+
if (!deserialized) {
471+
fatal(deserialized.takeError());
472+
}
473+
assert(deserialized.get());
474+
return deserialized.get();
475+
};
476+
465477
SmallVector<uint64_t, 8> scratch;
466478

467479
BCOffsetRAII restoreOffset(DeclTypeCursor);
468480
auto next = DeclTypeCursor.advance(AF_DontPopBlockAtEnd);
469-
if (next.Kind != llvm::BitstreamEntry::Record)
481+
if (next.Kind != llvm::BitstreamEntry::Record) {
482+
error();
470483
return nullptr;
484+
}
471485

472486
/// Local function to record the type of this pattern.
473487
auto recordPatternType = [&](Pattern *pattern, Type type) {
@@ -483,8 +497,7 @@ Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
483497
bool isImplicit;
484498
ParenPatternLayout::readRecord(scratch, isImplicit);
485499

486-
Pattern *subPattern = maybeReadPattern(owningDC);
487-
assert(subPattern);
500+
Pattern *subPattern = readPatternUnchecked(owningDC);
488501

489502
auto result = new (getContext()) ParenPattern(SourceLoc(),
490503
subPattern,
@@ -520,8 +533,7 @@ Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
520533
TuplePatternEltLayout::readRecord(scratch, labelID);
521534
Identifier label = getIdentifier(labelID);
522535

523-
Pattern *subPattern = maybeReadPattern(owningDC);
524-
assert(subPattern);
536+
Pattern *subPattern = readPatternUnchecked(owningDC);
525537
elements.push_back(TuplePatternElt(label, SourceLoc(), subPattern));
526538
}
527539

@@ -537,7 +549,14 @@ Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
537549
bool isImplicit;
538550
NamedPatternLayout::readRecord(scratch, varID, typeID, isImplicit);
539551

540-
auto var = cast<VarDecl>(getDecl(varID));
552+
auto deserialized = getDeclChecked(varID);
553+
if (!deserialized) {
554+
// Pass through the error. It's too bad that it affects the whole pattern,
555+
// but that's what we get.
556+
return deserialized.takeError();
557+
}
558+
559+
auto var = cast<VarDecl>(deserialized.get());
541560
auto result = new (getContext()) NamedPattern(var, isImplicit);
542561
recordPatternType(result, getType(typeID));
543562
restoreOffset.reset();
@@ -556,12 +575,15 @@ Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
556575
case decls_block::TYPED_PATTERN: {
557576
TypeID typeID;
558577
bool isImplicit;
559-
560578
TypedPatternLayout::readRecord(scratch, typeID, isImplicit);
561-
Pattern *subPattern = maybeReadPattern(owningDC);
562-
assert(subPattern);
563579

564-
auto result = new (getContext()) TypedPattern(subPattern, TypeLoc(),
580+
Expected<Pattern *> subPattern = readPattern(owningDC);
581+
if (!subPattern) {
582+
// Pass through any errors.
583+
return subPattern;
584+
}
585+
586+
auto result = new (getContext()) TypedPattern(subPattern.get(), TypeLoc(),
565587
isImplicit);
566588
recordPatternType(result, getType(typeID));
567589
restoreOffset.reset();
@@ -570,8 +592,8 @@ Pattern *ModuleFile::maybeReadPattern(DeclContext *owningDC) {
570592
case decls_block::VAR_PATTERN: {
571593
bool isImplicit, isLet;
572594
VarPatternLayout::readRecord(scratch, isLet, isImplicit);
573-
Pattern *subPattern = maybeReadPattern(owningDC);
574-
assert(subPattern);
595+
596+
Pattern *subPattern = readPatternUnchecked(owningDC);
575597

576598
auto result = new (getContext()) VarPattern(SourceLoc(), isLet, subPattern,
577599
isImplicit);
@@ -1244,8 +1266,8 @@ bool ModuleFile::readMembers(SmallVectorImpl<Decl *> &Members) {
12441266

12451267
// Silently drop the member if it had an override-related problem.
12461268
llvm::handleAllErrors(D.takeError(),
1247-
[](const OverrideError &) { /* expected */ },
1248-
[&](std::unique_ptr<llvm::ErrorInfoBase> unhandled){
1269+
[](const OverrideError &) { /* expected */ },
1270+
[this](std::unique_ptr<llvm::ErrorInfoBase> unhandled) {
12491271
fatal(std::move(unhandled));
12501272
});
12511273
continue;
@@ -2843,13 +2865,25 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
28432865
willSetID, didSetID, overriddenID,
28442866
rawAccessLevel, rawSetterAccessLevel);
28452867

2868+
Identifier name = getIdentifier(nameID);
2869+
2870+
Expected<Decl *> overridden = getDeclChecked(overriddenID);
2871+
if (!overridden) {
2872+
llvm::handleAllErrors(overridden.takeError(),
2873+
[](const XRefError &) { /* expected */ },
2874+
[this](std::unique_ptr<llvm::ErrorInfoBase> unhandled){
2875+
fatal(std::move(unhandled));
2876+
});
2877+
return llvm::make_error<OverrideError>(name);
2878+
}
2879+
28462880
auto DC = ForcedContext ? *ForcedContext : getDeclContext(contextID);
28472881
if (declOrOffset.isComplete())
28482882
return declOrOffset;
28492883

28502884
auto var = createDecl<VarDecl>(/*IsStatic*/isStatic, /*IsLet*/isLet,
2851-
/*IsCaptureList*/false, SourceLoc(),
2852-
getIdentifier(nameID), Type(), DC);
2885+
/*IsCaptureList*/false, SourceLoc(), name,
2886+
Type(), DC);
28532887
var->setHasNonPatternBindingInit(hasNonPatternBindingInit);
28542888
declOrOffset = var;
28552889

@@ -2881,8 +2915,8 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
28812915
if (isImplicit)
28822916
var->setImplicit();
28832917

2884-
if (auto overridden = cast_or_null<VarDecl>(getDecl(overriddenID))) {
2885-
var->setOverriddenDecl(overridden);
2918+
if (auto overriddenVar = cast_or_null<VarDecl>(overridden.get())) {
2919+
var->setOverriddenDecl(overriddenVar);
28862920
AddAttribute(new (ctx) OverrideAttr(SourceLoc()));
28872921
}
28882922

@@ -2963,8 +2997,8 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
29632997
Expected<Decl *> overridden = getDeclChecked(overriddenID);
29642998
if (!overridden) {
29652999
llvm::handleAllErrors(overridden.takeError(),
2966-
[](const XRefError &) { /* expected */ },
2967-
[&](std::unique_ptr<llvm::ErrorInfoBase> unhandled){
3000+
[](const XRefError &) { /* expected */ },
3001+
[this](std::unique_ptr<llvm::ErrorInfoBase> unhandled) {
29683002
fatal(std::move(unhandled));
29693003
});
29703004
return llvm::make_error<OverrideError>(name);
@@ -3082,10 +3116,30 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
30823116
}
30833117

30843118
auto dc = getDeclContext(contextID);
3119+
3120+
SmallVector<std::pair<Pattern *, DeclContextID>, 4> patterns;
3121+
for (unsigned i = 0; i != numPatterns; ++i) {
3122+
auto pattern = readPattern(dc);
3123+
if (!pattern) {
3124+
// Silently drop the pattern if it had an override-related problem.
3125+
llvm::handleAllErrors(pattern.takeError(),
3126+
[](const OverrideError &) { /* expected */ },
3127+
[this](std::unique_ptr<llvm::ErrorInfoBase> unhandled) {
3128+
fatal(std::move(unhandled));
3129+
});
3130+
// ...but continue to read any further patterns we're expecting.
3131+
continue;
3132+
}
3133+
3134+
patterns.emplace_back(pattern.get(), DeclContextID());
3135+
if (!initContextIDs.empty())
3136+
patterns.back().second = initContextIDs[i];
3137+
}
3138+
30853139
auto binding =
30863140
PatternBindingDecl::createDeserialized(ctx, SourceLoc(),
30873141
StaticSpelling.getValue(),
3088-
SourceLoc(), numPatterns, dc);
3142+
SourceLoc(), patterns.size(), dc);
30893143
binding->setEarlyAttrValidation(true);
30903144
declOrOffset = binding;
30913145

@@ -3094,13 +3148,9 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
30943148
if (isImplicit)
30953149
binding->setImplicit();
30963150

3097-
for (unsigned i = 0; i != numPatterns; ++i) {
3098-
auto pattern = maybeReadPattern(dc);
3099-
assert(pattern);
3100-
DeclContext *initContext = nullptr;
3101-
if (!initContextIDs.empty())
3102-
initContext = getDeclContext(initContextIDs[i]);
3103-
binding->setPattern(i, pattern, initContext);
3151+
for (unsigned i = 0; i != patterns.size(); ++i) {
3152+
DeclContext *initContext = getDeclContext(patterns[i].second);
3153+
binding->setPattern(i, patterns[i].first, initContext);
31043154
}
31053155

31063156
break;
@@ -3475,6 +3525,22 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
34753525
overriddenID, rawAccessLevel,
34763526
rawSetterAccessLevel,
34773527
argNameIDs);
3528+
// Resolve the name ids.
3529+
SmallVector<Identifier, 2> argNames;
3530+
for (auto argNameID : argNameIDs)
3531+
argNames.push_back(getIdentifier(argNameID));
3532+
DeclName name(ctx, ctx.Id_subscript, argNames);
3533+
3534+
Expected<Decl *> overridden = getDeclChecked(overriddenID);
3535+
if (!overridden) {
3536+
llvm::handleAllErrors(overridden.takeError(),
3537+
[](const XRefError &) { /* expected */ },
3538+
[this](std::unique_ptr<llvm::ErrorInfoBase> unhandled) {
3539+
fatal(std::move(unhandled));
3540+
});
3541+
return llvm::make_error<OverrideError>(name);
3542+
}
3543+
34783544
auto parent = getDeclContext(contextID);
34793545
if (declOrOffset.isComplete())
34803546
return declOrOffset;
@@ -3483,12 +3549,6 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
34833549
if (declOrOffset.isComplete())
34843550
return declOrOffset;
34853551

3486-
// Resolve the name ids.
3487-
SmallVector<Identifier, 2> argNames;
3488-
for (auto argNameID : argNameIDs)
3489-
argNames.push_back(getIdentifier(argNameID));
3490-
3491-
DeclName name(ctx, ctx.Id_subscript, argNames);
34923552
auto subscript = createDecl<SubscriptDecl>(name, SourceLoc(), nullptr,
34933553
SourceLoc(), TypeLoc(),
34943554
parent, genericParams);
@@ -3523,8 +3583,8 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
35233583

35243584
if (isImplicit)
35253585
subscript->setImplicit();
3526-
if (auto overridden = cast_or_null<SubscriptDecl>(getDecl(overriddenID))) {
3527-
subscript->setOverriddenDecl(overridden);
3586+
if (auto overriddenSub = cast_or_null<SubscriptDecl>(overridden.get())) {
3587+
subscript->setOverriddenDecl(overriddenSub);
35283588
AddAttribute(new (ctx) OverrideAttr(SourceLoc()));
35293589
}
35303590
break;

‎test/Serialization/Recovery/Inputs/custom-modules/Overrides.h

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@
77
- (void)disappearingMethod;
88
- (nullable id)nullabilityChangeMethod;
99
- (nonnull id)typeChangeMethod;
10+
@property (readonly) long disappearingProperty;
1011
#else
1112
//- (void)disappearingMethod;
1213
- (nonnull id)nullabilityChangeMethod;
1314
- (nonnull Base *)typeChangeMethod;
15+
// @property (readonly) long disappearingProperty;
1416
#endif
1517
@end
1618

@@ -33,3 +35,36 @@
3335
- (nonnull Element)typeChangeMethod;
3436
#endif
3537
@end
38+
39+
40+
@interface IndexedSubscriptDisappearsBase : Object
41+
#if !BAD
42+
- (nonnull id)objectAtIndexedSubscript:(long)index;
43+
#else
44+
//- (nonnull id)objectAtIndexedSubscript:(long)index;
45+
#endif
46+
@end
47+
48+
@interface KeyedSubscriptDisappearsBase : Object
49+
#if !BAD
50+
- (nonnull id)objectForKeyedSubscript:(nonnull id)key;
51+
#else
52+
// - (nonnull id)objectForKeyedSubscript:(nonnull id)key;
53+
#endif
54+
@end
55+
56+
@interface GenericIndexedSubscriptDisappearsBase<Element> : Object
57+
#if !BAD
58+
- (nonnull Element)objectAtIndexedSubscript:(long)index;
59+
#else
60+
// - (nonnull Element)objectAtIndexedSubscript:(long)index;
61+
#endif
62+
@end
63+
64+
@interface GenericKeyedSubscriptDisappearsBase<Element> : Object
65+
#if !BAD
66+
- (nonnull Element)objectAtIndexedSubscript:(nonnull id)key;
67+
#else
68+
// - (nonnull Element)objectAtIndexedSubscript:(nonnull id)key;
69+
#endif
70+
@end

‎test/Serialization/Recovery/overrides.swift

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,15 @@ public class A_Sub: Base {
1919
public override func nullabilityChangeMethod() -> Any? { return nil }
2020
public override func typeChangeMethod() -> Any { return self }
2121
public override func disappearingMethodWithOverload() {}
22+
public override var disappearingProperty: Int { return 0 }
2223
}
2324

2425
// CHECK-LABEL: class A_Sub : Base {
2526
// CHECK-NEXT: func disappearingMethod()
2627
// CHECK-NEXT: func nullabilityChangeMethod() -> Any?
2728
// CHECK-NEXT: func typeChangeMethod() -> Any
2829
// CHECK-NEXT: func disappearingMethodWithOverload()
30+
// CHECK-NEXT: var disappearingProperty: Int { get }
2931
// CHECK-NEXT: init()
3032
// CHECK-NEXT: {{^}$}}
3133

@@ -52,4 +54,60 @@ public class B_GenericSub : GenericBase<Base> {
5254

5355
// CHECK-RECOVERY-LABEL: class B_GenericSub : GenericBase<Base> {
5456
// CHECK-RECOVERY-NEXT: init()
55-
// CHECK-RECOVERY-NEXT: {{^}$}}
57+
// CHECK-RECOVERY-NEXT: {{^}$}}
58+
59+
60+
public class C1_IndexedSubscriptDisappears : IndexedSubscriptDisappearsBase {
61+
public override subscript(index: Int) -> Any { return self }
62+
}
63+
64+
// CHECK-LABEL: class C1_IndexedSubscriptDisappears : IndexedSubscriptDisappearsBase {
65+
// CHECK-NEXT: subscript(index: Int) -> Any { get }
66+
// CHECK-NEXT: init()
67+
// CHECK-NEXT: {{^}$}}
68+
69+
// CHECK-RECOVERY-LABEL: class C1_IndexedSubscriptDisappears : IndexedSubscriptDisappearsBase {
70+
// CHECK-RECOVERY-NEXT: init()
71+
// CHECK-RECOVERY-NEXT: {{^}$}}
72+
73+
74+
public class C2_KeyedSubscriptDisappears : KeyedSubscriptDisappearsBase {
75+
public override subscript(key: Any) -> Any { return key }
76+
}
77+
78+
// CHECK-LABEL: class C2_KeyedSubscriptDisappears : KeyedSubscriptDisappearsBase {
79+
// CHECK-NEXT: subscript(key: Any) -> Any { get }
80+
// CHECK-NEXT: init()
81+
// CHECK-NEXT: {{^}$}}
82+
83+
// CHECK-RECOVERY-LABEL: class C2_KeyedSubscriptDisappears : KeyedSubscriptDisappearsBase {
84+
// CHECK-RECOVERY-NEXT: init()
85+
// CHECK-RECOVERY-NEXT: {{^}$}}
86+
87+
88+
public class C3_GenericIndexedSubscriptDisappears : GenericIndexedSubscriptDisappearsBase<Base> {
89+
public override subscript(index: Int) -> Base { fatalError() }
90+
}
91+
92+
// CHECK-LABEL: class C3_GenericIndexedSubscriptDisappears : GenericIndexedSubscriptDisappearsBase<Base> {
93+
// CHECK-NEXT: subscript(index: Int) -> Base { get }
94+
// CHECK-NEXT: init()
95+
// CHECK-NEXT: {{^}$}}
96+
97+
// CHECK-RECOVERY-LABEL: class C3_GenericIndexedSubscriptDisappears : GenericIndexedSubscriptDisappearsBase<Base> {
98+
// CHECK-RECOVERY-NEXT: init()
99+
// CHECK-RECOVERY-NEXT: {{^}$}}
100+
101+
102+
public class C4_GenericKeyedSubscriptDisappears : GenericKeyedSubscriptDisappearsBase<Base> {
103+
public override subscript(key: Any) -> Base { fatalError() }
104+
}
105+
106+
// CHECK-LABEL: class C4_GenericKeyedSubscriptDisappears : GenericKeyedSubscriptDisappearsBase<Base> {
107+
// CHECK-NEXT: subscript(key: Any) -> Base { get }
108+
// CHECK-NEXT: init()
109+
// CHECK-NEXT: {{^}$}}
110+
111+
// CHECK-RECOVERY-LABEL: class C4_GenericKeyedSubscriptDisappears : GenericKeyedSubscriptDisappearsBase<Base> {
112+
// CHECK-RECOVERY-NEXT: init()
113+
// CHECK-RECOVERY-NEXT: {{^}$}}

0 commit comments

Comments
 (0)