Avoid invalidating @_objcImplementation

In #69257, we modified `ObjCReason` to carry a pointer to the @implementation attribute for the `MemberOfObjCImplementationExtension` kind. This made it mark the @implementation attribute as invalid, suppressing diagnostics from the ObjCImplementationChecker.

However, invalidating the attribute *also* causes it to be skipped by serialization. That isn’t a problem if the diagnostics are errors, since we’ll never emit the serialized module, but #74135 softened these diagnostics to warnings for early adopters.

The upshot was that if Swift emitted one of these warnings when it compiled a library, clients of that library would see the objcImpl extension as a normal extension instead. This would cause various kinds of mischief: ambiguous name lookups because implementations weren’t being excluded, overrides failing because an implementation was `public` instead of `open`, asserts and crashes in SILGen and IRGen because stored properties were found in seemingly normal extensions, etc.

Fix this by setting a separate bit on ObjCImplementationAttr, rather than the invalid bit, and modifying the implementation checker to manually suppress many diagnostics when that bit is set.

Fixes rdar://134730183.

Author: beccadax

include/swift/AST/Attr.h

@@ -184,8 +184,9 @@ class DeclAttribute : public AttributeBase {
       isUnchecked : 1
     );
 
-    SWIFT_INLINE_BITFIELD(ObjCImplementationAttr, DeclAttribute, 2,
+    SWIFT_INLINE_BITFIELD(ObjCImplementationAttr, DeclAttribute, 3,
       isCategoryNameInvalid : 1,
+      hasInvalidImplicitLangAttrs : 1,
       isEarlyAdopter : 1
     );
 
@@ -2434,6 +2435,7 @@ class ObjCImplementationAttr final : public DeclAttribute {
       : DeclAttribute(DeclAttrKind::ObjCImplementation, AtLoc, Range, Implicit),
         CategoryName(CategoryName) {
     Bits.ObjCImplementationAttr.isCategoryNameInvalid = isCategoryNameInvalid;
+    Bits.ObjCImplementationAttr.hasInvalidImplicitLangAttrs = false;
     Bits.ObjCImplementationAttr.isEarlyAdopter = isEarlyAdopter;
   }
 
@@ -2451,6 +2453,16 @@ class ObjCImplementationAttr final : public DeclAttribute {
     Bits.ObjCImplementationAttr.isCategoryNameInvalid = newValue;
   }
 
+  /// Has at least one implicitly ObjC member failed to validate? If so,
+  /// diagnostics that might be duplicative will be suppressed.
+  bool hasInvalidImplicitLangAttrs() const {
+    return Bits.ObjCImplementationAttr.hasInvalidImplicitLangAttrs;
+  }
+
+  void setHasInvalidImplicitLangAttrs(bool newValue = true) {
+    Bits.ObjCImplementationAttr.hasInvalidImplicitLangAttrs = newValue;
+  }
+
   static bool classof(const DeclAttribute *DA) {
     return DA->getKind() == DeclAttrKind::ObjCImplementation;
   }

lib/Sema/TypeCheckDeclObjC.cpp

@@ -153,7 +153,12 @@ void ObjCReason::describe(const Decl *D) const {
 }
 
 void ObjCReason::setAttrInvalid() const {
-  if (requiresAttr(kind))
+  if (!requiresAttr(kind))
+    return;
+
+  if (kind == MemberOfObjCImplementationExtension)
+    cast<ObjCImplementationAttr>(getAttr())->setHasInvalidImplicitLangAttrs();
+  else
     getAttr()->setInvalid();
 }
 
@@ -3074,6 +3079,17 @@ class ObjCImplementationChecker {
 
   bool hasDiagnosed = false;
 
+  bool hasInvalidLangAttr(ValueDecl *cand) {
+    // If isObjC() found a problem, it will have set the invalid bit on either
+    // the candidate's ObjCAttr, if it has one, or the controlling
+    // ObjCImplementationAttr otherwise.
+    if (auto objc = cand->getAttrs()
+                            .getAttribute<ObjCAttr>(/*AllowInvalid=*/true))
+      return objc->isInvalid();
+
+    return getAttr()->hasInvalidImplicitLangAttrs() || getAttr()->isInvalid();
+  }
+
 public:
   ObjCImplementationChecker(Decl *D)
       : decl(D), hasDiagnosed(getAttr()->isInvalid())
@@ -3158,18 +3174,11 @@ class ObjCImplementationChecker {
       return;
 
     // Don't diagnose if we already diagnosed an unrelated ObjC interop issue,
-    // like an un-representable type. If there's an `@objc` attribute on the
-    // member, this will be indicated by its `isInvalid()` bit; otherwise we'll
-    // use the enclosing extension's `@_objcImplementation` attribute.
-    DeclAttribute *attr = afd->getAttrs()
-                              .getAttribute<ObjCAttr>(/*AllowInvalid=*/true);
-    if (!attr)
-      attr = member->getDeclContext()->getAsDecl()->getAttrs()
-                 .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
-    assert(attr && "expected @_objcImplementation on context of member checked "
-                   "by ObjCImplementationChecker");
-    if (attr->isInvalid())
+    // like an un-representable type.
+    if (hasInvalidLangAttr(member)) {
+      hasDiagnosed = true;
       return;
+    }
 
     if (auto init = dyn_cast<ConstructorDecl>(afd)) {
       if (!init->isObjC() && (init->isRequired() ||
@@ -3651,6 +3660,13 @@ class ObjCImplementationChecker {
 
   void diagnoseOutcome(MatchOutcome outcome, ValueDecl *req, ValueDecl *cand,
                        ObjCSelector explicitObjCName) {
+    // If the candidate was invalid, we've already diagnosed the likely cause of
+    // the mismatch. Don't dogpile.
+    if (hasInvalidLangAttr(cand)) {
+      hasDiagnosed = true;
+      return;
+    }
+
     auto reqObjCName = getObjCName(req);
 
     switch (outcome) {

test/Serialization/Inputs/module.modulemap

@@ -6,3 +6,7 @@ module CLibrary {
 module RawLayoutCXX {
   header "raw_layout_cxx.h"
 }
+
+module objc_implementation {
+  header "objc_implementation.h"
+}

test/Serialization/Inputs/objc_implementation.h

@@ -0,0 +1,11 @@
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface ObjCImpl : NSObject
+
+- (void)goodMethod;
+
+@end
+
+NS_ASSUME_NONNULL_END

test/Serialization/objc_implementation.swift

@@ -0,0 +1,25 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-typecheck-verify-swift -target %target-stable-abi-triple -module-name main -I %S/Inputs %s
+// RUN: %target-swift-frontend -emit-module -target %target-stable-abi-triple -module-name main -o %t -I %S/Inputs %s
+// RUN: llvm-bcanalyzer %t/main.swiftmodule > %t/main.swiftmodule.txt
+// RUN: %target-swift-ide-test -print-module -target %target-stable-abi-triple -module-to-print=main -I %t -source-filename=%s >%t/main.txt
+// RUN: %FileCheck %s --input-file=%t/main.txt
+
+// REQUIRES: objc_interop
+
+import Foundation
+import objc_implementation
+
+// rdar://134730183 - ensure that errors reduced to warnings by early adopter
+// syntax don't invalidate the @implementation attribute (and cause it to not
+// be serialized)
+
+// CHECK-LABEL: @_objcImplementation extension ObjCImpl
+@_objcImplementation extension ObjCImpl {
+  // CHECK-DAG: func cannotBeObjCMethod(_ value: Int?)
+  private func cannotBeObjCMethod(_ value: Int?) {}
+  // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+
+  // CHECK-DAG: @objc func goodMethod()
+  @objc public func goodMethod() {}
+}

test/decl/ext/Inputs/objc_implementation.h

@@ -127,6 +127,13 @@
 
 @end
 
+@interface ObjCClass (InvalidMembers)
+
+- (void)unimplementedMember;
+- (void)nonObjCMethod:(id)value;
+
+@end
+
 @interface ObjCClass (EmptyCategory)
 @end
 
@@ -162,6 +169,10 @@
 - (nullable id)nullableResult;
 - (void)nullableArgument:(nullable id)arg;
 
+@end
+
+@interface ObjCClass (TypeMatchOptionalityInvalid)
+
 - (int)nonPointerResult;
 - (void)nonPointerArgument:(int)arg;
 

test/decl/ext/objc_implementation.swift

@@ -229,11 +229,6 @@ protocol EmptySwiftProto {}
   // rdar://122280735 - crash when the parameter of a block property needs @escaping
   let rdar122280735: (() -> ()) -> Void = { _ in }
   // expected-error@-1 {{property 'rdar122280735' of type '(() -> ()) -> Void' does not match type '(@escaping () -> Void) -> Void' declared by the header}}
-
-  private func privateNonObjCMethod(_: EmptySwiftProto) {
-    // expected-error@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
-    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
-  }
 }
 
 @objc(PresentAdditions) @implementation extension ObjCClass {
@@ -432,11 +427,27 @@ protocol EmptySwiftProto {}
 
   func nullableResult() -> Any { fatalError() } // expected-error {{instance method 'nullableResult()' of type '() -> Any' does not match type '() -> Any?' declared by the header}}
   func nullableArgument(_: Any) {} // expected-error {{instance method 'nullableArgument' of type '(Any) -> ()' does not match type '(Any?) -> Void' declared by the header}}
+}
 
+@objc(TypeMatchOptionalityInvalid) @implementation extension ObjCClass {
   func nonPointerResult() -> CInt! { fatalError() } // expected-error{{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because its result type cannot be represented in Objective-C}}
   func nonPointerArgument(_: CInt!) {} // expected-error {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
 }
 
+@objc(InvalidMembers) @implementation extension ObjCClass {
+  // expected-error@-1 {{extension for category 'InvalidMembers' should provide implementation for instance method 'unimplementedMember()'}}
+
+  func nonObjCMethod(_: EmptySwiftProto) {
+    // expected-error@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
+  }
+
+  private func privateNonObjCMethod(_: EmptySwiftProto) {
+    // expected-error@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
+  }
+}
+
 // Intentionally using `@_objcImplementation` for this test; do not upgrade!
 @_objcImplementation(EmptyCategory) extension ObjCClass {
   // expected-warning@-1 {{'@_objcImplementation' is deprecated; use '@implementation' instead}} {{1-36=@implementation}} {{1-1=@objc(EmptyCategory) }}

test/decl/ext/objc_implementation_early_adopter.swift

@@ -229,11 +229,6 @@ protocol EmptySwiftProto {}
   // rdar://122280735 - crash when the parameter of a block property needs @escaping
   let rdar122280735: (() -> ()) -> Void = { _ in }
   // expected-warning@-1 {{property 'rdar122280735' of type '(() -> ()) -> Void' does not match type '(@escaping () -> Void) -> Void' declared by the header}}
-
-  private func privateNonObjCMethod(_: EmptySwiftProto) {
-    // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
-    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
-  }
 }
 
 @_objcImplementation(PresentAdditions) extension ObjCClass {
@@ -432,11 +427,27 @@ protocol EmptySwiftProto {}
 
   func nullableResult() -> Any { fatalError() } // expected-warning {{instance method 'nullableResult()' of type '() -> Any' does not match type '() -> Any?' declared by the header}}
   func nullableArgument(_: Any) {} // expected-warning {{instance method 'nullableArgument' of type '(Any) -> ()' does not match type '(Any?) -> Void' declared by the header}}
+}
 
+@_objcImplementation(TypeMatchOptionalityInvalid) extension ObjCClass {
   func nonPointerResult() -> CInt! { fatalError() } // expected-warning{{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because its result type cannot be represented in Objective-C}}
   func nonPointerArgument(_: CInt!) {} // expected-warning {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
 }
 
+@_objcImplementation(InvalidMembers) extension ObjCClass {
+  // expected-warning@-1 {{extension for category 'InvalidMembers' should provide implementation for instance method 'unimplementedMember()'}}
+
+  func nonObjCMethod(_: EmptySwiftProto) {
+    // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
+  }
+
+  private func privateNonObjCMethod(_: EmptySwiftProto) {
+    // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
+  }
+}
+
 // Intentionally using `@_objcImplementation` for this test; do not upgrade!
 @_objcImplementation(EmptyCategory) extension ObjCClass {
   // expected-warning@-1 {{'@_objcImplementation' is deprecated; use '@implementation' instead}} {{1-36=@implementation}} {{1-1=@objc(EmptyCategory) }}