Add mutable_hash rule

[evnik]

The rule tries to catch using of mutable property as a hash value, which might violate Hashable protocol requirements and lead to unexpected behavior (an example might be found in this thread).

The next cases won't be covered by the current implementation:

• pretty common use case:

  class HashableClass {
    var mutableProperty: Int = 42
  }
  extension HashableClass: Hashable {
    func hash(into hasher: inout Hasher) {
      // hard to check mutableProperty declaration, which might be even in a different file
      hasher.combine(mutableProperty)
    }
  }
  

• unlikely use cases:

  var mutableGlobalVar: Int = 42
  class HashableClass: Hashable {
    func hash(into hasher: inout Hasher) {
      // hard to check mutableGlobalVar declaration, which might be even in a different file
      hasher.combine(mutableGlobalVar)
    }
  }
  

  class HashableClass: Hashable {
    var immutableProperty: Int {
      // hard to catch it returns a mutable value
      mutableProperty
    }
    var mutableProperty: Int = 42
    func hash(into hasher: inout Hasher) {
      hasher.combine(immutableProperty)
    }
  }
  

  class HashableClass: Hashable {
    let immutableProperty: Int = 42
    var mutableProperty: Int = 42
    func hash(into hasher: inout Hasher) {
      let immutableProperty = self.mutableProperty / 2
      hasher.combine(immutableProperty)         // violation is not caught
      let mutableProperty = self.immutableProperty / 2
      hasher.combine(mutableProperty)         // false violation
    }
  }
  

[SwiftLintBot]

	12 Messages
:book:	Linting Aerial with this PR took 0.74s vs 0.72s on master (2% slower)
:book:	Linting Alamofire with this PR took 0.92s vs 0.92s on master (0% slower)
:book:	Linting Firefox with this PR took 3.34s vs 3.32s on master (0% slower)
:book:	Linting Kickstarter with this PR took 5.67s vs 5.66s on master (0% slower)
:book:	Linting Moya with this PR took 0.49s vs 0.48s on master (2% slower)
:book:	Linting Nimble with this PR took 0.41s vs 0.4s on master (2% slower)
:book:	Linting Quick with this PR took 0.19s vs 0.19s on master (0% slower)
:book:	Linting Realm with this PR took 1.96s vs 1.95s on master (0% slower)
:book:	Linting SourceKitten with this PR took 0.33s vs 0.33s on master (0% slower)
:book:	Linting Sourcery with this PR took 1.8s vs 1.8s on master (0% slower)
:book:	Linting Swift with this PR took 2.93s vs 2.93s on master (0% slower)
:book:	Linting WordPress with this PR took 6.33s vs 6.32s on master (0% slower)

Generated by :no_entry_sign: Danger

[jpsim]

I understand the basic premise of what you want this rule to do, but your examples confuse me, and I'm not clear that the Configuration source files you're changing were actually problematic to start off with.

Focusing in on the examples, you have structs with weak variables, which is illegal in Swift, so I don't know what you're trying to test.

[evnik]

I'm not clear that the Configuration source files you're changing were actually problematic to start off with.

The Vertix class was using mutable filePath to calculate hash value, so I've updated it to use immutable properties only. The originalRemoteString property was updated to always contain information about the file path.

Focusing in on the examples, you have structs with weak variables, which is illegal in Swift, so I don't know what you're trying to test.

Most of the examples are classes. Structs with weak variables are legal. Those particular examples might be not compilable, however they are sufficient to test the rule. Here is the full compilable example of that kind of struct that leads to undefined behavior:

import Foundation

struct WeakWrapper: Hashable {
    private(set) weak var value: AnyObject?

    init(_ value: AnyObject) {
        self.value = value
    }

    static func == (lhs: WeakWrapper, rhs: WeakWrapper) -> Bool {
        lhs.value === rhs.value
    }

    func hash(into hasher: inout Hasher) {
        value?.pointerValue?.hash(into: &hasher)
    }
}

class Test { }

func createSet(count: Int) -> Set<WeakWrapper> {
    var result = Set<WeakWrapper>()
    let values = (0..<count).map { _ in Test() }
    values.forEach { result.insert(WeakWrapper($0)) }
    return result
}

var set = createSet(count: 3)
let hasNilValues = set.contains { $0.value == nil }
if hasNilValues {
    print("Found nil values")
    let v4 = Test()
    set.insert(WeakWrapper(v4))
}

[jpsim]

Thanks for the additional details. I understand this a bit better now. There are more issues than simply breaking Hashable semantics in your latest sample, such as also breaking the assumption of structs with value semantics.

The Vertix situation was safe beforehand because it respects value semantics.

For example, if I mutate a member of a set after it's been inserted, it doesn't affect the copy in the set itself:

struct IntWrapper: Hashable {
    var value = 0

    init(_ value: Int) {
        self.value = value
    }

    static func == (lhs: IntWrapper, rhs: IntWrapper) -> Bool {
        lhs.value == rhs.value
    }

    func hash(into hasher: inout Hasher) {
        value.hash(into: &hasher)
    }
}

var firstItem = IntWrapper(0)

var result: Set<IntWrapper> = [
  firstItem,
  .init(1),
]

firstItem.value = 2

print(result)

result.insert(firstItem)

print(result)

Running that prints:

[main.IntWrapper(value: 0), main.IntWrapper(value: 1)]
[main.IntWrapper(value: 0), main.IntWrapper(value: 1), main.IntWrapper(value: 2)]

---

For this reason, I'm not supportive of this rule overall. I think if you wanted to add a rule that could catch patterns of structs not conforming to value semantics (fi that's even possible based on parsing the syntax tree alone in some subset of situations) that might better catch what you're trying to prevent here.