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.

Sign up for GitHub

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

Jump to bottom

Unexpected behavior when using the default implementations of Differentiable for Hashable #114

Open
yunhao opened this issue Sep 11, 2020 · 1 comment · May be fixed by #115
Open

Unexpected behavior when using the default implementations of Differentiable for Hashable #114

yunhao opened this issue Sep 11, 2020 · 1 comment · May be fixed by #115

Comments

@yunhao
Copy link

yunhao commented Sep 11, 2020

DifferenceKit provides a default implementation of Differentiable for Hashable. However, when using the default implementation, DifferenceKit always trigers a delete -> insert change instead of a update change.

Unexpected

I have a simple model that uses the default differenceIdentifier implementation.

struct ElementModel {
    // The identifier.
    var id: Int
    
    // The contents.
    var title: String
    var value: String
}

// Use the default `differenceIdentifier` implementation.
extension ElementModel: Hashable, Differentiable {
    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }
}

Create a changeset between two items that have the same id but different value.

let list1 = [ElementModel(id: 1, title: "color", value: "red")]
let list2 = [ElementModel(id: 1, title: "color", value: "blue")]

let changeset = StagedChangeset(source: list1, target: list2)
changeset.forEach { print($0) }

Unfortunately, the output message shows that there is a delete -> insert update.

Changeset(
    data: [],
    elementDeleted: [
        [element: 0, section: 0]
    ]
)
Changeset(
    data: [
        ElementModel(id: 1, title: "color", value: "blue")
    ],
    elementInserted: [
        [element: 0, section: 0]
    ]
)

Expected

If I explicitly implement the differenceIdentifier property by return hashValue, everything will be fine.

extension ElementModel: Hashable, Differentiable {
    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }
    
    var differenceIdentifier: Int {
        // Return `hashValue` insteal of `Self`
        return hashValue
    }
}

The expected update change.

Changeset(
    data: [
        ElementModel(id: 1, title: "color", value: "blue")
    ],
    elementUpdated: [
        [element: 0, section: 0]
    ]
)

Question

I'm not sure if this is a mistake of my code of a bug of DifferenceKit.

DifferenceKit provides a default implementation of Differentiable for Hashable, but the implementation of differenceIdentifier just return the Hashable instance itself. I think maybe the hashValue is the right return value.

// Return self
var differenceIdentifier: Self {
    return self
}

// Return hashValue
var differenceIdentifier: Int {
    return hashValue
}
@yunhao
Copy link
Author

yunhao commented Sep 11, 2020

After clone this project and modify the implementation to return hashValue, the unit test fails at func testSameHashValue(). I'm a little confused about this test case.

DifferenceKit/Tests/AlgorithmTest.swift

Lines 246 to 284 in f8c8d1e

func testSameHashValue() {
struct A: Hashable, Differentiable {
let actualValue: Int
init(_ actualValue: Int) {
self.actualValue = actualValue
}
func hash(into hasher: inout Hasher) {
hasher.combine(0)
}
func isContentEqual(to source: A) -> Bool {
return actualValue == source.actualValue
}
}
let section = 0
let source = [A(0), A(1)]
let target = [A(0), A(2)]
XCTAssertExactDifferences(
source: source,
target: target,
section: section,
expected: [
Changeset(
data: [A(0)],
elementDeleted: [ElementPath(element: 1, section: section)]
),
Changeset(
data: target,
elementInserted: [ElementPath(element: 1, section: section)]
)
]
)
}
}

I think it should be a update change instead of a delete -> insert change, because they have the same identifier (hasher.combine(0)) and different content.

It seems that DifferenceKit doesn't use the result of func hash(into hasher: inout Hasher). When using Hashable, we can use hasher.combine(identifier) to determine our identifier, but DifferenceKit will ignore it.

@yunhao yunhao linked a pull request Sep 11, 2020 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the default implementation of Differentiable for Hashable yunhao/DifferenceKit
1 participant
@yunhao