Merge pull request swiftlang#29255 from CodaFi/categorical-denials

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

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()
+}