SwiftLint icon indicating copy to clipboard operation
SwiftLint copied to clipboard

`unused_setter_value` incorrectly gets triggered for an empty setter

Open amomchilov opened this issue 3 years ago • 12 comments

New Issue Checklist

Describe the bug

The following code gets reported as a vioation of the unused_setter_value rule, even though it was one of the examples explicitly cited as being non-offending.

override var aValue: String {
  get {
    return Persister.shared.aValue
  }
  set() { }

There might be a meta-issue here, about why the example didn't cause a test failure.

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint

Environment

  • SwiftLint version: 0.46.3

  • Installation method used: Homebrew

  • Which Xcode version are you using (check xcodebuild -version)? Xcode 13.2.1 Build version 13C100

  • Do you have a sample that shows the issue? Yes:

    echo 'private var abc: Int { get { 123 } set() { } }' | swiftlint lint --no-cache --use-stdin --enable-all-rules`
    

amomchilov avatar Feb 24 '22 00:02 amomchilov

Hey @amomchilov,

I noticed that your sample doesn't use override. These cases are still triggering as I couldn't think of an example why you would want a non-overriding variable with an empty setter. Is that what you are trying to do? The code of the documentation is still valid in my test.

grosem avatar Feb 24 '22 05:02 grosem

@grosem

I'm using it to fulfill a protocol requirement, and those aren't adorned with override.

More specifically, it's for a OutlineViewNode protocol that I have:

// Requires NSObjectProtocol for the purpose of key-value-coding based sorting
public protocol OutlineViewNode: NSObjectProtocol {
	var childNodes: [OutlineViewNode] { get set }
}

For leaf nodes, I implement childNodes as:

public var childNodes: [OutlineViewNode] {
	get { [] }
	// swiftlint:disable:next unused_setter_value
	set { }
}

Arguably, perhaps I should use fatalError instead of no-op.

I did previously have two separate protocols:

  • OutlineViewParentNode
    • requires var childNodes: [OutlineViewNode] { get set }
    • Conformed to by all non-leaf nodes, including the root
    • Not conformed to by leaf nodes
  • OutlineViewChildNode
    • requires any node specific data, like icon: NSImage, title: String, etc.
    • Conformed to by all nodes beside the root

This removed the need for these empty child node arrays. It satisfied the ISP, but in practice, it complicated things more than it helped.

amomchilov avatar Feb 26 '22 19:02 amomchilov

Hello, I am having exactly the same issue. Using 0.64.3 installed via homebrew Screen Shot 2022-03-02 at 11 21 59

TimPapler avatar Mar 02 '22 10:03 TimPapler

@amomchilov That makes sense. It's easy enough to have the same behaviour for non-override variables. If everybody agrees that having an empty setter block is good enough (as it is now for an override var), I'll be happy to make the necessary changes. Frankly speaking, I don't have the experience to judge on that.

grosem avatar Mar 02 '22 10:03 grosem

Hello, I am having exactly the same issue. Using 0.64.3 installed via homebrew Screen Shot 2022-03-02 at 11 21 59

Ok, it appears after I deleted the swiftlint disable rule I had ghost error, after a clean it worked as expected. Sorry for false report.

TimPapler avatar Mar 02 '22 14:03 TimPapler

If this is resolved, please close the ticket. If it’s still not resolved, can you please explain the issue?

jpsim avatar Mar 02 '22 16:03 jpsim

If everybody agrees that having an empty setter block is good enough (as it is now for an override var)

I'm in favor! (though I might be biased :p )

Do we have a way to tell whether or not a particular setter is part of a protocol requirement or not?

amomchilov avatar Mar 14 '22 13:03 amomchilov

There are some esoteric cases where an empty setter value is required.

An example is when implementing a @propertyWrapper with static subscript(_enclosingInstance: T, wrapped: ReferenceWritableKeyPath<T, Value>, storage: ReferenceWritableKeyPath<T, Self>) (ie. a property wrapper that receives a reference to the containing object).

In this situation, the property wrapper must implement public var wrappedValue: Value with both a getter & a setter, in order to communicate to the synthesis system that the wrapped value is read-write, but these getters and setters will never be used (instead, Swift will delegate their usage to the getter and setter of the subscript mentioned above).

The result is something like this:

@propertyWrapper
public struct PublishedComputed<Value, T: ObservableObject> {
    public static subscript(
        _enclosingInstance observed: T,
        wrapped wrappedKeyPath: ReferenceWritableKeyPath<T, Value>,
        storage storageKeyPath: ReferenceWritableKeyPath<T, Self>
    )
        -> Value {
        get {
            observed[keyPath: storageKeyPath].getStuff()
        }
        set {
            observed[keyPath: storageKeyPath].setStuff()
        }
    }

    @available(*, unavailable, message: "May only be used in object types.")
    public var wrappedValue: Value {
        get { fatalError() }
        set { fatalError() }
    }
}

SwiftLint currently triggers unused_setter_value on this.

This scenario could potentially be tested by checking for the existence of the subscript.

Note that if you do so, please also take into account the check for projectedValue:

    public var projectedValue: AnyPublisher<Value, Never> {
        get { fatalError() }
        set { fatalError() }
    }

    public private(set) static subscript(
        _enclosingInstance observed: T,
        projected wrappedKeyPath: ReferenceWritableKeyPath<T, AnyPublisher<Value, Never>>,
        storage storageKeyPath: ReferenceWritableKeyPath<T, Self>
    ) -> AnyPublisher<Value, Never> { get set }

lhunath avatar Jul 14 '22 21:07 lhunath

It's more than a year and currently using stable 0.52.2 (bottled) version. (installed via brew) Still can see the warning in my environment.

var aPointPosition: TimeInterval? {
    get { return nil }
    set { }
}

mohan-kurera avatar Jun 20 '23 01:06 mohan-kurera

It's more than a year and currently using stable 0.52.2 (bottled) version. (installed via brew) Still can see the warning in my environment.

var aPointPosition: TimeInterval? {
    get { return nil }
    set { }
}

If we want to stay with the fast rule we currently have, it's impossible to know if a property implements a protocol requirement. The override keyword is a marker on the property declaration itself. It's easy to recognize on a syntax-level.

SimplyDanny avatar Jun 21 '23 21:06 SimplyDanny