Skip to content
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

Closed
robertmryan opened this issue Apr 10, 2019 · 8 comments
Closed

Remove weak_computed_property #2712

robertmryan opened this issue Apr 10, 2019 · 8 comments

Comments

@robertmryan
Copy link

robertmryan commented Apr 10, 2019

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 a weak stored property backing this computed property.

For example, consider:

class SomeClass {
    private weak var _delegate: SomeClassDelegate?
    var delegate: SomeClassDelegate? {             // whoops; this should be `weak`
        get { return _delegate }
        set { _delegate = newValue }
    }
}

And

class CustomDelegate: SomeClassDelegate { ... }

Then

let object = SomeClass()
object.delegate = CustomDelegate()

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 is weak, this CustomDelegate 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:

class SomeClass {
    private weak var _delegate: SomeClassDelegate?
    weak var delegate: SomeClassDelegate? {        // great; matches underlying semantics
        get { return _delegate }
        set { _delegate = newValue }
    }
}

Then the programmer will receive a very helpful warning:

let object = SomeClass()
object.delegate = CustomDelegate()  // results in "Instance will be immediately deallocated because property 'delegate' is 'weak’"

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 private weak 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 of weak 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 marked weak; but (b) if the getter is retrieving values that are weak, the the converse is true, that it should be an error if the computed property is not marked weak. 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)

@jpsim
Copy link
Collaborator

jpsim commented Apr 29, 2019

weak has no effect on computed properties whatsoever. I encourage you to paste the code you shared in https://godbolt.org/ to compare the generated machine code both without and with the weak keyword applied to the computed property. The resulting machine code will be identical.

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
main:
        push    rbp
        mov     rbp, rsp
        xor     eax, eax
        mov     dword ptr [rbp - 4], edi
        mov     qword ptr [rbp - 16], rsi
        pop     rbp
        ret

variable initialization expression of output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9) : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        xor     eax, eax
        mov     ecx, eax
        mov     rax, rcx
        mov     rdx, rcx
        pop     rbp
        ret

output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).getter : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 64
        mov     rax, r13
        add     rax, 16
        mov     ecx, 32
        mov     edx, ecx
        xor     ecx, ecx
        lea     rsi, [rbp - 24]
        mov     rdi, rax
        mov     qword ptr [rbp - 32], rsi
        mov     qword ptr [rbp - 40], r13
        mov     qword ptr [rbp - 48], rax
        call    swift_beginAccess@PLT
        mov     rdi, qword ptr [rbp - 48]
        call    swift_weakLoadStrong@PLT
        mov     rcx, qword ptr [rbp - 40]
        mov     rdx, qword ptr [rcx + 24]
        mov     rdi, qword ptr [rbp - 32]
        mov     qword ptr [rbp - 56], rax
        mov     qword ptr [rbp - 64], rdx
        call    swift_endAccess@PLT
        mov     rax, qword ptr [rbp - 56]
        mov     rdx, qword ptr [rbp - 64]
        add     rsp, 64
        pop     rbp
        ret

output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).setter : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 80
        mov     rax, r13
        add     rax, 16
        mov     ecx, 33
        mov     edx, ecx
        xor     ecx, ecx
        lea     r8, [rbp - 24]
        mov     qword ptr [rbp - 32], rdi
        mov     rdi, rax
        mov     qword ptr [rbp - 40], rsi
        mov     rsi, r8
        mov     qword ptr [rbp - 48], rax
        mov     qword ptr [rbp - 56], r13
        mov     qword ptr [rbp - 64], r8
        call    swift_beginAccess@PLT
        mov     rax, qword ptr [rbp - 56]
        mov     rcx, qword ptr [rbp - 40]
        mov     qword ptr [rax + 24], rcx
        mov     rdi, qword ptr [rbp - 48]
        mov     rsi, qword ptr [rbp - 32]
        call    swift_weakAssign@PLT
        mov     rdi, qword ptr [rbp - 64]
        mov     qword ptr [rbp - 72], rax
        call    swift_endAccess@PLT
        mov     rdi, qword ptr [rbp - 32]
        mov     rsi, qword ptr [rbp - 40]
        call    (outlined consume of output.SomeClassDelegate?)
        add     rsp, 80
        pop     rbp
        ret

