Skip to content

Better debugDescription for EncodingError and DecodingError #80941

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ZevEisenberg
Copy link
Contributor

@ZevEisenberg ZevEisenberg commented Apr 21, 2025

Now open as an SE proposal draft: swiftlang/swift-evolution#2843

Context

Re: Swift forum post, where a discussion about future serialization tools in Swift prompted Kevin Perry to suggest that some proposed changes could actually be made in today's stdlib.

Summary of Changes

Conforms EncodingError and DecodingError to CustomDebugStringConvertible and adds a more-readable debugDescription.

Future Directions

This is a pared-down version of some experiments I did in UsefulDecode. The changes in this PR are the best I could do without changing the public interface of DecodingError and EncodingError, and without modifying the way the JSON/PropertyList Encoder/Decoder in Foundation generate their errors' debug descriptions.

In the above-linked UsefulDecode repo, when JSON decoding fails, I go back and re-decode the JSON using JSONSerialization in order to provide more context about what failed, and why. I didn't attempt to make such a change here, but I'd like to discuss what may be possible.

Examples

To illustrate the effect of the changes in this PR, I removed my changes to stdlib/public/core/Codable.swift and ran my new test cases again. Here are the resulting diffs.

test_encodingError_invalidValue_nonEmptyCodingPath_nilUnderlyingError

+Invalid value: 234 (Int)
+You cannot do that!
+Path: first/second/[2]

test_decodingError_valueNotFound_nilUnderlyingError

-valueNotFound(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "firstName", intValue: nil)], debugDescription: "Description for debugging purposes", underlyingError: nil))
+Expected value of type String but found null instead.
+Debug description: Description for debugging purposes
+Path: [0]/firstName

test_decodingError_keyNotFound_nonNilUnderlyingError

-keyNotFound(GenericCodingKey(stringValue: "name", intValue: nil), Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil)], debugDescription: "Just some info to help you out", underlyingError: Optional(main.GenericError(name: "hey, who turned out the lights?"))))
+Key \'name\' not found in keyed decoding container.
+Debug description: Just some info to help you out
+Underlying error: GenericError(name: "hey, who turned out the lights?")
+Path: [0]/address/city

test_decodingError_typeMismatch_nilUnderlyingError

-typeMismatch(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil), GenericCodingKey(stringValue: "birds", intValue: nil), GenericCodingKey(stringValue: "1", intValue: 1), GenericCodingKey(stringValue: "name", intValue: nil)], debugDescription: "This is where the debug description goes", underlyingError: nil))
+Type mismatch: expected value of type String.
+Debug description: This is where the debug description goes
+Path: [0]/address/city/birds/[1]/name

test_decodingError_dataCorrupted_nonEmptyCodingPath

-dataCorrupted(Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "first", intValue: nil), GenericCodingKey(stringValue: "second", intValue: nil), GenericCodingKey(stringValue: "2", intValue: 2)], debugDescription: "There was apparently some data corruption!", underlyingError: Optional(main.GenericError(name: "This data corruption is getting out of hand"))))
+Data was corrupted.
+Debug description: There was apparently some data corruption!
+Underlying error: GenericError(name: "This data corruption is getting out of hand")
+Path: first/second/[2]

test_decodingError_valueNotFound_nonNilUnderlyingError

-valueNotFound(Swift.Int, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "population", intValue: nil)], debugDescription: "Here is the debug description for value-not-found", underlyingError: Optional(main.GenericError(name: "these aren\'t the droids you\'re looking for"))))
+Expected value of type Int but found null instead.
+Debug description: Here is the debug description for value-not-found
+Underlying error: GenericError(name: "these aren\'t the droids you\'re looking for")
+Path: [0]/population

test_encodingError_invalidValue_emptyCodingPath_nonNilUnderlyingError

-invalidValue(345, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: Optional(main.GenericError(name: "You really cannot do that"))))
+Invalid value: 345 (Int)
+You cannot do that!
+Underlying error: GenericError(name: "You really cannot do that")

test_decodingError_typeMismatch_nonNilUnderlyingError

-typeMismatch(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "1", intValue: 1), GenericCodingKey(stringValue: "street", intValue: nil)], debugDescription: "Some debug description", underlyingError: Optional(main.GenericError(name: "some generic error goes here"))))
+Type mismatch: expected value of type String.
+Debug description: Some debug description
+Underlying error: GenericError(name: "some generic error goes here")
+Path: [0]/address/[1]/street

test_encodingError_invalidValue_emptyCodingPath_nilUnderlyingError

