fix(stdlib)!: parse_logfmt performs escaping (#777)

[drmason13]

So that parse_logmt(encode_logfmt(X)) == X, parse_logfmt now processes the escape sequences currently added by encode_logfmt:

• \n
• \"
• \\

This is a breaking change, since the output of the parse_logfmt function is now different for certain inputs (that contain the above escape sequences).

A possible alternative is to avoid escaping while encoding, but that is also a breaking change and makes less sense to me at least.

Another alternative is to simply do nothing. Leave round trip encoding-decoding of logfmt "broken", however this is surprising for users and I don't think it was originally intended to work this way.

I do think it's worth considering other parse/encode pairs and whether they can and should roundtrip, but I haven't yet done this.

As for the implementation, parsers now return a Cow<'a, str> instead of just a &'a str because they may need to remove escape characters from the input, and thus it is no longer possible to represent as a string slice.

Performance

Returning Cow as opposed to plain String avoids allocation in the "happy path" of no escapes being present, however there is always a performance cost as each str is checked upfront for the presence of any escape characters to determine whether to allocate a Cow::Owned (regardless of whether any of the escape characters are actually removed during processing).
See parse_key_value::escape_str for the implementation.

"invalid" escape sequences

I've not (yet?) added an error for trailing escape characters. At the moment they would be accepted and kept as-is, the same goes for other escape sequences such as \r, \t, \u{0123} and "invalid sequences" such as say \z. If they're not one of the escape sequences encoded by encode_logfmt then they are ignored.
See parse_key_value::escape_char for the implementation.

[bruceg]

The code makes sense to me, as does introducing a breaking change for it. The match in escape_char is a little awkward but I can't think of a cleaner way of doing it myself. I think the changelog filename needs to change, though.

[bruceg]

Since this is a breaking change, the filename for the changelog fragment needs to be X.breaking.md.

[drmason13]

The code makes sense to me, as does introducing a breaking change for it. The match in escape_char is a little awkward but I can't think of a cleaner way of doing it myself. I think the changelog filename needs to change, though.

I've looked at it again, and don't want to swap out the match for some sort of and_then or chain.

Some(_) => c

This branch is the most confusing. Might be worth a comment to highlight that we are ignoring the escape sequence entirely in that case by returning c (the backslash), and that this is because we only concern ourselves with escapes that are added by encode_key_value

[bruceg]

I've looked at it again, and don't want to swap out the match for some sort of and_then or chain.

No, that would definitely be a step backwards. I was more thinking of how to deduplicate the repeated _ = rest.next(); but I couldn't see a good way, so my comment was not a complaint.