output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).modify : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        push    r13
        sub     rsp, 56
        mov     eax, 48
        mov     ecx, eax
        mov     qword ptr [rbp - 16], rdi
        mov     rdi, rcx
        mov     qword ptr [rbp - 24], r13
        call    malloc@PLT
        mov     rcx, qword ptr [rbp - 16]
        mov     qword ptr [rcx], rax
        mov     rdi, rax
        add     rdi, 8
        mov     r13, rax
        add     r13, 32
        mov     rdx, qword ptr [rbp - 24]
        mov     qword ptr [rax], rdx
        add     rdx, 16
        mov     esi, 33
        mov     r8d, esi
        xor     esi, esi
        mov     ecx, esi
        mov     qword ptr [rbp - 32], rdi
        mov     rdi, rdx
        mov     rsi, qword ptr [rbp - 32]
        mov     qword ptr [rbp - 40], rdx
        mov     rdx, r8
        mov     qword ptr [rbp - 48], rax
        mov     qword ptr [rbp - 56], r13
        call    swift_beginAccess@PLT
        mov     rdi, qword ptr [rbp - 40]
        call    swift_weakLoadStrong@PLT
        mov     rcx, qword ptr [rbp - 24]
        mov     rdx, qword ptr [rcx + 24]
        mov     rsi, qword ptr [rbp - 48]
        mov     qword ptr [rsi + 32], rax
        mov     qword ptr [rsi + 40], rdx
        lea     rax, [rip + (output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).modify : output.SomeClassDelegate? with unmangled suffix ".resume.0")]
        mov     rdx, qword ptr [rbp - 56]
        add     rsp, 56
        pop     r13
        pop     rbp
        ret

output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).modify : output.SomeClassDelegate? with unmangled suffix ".resume.0":
        push    rbp
        mov     rbp, rsp
        push    rbx
        sub     rsp, 72
        mov     al, sil
        mov     rdi, qword ptr [rdi]
        mov     rcx, rdi
        add     rcx, 32
        mov     rdx, rdi
        add     rdx, 8
        mov     r8, qword ptr [rdi]
        mov     r9, r8
        add     r9, 16
        mov     r10, qword ptr [rdi + 32]
        mov     r11, qword ptr [rdi + 40]
        mov     qword ptr [r8 + 24], r11
        mov     qword ptr [rbp - 16], rdi
        mov     rdi, r9
        mov     rsi, r10
        mov     byte ptr [rbp - 17], al
        mov     qword ptr [rbp - 32], r10
        mov     qword ptr [rbp - 40], rcx
        mov     qword ptr [rbp - 48], rdx
        mov     qword ptr [rbp - 56], r11
        call    swift_weakAssign@PLT
        mov     bl, byte ptr [rbp - 17]
        test    bl, 1
        mov     qword ptr [rbp - 64], rax
        jne     .LBB5_2
        mov     rdi, qword ptr [rbp - 32]
        mov     rsi, qword ptr [rbp - 56]
        call    (outlined consume of output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 48]
        call    swift_endAccess@PLT
        jmp     .LBB5_3
.LBB5_2:
        mov     rdi, qword ptr [rbp - 40]
        call    (outlined destroy of output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 48]
        mov     qword ptr [rbp - 72], rax
        call    swift_endAccess@PLT
.LBB5_3:
        mov     rdi, qword ptr [rbp - 16]
        call    free@PLT
        add     rsp, 72
        pop     rbx
        pop     rbp
        ret

output.SomeClass.delegate.getter : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 80
        mov     qword ptr [rbp - 8], 0
        mov     qword ptr [rbp - 8], r13
        mov     rax, r13
        add     rax, 16
        mov     ecx, 32
        mov     edx, ecx
        xor     ecx, ecx
        lea     rsi, [rbp - 32]
        mov     rdi, rax
        mov     qword ptr [rbp - 40], rsi
        mov     qword ptr [rbp - 48], r13
        mov     qword ptr [rbp - 56], rax
        call    swift_beginAccess@PLT
        mov     rdi, qword ptr [rbp - 56]
        call    swift_weakLoadStrong@PLT
        mov     rcx, qword ptr [rbp - 48]
        mov     rdx, qword ptr [rcx + 24]
        mov     rdi, qword ptr [rbp - 40]
        mov     qword ptr [rbp - 64], rax
        mov     qword ptr [rbp - 72], rdx
        call    swift_endAccess@PLT
        mov     rax, qword ptr [rbp - 64]
        mov     rdx, qword ptr [rbp - 72]
        add     rsp, 80
        pop     rbp
        ret

