Issue 44986/fix windows ui path

[clippered]

#44968

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nikomatsakis]

Can you squash this down to one commit -- and in particular remove the merge commits?

[nikomatsakis]

Also, I guess this doesn't really fix #44986 -- that seems to be unrelated?

[clippered]

Thanks for the feedback. I updated the PR to be a single commit. And I had a typo, all this time. It should refer to issue #44968

[estebank]

@clippered looks great, neat solution! Assuming travis passes, this is good to merge (r=me).

[estebank]

@bors r+ rollup

[bors]

📌 Commit c8025d3 has been approved by estebank

[kennytm]

@bors r-

Basically every UI test failed on Windows, for details see https://ci.appveyor.com/project/rust-lang/rust/build/1.0.4949/job/fv87h5oarj17tgqr. Example diff:

 error[E0308]: mismatched types
- --> $DIR/block-must-not-have-result-while.rs:13:9+ --> C:/projects/rust/src/test/ui/block-result/block-must-not-have-result-while.rs:13:9
|
13 | true //~ ERROR mismatched types
| ^^^^ expected (), found bool
|
= note: expected type `()`
found type `bool`
error: aborting due to previous error

[estebank]

@kennytm my bad, I should have done a bors try first.

@clippered could you change src/runtest.rs? What you'd have to do is the output.replace(..., "$DIR") for bothparent_dir.display().to_string().replace("\\", "\\\\") and parent_dir.display().to_string().

[nikomatsakis]

r? @estebank

[clippered]

Apologies for that fix. I'll try to resolve this soon. Thanks for your help @estebank , but can you rephrase what you said? I somewhat did not get what you meant. Aren't the parent_dir.display().to_string().replace("\\", "\\")andparent_dir.display().to_string()` the same thing?

[estebank]

Sure thing! The compiler output to stderr can be one of two things (as far as we care about here):

• Normal diagnostic output:

warning: unnecessary parentheses around assigned value
--> /path/to/file/suggestions.rs:17:21
|
17 | let mut a = (1); // should suggest no `mut`, no parens
| ^^^ help: remove these parentheses
|
= note: #[warn(unused_parens)] on by default

• JSON output:

{"message":"unnecessary parentheses around assigned value","code":null,"level":"warning","spans":[{"file_name":"/path/to/file/unused_parens_json_suggestion.rs","byte_start":976,"byte_end":989,"line_start":22,"line_end":22,"column_start":14,"column_end":27,"is_primary":true,"text":[{"text":" let _a = (1 / (2 + 3));","highlight_start":14,"highlight_end":27}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"#[warn(unused_parens)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/path/to/file/unused_parens_json_suggestion.rs","byte_start":976,"byte_end":989,"line_start":22,"line_end":22,"column_start":14,"column_end":27,"is_primary":true,"text":[{"text":" let _a = (1 / (2 + 3));","highlight_start":14,"highlight_end":27}],"label":null,"suggested_replacement":"1 / (2 + 3)","expansion":null}],"children":[],"rendered":" let _a = 1 / (2 + 3);"}],"rendered":null}

Under windows systems the json output looks the following way:

{"message":"unnecessary parentheses around assigned value","code":null,"level":"warning","spans":[{"file_name":"C:\\projects\\rust\\src\\test\\ui\\lint\\unused_parens_json_suggestion.rs","byte_start":976,"byte_end":989,"line_start":22,"line_end":22,"column_start":14,"column_end":27,"is_primary":true,"text":[{"text":" let _a = (1 / (2 + 3));","highlight_start":14,"highlight_end":27}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"#[warn(unused_parens)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"C:\\projects\\rust\\src\\test\\ui\\lint\\unused_parens_json_suggestion.rs","byte_start":976,"byte_end":989,"line_start":22,"line_end":22,"column_start":14,"column_end":27,"is_primary":true,"text":[{"text":" let _a = (1 / (2 + 3));","highlight_start":14,"highlight_end":27}],"label":null,"suggested_replacement":"1 / (2 + 3)","expansion":null}],"children":[],"rendered":" let _a = 1 / (2 + 3);"}],"rendered":null}

By replacing \ with \\ in parent_dir_str you fix the issue for the json output, but break it for the stderr output, as json needs the \ to be escaped when printed out, but the terminal doesn't:

warning: unnecessary parentheses around assigned value
--> c:\path\to\file\suggestions.rs:17:21
|
17 | let mut a = (1); // should suggest no `mut`, no parens
| ^^^ help: remove these parentheses
|
= note: #[warn(unused_parens)] on by default

This doesn't happen on unix-like systems because the / doesn't need escaping on json strings. Because of all of this, in the normalization step you can get away with just doing a replacement of c:\path\to\file\ and c:\\path\\to\\file\\ for $DIR/ and it would work for all cases. On unix-like system you'd be wasting time doing the replacement of /path/to/file/ for $DIR/ twice, as the second time nothing would be found and it would be a noop. Does this make sense?

[clippered]

I see, I will need to add the check for --error-format json before adding the replace then.

Pardon me for asking again but which test should I run to test the normal-diagnostic output? I have only ran python x.py test src/test/ui

[clippered]

@estebank I hope Travis accepts this one.
Thanks for your patience. I also realised that running python test will ignore all previous tests that passed and does not rerun them again, that's why I missed the other test cases.

[estebank]

@bors try

[bors]

⌛ Trying commit 057bc7d with merge 0b6db7cc31fb5d2a9027fc0641092ff3cf0e8440...

[kennytm]

@estebank The try branch is not enabled on AppVeyor i.e. it won't be tested on Windows, so I don't think this can check whether the changeset is valid.

[estebank]

@kennytm wasn't aware of that... Ok, I'll try to get a windows machine to test this myself.

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[aidanhs]

@estebank have you managed to find a windows machine to give this a try?

[estebank]

@aidanhs I have been on the road for the past week.

I'm giving it a @bors r+ for any errors to be caught by appveyor. To anyone thinking about rolling this PR up, don't so that if it fails (it shouldn't) it doesn't block the other PRs.

[bors]

📌 Commit 057bc7d has been approved by estebank

[kennytm]

@bors rollup-

The rollup status has not cleared 😓

[estebank]

@kennytm, oops. Thank you for catching that.

[bors]

⌛ Testing commit 057bc7d with merge 4750c1e...

[bors]

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