Skip to content

Suggest removing &s #48834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bors merged 12 commits into from
Mar 20, 2018
Merged

Suggest removing &s #48834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fab7020
Add span_suggestion while removing TyRefs based on the snippet String.
ysiraichi Feb 9, 2018
4dd4506
Refactored with high-order functions.
ysiraichi Mar 11, 2018
e0fb013
Test added.
ysiraichi Mar 13, 2018
f44b945
New test added.
ysiraichi Mar 13, 2018
97b66d2
Review fixes.
ysiraichi Mar 13, 2018
f41dc77
Keeping code formatting.
ysiraichi Mar 14, 2018
52cd07a
Created multiple line test.
ysiraichi Mar 14, 2018
f6bffd1
Rebased with master.
ysiraichi Mar 14, 2018
c1ba5ac
Reporting with `span_suggestion_short`.
ysiraichi Mar 16, 2018
74a4928
Review fixes.
ysiraichi Mar 17, 2018
0b36b20
CodeMap functions refactored.
ysiraichi Mar 18, 2018
736ba43
Cleaned comments and extras s.
ysiraichi Mar 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
New test added.
  • Loading branch information
@ysiraichi
ysiraichi committed Mar 18, 2018
commit f44b945e0ed73c6d108a40655d3bed14133ec7db
0 src/test/ui/suggest-remove-refs.rs → src/test/ui/suggest-remove-refs-1.rs
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions src/test/ui/suggest-remove-refs.stderr → src/test/ui/suggest-remove-refs-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error[E0277]: the trait bound `&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>: std::iter::Iterator` is not satisfied
--> $DIR/suggest-remove-refs.rs:14:19
--> $DIR/suggest-remove-refs-1.rs:14:19
|
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 `&`s like: `v.iter().enumerate()`
| help: consider removing 1 references `&`: `v.iter().enumerate()`
|
= help: the trait `std::iter::Iterator` is not implemented for `&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>`
= note: required by `std::iter::IntoIterator::into_iter`
Expand Down
18 changes: 18 additions & 0 deletions src/test/ui/suggest-remove-refs-2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <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.

fn main() {
let v = vec![0, 1, 2, 3];

for (i, n) in & & & & &v.iter().enumerate() {
Copy link
Contributor

@estebank estebank Mar 13, 2018

Choose a reason for hiding this comment

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

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

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

//~^ ERROR the trait bound
println!("{}", i);
}
}
15 changes: 15 additions & 0 deletions src/test/ui/suggest-remove-refs-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0277]: the trait bound `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>: std::iter::Iterator` is not satisfied
--> $DIR/suggest-remove-refs-2.rs:14:19
|
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 5 references `&`: `v.iter().enumerate()`
Copy link
Contributor

@estebank estebank Mar 14, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Mar 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ysiraichi ysiraichi Mar 14, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@estebank estebank Mar 14, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ysiraichi ysiraichi Mar 14, 2018

Choose a reason for hiding this comment

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

Sure... Which one?

Copy link
Contributor

@estebank estebank Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

|
= help: the trait `std::iter::Iterator` is not implemented for `&&&&&std::iter::Enumerate<std::slice::Iter<'_, {integer}>>`
= note: required by `std::iter::IntoIterator::into_iter`

error: aborting due to previous error

If you want more information on this error, try using "rustc --explain E0277"