map_entry: don't suggest when {e.insert(_); None } would be required

[ada4a]

Fixes #15307
Supersedes #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.¹

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

Footnotes

1. Unfortunately, I wasn't able to nicely ? out of these cases -- maybe try-blocks would've helped ↩

[Jarcho]

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.

[ada4a]

Yeah, I did think about that as well.. But this is what was suggested to me in #15759 (comment) (or at least how I understood that comment), so I'd like to hear @samueltardieu's thoughts?

[Jarcho]

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.

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

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.