-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove weak_computed_property #2712
Comments
I just did this myself with the following two code snippets: Snippet 1 protocol SomeClassDelegate: AnyObject {}
class SomeClass {
private weak var _delegate: SomeClassDelegate?
var delegate: SomeClassDelegate? {
get { return _delegate }
set { _delegate = newValue }
}
} Snippet 2 protocol SomeClassDelegate: AnyObject {}
class SomeClass {
private weak var _delegate: SomeClassDelegate?
weak var delegate: SomeClassDelegate? {
get { return _delegate }
set { _delegate = newValue }
}
} And they both generated this output on x64-64: Output
|
Lol. Of course it has no affect on the generated code. (As an aside, many SwiftLint warnings have nothing to do with the generated code, so I don’t know why you’re bringing that up here.) Hey, I completely get it. It’s easy to presume that So, it’s not about the generated code, but rather it’s about reasoning about memory semantics and the compile-time checks/warnings the compiler can supply if we correctly use the For example, take your snippet 1 (without the let foo = SomeClass()
foo.delegate = DelegateObject()
... This is obviously wrong. (There’s nothing keeping a strong reference to Unfortunately, a developer looking at the But if you add Snippet 1 is misleading at best (and, IMHO, it’s just plain wrong). Snippet 2 is better, allowing both the compiler and future developers to reason about what’s going on. But this new SwiftLint warning that will actively inform a developer who correctly implemented your snippet 2 and encourage them to change it to snippet 1, which is incorrect. Bottom line, there’s a reason why the compiler permits |
I agree with @robertmryan, this rule should be removed outright. class FooView: UIView {
weak var scrollDelegate: UIScrollViewDelegate {
get { return _scrollView.delegate }
set { _scrollView.delegate = newValue }
}
…
} Without the |
Oh, I'm afraid I misread the initial description of the issue. I was under the impression that you were saying that Thanks for being patient in re-explaining this to me both of you. |
Thanks! IMHO, it’s more than just “documentation”, but rather, as Apple says, “it's best practice for the property attribute to show the resultant behavior.” |
I still don't think the rule should be removed. It should rather be opt-in. |
It should be removed because the rule is promoting an antipattern. Putting |
* Remove WeakComputedProperyRule Addresses #2712 * fixup! Remove WeakComputedProperyRule
Removed in #2761. Thanks for improving SwiftLint! |
As a result of issue #2596, the
weak_computed_property
issue was created. While we can (and I have in my installations) obviously disable it, I wonder if this rule should exist at all.Bottom line, there absolutely are situations where you actually want to use
weak
with computed properties, namely where there is aweak
stored property backing this computed property.For example, consider:
And
Then
In the absence of the the
weak
qualifier on the computed property and without diving into the implementation details, the programmer might incorrectly conclude that the above is fine. But it’s not. Because the underlying_delegate
isweak
, thisCustomDelegate
instance will be deallocated immediately, and the object will end up with no delegate object. And there’s no compiler warning about this behavior.If, however, we fix
SomeClass
like so:Then the programmer will receive a very helpful warning:
They’ll then correctly deduce that they should keep their own strong reference to this
CustomDelegate
for the code to work properly.So, bottom line, you don’t technically need the
weak
qualifier on the computed property that is backed by a privateweak
stored property, but it’s prudent to do so to avoid mysterious bugs and/or misunderstandings about the underlying semantics. I personally would consider the absence ofweak
qualifier in this scenario as a bug.It strikes me that the warning should be (a) if the getter is retrieving values that are not
weak
, then there should be an error if the computed property is markedweak
; but (b) if the getter is retrieving values that areweak
, the the converse is true, that it should be an error if the computed property is not markedweak
. And if this level of complexity is not easily captured in SwiftLint, I might suggest disabling this rule altogether or, at the very least, make this an “opt in” rule.(https://stackoverflow.com/a/55348456/1271826)
The text was updated successfully, but these errors were encountered: