Include space in suggestion `mut` in bindings #47465

estebank · 2018-01-15T21:02:53Z

rust-highfive · 2018-01-15T21:03:04Z

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

oli-obk · 2018-01-16T06:37:09Z

src/test/ui/lint/suggestions.stderr

@@ -22,7 +22,7 @@ warning: variable does not need to be mutable
  --> $DIR/suggestions.rs:36:13
   |
 36 |         let mut a = (1); // should suggest no `mut`, no parens
-   |             ---^^
+   |             ----^


I did something similar in #45741

The terminal output should not be showing whitespace.

This is the opposite problem to that. The span needs to point to the whitespace as well, so that the suggestion removes that whitespace. The suggestion content does not have trailing whitespaces.

Right. But the human readable output should not highlight the whitespace. Only json should have it

ollie27 · 2018-01-16T09:20:51Z

src/librustc_borrowck/borrowck/unused.rs

@@ -77,7 +77,7 @@ impl<'a, 'tcx> UnusedMutCx<'a, 'tcx> {
                continue
            }

-            let mut_span = tcx.sess.codemap().span_until_char(ids[0].2, ' ');
+            let mut_span = tcx.sess.codemap().span_through_char(ids[0].2, ' ');


What if there is more than one space after the mut? Also, and this is a bug with the existing code as well, what if the white-space after the mut doesn't contain any spaces at all?

The following highlights until the end of foo.

fn main() { let mut foo = 1; println!("{}", foo); }

I don't think multiple spaces are an issue, because there'll be the same number of multiple spaces as there were before.

This is a blocking issue. I'll take a look at it.

The code now accounts for any whitespace characters, in any amount, so that the resulting code will always be one space. I could see this breaking formatting sometimes, but should never produce incorrect code or trailing whitespace in the resulting code.

bors · 2018-01-17T22:25:16Z

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

ollie27 · 2018-01-23T21:10:15Z

src/libsyntax/codemap.rs

+            // get the bytes width of all the non-whitespace characters
+            for (i, c) in snippet.chars().take_while(|c| !c.is_whitespace()).enumerate() {
+                offset += c.len_utf8();
+                pos = i + 1;


The pos variable doesn't seem right. It's storing the character index but it's being used as a byte index below. The following should work I think:

let mut offset = 0; // get the bytes width of all the non-whitespace characters for c in snippet.chars().take_while(|c| !c.is_whitespace()) { offset += c.len_utf8(); } // get the bytes width of all the whitespace characters after that for c in snippet[offset..].chars().take_while(|c| c.is_whitespace()) { offset += c.len_utf8(); }

You're right, I was thinking about slicing on a char slice, not the original string. Will change to your proposed code.

ollie27 · 2018-01-23T22:00:25Z

src/libsyntax/codemap.rs

+                offset += c.len_utf8();
+            }
+            // get the bytes width of all the whitespace characters after that
+            for c in snippet[offset+1..].chars().take_while(|c| c.is_whitespace()) {


What is the +1 for?

Unless I'm missing something, !snippet[offset].is_whitespace(), right? Then snippet[offset+1].is_whitespace(). I think. Waiting for CI to finish in order to be 100% sure.

No, offset is pointing to the byte immediately after the last non whitespace character.

estebank · 2018-01-25T19:28:46Z

Addressed review comments.

I'm planning on having a follow up so that suggestions where the span is pointing at multiple lines only because of whitespace are shown on their own so that

warning: variable does not need to be mutable
  --> $DIR/suggestions.rs:52:13
   |
52 |            let mut
   |   _____________^
   |  |_____________|
   | ||
53 | ||             b = 1;
   | ||____________-^
   |  |____________|
   |               help: remove this `mut`

becomes

warning: variable does not need to be mutable
  --> $DIR/suggestions.rs:52:13
   |
52 |           let mut
   |  _____________^
53 | |             b = 1;
   | |_____________^
help: remove the `mut`:
   |
52 |           let b = 1;
   |               ^

estebank · 2018-01-29T18:45:19Z

r? @nikomatsakis

nikomatsakis · 2018-01-29T19:59:06Z

src/libsyntax/codemap.rs

+    /// the original `Span`.
+    ///
+    /// If `sp` points to `"let mut x"`, then a span pointing at `"let "` will be returned.
+    pub fn span_until_non_whitespace(&self, sp: Span) -> Span {


Nit: maybe we can implement this and span_through_char in terms of some common

fn span_until_predicate(&self, predicate: impl Fn(char) -> bool) -> Span { ... }

?

Oh, I see that the code is quite different. Never mind, not needed (yet).

nikomatsakis · 2018-01-29T20:00:22Z

src/test/ui/lint/suggestions.stderr

-46 |         let mut a = (1); // should suggest no `mut`, no parens
-   |             ---^^
+48 |         let mut a = (1); // should suggest no `mut`, no parens
+   |             ----^


doesn't it seem like this is underlining one character more than is needed?

oh, hmm, I guess the point is that you would have to delete this character? Still, I feel like when we display it here we ought to strip trailing whitespace from the underlined stuff =)

I'm pushing that to a follow up PR. When the span is a single line, it'd make sense to do so, but on a multiline span it'd be better to just show the suggestion on its own, as proposed in the comment above.

nikomatsakis · 2018-01-30T18:42:49Z

@bors r+

bors · 2018-01-30T18:42:50Z

📌 Commit 1affbd4 has been approved by nikomatsakis

bors · 2018-02-01T02:53:23Z

⌛ Testing commit 1affbd4fecf60759132f2fba509eb438840b4448 with merge e0bde94f0ac5379da54e5c29a3cf3989b43e06c1...

bors · 2018-02-01T04:47:43Z

💔 Test failed - status-appveyor

estebank · 2018-02-01T06:27:44Z

I find the error a bit bizarre:

Compiling style_traits v0.0.1 (file:///C:/projects/rust/build/ct/servo/components/style_traits)
thread 'rustc' panicked at 'byte index 13 is out of bounds of `MallocSizeOf`', libcore\str\mod.rs:2221:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: rustc 1.25.0-dev running on x86_64-pc-windows-msvc
error: Could not compile `style_traits`.
warning: build failed, waiting for other jobs to finish...
error: build failed
thread 'main' panicked at 'tests failed for https://github.com/servo/servo', tools\cargotest\main.rs:100:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
command did not execute successfully: "C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage0-tools-bin\\cargotest.exe" "C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage0/bin\\cargo.exe" "C:\\projects\\rust\\build\\ct"
expected success, got: exit code: 101
failed to run: C:\projects\rust\build\bootstrap\debug\bootstrap test src/tools/cargo src/tools/cargotest src/test/pretty src/test/run-pass/pretty src/test/run-fail/pretty src/test/run-pass-valgrind/pretty src/test/run-pass-fulldeps/pretty src/test/run-fail-fulldeps/pretty
Build completed unsuccessfully in 0:33:42
make: *** [Makefile:54: check-aux] Error 1
Command exited with code 2
cat %CD%\sccache.log || exit 0
WARN:sccache::cache::s3: Got AWS error: Error(BadHTTPStatus(NotFound), State { next_error: None })

@bors retry

ollie27 · 2018-02-01T12:32:23Z

src/libsyntax/codemap.rs

+    /// If `sp` points to `"let mut x"`, then a span pointing at `"let "` will be returned.
+    pub fn span_until_non_whitespace(&self, sp: Span) -> Span {
+        if let Ok(snippet) = self.span_to_snippet(sp) {
+            let mut offset = 1;


This should start at 0. I believe this is what caused CI to fail because if snippet is "MallocSizeOf" it will panic the same way: playground.

bors · 2018-02-01T13:25:17Z

⌛ Testing commit 1affbd4fecf60759132f2fba509eb438840b4448 with merge b4a099a5b09379e28b71f233f5de68c65f84e4c6...

bors · 2018-02-01T14:58:10Z

💔 Test failed - status-travis

kennytm · 2018-02-01T15:09:20Z

Failed again. Legit.

[01:27:46]    Compiling style_traits v0.0.1 (file:///checkout/obj/build/ct/servo/components/style_traits)
[01:27:48] thread 'rustc' panicked at 'byte index 13 is out of bounds of `MallocSizeOf`', libcore/str/mod.rs:2221:9
[01:27:48] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:27:48] 
[01:27:48] error: internal compiler error: unexpected panic
[01:27:48] 
[01:27:48] note: the compiler unexpectedly panicked. this is a bug.
[01:27:48] 
[01:27:48] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:27:48] 
[01:27:48] note: rustc 1.25.0-dev running on x86_64-unknown-linux-gnu
[01:27:48] 
[01:27:48] error: Could not compile `style_traits`.
[01:27:48] warning: build failed, waiting for other jobs to finish...
[01:27:48] error: build failed
[01:27:48] thread 'main' panicked at 'tests failed for https://github.com/servo/servo', tools/cargotest/main.rs:100:9
[01:27:48] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:27:48] 
[01:27:48] 
[01:27:48] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/cargotest" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/ct"
[01:27:48] expected success, got: exit code: 101

estebank · 2018-02-01T22:08:26Z

@bors r=nikomatsakis

bors · 2018-02-01T22:08:26Z

📌 Commit df412ce has been approved by nikomatsakis

bors · 2018-02-02T04:19:18Z

⌛ Testing commit df412ce with merge 616b66d...

Include space in suggestion `mut` in bindings Fix #46614.

bors · 2018-02-02T07:05:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 616b66d to master...

rust-highfive assigned michaelwoerister Jan 15, 2018

estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2018

oli-obk reviewed Jan 16, 2018

View reviewed changes

ollie27 reviewed Jan 16, 2018

View reviewed changes

killercup mentioned this pull request Jan 16, 2018

Broken suggestion for const_static_lifetime rust-lang/rust-clippy#2365

Closed

estebank 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 18, 2018

estebank force-pushed the include-space-after-mut branch from 284639e to d3c0701 Compare January 23, 2018 19:29

estebank 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 23, 2018

ollie27 reviewed Jan 23, 2018

View reviewed changes

estebank force-pushed the include-space-after-mut branch from e835bc2 to 1affbd4 Compare January 23, 2018 23:02

estebank added 3 commits January 26, 2018 14:24

Include space in suggestion mut in bindings

a8f77e1

Consider all whitespace when preparing span

d0bd090

review comment

fa7767e

rust-highfive assigned nikomatsakis and unassigned michaelwoerister Jan 29, 2018

nikomatsakis reviewed Jan 29, 2018

View reviewed changes

estebank 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 Jan 30, 2018

ollie27 reviewed Feb 1, 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2018

estebank force-pushed the include-space-after-mut branch from 1affbd4 to 79f24fe Compare February 1, 2018 20:02

Change offset to 0

df412ce

estebank force-pushed the include-space-after-mut branch from 79f24fe to df412ce Compare February 1, 2018 20:18

estebank 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2018

bors added a commit that referenced this pull request Feb 2, 2018

Auto merge of #47465 - estebank:include-space-after-mut, r=nikomatsakis

616b66d

Include space in suggestion `mut` in bindings Fix #46614.

bors merged commit df412ce into rust-lang:master Feb 2, 2018

Include space in suggestion mut in bindings #47465

Include space in suggestion mut in bindings #47465

Conversation

estebank commented Jan 15, 2018

rust-highfive commented Jan 15, 2018

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 Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

bors commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jan 25, 2018 • edited Loading

estebank commented Jan 29, 2018

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

nikomatsakis commented Jan 30, 2018

bors commented Jan 30, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

estebank commented Feb 1, 2018

Choose a reason for hiding this comment

bors commented Feb 1, 2018

bors commented Feb 1, 2018

kennytm commented Feb 1, 2018

estebank commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 2, 2018

bors commented Feb 2, 2018

Include space in suggestion `mut` in bindings #47465

Include space in suggestion `mut` in bindings #47465

estebank Jan 23, 2018 •

edited

Loading

estebank Jan 23, 2018 •

edited

Loading

estebank commented Jan 25, 2018 •

edited

Loading