[ClangImporter] Handle subscript getters redeclared in subclasses (#13470)

Objective-C subscripts don't have special declarations like
properties; they're just specially-named methods (or method pairs),
and we have to make an independent SubscriptDecl in Swift. This means
that when a subclass wants to make a subscript settable, they just add
the appropriately-named setter method.

Swift handled this by detecting when the getter and setter weren't
declared in the same type, and assuming this meant it was a subclass
adding a setter. Unfortunately, the same condition /also/ picked up
the case where the getter (and only the getter) is /redeclared/ in a
subclass (perhaps to add an attribute), and the new subscript was
getting added to the base class instead of the subclass.

The fix relies on the fact that the original decl we provide is what
we use to look up the other accessor. If the getter and setter are in
different types, whichever one we started with must be the
more-derived one. So the final change is just "did we start with the
setter?" rather than "is there a setter at all?".

I'm not sure why this is only just now causing problems, given that we
seem to have been getting this wrong for years, but it definitely
/was/ wrong and now it's not.

rdar://problem/36033356

Author: jrose-apple

lib/ClangImporter/ImportDecl.cpp

@@ -6345,8 +6345,6 @@ SwiftDeclConverter::importSubscript(Decl *decl,
              ->getAsNominalTypeOrNominalTypeExtensionContext() ==
          setter->getDeclContext()
              ->getAsNominalTypeOrNominalTypeExtensionContext());
-    // TODO: Possible that getter and setter are different instantiations
-    // of the same objc generic type?
 
     // Whether we can update the types involved in the subscript
     // operation.
@@ -6401,8 +6399,16 @@ SwiftDeclConverter::importSubscript(Decl *decl,
     }
   }
 
-  // The context into which the subscript should go.
-  bool associateWithSetter = setter && !getterAndSetterInSameType;
+  // The context into which the subscript should go. We prefer wherever the
+  // getter is declared unless the two accessors are in different types and the
+  // one we started with is the setter. This happens when:
+  // - A read-only subscript is made read/write is a subclass.
+  // - A setter is redeclared in a subclass, but not the getter.
+  // And not when:
+  // - A getter is redeclared in a subclass, but not the setter.
+  // - The getter and setter are part of the same type.
+  // - There is no setter.
+  bool associateWithSetter = !getterAndSetterInSameType && setter == decl;
   DeclContext *dc =
       associateWithSetter ? setter->getDeclContext() : getter->getDeclContext();
 

test/ClangImporter/Inputs/custom-modules/ObjCSubscripts.h

@@ -29,3 +29,34 @@
 - (NSString *)objectForKeyedSubscript:(NSString *)subscript;
 - (void)setObject:(NSString *)object forKeyedSubscript:(NSString *)key;
 @end
+
+
+// rdar://problem/36033356 failed specifically when the base class was never
+// subscripted, so please don't mention this class in the .swift file.
+@interface KeySubscriptBase
+- (id)objectForKeyedSubscript:(NSString *)subscript;
+- (void)setObject:(id)object forKeyedSubscript:(NSString *)key;
+@end
+
+@interface KeySubscriptOverrideGetter : KeySubscriptBase
+- (id)objectForKeyedSubscript:(NSString *)subscript;
+@end
+
+@interface KeySubscriptOverrideSetter : KeySubscriptBase
+- (void)setObject:(id)object forKeyedSubscript:(NSString *)key;
+@end
+
+// rdar://problem/36033356 failed specifically when the base class was never
+// subscripted, so please don't mention this class in the .swift file.
+@interface KeySubscriptReversedBase
+- (void)setObject:(id)object forKeyedSubscript:(NSString *)key;
+- (id)objectForKeyedSubscript:(NSString *)subscript;
+@end
+
+@interface KeySubscriptReversedOverrideGetter : KeySubscriptReversedBase
+- (id)objectForKeyedSubscript:(NSString *)subscript;
+@end
+
+@interface KeySubscriptReversedOverrideSetter : KeySubscriptReversedBase
+- (void)setObject:(id)object forKeyedSubscript:(NSString *)key;
+@end

test/ClangImporter/objc_subscript.swift

@@ -48,3 +48,24 @@ class ConformsToKeySubscriptProto2 : KeySubscriptProto2 {
     set { }
   }
 }
+
+func testOverridesWithoutBase(
+  o1: KeySubscriptOverrideGetter,
+  o2: KeySubscriptOverrideSetter,
+  o3: KeySubscriptReversedOverrideGetter,
+  o4: KeySubscriptReversedOverrideSetter
+) {
+  // rdar://problem/36033356 failed specifically when the base class was never
+  // subscripted, so please don't mention the base classes here.
+  _ = o1["abc"]
+  o1["abc"] = "xyz"
+
+  _ = o2["abc"]
+  o2["abc"] = "xyz"
+
+  _ = o3["abc"]
+  o3["abc"] = "xyz"
+
+  _ = o4["abc"]
+  o4["abc"] = "xyz"
+}