output.SomeClass.delegate.setter : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 96
        xorps   xmm0, xmm0
        movaps  xmmword ptr [rbp - 16], xmm0
        mov     qword ptr [rbp - 24], 0
        mov     qword ptr [rbp - 16], rdi
        mov     qword ptr [rbp - 8], rsi
        mov     qword ptr [rbp - 24], r13
        mov     rax, r13
        add     rax, 16
        mov     ecx, 33
        mov     edx, ecx
        xor     ecx, ecx
        lea     r8, [rbp - 48]
        mov     qword ptr [rbp - 56], rdi
        mov     rdi, rax
        mov     qword ptr [rbp - 64], rsi
        mov     rsi, r8
        mov     qword ptr [rbp - 72], rax
        mov     qword ptr [rbp - 80], r13
        mov     qword ptr [rbp - 88], r8
        call    swift_beginAccess@PLT
        mov     rax, qword ptr [rbp - 80]
        mov     rcx, qword ptr [rbp - 64]
        mov     qword ptr [rax + 24], rcx
        mov     rdi, qword ptr [rbp - 72]
        mov     rsi, qword ptr [rbp - 56]
        call    swift_weakAssign@PLT
        mov     rdi, qword ptr [rbp - 88]
        mov     qword ptr [rbp - 96], rax
        call    swift_endAccess@PLT
        mov     rdi, qword ptr [rbp - 56]
        mov     rsi, qword ptr [rbp - 64]
        call    (outlined consume of output.SomeClassDelegate?)
        add     rsp, 96
        pop     rbp
        ret

outlined consume of output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 16
        cmp     rdi, 0
        mov     qword ptr [rbp - 8], rdi
        mov     qword ptr [rbp - 16], rsi
        je      .LBB8_2
        mov     rdi, qword ptr [rbp - 8]
        call    swift_release@PLT
.LBB8_2:
        add     rsp, 16
        pop     rbp
        ret

output.SomeClass.delegate.modify : output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 16
        mov     rax, rdi
        add     rax, 8
        mov     qword ptr [rdi], r13
        mov     qword ptr [rbp - 8], rax
        mov     qword ptr [rbp - 16], rdi
        call    (output.SomeClass.delegate.getter : output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 16]
        mov     qword ptr [rdi + 8], rax
        mov     qword ptr [rdi + 16], rdx
        lea     rax, [rip + (output.SomeClass.delegate.modify : output.SomeClassDelegate? with unmangled suffix ".resume.0")]
        mov     rdx, qword ptr [rbp - 8]
        add     rsp, 16
        pop     rbp
        ret

output.SomeClass.delegate.modify : output.SomeClassDelegate? with unmangled suffix ".resume.0":
        push    rbp
        mov     rbp, rsp
        push    r13
        sub     rsp, 40
        mov     al, sil
        mov     rcx, rdi
        add     rcx, 8
        mov     rdx, qword ptr [rdi]
        mov     r8, qword ptr [rdi + 8]
        mov     rdi, qword ptr [rdi + 16]
        test    al, 1
        mov     qword ptr [rbp - 16], rdi
        mov     qword ptr [rbp - 24], r8
        mov     qword ptr [rbp - 32], rcx
        mov     qword ptr [rbp - 40], rdx
        jne     .LBB10_2
        mov     rdi, qword ptr [rbp - 24]
        mov     rsi, qword ptr [rbp - 16]
        mov     r13, qword ptr [rbp - 40]
        call    (output.SomeClass.delegate.setter : output.SomeClassDelegate?)
        jmp     .LBB10_3
