Skip to content

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Feb 27, 2020

  • Explanation:

In #28135, we added a new warning when a non-@objc dynamic property is passed to the KVO observe method, because passing such a property can lead to unexpected behaviour or runtime trap.

However, the user might have an @objc property (without the dynamic modifier) and perform the updates manually, by calling willChangeValue and didChangeValue:

private var _bar: Int = 0
@objc var bar: Int {
    get { _bar }
    set {
     guard newValue != _bar else { return }
     willChangeValue(for: \.bar)
     _bar = newValue
     didChangeValue(for: \.bar)
    }
  }

This is okay to do, so suppress the warning if we have an explicit setter on the @objc property. We could go further and check for those specific calls, but it's probably safe to assume that those calls are being made if there's an explicit setter.

  • Issue: SR-12286 | rdar://problem/59702530 | FB7595420

  • Scope: Affects use of @objc properties with manual KVO events when passed to the KVO observe method.

  • Risk: Very Low. We're just suppressing a warning diagnostic, which shouldn't appear in the above scenario.

  • Testing: Added a new test case.

  • Reviewed by: @xedin

Cherry pick of #30098

Resolves: rdar://problem/59702530

@theblixguy theblixguy requested a review from xedin February 27, 2020 19:24
@theblixguy theblixguy requested a review from a team as a code owner February 27, 2020 19:24
@xedin
Copy link
Contributor

xedin commented Feb 27, 2020

@swift-ci please test

@xedin xedin merged commit 33150e3 into swiftlang:swift-5.2-branch Feb 27, 2020
@theblixguy theblixguy deleted the fix/SR-12286_5.2 branch February 27, 2020 22:05
@AnthonyLatsis AnthonyLatsis added swift 5.2 🍒 release cherry pick Flag: Release branch cherry picks labels Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants