Extend implicit_clone to handle to_string calls

[smoelius]

Put another way, merge string_to_string into implicit_clone, as suggested here: #14173 (comment)

Note: I wrote this comment:

rust-clippy/clippy_lints/src/methods/implicit_clone.rs

Lines 43 to 45 in 6cdb7f6

	/// Note that `to_string` is not flagged by `implicit_clone`. So other lints that call
	/// `is_clone_like` and that do flag `to_string` must handle it separately. See, e.g.,
	/// `is_to_owned_like` in `unnecessary_to_owned.rs`.

Here is the context for why I wrote it: #7978 (comment)

Regardless, it's probably time for the comment to go away. Extending implicit_clone to handle to_string calls yields many hits within Clippy's codebase.

changelog: extend implicit_clone to handle to_string calls

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[smoelius]

@y21 Sorry for the force pushes. I think it should be good now.

[y21]

Makes sense to me. I opened a thread on Zulip to give people a chance and some time to comment on this since this involves a deprecation

[samueltardieu]

nit: tests/ui/string_to_string.{rs,fixed,stderr} should probably be removed, this no longer corresponds to a lint, and is covered by the new tests/ui/implicit_clone.rs tests.

[smoelius]

Hmm. sting_to_string.stderr is hard-coded in .github/driver.sh:

rust-clippy/.github/driver.sh

Lines 48 to 52 in f83c94c

	# Run a lint and make sure it produces the expected output. It's also expected to exit with code 1
	# FIXME: How to match the clippy invocation in compile-test.rs?
	./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/string_to_string.rs 2>string_to_string.stderr && exit 1
	sed -e "/= help: for/d" string_to_string.stderr > normalized.stderr
	diff -u normalized.stderr tests/ui/string_to_string.stderr

[smoelius]

From what I can tell, driver.sh requires a stderr file with a certain structure. I picked the first one I could find that worked. That happened to be char_lit_as_u8.stderr.

[y21]

From what I can tell, driver.sh requires a stderr file with a certain structure. I picked the first one I could find that worked. That happened to be char_lit_as_u8.stderr.

Yeah, I think that should be fine. Looking at git blame for that line, looks like the lint name on that line has changed a couple of times before.

[y21]

Looks good, and the thread on Zulip got a bunch of positive reactions so we should be fine here with deprecating/merging the lint.

However, going to tag @flip1995 here for the change in .github/driver.sh. It looks correct to me, but just making sure because he's said he wants to be notified of any changes that affect CI.

[smoelius]

@y21 I really appreciate your efforts.

Unfortunately, I think this PR is presently unmergeable as a result of #14396.

The only path forward I can see to to migrate that PR's improvements to implicit_clone.

Do you agree?

[y21]

Yeah, detecting .map(String::to_string) (and I suppose .map(<_>::to_owned) as part of implicit_clone makes sense to me.

However, I don't think special-casing .map(|v| v.to_string()) to suggest .cloned() as is done in that PR is necessary (if I understood that correctly from the PR) -- this lint would already detect it and if it would just suggest .map(|v| v.clone()) as it naturally does without any special handling, the map_clone lint dedicated to looking for that pattern will suggest .cloned() in a subsequent clippy run.

[smoelius]

The only path forward I can see to to migrate that PR's improvements to implicit_clone.

Scratch that. I am undoing the following commits. I explain why below.

• Deprecate string_to_string
• Update clippy_lints/src/deprecated_lints.rs
• Remove tests/ui/string_to_string.{rs,fixed,stderr}

#14396 matches on both method calls and calls to type-relative paths. But the code that calls implicit_clone matches on just method calls.

I think it would be desirable to have implicit_clone work for calls to type-relative paths. But I think that would require a larger change to both implicit_clone and the code that calls it.

So string_to_string will continue to live on, but only for the functionality added in #14396.

I find the present situation unfortunate, but I guess it's what we've got.

[smoelius]

@y21 Could I impose on you to please try to merge this?

[y21]

One issue with this split as it is now is that we get two warnings with different suggestions on the same code, Some(String::new()).as_ref().map(|s| s.to_string());.

I still don't think that we really need the .to_string()-in-map specific suggestion that was added in the other PR, and if we just drop the ExprKind::MethodCall code in string_to_string (type-relative path would stay), that would resolve this issue. The map_clone/useless_asref lints can also emit more specialized warnings

[smoelius]

The map_clone/useless_asref lints can also emit more specialized warnings

Sorry, you mentioned map_clone in #14177 (comment) and I didn't catch it. I humbly think that extending map_clone is a brilliant idea.

So this PR is back to deprecating string_to_string.

A quick note: to avoid overlapping warnings between implict_clone and map_clone, I copied is_called_from_map_like and is_parent_map_like to implicit_clone, and call them here:

rust-clippy/clippy_lints/src/methods/implicit_clone.rs

Line 24 in 30577e2

&& is_called_from_map_like(cx, expr).is_none()

Sorry to give you more code to review.

[smoelius]

The force push was to make one comment slightly less verbose.

[smoelius]

Hey, @y21. Is anything holding this PR back?

[y21]

Sorry, you mentioned map_clone in #14177 (comment) and I didn't catch it. I humbly think that extending map_clone is a brilliant idea.

Is this actually needed? I'm still not entirely convinced that even moving the logic that was added in that PR elsewhere is worth it. My suggestion was to just deprecate the string_to_string lint as this PR did from the start and leave the extra logic that the other PR added behind, entirely, not moving it to/extending other lints.

My reasoning is that map_clone and string_to_string in combination already handle this today (at least from some quick testing, please correct me if I'm wrong), without us needing this extra handling.

Let's take this newly added test case from tests/ui/map_clone.rs as an example: let _ = variable2.map(|x| x.to_string());

If we just deprecate the lint entirely and move only the to_string -> clone detection to implicit_clone, then implicit_clone would change that to let _ = variable2.map(|x| x.clone());. If you then run clippy on it again, clippy will further refactor that to let _ = variable2.cloned(); (naturally by map_clone - again this is without any changes to it).

So even without this extra logic, we end up with the same ideal code, it just takes two clippy runs instead of one to get there, but IMHO this is a natural interaction between two lints (progressively refactoring it further). I think rustfix also handles this pretty well and applies multiple consecutive --fixes if new lints arise in suggested code

So, rephrasing, just to be clear, my suggestion would be:

• Deprecate string_to_string, removing all code from that lint, even what was added in that PR
• Lint .to_string() on String as part of implicit_clone

Which is essentially what the PR was doing from the start, I believe. WDYT? It would also simplify the PR a bit. If you do think that the extra logic from the PR to go to the ideal suggestion directly instead of having it naturally handled by the two lints is really worth keeping, I'm also ok accepting the changes you've proposed (I merely wanted to elaborate on what exactly I meant). It's also possible that my suggestion doesn't work because I've missed a subtle detail

[smoelius]

My suggestion was to just deprecate the string_to_string lint as this PR did from the start and leave the extra logic that the other PR added behind, entirely, not moving it to/extending other lints.

To be sure I understand, what you're suggesting would amount to reverting #14396. Is that right?

If so, I'm not sure my opinion matters. Should we involve the author and reviewers of #14396?