-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
base: main
Are you sure you want to change the base?
Conversation
4cd346f
to
1ae5683
Compare
CC @kperryua |
description
for DecodingError
description
for DecodingError
and CodingKey
There was a problem hiding this 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 | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 CodingKey
s 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.
There was a problem hiding this comment.
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 Reading comprehension - your intent is clearer than I realized.description
regardless of concerns around breaking compatibility?
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. |
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. |
JSONPath is a query expression syntax for JSON, like XPath for XML or perhaps CSS selectors. The key paths your PR constructs for However, after reading over the diff, I realize there's nothing in |
@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 I tried to keep the commits atomic, but I figure I'll squash them all down once we're ready to merge. |
@swift-ci test |
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? |
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. |
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. |
|
There was a problem hiding this 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.
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) |
(ah, here's the PR for 6.3: #80870) |
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 |
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). |
67f6878
to
6d5c8fc
Compare
stdlib/public/core/Codable.swift
Outdated
@@ -3724,6 +3727,42 @@ public enum DecodingError: Error { | |||
} | |||
} | |||
|
|||
@available(SwiftStdlib 6.2, *) | |||
extension DecodingError: CustomStringConvertible { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
15f8de2
to
37d1255
Compare
FYI, I'm starting to put together an SE proposal, but I've made the following requested changes:
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)]" |
There was a problem hiding this comment.
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
?
test/stdlib/CodableTests.swift
Outdated
expectErrorDescription( | ||
#""" | ||
Here is the debug description for value-not-found | ||
Underlying error: GenericError(name: "these aren\'t the droids you\'re looking for") |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description
for DecodingError
and CodingKey
debugDescription
for EncodingError
and DecodingError
448590f
to
0948d52
Compare
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. |
…le and return a tidy debugDescription to make errors easier to read in debug output.
Now that SE-0489 has been accepted, could someone please remove the "swift evolution proposal needed" label? |
I've started a forum thread to discuss the precise formatting details. Question: who is going to be the final approver for this work? I'd like to make sure I know what we need to get to "yes". |
I'll take responsibility for final sign-off on it. Ping me when you're ready. |
by the way, is there any update on whether the availability annotations should be 6.2, 6.3, or something else? |
Definitely not 6.2, that's 99.9% baked at this point. 6.3 is likely correct, but we can tweak them when everything else is ready. |
Now accepted as SE-0489, but we still need to refine the implementation.
To Do
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
andDecodingError
toCustomDebugStringConvertible
and adds a more-readabledebugDescription
.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
andEncodingError
, and without modifying the way theJSON
/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
Before
invalidValue(234, 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: nil))
After
EncodingError.invalidValue: 234 (Int). Path: first/second/[2]. Debug description: You cannot do that!
test_decodingError_valueNotFound_nilUnderlyingError
Before
valueNotFound(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "firstName", intValue: nil)], debugDescription: "Description for debugging purposes", underlyingError: nil))
After
DecodingError.valueNotFound: Expected value of type String but found null instead. Path: [0]/firstName. Debug description: Description for debugging purposes
test_decodingError_keyNotFound_nonNilUnderlyingError
Before
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?"))))
After
DecodingError.keyNotFound: Key \'name\' not found in keyed decoding container. Path: [0]/address/city. Debug description: Just some info to help you out. Underlying error: GenericError(name: "hey, who turned out the lights?")
test_decodingError_typeMismatch_nilUnderlyingError
Before
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))
After
DecodingError.typeMismatch: expected value of type String. Path: [0]/address/city/birds/[1]/name. Debug description: This is where the debug description goes
test_decodingError_dataCorrupted_nonEmptyCodingPath
Before
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"))))
After
DecodingError.dataCorrupted: Data was corrupted. Path: first/second/[2]. Debug description: There was apparently some data corruption!. Underlying error: GenericError(name: "This data corruption is getting out of hand")
test_decodingError_valueNotFound_nonNilUnderlyingError
Before
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"))))
After
DecodingError.valueNotFound: Expected value of type Int but found null instead. Path: [0]/population. Debug description: Here is the debug description for value-not-found. Underlying error: GenericError(name: "these aren\\\'t the droids you\\\'re looking for")
test_encodingError_invalidValue_emptyCodingPath_nonNilUnderlyingError
Before
invalidValue(345, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: Optional(main.GenericError(name: "You really cannot do that"))))
After
EncodingError.invalidValue: 345 (Int). Debug description: You cannot do that!. Underlying error: GenericError(name: "You really cannot do that")
test_decodingError_typeMismatch_nonNilUnderlyingError
Before
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"))))
After
DecodingError.typeMismatch: expected value of type String. Path: [0]/address/[1]/street. Debug description: Some debug description. Underlying error: GenericError(name: "some generic error goes here")
test_encodingError_invalidValue_emptyCodingPath_nilUnderlyingError
Before
invalidValue(123, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: nil))
After
EncodingError.invalidValue: 123 (Int). Debug description: You cannot do that!
test_decodingError_keyNotFound_nilUnderlyingError
Before
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))
After
DecodingError.keyNotFound: Key \'name\' not found in keyed decoding container. Path: [0]/address/city. Debug description: How would you describe your relationship with your debugger?
test_decodingError_dataCorrupted_emptyCodingPath
Before
dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "The given data was not valid JSON", underlyingError: Optional(main.GenericError(name: "just some data corruption"))))
After
DecodingError.dataCorrupted: 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
Before
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"))))
After
EncodingError.invalidValue: 456 (Int). Path: first/second/[2]. Debug description: You cannot do that!. Underlying error: GenericError(name: "You really cannot do that")