Merge pull request #28135 from theblixguy/fix/SR_5115

[MiscDiagnostics] Diagnose passing a non-@objc dynamic KeyPath property to KVO observe

Author: theblixguy

include/swift/AST/DiagnosticsSema.def

@@ -4540,6 +4540,9 @@ WARNING(variable_never_mutated, none,
 WARNING(variable_never_read, none,
         "variable %0 was written to, but never read",
         (Identifier))
+WARNING(observe_keypath_property_not_objc_dynamic, none,
+        "passing reference to non-'@objc dynamic' property %0 to KVO method %1 "
+        "may lead to unexpected behavior or runtime trap", (DeclName, DeclName))
 
 //------------------------------------------------------------------------------
 // MARK: Debug diagnostics

lib/Sema/MiscDiagnostics.cpp

@@ -3990,6 +3990,59 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
   const_cast<Expr *>(E)->walk(Walker);
 }
 
+static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
+                                                     const DeclContext *DC) {
+  class KVOObserveCallWalker : public ASTWalker {
+    const ASTContext &C;
+
+  public:
+    KVOObserveCallWalker(ASTContext &ctx) : C(ctx) {}
+
+    void maybeDiagnoseCallExpr(CallExpr *expr) {
+      auto fn = expr->getCalledValue();
+      if (!fn)
+        return;
+      if (fn->getModuleContext()->getName() != C.Id_Foundation)
+        return;
+      if (!fn->getFullName().isCompoundName("observe",
+                                            {"", "options", "changeHandler"}))
+        return;
+      auto args = cast<TupleExpr>(expr->getArg());
+      auto firstArg = dyn_cast<KeyPathExpr>(args->getElement(0));
+      if (!firstArg)
+        return;
+      auto lastComponent = firstArg->getComponents().back();
+      if (lastComponent.getKind() != KeyPathExpr::Component::Kind::Property)
+        return;
+      auto property = lastComponent.getDeclRef().getDecl();
+      if (!property)
+        return;
+      if (property->isObjCDynamic())
+        return;
+      C.Diags
+          .diagnose(expr->getLoc(),
+                    diag::observe_keypath_property_not_objc_dynamic,
+                    property->getFullName(), fn->getFullName())
+          .highlight(lastComponent.getLoc());
+    }
+
+    std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+      if (!E || isa<ErrorExpr>(E) || !E->getType())
+        return {false, E};
+
+      if (auto *CE = dyn_cast<CallExpr>(E)) {
+        maybeDiagnoseCallExpr(CE);
+        return {false, E};
+      }
+
+      return {true, E};
+    }
+  };
+
+  KVOObserveCallWalker Walker(DC->getASTContext());
+  const_cast<Expr *>(E)->walk(Walker);
+}
+
 //===----------------------------------------------------------------------===//
 // High-level entry points.
 //===----------------------------------------------------------------------===//
@@ -4004,6 +4057,7 @@ void swift::performSyntacticExprDiagnostics(const Expr *E,
   diagRecursivePropertyAccess(E, DC);
   diagnoseImplicitSelfUseInClosure(E, DC);
   diagnoseUnintendedOptionalBehavior(E, DC);
+  maybeDiagnoseCallToKeyValueObserveMethod(E, DC);
   if (!ctx.isSwiftVersionAtLeast(5))
     diagnoseDeprecatedWritableKeyPath(E, DC);
   if (!ctx.LangOpts.DisableAvailabilityChecking)

test/expr/primary/keypath/keypath-observe-objc.swift

@@ -0,0 +1,39 @@
+// RUN: %target-typecheck-verify-swift
+// REQUIRES: objc_interop
+
+import Foundation
+
+class Foo: NSObject {
+  var number1 = 1
+  dynamic var number2 = 2
+  @objc var number3 = 3
+  @objc dynamic var number4 = 4
+}
+
+class Bar: NSObject {
+  @objc dynamic let foo: Foo
+  
+  init(foo: Foo) {
+    self.foo = foo
+    super.init()
+    
+    _ = observe(\.foo.number1, options: [.new]) { _, change in
+      // expected-warning@-1 {{passing reference to non-'@objc dynamic' property 'number1' to KVO method 'observe(_:options:changeHandler:)' may lead to unexpected behavior or runtime trap}}
+      print("observer1")
+    }
+    
+    _ = observe(\.foo.number2, options: [.new]) { _, change in
+      // expected-warning@-1 {{passing reference to non-'@objc dynamic' property 'number2' to KVO method 'observe(_:options:changeHandler:)' may lead to unexpected behavior or runtime trap}}
+      print("observer2")
+    }
+    
+    _ = observe(\.foo.number3, options: [.new]) { _, change in
+      // expected-warning@-1 {{passing reference to non-'@objc dynamic' property 'number3' to KVO method 'observe(_:options:changeHandler:)' may lead to unexpected behavior or runtime trap}}
+      print("observer3")
+    }
+    
+    _ = observe(\.foo.number4, options: [.new]) { _, change in // Okay
+      print("observer4")
+    }
+  }
+}