Don't decode explicit strings as bools, regardless of content #440
Conversation
|
The Windows CI fail doesn't seem to be caused by this PR, I don't think? |
|
Thank you for the PR! This change looks great! 😊 I'd like to request one small addition to the test you've added. See review comments. 🙂 I'm unsure what's causing the windows failure. It looks like VSCode isn't setup properly. Will spend some time Investigating. |
| XCTAssertEqual(desired.explicitStringNotBool, "true") | ||
| XCTAssertEqual(desired.singleQuotedStringNotBool, "true") | ||
| XCTAssertEqual(desired.doubleQuotedStringNotBool, "true") | ||
| } |
There was a problem hiding this comment.
This is a great test, thank you.
I think your point/intention would be made even more clearly and strongly if we add a call to _testRoundTrip here.
Particularly, I think it would be helpful to show that the decoded form has XCTAssertNotEqual(desired.doubleQuotedStringNotBool, .bool(true))
There was a problem hiding this comment.
I tried _testRoundTrip, but the out YAML doesn't include the three different ways of encoding the string, so it failed.
I did update the equality assertions though - I agree, it's much clearer when not using ExpressibleBy... protocols: 3b15363
There was a problem hiding this comment.
I see your point. We'd have to introduce a funny type that conforms to YamlTagProviding and RawRepresentable and basically it's weird.
I've approved. ✅😁
The windows failures are not due to this change. I re-ran the checks on another branch and they are failing the same way. I'm not sure what the solution is right now.
|
Thanks @lynchsft, I'll let you merge when you see fit. |
|
@jpsim Do you understand the CI failure on Windows? The matrix runs in parallel, I've seen at lease three different swift versions fail the same way so I don't think it's related to any swift version. |
Summary
A scalar with the text
true, for example, can be decoded as a Bool today - that's expected.However, when using a YAMLDecoder, the scalars
!!str true,"true", and'true'also successfully decode as Bools - that's unexpected, especially with Decodable implementations often trying multiple different types to decode as, and use the first one that works.The suggested change checks the scalar's tag property, and if it's explicitly marked as a string, doesn't allow creating a Bool value from it.
This fixes AnyCodable-style decoding of YAML values, which in turn affects correctness of round-tripping YAML through Yams and back into YAML.
Test Plan
Added unit tests, verified they failed before this change and pass with the change.