.LBB10_2:
        mov     rdi, qword ptr [rbp - 24]
        mov     rsi, qword ptr [rbp - 16]
        call    (outlined copy of output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 24]
        mov     rsi, qword ptr [rbp - 16]
        mov     r13, qword ptr [rbp - 40]
        call    (output.SomeClass.delegate.setter : output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 32]
        call    (outlined destroy of output.SomeClassDelegate?)
        mov     qword ptr [rbp - 48], rax
.LBB10_3:
        add     rsp, 40
        pop     r13
        pop     rbp
        ret

outlined copy of output.SomeClassDelegate?:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 32
        cmp     rdi, 0
        mov     qword ptr [rbp - 8], rdi
        mov     qword ptr [rbp - 16], rsi
        je      .LBB11_2
        mov     rdi, qword ptr [rbp - 8]
        call    swift_retain@PLT
        mov     qword ptr [rbp - 24], rax
.LBB11_2:
        add     rsp, 32
        pop     rbp
        ret

outlined destroy of output.SomeClassDelegate?:
        push    rax
        cmp     qword ptr [rdi], 0
        mov     qword ptr [rsp], rdi
        je      .LBB12_2
        mov     rax, qword ptr [rsp]
        mov     rdi, qword ptr [rax]
        call    swift_release@PLT
.LBB12_2:
        mov     rax, qword ptr [rsp]
        pop     rcx
        ret

output.SomeClass.__allocating_init() -> output.SomeClass:
        push    rbp
        mov     rbp, rsp
        push    r13
        sub     rsp, 24
        xor     eax, eax
        mov     edi, eax
        call    (type metadata accessor for output.SomeClass)
        mov     ecx, 32
        mov     esi, ecx
        mov     ecx, 7
        mov     edi, ecx
        mov     qword ptr [rbp - 16], rdi
        mov     rdi, rax
        mov     rax, qword ptr [rbp - 16]
        mov     qword ptr [rbp - 24], rdx
        mov     rdx, rax
        call    swift_allocObject@PLT
        mov     r13, rax
        call    (output.SomeClass.init() -> output.SomeClass)
        add     rsp, 24
        pop     r13
        pop     rbp
        ret

type metadata accessor for output.SomeClass:
        push    rbp
        mov     rbp, rsp
        mov     rax, qword ptr [rip + ($s6output9SomeClassCML)]
        cmp     rax, 0
        mov     qword ptr [rbp - 8], rax
        jne     .LBB14_2
        lea     rax, [rip + (full type metadata for output.SomeClass)]
        add     rax, 16
        lea     rcx, [rip + (full type metadata for output.SomeClass)+16]
        mov     qword ptr [rip + ($s6output9SomeClassCML)], rcx
        mov     qword ptr [rbp - 8], rax
.LBB14_2:
        mov     rax, qword ptr [rbp - 8]
        xor     ecx, ecx
        mov     edx, ecx
        pop     rbp
        ret

output.SomeClass.init() -> output.SomeClass:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 80
        mov     qword ptr [rbp - 8], 0
        mov     qword ptr [rbp - 8], r13
        mov     rax, r13
        add     rax, 16
        mov     ecx, 33
        mov     edx, ecx
        xor     ecx, ecx
        mov     esi, ecx
        lea     rdi, [rbp - 32]
        mov     qword ptr [rbp - 40], rdi
        mov     rdi, rax
        mov     r8, qword ptr [rbp - 40]
        mov     qword ptr [rbp - 48], rsi
        mov     rsi, r8
        mov     rcx, qword ptr [rbp - 48]
        mov     qword ptr [rbp - 56], r13
        mov     qword ptr [rbp - 64], rax
        call    swift_beginAccess@PLT
        mov     rax, qword ptr [rbp - 56]
        mov     qword ptr [rax + 24], 0
        mov     rdi, qword ptr [rbp - 64]
        mov     rsi, qword ptr [rbp - 48]
        call    swift_weakInit@PLT
        mov     rdi, qword ptr [rbp - 48]
        mov     rsi, qword ptr [rbp - 48]
        mov     qword ptr [rbp - 72], rax
        call    (outlined consume of output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 40]
        call    swift_endAccess@PLT
        mov     rax, qword ptr [rbp - 56]
        add     rsp, 80
        pop     rbp
        ret

