[Serialization] Store whether an override depends on its base for ABI (#27784)

In some circumstances, a Swift declaration in module A will depend on
another declaration (usually from Objective-C) that can't be loaded,
for whatever reason. If the Swift declaration is *overriding* the
missing declaration, this can present a problem, because the way
methods are dispatched in Swift can depend on knowing the original
class that introduced the method. However, if the compiler can prove
that the override can still be safely invoked/used in all cases, it
doesn't need to worry about the overridden declaration being missing.

This is especially relevant for property accessors, because there's
currently no logic to recover from a property being successfully
deserialized and then finding out that an accessor couldn't be.

The decision of whether or not an override can be safely invoked
without knowledge of the base method is something to be cautious
about---a mistaken analysis would effectively be a miscompile. So up
until now, this was limited to one case: when a method is known to be
`@objc dynamic`, i.e. always dispatched through objc_msgSend. (Even
this may become questionable if we have first-class method references,
like we do for key paths.) This worked particularly well because the
compiler infers 'dynamic' for any overload of an imported Objective-C
method or accessor, in case it imports differently in a different
-swift-version and a client ends up subclassing it.

However...that inference does not apply if the class is final, because
then there are no subclasses to worry about.

This commit changes the test to be more careful: if the /missing/
declaration was `@objc dynamic`, we know that it can't affect ABI,
because either the override is properly `@objc dynamic` as well, or
the override has introduced its own calling ABI (in practice, a direct
call for final methods) that doesn't depend on the superclass. Again,
this isn't 100% correct in the presence of first-class methods, but it
does fix the issue in practice where a property accessor in a parent
class goes missing. And since Objective-C allows adding property
setters separately from the original property declaration, that's
something that can happen even under normal circumstances. Sadly.

This approach could probably be extended to constructors as well. I'm
a little more cautious about throwing vars and subscripts into the mix
because of the presence of key paths, which do allow identity-based
comparison of overrides and bases.

rdar://problem/56388950

Author: jrose-apple

lib/Serialization/Deserialization.cpp

@@ -2822,7 +2822,7 @@ class swift::DeclDeserializer {
     DeclID associatedDeclID;
     DeclID overriddenID;
     DeclID accessorStorageDeclID;
-    bool needsNewVTableEntry, isTransparent;
+    bool overriddenAffectsABI, needsNewVTableEntry, isTransparent;
     DeclID opaqueReturnTypeID;
     ArrayRef<uint64_t> nameAndDependencyIDs;
 
@@ -2835,6 +2835,7 @@ class swift::DeclDeserializer {
                                           resultInterfaceTypeID,
                                           isIUO,
                                           associatedDeclID, overriddenID,
+                                          overriddenAffectsABI,
                                           numNameComponentsBiased,
                                           rawAccessLevel,
                                           needsNewVTableEntry,
@@ -2849,6 +2850,7 @@ class swift::DeclDeserializer {
                                               resultInterfaceTypeID,
                                               isIUO,
                                               overriddenID,
+                                              overriddenAffectsABI,
                                               accessorStorageDeclID,
                                               rawAccessorKind,
                                               rawAccessLevel,
@@ -2914,20 +2916,11 @@ class swift::DeclDeserializer {
       overridden = overriddenOrError.get();
     } else {
       llvm::consumeError(overriddenOrError.takeError());
-      // There's one case where we know it's safe to ignore a missing override:
-      // if this declaration is '@objc' and 'dynamic'.
-      bool canIgnoreMissingOverriddenDecl = false;
-      if (isObjC && ctx.LangOpts.EnableDeserializationRecovery) {
-        canIgnoreMissingOverriddenDecl =
-            std::any_of(DeclAttributes::iterator(DAttrs),
-                        DeclAttributes::iterator(nullptr),
-                        [](const DeclAttribute *attr) -> bool {
-          return isa<DynamicAttr>(attr);
-        });
-      }
-      if (!canIgnoreMissingOverriddenDecl)
+
+      if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) {
         return llvm::make_error<OverrideError>(
             name, errorFlags, numVTableEntries);
+      }
 
       overridden = nullptr;
     }

lib/Serialization/ModuleFormat.h

@@ -52,7 +52,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// 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.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 519; // SIL function availability
+const uint16_t SWIFTMODULE_VERSION_MINOR = 521; // "overridden affects ABI" flag
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
@@ -1200,6 +1200,7 @@ namespace decls_block {
     BCFixed<1>,   // IUO result?
     DeclIDField,  // operator decl
     DeclIDField,  // overridden function
+    BCFixed<1>,   // whether the overridden decl affects ABI
     BCVBR<5>,     // 0 for a simple name, otherwise the number of parameter name
                   // components plus one
     AccessLevelField, // access level
@@ -1241,6 +1242,7 @@ namespace decls_block {
     TypeIDField,  // result interface type
     BCFixed<1>,   // IUO result?
     DeclIDField,  // overridden function
+    BCFixed<1>,   // whether the overridden decl affects ABI
     DeclIDField,  // AccessorStorageDecl
     AccessorKindField, // accessor kind
     AccessLevelField, // access level

lib/Serialization/Serialization.cpp

@@ -2593,6 +2593,24 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     return count;
   }
 
+  /// Returns true if a client can still use decls that override \p overridden
+  /// even if \p overridden itself isn't available (isn't found, can't be
+  /// imported, can't be deserialized, whatever).
+  ///
+  /// This should be kept conservative. Compiler crashes are still better than
+  /// miscompiles.
+  static bool overriddenDeclAffectsABI(const ValueDecl *overridden) {
+    if (!overridden)
+      return false;
+    // There's one case where we know a declaration doesn't affect the ABI of
+    // its overrides after they've been compiled: if the declaration is '@objc'
+    // and 'dynamic'. In that case, all accesses to the method or property will
+    // go through the Objective-C method tables anyway.
+    if (overridden->hasClangNode() || overridden->isObjCDynamic())
+      return false;
+    return true;
+  }
+
 public:
   DeclSerializer(Serializer &S, DeclID id) : S(S), id(id) {}
   ~DeclSerializer() {
@@ -3212,6 +3230,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
                            fn->isImplicitlyUnwrappedOptional(),
                            S.addDeclRef(fn->getOperatorDecl()),
                            S.addDeclRef(fn->getOverriddenDecl()),
+                           overriddenDeclAffectsABI(fn->getOverriddenDecl()),
                            fn->getFullName().getArgumentNames().size() +
                              fn->getFullName().isCompoundName(),
                            rawAccessLevel,
@@ -3267,6 +3286,9 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     uint8_t rawAccessorKind =
       uint8_t(getStableAccessorKind(fn->getAccessorKind()));
 
+    bool overriddenAffectsABI =
+        overriddenDeclAffectsABI(fn->getOverriddenDecl());
+
     Type ty = fn->getInterfaceType();
     SmallVector<IdentifierID, 4> dependencies;
     for (auto dependency : collectDependenciesFromType(ty->getCanonicalType()))
@@ -3288,6 +3310,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
                                S.addTypeRef(fn->getResultInterfaceType()),
                                fn->isImplicitlyUnwrappedOptional(),
                                S.addDeclRef(fn->getOverriddenDecl()),
+                               overriddenAffectsABI,
                                S.addDeclRef(fn->getStorage()),
                                rawAccessorKind,
                                rawAccessLevel,

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

@@ -8,11 +8,13 @@
 - (nullable id)nullabilityChangeMethod;
 - (nonnull id)typeChangeMethod;
 @property (readonly) long disappearingProperty;
+@property (readwrite) long disappearingPropertySetter;
 #else
 //- (void)disappearingMethod;
 - (nonnull id)nullabilityChangeMethod;
 - (nonnull Base *)typeChangeMethod;
 // @property (readonly) long disappearingProperty;
+@property (readonly) long disappearingPropertySetter;
 #endif
 @end
 

test/Serialization/Recovery/overrides.swift

@@ -72,19 +72,36 @@ public class A_Sub: Base {
   public override func typeChangeMethod() -> Any { return self }
   public override func disappearingMethodWithOverload() {}
   public override var disappearingProperty: Int { return 0 }
+  public override var disappearingPropertySetter: Int {
+    get { return 0 }
+    set {}
+  }
 }
 
 public class A_Sub2: A_Sub {
   public override func disappearingMethod() {}
 }
 
+public final class A_Sub3Final: Base {
+  public override func disappearingMethod() {}
+  public override func nullabilityChangeMethod() -> Any? { return nil }
+  public override func typeChangeMethod() -> Any { return self }
+  public override func disappearingMethodWithOverload() {}
+  public override var disappearingProperty: Int { return 0 }
+  public override var disappearingPropertySetter: Int {
+    get { return 0 }
+    set {}
+  }
+}
+
 
 // CHECK-LABEL: class A_Sub : Base {
 // CHECK-NEXT: func disappearingMethod()
 // CHECK-NEXT: func nullabilityChangeMethod() -> Any?
 // CHECK-NEXT: func typeChangeMethod() -> Any
 // CHECK-NEXT: func disappearingMethodWithOverload()
 // CHECK-NEXT: var disappearingProperty: Int { get }
+// CHECK-NEXT: var disappearingPropertySetter: Int{{$}}
 // CHECK-NEXT: init()
 // CHECK-NEXT: {{^}$}}
 
@@ -93,12 +110,23 @@ public class A_Sub2: A_Sub {
 // CHECK-NEXT: init()
 // CHECK-NEXT: {{^}$}}
 
+// CHECK-LABEL: final class A_Sub3Final : Base {
+// CHECK-NEXT: func disappearingMethod()
+// CHECK-NEXT: func nullabilityChangeMethod() -> Any?
+// CHECK-NEXT: func typeChangeMethod() -> Any
+// CHECK-NEXT: func disappearingMethodWithOverload()
+// CHECK-NEXT: var disappearingProperty: Int { get }
+// CHECK-NEXT: var disappearingPropertySetter: Int{{$}}
+// CHECK-NEXT: init()
+// CHECK-NEXT: {{^}$}}
+
 // CHECK-RECOVERY-LABEL: class A_Sub : Base {
 // CHECK-RECOVERY-NEXT: func disappearingMethod()
 // CHECK-RECOVERY-NEXT: func nullabilityChangeMethod() -> Any?
 // CHECK-RECOVERY-NEXT: func typeChangeMethod() -> Any
 // CHECK-RECOVERY-NEXT: func disappearingMethodWithOverload()
 // CHECK-RECOVERY-NEXT: /* placeholder for disappearingProperty */
+// CHECK-RECOVERY-NEXT: var disappearingPropertySetter: Int{{$}}
 // CHECK-RECOVERY-NEXT: init()
 // CHECK-RECOVERY-NEXT: {{^}$}}
 
@@ -107,6 +135,16 @@ public class A_Sub2: A_Sub {
 // CHECK-RECOVERY-NEXT: init()
 // CHECK-RECOVERY-NEXT: {{^}$}}
 
+// CHECK-RECOVERY-LABEL: class A_Sub3Final : Base {
+// CHECK-RECOVERY-NEXT: func disappearingMethod()
+// CHECK-RECOVERY-NEXT: func nullabilityChangeMethod() -> Any?
+// CHECK-RECOVERY-NEXT: func typeChangeMethod() -> Any
+// CHECK-RECOVERY-NEXT: func disappearingMethodWithOverload()
+// CHECK-RECOVERY-NEXT: /* placeholder for disappearingProperty */
+// CHECK-RECOVERY-NEXT: var disappearingPropertySetter: Int{{$}}
+// CHECK-RECOVERY-NEXT: init()
+// CHECK-RECOVERY-NEXT: {{^}$}}
+
 extension Base {
   @nonobjc func disappearingMethodWithOverload() -> SwiftOnlyClass? { return nil }
 }