Skip to content
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.

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

Add convert_for_to_iter_for_each assist #7741

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

mattyhall
Copy link
Contributor

Implements one direction of #7681

I wonder if this tries to guess too much at the right thing here. A common pattern is:

let col = vec![1, 2, 3];
for v in &mut col {
  *v *= 2;
}
// equivalent to:
col.iter_mut().for_each(|v| *v *= 2);

I've tried to detect this case by checking if the expression after the in is a (mutable) reference and if not inserting iter()/iter_mut(). This is just a convention used in the stdlib however, so could sometimes be wrong. I'd be happy to make an improvement for this, but not sure what would be best. A few options spring to mind:

  1. Only allow this for types that are known to have iter/iter_mut (ie stdlib types)
  2. Try to check if iter/iter_mut exists and they return the right iterator type
  3. Don't try to do this and just add .into_iter() to whatever is after in

@Veykril
Copy link
Member

Veykril commented Feb 22, 2021

You'll have to rebase as the assists crate has been renamed to ide_assists

@Veykril
Copy link
Member

Veykril commented Feb 22, 2021

I think the best approach here is to just wrap the iterable in (_).into_iter() and special case it to iter{_mut} when we know those functions exist and are applicable.

Edit: Actually we should be able to check if an iter{_mut} function exists on a type with code_model::Type::iterate_method_candidates. That seems better than special casing.

* Move code to build replacement into closure
* Look for iter/iter_mut methods on types behind reference
@Veykril
Copy link
Member

Veykril commented Feb 24, 2021

2 more things aside from that it looks good to me 👍
bors d+

@bors
Copy link
Contributor

bors bot commented Feb 24, 2021

✌️ mattyhall can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

* Use known names for iter/iter_mut method (simplifies checking if the
  method exists
* Extract code to check assist with fixtures to function
@mattyhall
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 24, 2021

@bors bors bot merged commit dc14c43 into rust-lang:master Feb 24, 2021
@mattyhall mattyhall deleted the for_to_iter_foreach branch February 24, 2021 19:35
@Veykril
Copy link
Member

Veykril commented Feb 25, 2021

0FiZvMGsHL

@masklinn
Copy link

FWIW this seems to somewhat unhelpfully trigger on for loops using flow control (continue/break/return) yielding broken code. Is that known / intended?

I think in that case the assist either should not trigger, or it should use try_for_each.

@Veykril
Copy link
Member

Veykril commented Dec 23, 2021

That is not intended and was an oversight, could you open an issue for that?

@masklinn
Copy link

masklinn commented Dec 24, 2021

That is not intended and was an oversight, could you open an issue for that?

Gladly, sorry I wasted such a fun issue number as #11111 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants