Nail Down The Emergent Behaviors of the ClangImporter's Override Checking

The Clang Importer allows for the creation of otherwise illegal Swift
ASTs because it does not run redeclaration checking or override
checking. These behaviors are also not a part of our test suite.

Correct the first issue by re-instating the old emergent behavior of
member loading occurring at all points in the class hierarchy before
a (lazy) lookup.

Correct the second issue by adding a regression test for a common
failure mode in the post-re-entrant-lookup Clang Importer.

We cannot stop accepting these cases, but a future compiler will warn
about them.

Attacks rdar://58493370

Author: CodaFi

lib/ClangImporter/ClangImporter.cpp

@@ -3726,6 +3726,9 @@ void ClangImporter::Implementation::lookupAllObjCMembers(
 // deserialized before loading the named member of this class. This allows the
 // decl members table to be warmed up and enables the correct identification of
 // overrides.
+//
+// FIXME: Very low hanging fruit: Loading everything is extremely wasteful. We
+// should be able to just load the name lazy member loading is asking for.
 static void ensureSuperclassMembersAreLoaded(const ClassDecl *CD) {
   if (!CD)
     return;
@@ -3735,6 +3738,9 @@ static void ensureSuperclassMembersAreLoaded(const ClassDecl *CD) {
     return;
 
   CD->loadAllMembers();
+
+  for (auto *ED : const_cast<ClassDecl *>(CD)->getExtensions())
+    ED->loadAllMembers();
 }
 
 Optional<TinyPtrVector<ValueDecl *>>

lib/ClangImporter/ImportDecl.cpp

@@ -8443,7 +8443,7 @@ createUnavailableDecl(Identifier name, DeclContext *dc, Type type,
 // deserialized before loading the members of this class. This allows the
 // decl members table to be warmed up and enables the correct identification of
 // overrides.
-static void loadAllMembersOfSuperclassIfNeeded(const ClassDecl *CD) {
+static void loadAllMembersOfSuperclassIfNeeded(ClassDecl *CD) {
   if (!CD)
     return;
 
@@ -8452,6 +8452,9 @@ static void loadAllMembersOfSuperclassIfNeeded(const ClassDecl *CD) {
     return;
 
   CD->loadAllMembers();
+
+  for (auto E : CD->getExtensions())
+    E->loadAllMembers();
 }
 
 void

test/ClangImporter/Inputs/frameworks/CategoryOverrides.framework/Headers/CategoryOverrides.h

@@ -0,0 +1,16 @@
+__attribute__((objc_root_class))
+@interface Base
+- (nonnull instancetype)init;
+@end
+
+@interface MyColor : Base
+@property (class, nonatomic, readonly) MyColor *systemRedColor;
+@end
+
+@interface MyBaseClass : Base
+// @property (nonatomic, strong, nullable) Base *derivedMember;
+@end
+
+@interface MyDerivedClass : MyBaseClass
+@property (nonatomic, strong, nullable) Base *derivedMember;
+@end

test/ClangImporter/Inputs/frameworks/CategoryOverrides.framework/Modules/module.modulemap

@@ -0,0 +1,7 @@
+framework module CategoryOverrides {
+  umbrella header "CategoryOverrides.h"
+  module * {
+    export *
+  }
+  exclude header "Private.h"
+}

test/ClangImporter/Inputs/frameworks/CategoryOverrides.framework/PrivateHeaders/Private.h

@@ -0,0 +1,9 @@
+#import <CategoryOverrides/CategoryOverrides.h>
+
+@interface MyBaseClass ()
+@property (nonatomic, strong, nullable) Base *derivedMember;
+@end
+
+@interface MyColor ()
++ (MyColor * _Null_unspecified) systemRedColor;
+@end

test/ClangImporter/objc_redeclared_properties_categories.swift

@@ -0,0 +1,41 @@
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-objc-interop -typecheck -F %S/Inputs/frameworks %s 2>&1 | %FileCheck -check-prefix=CHECK -check-prefix=CHECK-PUBLIC %s
+
+// RUN: echo '#import <CategoryOverrides/Private.h>' > %t.h
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -enable-objc-interop -import-objc-header %t.h %s 2>&1 | %FileCheck -check-prefix=CHECK -check-prefix=CHECK-PRIVATE %s --allow-empty
+
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-objc-interop -emit-pch -F %S/Inputs/frameworks -o %t.pch %t.h
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -enable-objc-interop -import-objc-header %t.pch %s 2>&1 | %FileCheck --allow-empty -check-prefix=CHECK -check-prefix=CHECK-PRIVATE %s
+// RUNT: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -enable-objc-interop -import-objc-header %t.h -pch-output-dir %t/pch %s 2>&1 | %FileCheck --allow-empty -check-prefix=CHECK -check-prefix=CHECK-PRIVATE %s
+
+import CategoryOverrides
+
+// Nail down some emergent behaviors of the Clang Importer's override checking:
+
+
+// A category declared in a (private) header can happen to double-import a property
+// and a function with the same name - both before and after omit-needless-words -
+// as long as they have different contextual types.
+//
+// This configuration appears as an undiagnosed redeclaration of a property and
+// function, which is illegal.
+func colors() {
+  let _ : MyColor = MyColor.systemRed
+  let _ : MyColor = MyColor.systemRed()!
+}
+
+// A category declared in a (private) header can introduce overrides of a property
+// that is otherwise not declared in a base class.
+//
+// This configuration appears as an undiagnosed override in a Swift extension,
+// which is illegal.
+func takesADerivedClass(_ x: MyDerivedClass) {
+  // CHECK-PUBLIC-NOT: has no member 'derivedMember'
+  // CHECK-PRIVATE-NOT: has no member 'derivedMember'
+  x.derivedMember = Base()
+}
+
+func takesABaseClass(_ x: MyBaseClass) {
+  // CHECK-PUBLIC: has no member 'derivedMember'
+  // CHECK-PRIVATE-NOT: has no member 'derivedMember'
+  x.derivedMember = Base()
+}