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.