Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint for &String::into(): String on final use of input string #61994

Open
pnkfelix opened this issue Jun 20, 2019 · 1 comment
Open

Lint for &String::into(): String on final use of input string #61994

pnkfelix opened this issue Jun 20, 2019 · 1 comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Jun 20, 2019

PR #59825 added a impl From<&String> for String.

While perusing the rustc source, I am now seeing instances of String values being constructed and then passed via-reference into a function that wants a String. These would not have compiled without the change added by PR #59825.

It might be worthwhile to try to make a lint that detects the last use of a String that is then being used as an argument to & which is then itself passed to this conversion method (which effectively hides the clone taking place.

(You can see discussion of the hidden clone on #59827; I'm not seeking debate on the merits of PR #59825. I just am wondering if we could easily highlight the unnecessary clones via a lint.)

@jonas-schievink jonas-schievink added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Jun 20, 2019
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 20, 2019
@awestlake87
Copy link

Yeah, I'm not sure I like that change. I always liked that potentially expensive operations like clone() were explicit and searchable. Also, I usually think of .into() and ::from as a move, not a copy, since it consumes the callee.

In C++, it's extremely easy to make copies accidentally and while impl From<&String> for String might be reasonably harmless, I feel like an impl <T: Clone> From<&T> for T kinda pushes Rust too far in that direction.

If that feature is here to stay, then I would probably always enable this lint as an error in my projects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants