Skip to content

lint String::as_str() #874

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

Open
llogiq opened this issue Apr 23, 2016 · 9 comments
Open

lint String::as_str() #874

llogiq opened this issue Apr 23, 2016 · 9 comments
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types

Comments

@llogiq
Copy link
Contributor

llogiq commented Apr 23, 2016

Using my_string.as_str() is noisy and adds no value in comparison to &my_string. This lint should not apply to using it in a .map(_) or some other higher-order stuff, just calling the function directly.

@llogiq llogiq added good first issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types A-lint Area: New lints labels Apr 23, 2016
@mseri
Copy link

mseri commented Jul 11, 2016

I am giving a shot to this but I am lost at making sure the lint is not applied to higher-order functions. Is there a standard way to check if we are inside a closure in some higher order function or I need to make up one?

@llogiq
Copy link
Contributor Author

llogiq commented Jul 11, 2016

If the function is directly called (match on MethodCall with ident + 0th argument type), you can be quite sure it was called. Perhaps add a macro check for good measure.

@theemathas
Copy link
Contributor

What's the status of this?

@Manishearth
Copy link
Member

Feel free to work on it if you want to. (If someone has claimed an issue and not shown activity for months just feel free to claim it yourself)

@mseri
Copy link

mseri commented Dec 29, 2016

I was stuck on it and then had no time to try again. Feel free to work on it. It is outdated now, but this is what I had https://github.com/mseri/rust-clippy/commits/string_as_str (I was not able to distinguish being inside a higher order function or not, I did not push the few attempts I made)

@theemathas
Copy link
Contributor

I think it should be possible to just check if the method is immediately called instead of checking if it's inside a higher-order function. I'll implement it.

@theemathas
Copy link
Contributor

theemathas commented Dec 30, 2016

I think this lint should still trigger on something like slice.iter().any(|v| v.as_str() == "1"). When @llogiq said that "This lint should not apply to using it in a .map(_) or some other higher-order stuff", I think he meant something like slice.iter().any(String::as_str)

@theemathas
Copy link
Contributor

I just realized that |s: String| s.as_str() has different semantics from |s: String| &s. The former returns &str, while the latter returns &String. To make it return &str without using .as_str(), it has to be |s: String| &*s.

How do I deal with this case?

@camsteffen
Copy link
Contributor

I'd generalize this as coercible_deref_method - using a method that derefs the value where the deref could be coerced implicitly. This would also apply to Vec::as_slice for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants