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

[SR-964] Warn if a computed property setter doesn't reference the new value #43576

Open
lilyball mannequin opened this issue Mar 16, 2016 · 5 comments
Open

[SR-964] Warn if a computed property setter doesn't reference the new value #43576

lilyball mannequin opened this issue Mar 16, 2016 · 5 comments
Labels
compiler The Swift compiler in itself improvement

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Mar 16, 2016

Previous ID SR-964
Radar None
Original Reporter @lilyball
Type Improvement
Status Reopened
Resolution
Additional Detail from JIRA
Votes 2
Component/s Compiler
Labels Improvement
Assignee KingOfBrian (JIRA)
Priority Medium

md5: 63ff5a5ab70edfee28c9a22f818da6f7

is duplicated by:

  • SR-6354 Compiler should warn if a setter does not use newValue

Issue Description:

I think we should add a warning if a computed property defines a setter that doesn't reference the new value. I just discovered a bug in my app today that was cause by code that looked like

var someProp: Foo {
    get {
        var prop: Foo!
        dispatch_sync(queue, { prop = _inner.prop })
        return prop
    }
    set {
        let value = someProp.copy() as! Foo
        dispatch_barrier_async(queue, { _inner.prop = value })
    }
}

Note how the setter is accessing the someProp getter instead of the implicit newValue parameter. And this isn't the first time I've made this mistake. I think the source of the problem is that I usually write didSet observers, where you do access the getter, and so it's easy to slip into using the getter when writing a set.

I considered having a warning if a set accesses the getter, but there are legitimate reasons to want to do that. But warning if the setter never accesses the newValue parameter seems like a good approach. I'm unlikely to use both newValue and the computed getter in the same setter by accident, and if I really do want to ignore the new value I can suppress the warning with _ = newValue.

@belkadan
Copy link
Contributor

Brian fixed this a while back in #11465 which will make it into Swift 4.1!

@marcelofabri
Copy link
Contributor

Can we reopen this? I'm not getting a warning on Swift 4.2:

class Foo {
    private var _value: Int = 0
    var value: Int {
        get {
            return _value
        }
        set {
            _value = value
        }
    }
}

@belkadan
Copy link
Contributor

Huh, I'm not in master either. KingOfBrian (JIRA User), you still working on Swift these days, or should we take a look?

@swift-ci
Copy link
Collaborator

Comment by Brian (JIRA)

Ugh, sorry about this, it got lost in the shuffle. I had the fix based to the 4.1 release branch but it got closed and... life, tooling and disk space got in the way.

I just built and tested on a branch off master, and things should be good to go. Thanks for re-opening this @marcelofabri and sorry to swift users everywhere for letting it sit for so long.

#21983

@marcelofabri
Copy link
Contributor

KingOfBrian (JIRA User) Thank YOU for implementing this!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself improvement
Projects
None yet
Development

No branches or pull requests

3 participants