Escape combining characters in char::Debug

[varkor]

Although combining characters are technically printable, they make little sense to print on their own with Debug: it'd be better to escape them like non-printable characters.

This is a breaking change, but I imagine the fact escape_debug is rare and almost certainly primarily used for debugging that this is an acceptable change.
Resolves#41922.

r? @alexcrichton
cc @clarcharr

[clarfonthey]

LGTM although the character ranges should probably be generated with a script instead of hard-coded, as they can change across unicode versions.

[varkor]

@clarcharr: have you got any ideas for how to detect ranges programmatically? Apart from parsing the Unicode symbol names (which does not seem robust), I couldn't find any reasonable suggestion for how to do this automatically.

[alexcrichton]

r? @SimonSapin

[SimonSapin]

This PR is modifying the char::escape_debug. When calling this method directly or through impl Debug for char, there is only one code point being printed so the idea of escaping code points that do not make much sense on their own seems fine.

However this method is also used in impl Debug for str and in str::escape_debug. #41922 itself does not expect the behavior to change there and I agree. However that means making them based on something other than char::escape_debug. Should that other thing be exposed in new public API?

---

Regarding this being a breaking change, I think I’d be ok with documenting that the exact set of code points being escaped or not is not stable outside of the ASCII range.

The current doc-comment is already out of sync with the implementation: it claims that anything non-ASCII is escaped, which hasn’t been the case since #34485.

---

I also agree with not hard-coding code point ranges, and instead extracting more Unicode data in src/libstd_unicode/unicode.py. However it’s unclear to me what exactly should be included. Canonical_Combining_Class is about reordering during normalization and probably not what we want. The Diacritic property is defined as:

Characters that linguistically modify the meaning of another character to which they apply. Some diacritics are not combining characters, and some combining characters are not diacritics.

Then there’s the *Mark properties:

Abbr	Long	Description
Mn	Nonspacing_Mark	a nonspacing combining mark (zero advance width)
Mc	Spacing_Mark	a spacing combining mark (positive advance width)
Me	Enclosing_Mark	an enclosing combining mark
M	Mark	Mn | Mc | Me

However #41922 suggests that only nonspacing marks are problematic?

[clarfonthey]

I'd be fine with just going for nonspacing marks.

As for str::escape_debug goes, I'd personally opt for escaping any combination of nonspacing marks at the beginning of the string, although it's unclear if that would actually solve the problem. I'm fine with just leaving it with the current behaviour for now.

[varkor]

However that means making them based on something other than char::escape_debug. Should that other thing be exposed in new public API?

Yeah, I'm not sure about this. It'd be nicer if there was an extra argument (single: bool), so the behaviour could be different for individual characters, but we obviously can't change the signature. A specific public extra method that just special-cases non-spacing combining characters seems a little specialised, though.

The current doc-comment is already out of sync with the implementation: it claims that anything non-ASCII is escaped, which hasn’t been the case since #34485.

Which doc comment are you referring to here?

I also agree with not hard-coding code point ranges, and instead extracting more Unicode data in src/libstd_unicode/unicode.py

I have a change that uses Mn instead of hard-coding ranges, pending the decision about the method / effect on str impls.

[varkor]

@SimonSapin: as far as I can tell, there's an issue using src/libstd_unicode/unicode.py here, as it's not available for core, which is where the formatting trait is defined. Is it worth moving it, or taking a different approach?

[clarfonthey]

Wasn't the goal of having libstd_unicode separate from libcore to avoid having to include Unicode tables in libcore? Which makes it suddenly interesting considering how the changes to escape_debug make that less necessary. Or, I'm probably missing something.

Personally I really dislike the libstd_unicode façade, as it tends to get way less attention than libcore, liballoc, and libstd, which leads to bugs like #44303. I'm still confused why it exists and I don't think I've actually seen it explained somewhere.

[SimonSapin]

#49698 merges std_unicode into core.

It might be interesting to replace printable.py (formerly char_private.py) and unicode.py with something based on https://github.com/BurntSushi/ucd-generate.

[bors]

☔ The latest upstream changes (presumably #49698) made this pull request unmergeable. Please resolve the merge conflicts.

[varkor]

@SimonSapin: #49698 makes everything much more pleasant :)

I've rebased on top of it (and also cleaned up unicode.py whilst I was at it, because it had some slightly annoying behaviour before).

[SimonSapin]

@varkor Thanks for your work on this. However I’m still not sure what the right thing to do here is, in terms of what characters exactly to escape or not in which context. Do you know what other languages do for debug-printing Unicode strings and characters?

[varkor]

@SimonSapin: Swift does something similar to the proposed change here. I haven't tried working out exactly which characters they choose to escape, but it seems reasonable to assume they choose either the same, or a similar category.

[varkor]

Ah, so Swift actually escapes any non-ASCII character, which is what Rust used to do before #24588:
https://github.com/apple/swift/blob/c35d508600516b892732e2fd3f0f0a17ca4562ba/stdlib/public/core/UnicodeScalar.swift#L197-L252
So this is new territory, and it makes sense to go with what we think is most sensible: in this case, I think the suggestion of Mn is a reasonable choice.

[TimNN]

@SimonSapin: Friendly triage ping :)

[SimonSapin]

@bors r+

[bors]

📌 Commit b653937 has been approved by SimonSapin

[bors]

⌛ Testing commit b653937 with merge 65a16c0...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 65a16c0 to master...