Skip to content

If inside for_each to filter() code assist #11665

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from

Conversation

AppCoder1234
Copy link

Hi everyone,
One of my first MR so I'm not really experienced yet in that thing.
I've been coding a code assist which could "lambdify" for statements in Rust. At this point, it replaces if inside .for_each() closures with the corresponding .filter().
I could go even further with more asists (ex. sums) but I wanted to have feedback first on the code.

At the moment I still have one problem: I copied the validate_method_call_expr from modify_for_each.rs but needed to remove this part:

    let it_type = sema.type_of_expr(&receiver)?.adjusted();
    let module = sema.scope(receiver.syntax()).module()?;
    let krate = module.krate();

    let iter_trait = FamousDefs(sema, Some(krate)).core_iter_Iterator()?;
    it_type.impls_trait(sema.db, iter_trait, &[]).then(|| (expr, receiver))

for the code assist to work (without me understanding why).

I created a test case which works (which the small tweak mentioned above)

Any comments welcome!

Detect simple cases where if can be transformed into filter
Recursively remove mut idents in filter closure
Apply code assist only when cursor is well positioned
@bors
Copy link
Contributor

bors commented May 1, 2022

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

@Veykril Veykril self-assigned this Jun 21, 2022
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

I'm very sorry for not having gotten to this sooner, this kind of fell off of my radar due to me being unsure about the usability of it. It seems rather rigid as it expects a very specific pattern of code.

Comment on lines +20 to +24
// it.for_each$0(|x| {
// if x > 4 {
// println!("{}", x);
// };
// });
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to make this assist applicable if the cursor is on the if x > 4 { part (note not the body of the if, only the condition). That would feel more intuitive to me.

)
}

// Assist: convert_sum_call
Copy link
Member

Choose a reason for hiding this comment

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

I am unsure whether this assist has any value really, I don't see when someone would end up with a for_each exactly like this.

)
}

// Assist: convert_all_call
Copy link
Member

Choose a reason for hiding this comment

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

similar as to the sum one, it is unlikely that you'll have a for each exactly as the one here required

let (method, receiver) = validate_method_call_expr(ctx, method)?;

let param_list = closure.param_list()?;
let param = param_list.params().next()?.pat()?;
Copy link
Member

Choose a reason for hiding this comment

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

What about cases where not only the first parameter is used for the early return?

Comment on lines +58 to +60
let mut stmts = block.statements();
let fst_stmt = stmts.next()?;
continue_iff(stmts.next().is_none())?; // Only one statement
Copy link
Member

Choose a reason for hiding this comment

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

This assist doesn't seem too useful if it only applies when block consists of a single if expression

@AppCoder1234
Copy link
Author

First of all, thanks for reviewing it!

To give you more context about these assists, they stemmed from a spinoff from a project I had which was about Java code refactoring to make use of the new Java 8 Streams, and based on the findings of this article: https://dl.acm.org/doi/pdf/10.1145/3133909 (and feedback about how useful it could be in Java). The idea behind the Java counterpart of these assists was that we might split off the refactoring of a foreach into several small steps that would lean towards a functional version of it. So the user wouldn't necessarily have put a sum += i into a for_each(), but maybe he wrote an imperative version of the sum with something like

for (Integer i : list) {
    sum += i;
}

which would first be refactored into the corresponding for_each(), and then finally into a sum(), with two different refactoring processors in the process.

I got interested in how it was done in Rust via code assists and it seemed to me that although there was the for_each code assist, there was not anything that would do the sum() final refactoring process that I mentioned above (correct me if I'm wrong). So that's where this MR comes from. There's a lot of others cases that might fall into this pattern, but I wasn't able to cover them all but thought this could nonetheless prove to be useful and possibly be extended further.

Anyway, I'll check your comments; it's been a while since I delved into that crate so might need some refreshing before.

(and kudos to you if you read that far)

@Veykril Veykril removed their assignment Aug 31, 2022
@Veykril Veykril closed this Sep 20, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants