Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes infinite recursion when casting AnySerializable to wrong type #147

Merged
merged 3 commits into from
Sep 3, 2023

Conversation

leoMehlig
Copy link
Contributor

If a values associated type Serializable , equals the values type, toValue would infinitely call itself.

The test testWrongCast reproduces this and I fixed it by comparing the nextType with the current type.

If a values associated type `Serializable` , equals the values type, `toValue` would infinitely call itself.

The test `testWrongCast` reproduces this and I fixed it by comparing the `nextType` with the current type.
Copy link
Collaborator

@hank121314 hank121314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Many thanks for the fix!
And please also fix the comments below. Thanks 🙇 !

@@ -194,7 +194,7 @@ extension Defaults.Serializable {
return anyObject
}

guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type else {
guard let nextType = T.Serializable.self as? any Defaults.Serializable.Type, nextType != T.self else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the indention, we are using tab indention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed that. Sorry about that.

Comment on lines 470 to 474
func testWrongCast() {
let value = Defaults.AnySerializable(false)
XCTAssertEqual(value.get(Bool.self), false)
XCTAssertNil(value.get(String.self))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above comment, and please use XCTAssertFalse instead of XCTAssertEqual as linter suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially used that, but since get returns on optional, I'd have to provide a default value. That feels less clean.

@sindresorhus sindresorhus merged commit 03d5386 into sindresorhus:main Sep 3, 2023
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants