Display spans correctly when there are zero-width or wide characters #45711

tirr-c · 2017-11-02T03:59:11Z

Hopefully...

fixes Error span is in incorrect place due to Unicode fullwidth characters #45211
fixes Bad span computations with unicode characters, should be handling them as graphemes #8706

Before:

error: invalid width `7` for integer literal
  --> unicode_2.rs:12:25
   |
12 |     let _ = ("a̐éö̲", 0u7);
   |                         ^^^
   |
   = help: valid widths are 8, 16, 32, 64 and 128

error: invalid width `42` for integer literal
  --> unicode_2.rs:13:20
   |
13 |     let _ = ("아あ", 1i42);
   |                    ^^^^
   |
   = help: valid widths are 8, 16, 32, 64 and 128

error: aborting due to 2 previous errors

After:

error: invalid width `7` for integer literal
  --> unicode_2.rs:12:25
   |
12 |     let _ = ("a̐éö̲", 0u7);
   |                     ^^^
   |
   = help: valid widths are 8, 16, 32, 64 and 128

error: invalid width `42` for integer literal
  --> unicode_2.rs:13:20
   |
13 |     let _ = ("아あ", 1i42);
   |                      ^^^^
   |
   = help: valid widths are 8, 16, 32, 64 and 128

error: aborting due to 2 previous errors

Spans might display incorrectly on the browser.

r? @estebank

tirr-c · 2017-11-02T04:07:50Z

Is this failure a travis issue?

est31 · 2017-11-02T05:20:13Z

@tirr-c its no travis issue:

[00:03:44] tidy error: /checkout/src/libsyntax_pos/lib.rs:829: TODO is deprecated; use FIXME

[00:03:46] some tidy checks failed

You should also read contributing.md.

As one thing to check, does your code compute the correct number of ^ as well? E.g. in something like let _ = (42, a̐é); where should underline the a̐é with two ^ markers.

tirr-c · 2017-11-02T06:09:18Z

Oh, I've run tidy test before adding some comments. I thought the failure is because of travis since I didn't see any build logs after failure. Fixed.

And yes, it displays two ^'s:

error: non-ascii idents are not fully supported. (see issue #28979)
 --> test.rs:2:18
  |
2 |     let _ = (42, a̐é);
  |                  ^^
  |
  = help: add #![feature(non_ascii_idents)] to the crate attributes to enable

tirr-c · 2017-11-02T07:24:22Z

Maybe I need to rename fields and variables. The term "narrow" seems more appropriate than "half-width" here.

est31 · 2017-11-02T07:44:19Z

"half-width" is an actual term so IMO it is okay: https://en.wikipedia.org/wiki/Halfwidth_and_fullwidth_forms

tirr-c · 2017-11-02T08:34:27Z

My understanding is that the term "narrow" is broader than "halfwidth." Also, most ASCII characters are defined as narrow, not halfwidth, so narrow would be better than halfwidth.

http://www.unicode.org/reports/tr11/

estebank

I'm ok with the approach, but I can't shake the feeling that performing the translation from bytepos to visual column in src/librustc_errors/emitter.rs's render_source_line by looking at the line itself and fixing the annotation positions there might be slightly less/more straightforward code.

That being said, the current approach is sound. Could you add the requested test? Once that's done, r=me.

estebank · 2017-11-02T16:50:41Z

src/libsyntax_pos/lib.rs

@@ -618,7 +634,7 @@ impl Encodable for FileMap {
 impl Decodable for FileMap {
    fn decode<D: Decoder>(d: &mut D) -> Result<FileMap, D::Error> {

-        d.read_struct("FileMap", 6, |d| {


Was the 6 a bug?

I have no idea, but for me 6 there didn't make sense. There were total of 7 (now 8) fields.

estebank · 2017-11-02T16:57:32Z

src/libsyntax_pos/lib.rs

+    pub pos: BytePos,
+    /// The width of the character, 0 (zero-width) or 2 (full-width)
+    pub width: usize,
+}


I'm thinking it'd make sense to make NonNarrowChar an enum with ZeroWidth(BytePos) and FullWidth(BytePos). The way it is being used below this is not a big issue, but it is good style to encode restrictions like this in the type system and provide either a Deref for usize or a method width(&self) that will give you the usize representation. This way you're assured no-one will come along in the future and try to use NonNarrowChar with a width of 12 😬

I totally agree!

estebank · 2017-11-02T16:58:46Z

src/libsyntax_pos/lib.rs

+            '\t' | '\n' =>
+                // Tabs will consume one column.
+                // Make newlines take one column so that displayed spans can point them.
+                1,


~~As you're adding support for characters with varying widths, adding support for either 4 or 8 width tabs should be easy to add. Let's do so in a subsequent PR.~~

Done in #45953.

estebank · 2017-11-02T17:00:05Z

src/test/ui/codemap_tests/unicode_2.stderr

+  --> $DIR/unicode_2.rs:13:20
+   |
+13 |     let _ = ("아あ", 1i42);
+   |                      ^^^^


As per @est31's comment, could you add a case that points at the string itself to make sure regressions would get caught?

tirr-c · 2017-11-02T17:57:08Z

Made NonNarrowChar into a enum, and added a new test.

estebank · 2017-11-02T18:08:30Z

src/libsyntax_pos/lib.rs

+    pub fn pos(&self) -> BytePos {
+        match *self {
+            NonNarrowChar::ZeroWidth(p) => p,
+            NonNarrowChar::Wide(p) => p,


This can be simplified to

match *self { NonNarrowChar::ZeroWidth(p) | NonNarrowChar::Wide(p) => p, }

estebank · 2017-11-02T18:18:53Z

@bors r+

bors · 2017-11-02T18:18:54Z

📌 Commit 272c2fa has been approved by estebank

bors · 2017-11-03T07:42:20Z

⌛ Testing commit 272c2fa with merge 3e20b2c69630dc4e6752d30f9b3788c4456272a6...

bors · 2017-11-03T07:48:08Z

💔 Test failed - status-travis

kennytm · 2017-11-03T14:19:13Z

@bors retry

bors · 2017-11-04T23:09:25Z

⌛ Testing commit 272c2fa with merge 12e6b53...

@estebank

Display spans correctly when there are zero-width or wide characters Hopefully... * fixes #45211 * fixes #8706 --- Before: ``` error: invalid width `7` for integer literal --> unicode_2.rs:12:25 | 12 | let _ = ("a̐éö̲", 0u7); | ^^^ | = help: valid widths are 8, 16, 32, 64 and 128 error: invalid width `42` for integer literal --> unicode_2.rs:13:20 | 13 | let _ = ("아あ", 1i42); | ^^^^ | = help: valid widths are 8, 16, 32, 64 and 128 error: aborting due to 2 previous errors ``` After: ``` error: invalid width `7` for integer literal --> unicode_2.rs:12:25 | 12 | let _ = ("a̐éö̲", 0u7); | ^^^ | = help: valid widths are 8, 16, 32, 64 and 128 error: invalid width `42` for integer literal --> unicode_2.rs:13:20 | 13 | let _ = ("아あ", 1i42); | ^^^^ | = help: valid widths are 8, 16, 32, 64 and 128 error: aborting due to 2 previous errors ``` Spans might display incorrectly on the browser. r? @estebank

bors · 2017-11-05T01:40:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 12e6b53 to master...

Display `\t` in diagnostics code as four spaces Follow up to #44386 using the unicode variable width machinery from #45711 to replace tabs in the source code when displaying a diagnostic error with four spaces (instead of only one), while properly accounting for this when calculating underlines. Partly addresses #44618.

rust-highfive assigned estebank Nov 2, 2017

tirr-c force-pushed the unicode-span branch from 0903798 to 330b9bc Compare November 2, 2017 06:07

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 2, 2017

tirr-c changed the title ~~Display spans correctly when there are non-half-width characters~~ Display spans correctly when there are zero-width or wide characters Nov 2, 2017

estebank approved these changes Nov 2, 2017

View reviewed changes

tirr-c force-pushed the unicode-span branch from a03879c to 0b25ef8 Compare November 2, 2017 17:55

estebank approved these changes Nov 2, 2017

View reviewed changes

Display spans correctly when there are non-half-width characters

272c2fa

tirr-c force-pushed the unicode-span branch from 0b25ef8 to 272c2fa Compare November 2, 2017 18:15

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2017

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 3, 2017

bors merged commit 272c2fa into rust-lang:master Nov 5, 2017

tirr-c deleted the unicode-span branch November 5, 2017 07:02

estebank mentioned this pull request Nov 13, 2017

Display \t in diagnostics code as four spaces #45953

Merged

est31 mentioned this pull request Jan 12, 2018

Beta regression in span printing with tabs involved #47377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display spans correctly when there are zero-width or wide characters #45711

Display spans correctly when there are zero-width or wide characters #45711

tirr-c commented Nov 2, 2017

tirr-c commented Nov 2, 2017

est31 commented Nov 2, 2017

tirr-c commented Nov 2, 2017

tirr-c commented Nov 2, 2017

est31 commented Nov 2, 2017

tirr-c commented Nov 2, 2017 •

edited

Loading

estebank left a comment

estebank Nov 2, 2017

tirr-c Nov 2, 2017

estebank Nov 2, 2017

tirr-c Nov 2, 2017

estebank Nov 2, 2017 •

edited

Loading

estebank Nov 2, 2017

tirr-c commented Nov 2, 2017

estebank Nov 2, 2017

estebank commented Nov 2, 2017

bors commented Nov 2, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

kennytm commented Nov 3, 2017

bors commented Nov 4, 2017

bors commented Nov 5, 2017

Display spans correctly when there are zero-width or wide characters #45711

Display spans correctly when there are zero-width or wide characters #45711

Conversation

tirr-c commented Nov 2, 2017

tirr-c commented Nov 2, 2017

est31 commented Nov 2, 2017

tirr-c commented Nov 2, 2017

tirr-c commented Nov 2, 2017

est31 commented Nov 2, 2017

tirr-c commented Nov 2, 2017 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

estebank Nov 2, 2017

Choose a reason for hiding this comment

tirr-c Nov 2, 2017

Choose a reason for hiding this comment

estebank Nov 2, 2017

Choose a reason for hiding this comment

tirr-c Nov 2, 2017

Choose a reason for hiding this comment

estebank Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

estebank Nov 2, 2017

Choose a reason for hiding this comment

tirr-c commented Nov 2, 2017

estebank Nov 2, 2017

Choose a reason for hiding this comment

estebank commented Nov 2, 2017

bors commented Nov 2, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

kennytm commented Nov 3, 2017

bors commented Nov 4, 2017

bors commented Nov 5, 2017

tirr-c commented Nov 2, 2017 •

edited

Loading

estebank Nov 2, 2017 •

edited

Loading