Better debugDescription for EncodingError and DecodingError
#80941
Conversation
ZevEisenberg
force-pushed
the
main
branch
2 times, most recently
from
4cd346f to
1ae5683
Compare
|
CC @kperryua |
ZevEisenberg
changed the title
description for DecodingErrordescription for DecodingError and CodingKey
There was a problem hiding this comment.
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.
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.
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.
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.
There was a problem hiding this comment.
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. |
|
|
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). |
ZevEisenberg
force-pushed
the
main
branch
2 times, most recently
from
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.
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
CustomDebugStringConvertibleconveys 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.
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
added
the
swift evolution proposal needed
ZevEisenberg
force-pushed
the
main
branch
2 times, most recently
from
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.
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.
I'm not sure where the \' escape sequences are coming from 🤔
There was a problem hiding this comment.
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.
ZevEisenberg
changed the title
description for DecodingError and CodingKeydebugDescription for EncodingError and DecodingError
ZevEisenberg
force-pushed
the
main
branch
3 times, most recently
from
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 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
EncodingErrorandDecodingErrortoCustomDebugStringConvertibleand 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
DecodingErrorandEncodingError, and without modifying the way theJSON/PropertyListEncoder/Decoderin 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
JSONSerializationin 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_nilUnderlyingErrortest_decodingError_valueNotFound_nilUnderlyingErrortest_decodingError_keyNotFound_nonNilUnderlyingErrortest_decodingError_typeMismatch_nilUnderlyingErrortest_decodingError_dataCorrupted_nonEmptyCodingPathtest_decodingError_valueNotFound_nonNilUnderlyingErrortest_encodingError_invalidValue_emptyCodingPath_nonNilUnderlyingErrortest_decodingError_typeMismatch_nonNilUnderlyingErrortest_encodingError_invalidValue_emptyCodingPath_nilUnderlyingErrortest_decodingError_keyNotFound_nilUnderlyingErrortest_decodingError_dataCorrupted_emptyCodingPathtest_encodingError_invalidValue_nonEmptyCodingPath_nonNilUnderlyingError