Fix mispositioned span in suggestions with wide characters #47407

gaurikholkar-zz · 2018-01-13T06:38:31Z

The output now looks like this

error[E0369]: binary operation `+` cannot be applied to type `&str`
 --> h.rs:3:11
  |
3 |     let _a = b + ", World!";
  |              ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings
help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left
  |
3 |     let _a = b.to_owned() + ", World!";
  |              ^^^^^^^^^

error: aborting due to previous error

For the case when emojis are involved, it gives the new output for proper indentation.
But for an indentation as follows,

fn main() {
let b = "hello";
    let _a = b + ", World!";
}

it still mispositions the span

3 |     println!("🦀🦀🦀🦀🦀"); let _a = b + ", World!";
  |                                           ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings
  |
3 |     println!("🦀🦀🦀🦀🦀"); let _a = b.to_owned() + ", World!";
  |                                           ^^^^^^^
error: aborting due to previous erro

cc @estebank @est31

rust-highfive · 2018-01-13T06:38:35Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

kennytm · 2018-01-13T07:41:50Z

src/test/ui/lint/use_suggestion_json.stderr

-# fn main() {} // don't insert it for us; that'll break imports
-```
-"
+    "explanation": null


Why is the explanation deleted?

@kennytm it was failing locally. Just thought I'll push and see what happens

estebank · 2018-01-13T09:12:35Z

src/librustc_errors/emitter.rs

@@ -1187,7 +1187,7 @@ impl EmitterWriter {
                    let sub_len = parts[0].snippet.trim().chars().fold(0, |acc, ch| {
                        acc + unicode_width::UnicodeWidthChar::width(ch).unwrap_or(0)
                    });
-                    let underline_start = span_start_pos.col.0 + start;
+                    let underline_start = span_start_pos.col_display + start;
                    let underline_end = span_start_pos.col.0 + start + sub_len;


I think you need to change this line too to use col_display.

estebank · 2018-01-13T09:14:04Z

Please add an ui test with both the tab and emoji cases.

gaurikholkar-zz · 2018-01-13T11:44:57Z

Have made the suggested changes. Investigating why it is not working for the emoji example.

estebank

Some tidy check errors to fix:

[00:03:36] tidy error: /checkout/src/test/ui/issue-47377-1.rs:13: line longer than 100 chars
[00:03:36] tidy error: /checkout/src/test/ui/issue-47377-1.rs: missing trailing newline
[00:03:36] tidy error: /checkout/src/test/ui/issue-47377.rs:13: line longer than 100 chars
[00:03:36] tidy error: /checkout/src/test/ui/issue-47377.rs: missing trailing newline
[00:03:37] some tidy checks failed

estebank · 2018-01-14T05:09:31Z

src/test/ui/issue-47377-1.rs

+
+fn main(){
+    let b = "hello";
+        println!("🦀🦀🦀🦀🦀"); let _a = b + ", World!"; //~ERROR 13:37: 13:51: binary operation `+` cannot be applied to type `&str` [E0369]


Move the error check to the line below

Also you should probably use 4 spaces of indentation here, as well as rename the file to issue-47380.rs.

estebank · 2018-01-14T05:09:48Z

src/test/ui/issue-47377.rs

+
+fn main(){
+    let b = "hello";
+    let _a = b + ", World!"; //~ERROR 13:14: 13:28: binary operation `+` cannot be applied to type `&str` [E0369]


Move the error check to the line below (//~^ ERROR binary operation...)

estebank · 2018-01-14T05:10:52Z

src/test/ui/issue-47377.rs

+fn main(){
+    let b = "hello";
+    let _a = b + ", World!"; //~ERROR 13:14: 13:28: binary operation `+` cannot be applied to type `&str` [E0369]
+}


Add new line (your editor should be doing this by default).

est31 · 2018-01-14T05:56:06Z

src/test/ui/issue-47377.rs

+// except according to those terms.
+
+fn main(){
+    let b = "hello";


This file doesn't use tabs. You need to change the indentation of these two lines to use tabs and then add a line // ignore-tidy-tab to get tidy passing, like done in src/test/ui/codemap_tests/tab_2.rs.

@est31 I changed it to ~^ ERROR E0369 }. Do I still need //ignore-tidy-tab?

I think ignore-tidy-tab is so that the tidy check for the project doesn't block the merge of the PR, while keeping the tabs in the code so that we avoid regressions in the handling of them. I believe your editor might be "helpfully" replacing tabs with spaces.

Yeah it was. Can you have a look now?

estebank · 2018-01-16T20:10:11Z

src/test/ui/issue-47377.rs

+// except according to those terms.
+
+fn main(){
+    let b = "hello";


I think ignore-tidy-tab is so that the tidy check for the project doesn't block the merge of the PR, while keeping the tabs in the code so that we avoid regressions in the handling of them. I believe your editor might be "helpfully" replacing tabs with spaces.

estebank · 2018-01-16T20:10:59Z

src/test/ui/issue-47380.stderr

+   |                                      ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings
+help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left
+   |
+13 |     println!("🦀🦀🦀🦀🦀"); let _a = b.to_owned() + ", World!";


Note for reviewers: this looks misaligned on the browser, but not in terminals that handle emojis correctly.

@estebank the output is misaligned in iTerm though

estebank · 2018-01-16T20:12:43Z

src/test/ui/issue-47377.rs

+// except according to those terms.
+fn main(){
+    let b = "hello";
+    let _a = b + ", World!";


This is still using spaces instead of tabs.

est31 · 2018-01-16T20:16:25Z

src/test/ui/issue-47380.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+// ignore-tidy-tab


This is not the file that should get this line, the other one should get it with the tabs. Here we are testing for emojis.

estebank · 2018-01-17T02:01:45Z

@bors r+ rollup

bors · 2018-01-17T02:01:46Z

📌 Commit efe3d69 has been approved by estebank

@estebank

fix mispositioned span This fixes rust-lang#47377 The output now looks like this ``` error[E0369]: binary operation `+` cannot be applied to type `&str` --> h.rs:3:11 | 3 | let _a = b + ", World!"; | ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left | 3 | let _a = b.to_owned() + ", World!"; | ^^^^^^^^^ error: aborting due to previous error ``` For the case when emojis are involved, it gives the new output for proper indentation. But for an indentation as follows, ``` fn main() { let b = "hello"; let _a = b + ", World!"; } ``` it still mispositions the span ``` 3 | println!("🦀🦀🦀🦀🦀"); let _a = b + ", World!"; | ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings | 3 | println!("🦀🦀🦀🦀🦀"); let _a = b.to_owned() + ", World!"; | ^^^^^^^ error: aborting due to previous erro ``` cc @estebank @est31

@estebank

fix mispositioned span This fixes rust-lang#47377 The output now looks like this ``` error[E0369]: binary operation `+` cannot be applied to type `&str` --> h.rs:3:11 | 3 | let _a = b + ", World!"; | ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left | 3 | let _a = b.to_owned() + ", World!"; | ^^^^^^^^^ error: aborting due to previous error ``` For the case when emojis are involved, it gives the new output for proper indentation. But for an indentation as follows, ``` fn main() { let b = "hello"; let _a = b + ", World!"; } ``` it still mispositions the span ``` 3 | println!("🦀🦀🦀🦀🦀"); let _a = b + ", World!"; | ^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings | 3 | println!("🦀🦀🦀🦀🦀"); let _a = b.to_owned() + ", World!"; | ^^^^^^^ error: aborting due to previous erro ``` cc @estebank @est31

Rollup of 21 pull requests - Successful merges: #47302, #47333, #47387, #47404, #47407, #47426, #47427, #47436, #47444, #47456, #47458, #47467, #47479, #47481, #47483, #47487, #47497, #47498, #47505, #47509, #47514 - Failed merges:

fix mispositioned span

4cedbfc

rust-highfive assigned estebank Jan 13, 2018

kennytm reviewed Jan 13, 2018

View reviewed changes

revert changes to ui test

eb1ada2

gaurikholkar-zz force-pushed the master branch from b23c516 to eb1ada2 Compare January 13, 2018 08:07

estebank reviewed Jan 13, 2018

View reviewed changes

add ui test

3c8c505

estebank reviewed Jan 14, 2018

View reviewed changes

est31 reviewed Jan 14, 2018

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2018

fix tidy checks

d769539

estebank reviewed Jan 16, 2018

View reviewed changes

est31 reviewed Jan 16, 2018

View reviewed changes

gaurikholkar added 2 commits January 17, 2018 02:00

using tabs in ui tests

a4e3f36

fix minor errors

efe3d69

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2018

estebank changed the title ~~fix mispositioned span~~ Fix mispositioned span in suggestions with wide characters Jan 17, 2018

kennytm mentioned this pull request Jan 17, 2018

Rollup of 19 pull requests #47513

Closed

kennytm mentioned this pull request Jan 17, 2018

Rollup of 21 pull requests #47522

Merged

bors merged commit efe3d69 into rust-lang:master Jan 17, 2018

est31 mentioned this pull request Jan 17, 2018

Mispositioned help span indicator when emojis are involved #47380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mispositioned span in suggestions with wide characters #47407

Fix mispositioned span in suggestions with wide characters #47407

gaurikholkar-zz commented Jan 13, 2018 •

edited by estebank

Loading

rust-highfive commented Jan 13, 2018

kennytm Jan 13, 2018

gaurikholkar-zz Jan 13, 2018 •

edited

Loading

estebank Jan 13, 2018

estebank commented Jan 13, 2018

gaurikholkar-zz commented Jan 13, 2018

estebank left a comment

estebank Jan 14, 2018

est31 Jan 14, 2018

estebank Jan 14, 2018

estebank Jan 14, 2018

est31 Jan 14, 2018

gaurikholkar-zz Jan 16, 2018

estebank Jan 16, 2018

gaurikholkar-zz Jan 16, 2018

estebank Jan 16, 2018

estebank Jan 16, 2018

gaurikholkar-zz Jan 16, 2018

estebank Jan 16, 2018

est31 Jan 16, 2018

estebank commented Jan 17, 2018

bors commented Jan 17, 2018

Fix mispositioned span in suggestions with wide characters #47407

Fix mispositioned span in suggestions with wide characters #47407

Conversation

gaurikholkar-zz commented Jan 13, 2018 • edited by estebank Loading

rust-highfive commented Jan 13, 2018

Choose a reason for hiding this comment

gaurikholkar-zz Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jan 13, 2018

gaurikholkar-zz commented Jan 13, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jan 17, 2018

bors commented Jan 17, 2018

gaurikholkar-zz commented Jan 13, 2018 •

edited by estebank

Loading

gaurikholkar-zz Jan 13, 2018 •

edited

Loading