output.SomeClass.deinit:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 32
        mov     qword ptr [rbp - 8], 0
        mov     qword ptr [rbp - 8], r13
        mov     rax, r13
        add     rax, 16
        mov     rdi, rax
        mov     qword ptr [rbp - 16], r13
        call    (outlined destroy of weak output.SomeClassDelegate?)
        mov     rdi, qword ptr [rbp - 16]
        mov     qword ptr [rbp - 24], rax
        mov     rax, rdi
        add     rsp, 32
        pop     rbp
        ret

outlined destroy of weak output.SomeClassDelegate?:
        push    rax
        mov     qword ptr [rsp], rdi
        call    swift_weakDestroy@PLT
        mov     rax, qword ptr [rsp]
        pop     rcx
        ret

output.SomeClass.__deallocating_deinit:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 16
        mov     qword ptr [rbp - 8], 0
        mov     qword ptr [rbp - 8], r13
        call    (output.SomeClass.deinit)
        mov     ecx, 32
        mov     esi, ecx
        mov     ecx, 7
        mov     edx, ecx
        mov     rdi, rax
        call    swift_deallocClassInstance@PLT
        add     rsp, 16
        pop     rbp
        ret

.Lcoro.devirt.trigger:
        mov     qword ptr [rsp - 8], rdi
        ret

"symbolic $s6output17SomeClassDelegateP":

.L__unnamed_1:
        .asciz  "output"

module descriptor output:
        .long   0
        .long   0
        .long   (.L__unnamed_1-(module descriptor output))-8

.L__unnamed_2:
        .asciz  "SomeClassDelegate"

"symbolic x":

protocol descriptor for output.SomeClassDelegate:
        .long   67
        .long   ((module descriptor output)-(protocol descriptor for output.SomeClassDelegate))-4
        .long   (.L__unnamed_2-(protocol descriptor for output.SomeClassDelegate))-8
        .long   1
        .long   0
        .long   0
        .long   31
        .long   ("symbolic x"-(protocol descriptor for output.SomeClassDelegate))-28
        .long   0

        .quad   16

.L__unnamed_3:
        .asciz  "SomeClass"

