rust-lang
`map_entry`: don't suggest when `{e.insert(_); None }` would be required
Fixes https://github.com/rust-lang/rust-clippy/issues/15307 Supersedes https://github.com/rust-lang/rust-clippy/pull/15759
Unfortuntaly, avoiding the ugliness in the suggestion seems to require adding a bit of ugliness to the lint emission logic:
Previously, both InsertSearchResult::snippet_{occupied,vacant} called InsertionSearchResult::snippet for the common bit of logic, but also provided write_wrapped, a special closure for the case where the insert-to-be-replaced was a final expression -- in that closure, InsertSearchResult::snippet_occupied just wrapped the insert call in Some, which was okay, but InsertSearchResult::snippet_vacant created { e.insert(_); None::</* value type */> } -- and that was deemed to be too ugly.
So the way I solved this was by making the closure return an Option<String>, with None returned when the required replacement would be too ugly, and returning None from InsertSearchResult::snippet if that case was encountered. Then, when constructing the sugg, I would make it None whenever a call to InsertSearchResult::snippet_vacant returned a None.[^1]
[^1]: Unfortunately, I wasn't able to nicely ? out of these cases -- maybe try-blocks would've helped
Diff (especially for the second commit) best viewed with whitespace ignored.
changelog: [map_entry]: don't suggest when {e.insert(_); None } would be required
r? @samueltardieu
This just needs to mark the suggestion as MaybeIncorrect. Removing the suggestion means that you're hiding how to convert the code to use the entry API.
Yeah, I did think about that as well.. But this is what was suggested to me in https://github.com/rust-lang/rust-clippy/pull/15759#discussion_r2387731050 (or at least how I understood that comment), so I'd like to hear @samueltardieu's thoughts?
If the suggestion were solely the {insert; None} part I would agree with not even bothering, but the more important part of the suggestion is what incantation or the entry API to use. The None part can almost always be removed afterwards manually.
This just needs to mark the suggestion as
MaybeIncorrect. Removing the suggestion means that you're hiding how to convert the code to use the entry API.
Suggesting None even if it is later removed is still a bad recommendation. The user should probably never have relied on the fact that insert() was returning None in the first place. I am really reluctant to just suggest quasi-nonsensical code just to satisfy some typing constraints and for the sake of suggesting a fix. Warning is what is most important in the first place, we should IMO emit fixes only when it makes sense (for example, fixing an operation into .is_multiple_of() is almost certainly always a good idea, here I don't think we should encourage keeping the None if it in addition requires adding a type and making things even uglier).
We could give the URL of the entry API in this case, which should contain enough examples for the user to figure it out.
Linking to the documentation does not immediately tell you how to use it. Clippy telling you exactly which utterance of it does and that alone is useful. The body with the insert could be hidden since it's not the most useful for display purposes.
Making the whole suggestion is still useful for tools like rust-analyzer since it allows them to apply the transformation. The fact that it needs to be fixed up afterwards by hand doesn't matter since even getting most of the way is providing value here.
:umbrella: The latest upstream changes (possibly 5a37f7bb280e0b0d57a6a16e845893f5b86c873f) made this pull request unmergeable. Please resolve the merge conflicts.
