-
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
?
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
186bd9c
to
448590f
Compare
…le and return a tidy debugDescription to make errors easier to read in debug output.
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. |
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
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
test_decodingError_valueNotFound_nilUnderlyingError
test_decodingError_keyNotFound_nonNilUnderlyingError
test_decodingError_typeMismatch_nilUnderlyingError
test_decodingError_dataCorrupted_nonEmptyCodingPath
test_decodingError_valueNotFound_nonNilUnderlyingError
test_encodingError_invalidValue_emptyCodingPath_nonNilUnderlyingError
test_decodingError_typeMismatch_nonNilUnderlyingError
test_encodingError_invalidValue_emptyCodingPath_nilUnderlyingError
test_decodingError_keyNotFound_nilUnderlyingError
test_decodingError_dataCorrupted_emptyCodingPath
test_encodingError_invalidValue_nonEmptyCodingPath_nonNilUnderlyingError