nominal type descriptor for output.SomeClass:
        .long   2147483728
        .long   ((module descriptor output)-(nominal type descriptor for output.SomeClass))-4
        .long   (.L__unnamed_3-(nominal type descriptor for output.SomeClass))-8
        .long   ((type metadata accessor for output.SomeClass)-(nominal type descriptor for output.SomeClass))-12
        .long   ((reflection metadata field descriptor output.SomeClass)-(nominal type descriptor for output.SomeClass))-16
        .long   0
        .long   2
        .long   18
        .long   8
        .long   1
        .long   10
        .long   11
        .long   7
        .long   18
        .long   ((output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).getter : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-56
        .long   19
        .long   ((output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).setter : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-64
        .long   20
        .long   ((output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).modify : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-72
        .long   18
        .long   ((output.SomeClass.delegate.getter : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-80
        .long   19
        .long   ((output.SomeClass.delegate.setter : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-88
        .long   20
        .long   ((output.SomeClass.delegate.modify : output.SomeClassDelegate?)-(nominal type descriptor for output.SomeClass))-96
        .long   1
        .long   ((output.SomeClass.__allocating_init() -> output.SomeClass)-(nominal type descriptor for output.SomeClass))-104

full type metadata for output.SomeClass:
        .quad   (output.SomeClass.__deallocating_deinit)
        .quad   ($sBoWV)
        .quad   0
        .quad   0
        .quad   0
        .quad   0
        .quad   1
        .long   3
        .long   0
        .long   32
        .short  7
        .short  0
        .long   160
        .long   16
        .quad   (nominal type descriptor for output.SomeClass)
        .quad   0
        .quad   16
        .quad   (output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).getter : output.SomeClassDelegate?)
        .quad   (output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).setter : output.SomeClassDelegate?)
        .quad   (output.SomeClass.(_delegate in _60494E8B9C642A7C4A26F3A3B6CECEB9).modify : output.SomeClassDelegate?)
        .quad   (output.SomeClass.delegate.getter : output.SomeClassDelegate?)
        .quad   (output.SomeClass.delegate.setter : output.SomeClassDelegate?)
        .quad   (output.SomeClass.delegate.modify : output.SomeClassDelegate?)
        .quad   (output.SomeClass.__allocating_init() -> output.SomeClass)

"symbolic _____ 6output9SomeClassC":

"symbolic ______pSgXw 6output17SomeClassDelegateP":

.L__unnamed_4:
        .asciz  "_delegate"

reflection metadata field descriptor output.SomeClass:
        .long   "symbolic _____ 6output9SomeClassC"-(reflection metadata field descriptor output.SomeClass)
        .long   0
        .short  1
        .short  12
        .long   1
        .long   2
        .long   ("symbolic ______pSgXw 6output17SomeClassDelegateP"-(reflection metadata field descriptor output.SomeClass))-20
        .long   (.L__unnamed_4-(reflection metadata field descriptor output.SomeClass))-24

@jpsim jpsim closed this as completed Apr 29, 2019
@robertmryan
Copy link
Author

robertmryan commented Apr 29, 2019

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 weak simply isn’t inapplicable to computed properties. But if you really delve into the example you supplied above, you’ll see that it’s a mistake to omit the weak qualifier for the computed property when the backing property is weak. A computed property should always reflect the actual underlying memory semantics. And the issue actually isn’t with the computed property, itself, but rather how one subsequently uses it.

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 weak qualifier on computed properties.

For example, take your snippet 1 (without the weak qualifier on the exposed delegate property) and then actually use that class like so:

let foo = SomeClass()
foo.delegate = DelegateObject()
...

This is obviously wrong. (There’s nothing keeping a strong reference to DelegateObject here, and it will be immediately deallocated.)

Unfortunately, a developer looking at the delegate interface has no way of reasoning about why the above is wrong. It looks like a strong property, so why wouldn’t they think the above is fine? You only can see what the problem is if you dive into the SomeClass implementation. And worse, the compiler can’t warn them about this misuse, either. It will compile without warning and the program will fail to call any of the delegate methods.

But if you add weak qualifier to the computed property so it reflects the memory semantics of its backing property, like you have in snippet 2, the programmer can now easily reason about what’s going on. And even better, with the weak qualifier on the computed property, the Swift compiler will automatically warn future developers if they misuse it like I have (deliberately) here:

Screen Shot 2019-04-29 at 10 00 04 AM

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 weak computed properties. I would suggest removing this new SwiftLint warning, or, at best, it should be an opt-in.

@lilyball
Copy link

I agree with @robertmryan, this rule should be removed outright. weak may not have any effect on the generated code of computed properties, but it's absolutely crucial for documentation purposes. Here's a trivial example:

class FooView: UIView {
    weak var scrollDelegate: UIScrollViewDelegate {
        get { return _scrollView.delegate }
        set { _scrollView.delegate = newValue }
    }}

Without the weak here, this looks to the caller as though it's a strong reference. In fact, SwiftLint already has another lint to make sure delegate properties are weak; I haven't tested but I would expect it to trigger on this property if the weak is removed.

@jpsim
Copy link
Collaborator

jpsim commented Apr 29, 2019

Oh, I'm afraid I misread the initial description of the issue. I was under the impression that you were saying that weak on computed properties impacted the correctness of the generated code. Instead you're effectively saying that it acts as compile-time enforced documentation. I ultimately agree with your assessment, and apologize for misunderstanding the issue at hand here.

Thanks for being patient in re-explaining this to me both of you.

@jpsim jpsim reopened this Apr 29, 2019
@robertmryan
Copy link
Author

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.”

@sindresorhus
Copy link

I still don't think the rule should be removed. It should rather be opt-in.

@lilyball
Copy link

lilyball commented May 9, 2019

It should be removed because the rule is promoting an antipattern. Putting weak on a computed property just means the programmer is responsible for ensuring it has weak semantics rather than the compiler, but it is in no way incorrect.

jpsim added a commit that referenced this issue May 16, 2019
jpsim added a commit that referenced this issue May 16, 2019
* Remove WeakComputedProperyRule

Addresses #2712

* fixup! Remove WeakComputedProperyRule
@jpsim
Copy link
Collaborator

jpsim commented May 16, 2019

Removed in #2761. Thanks for improving SwiftLint!

@jpsim jpsim closed this as completed May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants