unnecessary_join lint

[yoav-lavi]

changelog: Adds a lint called [`unnecessary_join`] that detects cases of .collect::<Vec<String>>.join("") or .collect::<Vec<_>>.join("") on an iterator, suggesting .collect::<String>() instead

Fixes: #8570

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

[flip1995]

To get the type of the turbofish inside the collect call, I would just check the type of the expr .iter()...collect(). You may have to use expr_ty_adjusted() to get the actual type.

Looking at your implementation: You try to get the parent expression inside the check function. You should be able to get the iterator from the methods/mod.rs file. The iterator expression preceding the collect call should be _arg when matching to the "join" method name.

[yoav-lavi]

Thank you @flip1995 for the quick and helpful response! Will look into this soon, passing the iterator from mod.rs would definitely be simpler (and faster).

Any input on how I would check that join is receiving an empty &str / String?

Any comment on the validity of the lint itself? I opened the issue recently so I want to make sure that I'm not missing anything that would make this invalid.

I'm new to both the clippy codebase and Rust in general so any advice or input would be appreciated!

Thanks again!

[flip1995]

Any input on how I would check that join is receiving an empty &str / String?

For "" you can just match again a ExprKind::Lit and check if it is the empty string. I'm not sure if it is worth handling the other cases, because writing code like this is really questionable in the first place. A separate lint about using an owned empty string where also a "" would be possible might be better to put time in than trying to cover all cases in this lint.

Any comment on the validity of the lint itself? I opened the issue recently so I want to make sure that I'm not missing anything that would make this invalid.

This lint sounds reasonable to me. I can't really think of when you would want to use .join("") over directly collecting to a string. Maybe there is a case where you can collect into Vec<String> but not directly into String? This could be covered by checking for a FromIterator<[iterator-item-type]> implementation for String and only trigger the lint if there is one.

[yoav-lavi]

@flip1995 made some updates and added tests, seems to work correctly

[flip1995]

I will have to think about the case where directly collecting into a String might not be possible.

But until then, I left some comments.

[yoav-lavi]

I suggest also adding .DS_Store to the .gitignore file, can do this in this PR or in a new one if this is agreed upon

[flip1995]

The correct command is

@rustbot ready

[yoav-lavi]

Thanks @flip1995!

[flip1995]

Impl looks great!

[yoav-lavi]

@flip1995 I think that's everything we discussed, there's the issue of cases where directly collecting into a String might not be possible, but I'm not sure that's still an issue if this is in pedantic

[flip1995]

I think it is pretty much impossible to add a Join implementation that would break this lint: Playground. So this shouldn't be a problem.

[flip1995]

Thanks for addressing the review so quickly! Please squash (some of) your commits and then we can merge this PR.

[yoav-lavi]

Sorry closed by mistake, reopening