-invalidValue(123, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: nil))
+Invalid value: 123 (Int)
+You cannot do that!

test_decodingError_keyNotFound_nilUnderlyingError

-keyNotFound(GenericCodingKey(stringValue: "name", intValue: nil), Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil)], debugDescription: "How would you describe your relationship with your debugger?", underlyingError: nil))
+Key \'name\' not found in keyed decoding container.
+Debug description: How would you describe your relationship with your debugger?
+Path: [0]/address/city

test_decodingError_dataCorrupted_emptyCodingPath

-dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "The given data was not valid JSON", underlyingError: Optional(main.GenericError(name: "just some data corruption"))))
+Data was corrupted.
+Debug description: The given data was not valid JSON
+Underlying error: GenericError(name: "just some data corruption")

test_encodingError_invalidValue_nonEmptyCodingPath_nonNilUnderlyingError

-invalidValue(456, Swift.EncodingError.Context(codingPath: [GenericCodingKey(stringValue: "first", intValue: nil), GenericCodingKey(stringValue: "second", intValue: nil), GenericCodingKey(stringValue: "2", intValue: 2)], debugDescription: "You cannot do that!", underlyingError: Optional(main.GenericError(name: "You really cannot do that"))))
+Invalid value: 456 (Int)
+You cannot do that!
+Underlying error: GenericError(name: "You really cannot do that")
+Path: first/second/[2]

@ZevEisenberg ZevEisenberg requested a review from a team as a code owner April 21, 2025 03:44
@ZevEisenberg ZevEisenberg force-pushed the main branch 2 times, most recently from 4cd346f to 1ae5683 Compare April 21, 2025 15:53
@stephentyrone
Copy link
Contributor

CC @kperryua

@ZevEisenberg ZevEisenberg changed the title Better description for DecodingError Better description for DecodingError and CodingKey Apr 21, 2025
Copy link
Contributor

@kperryua kperryua left a comment

Choose a reason for hiding this comment

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

Thank you for this awesome contribution. The coding path description look so much more readable than present. A fair number of comments and suggestions, but hopefully easily resolved.

With regards to your suggestions about improving the error types in general: feel free to discuss this more in depth on the existing serialization thread, or a new one in the forums. DecodingError and EncodingError have to be discarded anyway in the new designs, because of their reliance on existentials, so we'll have a clean slate with which to make any fundamental improvements.

} else {
stringValue
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like a lone CodingKey ought to have a more complete description than just "[0]" or "keyName". Could this instead be a separate property (probably named similarly to the [any CodingKey] property) that gets called specifically in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but an issue is that the description is also called by JSONDecoder in a few cases when providing debug descriptions for decoding error contexts, so it may clobber those too. Unless a) we're fine with that, or b) I'm misunderstanding what you're asking?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a great idea for any external entity (JSONDecoder here) to expect a certain format out of another value's description, even for the purposes of string interpolation unless it's explicitly defined, like one can expect from Int or Double or whatever.

If JSONDecoder wants a certain format when printing CodingKeys in its descriptions, then it should do the work to generate that format itself. At present, that might involve some duplication of code between CodingKey and JSONDecoder / PropertyListDecoder / etc. unless/until that format can be wrapped in some API that does explicitly define the string format.

So, I still think CodingKey's description should remain untouched, and we perhaps follow up with a swift-foundation PR to improve the description construction for the cases you're talking about.

Copy link
Contributor Author

@ZevEisenberg ZevEisenberg Apr 29, 2025

Choose a reason for hiding this comment

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

I can make that change if you'd like: removing the changes to the description, and exposing the more concise version via a different variable. Shall I?

Also, just to make sure I understand: is this specifically because you don't want to change the existing implementation in case someone is relying on it? Or do you prefer the implementation of description regardless of concerns around breaking compatibility? Reading comprehension - your intent is clearer than I realized.

@pepicrft
Copy link

This would be such a great addition 👏🏼. We console-output errors in a CLI assuming the readability of the output, and users end up getting something that's impossible to debug.

@robinkunde
Copy link
Contributor

I love it! What do you think about using JSONPath for the paths to the troublesome values? It can speed up looking up the actual value, especially in larger documents.

@ZevEisenberg
Copy link
Contributor Author

I love it! What do you think about using JSONPath for the paths to the troublesome values? It can speed up looking up the actual value, especially in larger documents.

I'm not familiar with JSONPath. Can you say more? Also, the changes I'm making here in the stdlib are agnostic to the kind of decoder being used. I'm wondering if this suggestion would find a better home in either a PR to swift-foundation's JSONDecoder, or else as part of the nascent successor to Codable being discussed here.

@robinkunde
Copy link
Contributor

I'm not familiar with JSONPath. Can you say more? Also, the changes I'm making here in the stdlib are agnostic to the kind of decoder being used. I'm wondering if this suggestion would find a better home in either a PR to swift-foundation's JSONDecoder, or else as part of the nascent successor to Codable being discussed here.

JSONPath is a query expression syntax for JSON, like XPath for XML or perhaps CSS selectors. The key paths your PR constructs for typeMismatch/etc are already quite similar to it.
For example: [0]/address/city/birds/[1]/name would be $[0].address.city.birds[1].name when using JSONPath.

However, after reading over the diff, I realize there's nothing in DecodingError.Context that would let us special case this representation for JSON decoding specifically, which is too bad.

@ZevEisenberg
Copy link
Contributor Author

ZevEisenberg commented Apr 26, 2025

@kperryua I've addressed all feedback, and update the description to give an easy view of the new before/after test cases. I left a custom description of CodingKey because of the issue discussed above, where JSONDecoder is using it to format its error messages. I'm not opposed to changing it; I just wanted to discuss it more here. I also raised a question about empty string keys.

I tried to keep the commits atomic, but I figure I'll squash them all down once we're ready to merge.

@parkera
Copy link
Contributor

parkera commented Apr 27, 2025

@swift-ci test

@ZevEisenberg
Copy link
Contributor Author

This is my first time trying to read Swift CI failure messages. I'm also on my phone at the moment and Safari is struggling to render the CI failure page. I think I'm seeing something about a mangled name changing? Is that because of the added conformances? Do we need to do some kind of retroactive conformance or availability annotation or something?

@kperryua
Copy link
Contributor

--- /Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/build/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-abi-with-asserts.test.tmp.tmp/stability-stdlib-abi.swift.expected.sorted	2025-04-27 06:56:06
+++ /Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/build/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-abi-with-asserts.test.tmp.tmp/changes.txt.tmp	2025-04-27 06:56:06
@@ -187,6 +187,7 @@
 Constructor UnsafeRawPointer.init(_:) is now with @_preInverseGenerics
 Constructor _SmallString.init(taggedCocoa:) has mangled name changing from 'Swift._SmallString.init(taggedCocoa: Swift.AnyObject) -> Swift._SmallString' to 'Swift._SmallString.init(taggedCocoa: Swift.AnyObject) -> Swift.Optional<Swift._SmallString>'
 Constructor _SmallString.init(taggedCocoa:) has return type change from Swift._SmallString to Swift._SmallString?
+Enum DecodingError has added a conformance to an existing protocol CustomStringConvertible
 Enum MemoryLayout has generic signature change from <T> to <T where T : ~Copyable, T : ~Escapable>
 Enum Never has added a conformance to an existing protocol Decodable
 Enum Never has added a conformance to an existing protocol Encodable
@@ -670,6 +671,7 @@
 Subscript UnsafeMutableBufferPointer.subscript(_unchecked:) has been removed
 Subscript UnsafeMutablePointer.subscript(_:) has been removed
 Subscript UnsafePointer.subscript(_:) has been removed
+Var DecodingError.description is a new API without '@available'

Availability is required on the new conformance at a minimum. There might be some other files to adjust that acknowledge the addition to ABI. I'll try find to out what those are after we rebuild with availability and see what the tests say.

@ZevEisenberg
Copy link
Contributor Author

What should I do for availability on the conformances? Is it gated to a Swift version (like 6.2 or something)? I'd appreciate some guidance here, and then I can make the necessary changes, or feel free to make them yourself if that's easier.

@kperryua
Copy link
Contributor

@available(SwiftStdlib 6.2, *) is appropriate for current main I believe.

Copy link
Contributor

@kperryua kperryua left a comment

Choose a reason for hiding this comment

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

Also: seems like similar improvements could be made to EncodingError. Happy to treat that as a separate PR though.

@stephentyrone
Copy link
Contributor

@available(SwiftStdlib 6.2, *) is appropriate for current main I believe.

We've already branched, so if the intention is for this to go into 6.2 it will need a cherry-pick for the release/6.2 branch after landing on main. Otherwise, it would get 6.3 availability (which I think someone still has to add)

@stephentyrone
Copy link
Contributor

(ah, here's the PR for 6.3: #80870)

@ZevEisenberg
Copy link
Contributor Author

I'm happy to target whichever release seems appropriate. This has been 4+ years in the making; there's no particular urgency to get it into 6.2. That said, if it's not too painful to get it in, it would be nice to ship it sooner rather than later!

And as for EncodingError: I've been wondering if someone would bring it up! Time allowing, I'd love to make another PR.

@ZevEisenberg
Copy link
Contributor Author

ZevEisenberg commented Apr 29, 2025

I rebased on main, squashed commits, and added the availability annotation for 6.2 as requested. But that may not be needed, because I'm going to tweak the implementation per #80941 (comment).

@ZevEisenberg ZevEisenberg force-pushed the main branch 2 times, most recently from 67f6878 to 6d5c8fc Compare May 1, 2025 13:41
@@ -3724,6 +3727,42 @@ public enum DecodingError: Error {
}
}

@available(SwiftStdlib 6.2, *)
extension DecodingError: CustomStringConvertible {
Copy link
Member

Choose a reason for hiding this comment

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

In the review notes of SE-0445, the Swift Language Steering Group has spelled out a clear demand to prefer CustomDebugStringConvertible over CustomStringConvertible in cases where the description is not intended for machine processing:

Instead, the group was inclined to agree with reviewer feedback that conformance to CustomDebugStringConvertible conveys the intended use of the corresponding textual representation—namely, that it is oriented specifically towards debugging, and no attempt should be made to parse, convert, or otherwise manipulate the output.

This definition does seem to fit into the exact same box -- given this decision, I believe this conformance must be changed to be for CustomDebugStringConvertible, not CustomStringConvertible.

Additionally, to the best of my knowledge, we have not received blessing to add any new conformances for public protocols to public types in the Swift Standard Library without going through the full Swift Evolution process. Accordingly, I believe this change requires either a proposal, or an official waiver.

Given that we keep erroneously conforming to CustomStringConvertible, I'd think a fast-pass waiver that allows this change to land as is would once again muddy up the distinction between these protocols. Clearly, there is a rift between general engineering practice and the rules that the LSG has established just half a year ago; if the LSG expects the rules to take root, it needs to actively reinforce them.

Copy link
Contributor Author

@ZevEisenberg ZevEisenberg May 2, 2025

Choose a reason for hiding this comment

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

Thanks for the feedback! I was not aware of that proposal, and I agree that it's a very similar situation. CustomDebugStringConvertible makes sense. And after making some of the other changes mentioned above, I'm happy to open an evolution proposal. Thanks for taking the time to have a look!

@lorentey lorentey added the swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal label May 2, 2025
@ZevEisenberg ZevEisenberg force-pushed the main branch 2 times, most recently from 15f8de2 to 37d1255 Compare May 10, 2025 00:45
@ZevEisenberg
Copy link
Contributor Author

ZevEisenberg commented May 10, 2025

FYI, I'm starting to put together an SE proposal, but I've made the following requested changes:

  • Conform DecodingError to CustomDebugStringConvertible instead of CustomStringConvertible.
  • Instead of altering the existing description of CodingKey, I moved the more compact formatting to a new property and used that in the new debugDescription of DecodingError.
  • Also conform EncodingError and add tests for that as well.

I also simplified the tests by not trying to exactly represent the kind of errors provided by JSONDecoder. It is now clearer that the error are just echoing back the debug string passed to them.

/// to form a path when printing debug information.
var errorPresentationDescription: String {
if let intValue {
"[\(intValue)]"
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'm starting to wonder whether we actually need the brackets. It's not like you'll ever be in a situation where you need help differentiating between keys and indices. This isn't PHP, for cryin' out loud. Thoughts on turning [0] into 0?

expectErrorDescription(
#"""
Here is the debug description for value-not-found
Underlying error: GenericError(name: "these aren\'t the droids you\'re looking for")
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'm not sure where the \' escape sequences are coming from 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it looks like it might be from String(reflecting:) vs String(describing:). I don't think we want it, but it might be unavoidable if we want to use String(debugDescription:), which seems appropriate given that we're inside a conformance to CustomDebugStringConvertible.

image

@ZevEisenberg ZevEisenberg changed the title Better description for DecodingError and CodingKey Better debugDescription for EncodingError and DecodingError May 10, 2025
@ZevEisenberg ZevEisenberg force-pushed the main branch 2 times, most recently from 186bd9c to 448590f Compare May 10, 2025 03:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…le and return a tidy debugDescription to make errors easier to read in debug output.
@ZevEisenberg
Copy link
Contributor Author

I've put up a draft SE proposal for these changes. There's an open question related to back-deployment that I could use some advice on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants