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 towards replacing from_str_radix with str::parse when the radix is 10 #6713

Closed
booleancoercion opened this issue Feb 10, 2021 · 6 comments · Fixed by #6717
Closed

Lint towards replacing from_str_radix with str::parse when the radix is 10 #6713

booleancoercion opened this issue Feb 10, 2021 · 6 comments · Fixed by #6717
Assignees
Labels
A-lint Area: New lints T-middle Type: Probably requires verifiying types

Comments

@booleancoercion
Copy link
Contributor

booleancoercion commented Feb 10, 2021

What it does

The lint suggests replacing e.g. u16::from_str_radix(&string, 10) with string.parse().
(Obviously, it could also be any other primitive with such associated function).

Categories (optional)

  • Kind: perhaps this could go in clippy::style.

What is the advantage of the recommended code over the original code?

The resulting code is shorter, easier to read and doesn't involve using 10 as a magic number.

Drawbacks

Using str::parse instead, the user might have to specify types in some cases.

Example

let input: &str = get_input();
let num = u16::from_str_radix(input, 10)?;

Could be written as:

let input: &str = get_input();
let num: u16 = input.parse()?;

(In a more complicated use case, the type of num will probably be inferred.)

@booleancoercion
Copy link
Contributor Author

If this lint makes sense, I'd like to try implementing it myself!

@xFrednet
Copy link
Member

xFrednet commented Feb 11, 2021

I believe this lint makes sense. We can easily change its category to clippy::pedantic if it's a lint that is seen to be to strict 🙃

clippy::pedantic lints which are rather strict or might have false positives allow

You can take this issue up and it seems like you've already found our Zulip. Could you comment @rustbot claim to assign the issue to you? This makes it clearer for others that someone is working on this. You can also unassign yourself at any point if you don't want to finish the lint.


And welcome to clippy 🙃

@xFrednet
Copy link
Member

@rustbot label +A-lint +T-middle

@rustbot rustbot added A-lint Area: New lints T-middle Type: Probably requires verifiying types labels Feb 11, 2021
@booleancoercion
Copy link
Contributor Author

Thanks! Yeah I'll assign it to myself.
Perhaps it could have false positives when someone uses a constant for the base that might be changed later I guess

@booleancoercion
Copy link
Contributor Author

@rustbot claim

@xFrednet
Copy link
Member

xFrednet commented Feb 11, 2021

Thank you 🙃 can also include the @rustbot in a lager comments. I've only put it extra because I forgot it at first 😅

Perhaps it could have false positives when someone uses a constant for the base

This should not be a problem if you only check for the literal 10. HIR references constant values using a path. This means that the lint would not see a constant like this u16::from_str_radix(input, 10)?; but rather as u16::from_str_radix(input, crate::VALUE_RADIX)?;. The actual optimizations and constant replacements are done after Clippy lints the code.

Feel free to ask here or on Zulip if you have any more questions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants