Suggest removing `&`s #48834

ysiraichi · 2018-03-08T01:14:02Z

This implements the error message discussed in #47744.
We check whether removing each & yields a type that satisfies the requested obligation.
Also, it was created a new NodeId field in ObligationCause in order to iterate through the &s. The way it's implemented now, it iterates through the obligation snippet and counts the number of &.

r? @estebank

rust-highfive · 2018-03-08T01:14:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (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.

estebank · 2018-03-08T02:12:42Z

(I know this is WIP, I'll look at it later. Please add a couple of tests.)

[00:05:37] tidy error: /checkout/src/librustc_lint/builtin.rs:1026: line longer than 100 chars
[00:05:37] tidy error: /checkout/src/librustc_mir/borrow_check/nll/region_infer/mod.rs:1214: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc/traits/select.rs:3322: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc/infer/anon_types/mod.rs:632: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/check/mod.rs:4646: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/check/mod.rs:4861: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/check/compare_method.rs:220: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/check/method/confirm.rs:408: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/lib.rs:231: line longer than 100 chars
[00:05:38] tidy error: /checkout/src/librustc_typeck/lib.rs:283: line longer than 100 chars
[00:05:39] some tidy checks failed

estebank · 2018-03-08T21:49:52Z

src/librustc/traits/error_reporting.rs

+        for c in snippet.chars() {
+            if c == '&' {
+                refs_number += 1;
+            }


} else if !c.is_whitespace() { break; }

to avoid iterating over the entire snippet (which can be arbitrarily large) and to account for code like & & & x.

This can be written as

let refs_number = snippet.chars().take_while(|c| c == '&' || !c.is_whitespace()).filter(|c| c == '&').count();

Shouldn't it be:

let refs_number = snippet.chars().filter(|c| !c.is_whitespace()).take_while(|c| c == '&').count();

I made a mistake, you want either & or whitespace, my code gives you only &. Your code would indeed work.

estebank · 2018-03-08T21:52:40Z

src/librustc/traits/error_reporting.rs

+                    }
+                } else {
+                    break;
+                }


wrong indentation

estebank · 2018-03-08T21:55:46Z

src/librustc/traits/mod.rs

@@ -826,17 +829,23 @@ impl<'tcx,O> Obligation<'tcx,O> {
 impl<'tcx> ObligationCause<'tcx> {
    pub fn new(span: Span,
               body_id: ast::NodeId,
+               node_id: ast::NodeId,


I haven't found any call to new that didn't use the same thing for body_id and node_id. Did I miss something?

For now, I changed the code so that I could later fill node_id with the appropriate node (so that I wouldn't have to iterate the snippet).

By the way, are we going to leave these changes (adding node_id to ObligationCause) even if we are not really using it (yet)?

Given it'd be dead code, lets remove it and land this PR. We can make that change again once the node_id is actually used.

estebank · 2018-03-13T00:57:08Z

src/librustc/traits/error_reporting.rs

+
+                    if selcx.evaluate_obligation(&new_obligation) {
+                        let suggest_snippet = snippet.chars()
+                            .skip(refs_number)


This doesn't account for possible whitespace between the &: & & & x, for example.

Oh. You are right! I will fix it.

estebank · 2018-03-13T00:57:43Z

src/librustc/traits/error_reporting.rs

+                            .collect::<String>();
+
+                        err.span_suggestion(span,
+                                            "consider removing `&`s like",


This is a bit more vague than I'd like, can we teak the sentence depending on the amount of &s that should be removed?

How does "consider removing N references &", where N is the number of references to be removed?

estebank

Added some comments to handle some corner cases. Like it a lot and can't wait to see it merged!

estebank · 2018-03-13T16:29:20Z

src/librustc/traits/error_reporting.rs

+                        let suggest_snippet = snippet.chars()
+                            .filter(|c| !c.is_whitespace())
+                            .skip(remove_refs)
+                            .collect::<String>();


Wouldn't this turn something like & ( a, b ) into (a,b) or & & & & v .iter() .enumerate() into v.iter().enumerate()?

Yes... It would. That was what I was going for, though. Is it bad?

So, for things like tuples, arrays and slices, we would like to keep the current formatting. For things like very long chains of method calls split each on its own line (because having them all in the same line would be over 100 chars wide) we would also like to keep the formatting.

I think you should instead advance the iterator while the char is whitespace or &, and return the everything after that. That way you trim the beginning the way you want, but keep the rest of the code with the current formatting.

estebank · 2018-03-13T16:32:03Z

src/test/ui/suggest-remove-refs-2.rs

+fn main() {
+    let v = vec![0, 1, 2, 3];
+
+    for (i, n) in & & & & &v.iter().enumerate() {


Could you also add another test where the above uses multiple lines?

for (i, n) in & & & &v .iter() .enumerate() { println!("{} {}", i, n); }

bors · 2018-03-13T18:24:09Z

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

estebank · 2018-03-14T17:28:28Z

src/test/ui/suggest-remove-refs-3.stderr

+   |
+LL |     for (i, n) in v
+LL |         .iter()
+LL |         .enumerate() {


estebank · 2018-03-14T17:51:17Z

src/test/ui/suggest-remove-refs-2.stderr

+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                   |
+   |                   `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator; maybe try calling `.iter()` or a similar method
+   |                   help: consider removing 5 references `&`: `v.iter().enumerate()`


After travis passes, I'm happy to merge, but I think we could tweak this wording slightly to make it clearer.

@nikomatsakis, @Mark-Simulacrum are you ok with this wording, or would you prefer something like

consider removing 5 `&`-references

or

consider removing the 5 leading `&`

I think we definitely want the "references" in there, and I think we should remove v.iter().enumerate() from the help since it somewhat looks odd (since it's not what we're removing). Otherwise though I think this is good as-is.

What about using node_id from ObligationCause? I am still searching only for occurrences of & in the snippet.
In case we don't need to use node_id, I should remove it from the codebase. Don't you think?

@Mark-Simulacrum we could replace the span_suggestion with span_suggestion_short so that the code is not being shown in the label when displayed inline, the suggestion is displayed in full when it is shown on its own (like the multiline case above) and the tooling will be able to apply the correct output as it would now.

@ysiraichi would you mind doing that change? Once that is done, r=me.

Sure... Which one?

span_suggestion -> span_suggestion_short and

consider removing the 5 leading `&`-references

This line would end up looking like this:

LL | for (i, n) in & & & & &v.iter().enumerate() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator; maybe try calling `.iter()` or a similar method | help: consider removing the 5 leading `&`-references

If you had the extra time and felt it was worth it, it'd be great to change the suggestion's span to be

LL | for (i, n) in & & & & &v.iter().enumerate() { | ---------^^^^^^^^^^^^^^^^^^^^^ | | | `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator; maybe try calling `.iter()` or a similar method | help: consider removing the 5 leading `&`-references

so that the suggestion will have a different color. That being said, I won't block this PR on that. If do change the span, the the replacement content would be an empty String.

bors · 2018-03-15T02:19:33Z

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

ysiraichi · 2018-03-17T19:16:01Z

src/libsyntax/codemap.rs

+            sp
+        }
+    }
+


Not sure if I should have done this here or inlined it in the function suggest_remove_refs. It seemed right to do it here... I am considering two options now:

inline this function into the only place it's being used; or

use this function in functions like span_until_whitespace which may use it.

@estebank what do you think?

It's good to separate it, it makes the code above easier to follow.

The latter option sounds good to me if you don't mind doing it :)

estebank · 2018-03-17T21:44:15Z

src/libsyntax/codemap.rs

+            sp
+        }
+    }
+


It's good to separate it, it makes the code above easier to follow.

The latter option sounds good to me if you don't mind doing it :)

estebank · 2018-03-17T21:44:29Z

src/test/ui/suggest-remove-refs-2.stderr

+   |                   ---------^^^^^^^^^^^^^^^^^^^^
+   |                   |
+   |                   `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator; maybe try calling `.iter()` or a similar method
+   |                   help: consider removing 5 leading `&`-references


The output is great!

yeah, this is good

- `suggest_snippet` handling space between refs; - Suggest message changing according to the number of refs that should be removed.

Suggesting snippet without changing the original formatting of the code.

- `span_suggestion` changed to `span_suggestion_short`; - `Span` used changed to contain only `&` refs; - Tests passing.

- Using `span_take_while` to implement others.

estebank · 2018-03-19T21:04:51Z

@bors r+ rollup

bors · 2018-03-19T21:04:52Z

📌 Commit 736ba43 has been approved by estebank

@estebank

…ebank Suggest removing `&`s This implements the error message discussed in rust-lang#47744. We check whether removing each `&` yields a type that satisfies the requested obligation. Also, it was created a new `NodeId` field in `ObligationCause` in order to iterate through the `&`s. The way it's implemented now, it iterates through the obligation snippet and counts the number of `&`. r? @estebank

Rollup of 17 pull requests - Successful merges: #46518, #48810, #48834, #48902, #49004, #49092, #49096, #49099, #49104, #49125, #49139, #49152, #49157, #49161, #49166, #49176, #49184 - Failed merges:

rust-highfive assigned estebank Mar 8, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2018

estebank reviewed Mar 8, 2018

View reviewed changes

ysiraichi force-pushed the suggest-remove-ref branch from 188435c to ff9d9b3 Compare March 11, 2018 05:23

estebank reviewed Mar 13, 2018

View reviewed changes

ysiraichi force-pushed the suggest-remove-ref branch from 5b23275 to 5503560 Compare March 14, 2018 09:51

estebank reviewed Mar 14, 2018

View reviewed changes

src/test/ui/suggest-remove-refs-3.stderr Outdated

|

LL | for (i, n) in v

LL | .iter()

LL | .enumerate() {

Copy link

Contributor

estebank Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

estebank reviewed Mar 14, 2018

View reviewed changes

estebank changed the title ~~WIP: suggest removing &s~~ Suggest removing &s Mar 14, 2018

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 Mar 16, 2018

Add span_suggestion while removing TyRefs based on the snippet String.

fab7020

ysiraichi force-pushed the suggest-remove-ref branch from 46a06e9 to 311e9ac Compare March 17, 2018 18:54

ysiraichi commented Mar 17, 2018

View reviewed changes

estebank approved these changes Mar 17, 2018

View reviewed changes

ysiraichi added 10 commits March 18, 2018 20:46

Refactored with high-order functions.

4dd4506

Test added.

e0fb013

New test added.

f44b945

Review fixes.

97b66d2

- `suggest_snippet` handling space between refs; - Suggest message changing according to the number of refs that should be removed.

Keeping code formatting.

f41dc77

Suggesting snippet without changing the original formatting of the code.

Created multiple line test.

52cd07a

Rebased with master.

f6bffd1

Reporting with span_suggestion_short.

c1ba5ac

Review fixes.

74a4928

- `span_suggestion` changed to `span_suggestion_short`; - `Span` used changed to contain only `&` refs; - Tests passing.

CodeMap functions refactored.

0b36b20

- Using `span_take_while` to implement others.

Cleaned comments and extras s.

736ba43

ysiraichi force-pushed the suggest-remove-ref branch from c6e5ae8 to 736ba43 Compare March 19, 2018 07:16

bors 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 Mar 19, 2018

kennytm mentioned this pull request Mar 19, 2018

Rollup of 17 pull requests #49190

Merged

bors merged commit 736ba43 into rust-lang:master Mar 20, 2018

Suggest removing &s #48834

Suggest removing &s #48834

Conversation

ysiraichi commented Mar 8, 2018

rust-highfive commented Mar 8, 2018

estebank commented Mar 8, 2018 • edited Loading

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 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

bors commented Mar 13, 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 Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 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

estebank commented Mar 19, 2018

bors commented Mar 19, 2018

Suggest removing `&`s #48834

Suggest removing `&`s #48834

estebank commented Mar 8, 2018 •

edited

Loading

estebank Mar 14, 2018 •

edited

Loading