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

Add lint for misstyped literal casting #3129

Merged
merged 3 commits into from Sep 7, 2018

Conversation

Projects
None yet
4 participants
@mipli
Contributor

mipli commented Sep 4, 2018

Adds a mistyped_literal_suffixes lint, to check if someone forgot to type i or u in a literal number suffix. For instance, wrote 3_32 instead of 3_u32.

Found no way good way to test if a literal number was negative, so the lint still suggest using _u32 even if the number is negative.

Solves #3091

@mipli mipli force-pushed the mipli:3091-numeric-typo branch from 20fec19 to 8ca008f Sep 4, 2018

@flip1995

Thanks for the contribution! Some changes need to be made.

fn do_lint(digits: &str) -> Result<usize, WarningType> {
fn do_lint(digits: &str, suffix: Option<&str>) -> Result<usize, WarningType> {
if let Some(suffix) = suffix {
if ["_8", "_32", "_64"].contains(&suffix) {

This comment has been minimized.

@flip1995

flip1995 Sep 4, 2018

Collaborator

_16 and _128?

Show resolved Hide resolved tests/ui/literals.rs
@@ -211,11 +232,18 @@ impl<'a> DigitInfo<'a> {
if self.radix == Radix::Hexadecimal && nb_digits_to_fill != 0 {
hint = format!("{:0>4}{}", &hint[..nb_digits_to_fill], &hint[nb_digits_to_fill..]);
}
let suffix_hint = match self.suffix {
Some(suffix) if ["_8", "_32", "_64"].contains(&suffix) => {
format!("_i{}` or `{}_u{}", &suffix[1..], hint, &suffix[1..])

This comment has been minimized.

@flip1995

flip1995 Sep 4, 2018

Collaborator

format!("_i{suffix} or {hint}_u{suffix}", suffix=&suffix[1..], hint=hint)
And I think the hint is missing infront of _i?

I just checked where the grouping_hint function is used:
https://github.com/rust-lang-nursery/rust-clippy/blob/c81d70e6bdf264c62136b571760f152be9638ec0/clippy_lints/src/literal_representation.rs#L234-L241
It is used in a span_lint_and_sugg function, which means, that this fix could be applied by rustfix automatically.

With this suggestion 1_32 would be changed to 1_i32 or 1_u32 by rustfix, which is not correct rust syntax. I think just suggesting the singed _i{} variant is fine here. Maybe a check could be added whether the number in front of _ is greater than i<whatever>::max_value() and suggest the u variant in that case. But this is optional and would get caught by the overflowing_literals lint of rustc anyway.

This comment has been minimized.

@mipli

mipli Sep 5, 2018

Contributor

Didn't know that rustfix used these suggestions like that.

Do we want this lint to be automatically applied? With support for suggesting _i128 instead of _128 we can end up with false positives, since _128 can be part of a properly formatted literal.

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

Oh your right. 256_128 is a valid formatting for a int literal and would even be suggested by one of those lints for 256128, which would then be linted to change it to 256_i128. I have no idea how to handle this. We can't really know what to do with _128 and I would suggest to just bail out and write this in the Known Problems section.

If you come up with a good idea, you can add it of course! If not just leave out the _128 case.

@@ -211,11 +232,18 @@ impl<'a> DigitInfo<'a> {
if self.radix == Radix::Hexadecimal && nb_digits_to_fill != 0 {
hint = format!("{:0>4}{}", &hint[..nb_digits_to_fill], &hint[nb_digits_to_fill..]);
}
let suffix_hint = match self.suffix {
Some(suffix) if ["_8", "_32", "_64"].contains(&suffix) => {

This comment has been minimized.

@flip1995

flip1995 Sep 4, 2018

Collaborator

what's with _16 and _128?

if !float && (d == 'i' || d == 'u') || float && (d == 'f' || d == 'e' || d == 'E') {
if !float && (d == 'i' || d == 'u') ||
float && (d == 'f' || d == 'e' || d == 'E') ||
(!float && d == '_' && (d_idx == len -3 || d_idx == len - 2)) {

This comment has been minimized.

@flip1995

flip1995 Sep 4, 2018

Collaborator

Uh with this check also _42 would get detected as a valid suffix. I think this breaks the grouping hint for numbers like 12334_42, which should be 1_233_442, but with this change it will be 12_334_42. This is the case with every number that ends in _X or _YZ. (Please also add test(s) for this)

@mipli

This comment has been minimized.

Contributor

mipli commented Sep 5, 2018

Thanks for the feedback, I'll get working on fixing those things!

@mipli mipli force-pushed the mipli:3091-numeric-typo branch from 8ca008f to 8e464b6 Sep 5, 2018

@@ -504,3 +551,11 @@ impl LiteralRepresentation {
Ok(())
}
}
fn is_mistyped_suffix(suffix: &str) -> bool {
["_8", "_16", "_32", "_64", "_128"].contains(&suffix)

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

NIT: indentation, please use 4 spaces.

This comment has been minimized.

@mati865

mati865 Sep 5, 2018

Contributor

I think calling cargo fmt --all should fix it

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

Yeah probably, but please make sure to not add unrelated formatted files.

This comment has been minimized.

@mipli

mipli Sep 5, 2018

Contributor

I'll check the files I've touched to make sure they're all properly transformed. Unsure why it happened, my editor should have read the .editorconfig and used the settings there.

if !float && (d == 'i' || d == 'u') || float && (d == 'f' || d == 'e' || d == 'E') {
if !float && (d == 'i' || d == 'u') ||
float && (d == 'f' || d == 'e' || d == 'E') ||
(!float && is_possible_suffix_index(&d, d_idx, len)) {
let suffix_start = if last_d == '_' { d_idx - 1 } else { d_idx };

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

Another interesting test case: 1__8. The suggestion should be 1_i8, but I think that it will be something different.

I think is_possible_suffix_index needs to check directly if the possible suffix is "_8", "_16", ... and not just if the length matches.

This comment has been minimized.

@mipli

mipli Sep 5, 2018

Contributor

I can probably add tests for 2__8 as well if we want to have lints for that as well. Don't think it will add too much complexity to this.

This comment has been minimized.

@mipli

mipli Sep 5, 2018

Contributor

I think is_possible_suffix_index needs to check directly if the possible suffix is "_8", "_16", ... and not just if the length matches.

The check for those specific values are done using the is_mistyped_suffix function. So cases were the index function matches on e.g _23 are discarded later on and the loop continues to check the next index.

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

Yes please add a test for this. 2__8 gets currently linted (consider: 28). But should be 2_i8 in the future, while 2__9 should stay consider: 29

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

e.g _23 are discarded later on and the loop continues to check the next index.

Yes but I think that will fail on __8.

This comment has been minimized.

@mipli

mipli Sep 5, 2018

Contributor

It still returns consider: 28 when you write 2__8, so I think it does work as expected today. But that doesn't really matter now, since I'll be updating the handling of cases with double underscores and adding a few different test cases for those.

I'll add some extra tests for other strange uses of multiple underscores as well, just to document how we handle those.

/// **Why is this bad?** This is most probably a typo
///
/// **Known problems:**
/// - Recommends unsigned suffix even if it's a negative number. Should only recommend the signed suffix on those cases.

This comment has been minimized.

@flip1995

flip1995 Sep 5, 2018

Collaborator

That is not true anymore. Maybe "always suggests singed integer", but I don't think this is really a problem.

@mipli mipli force-pushed the mipli:3091-numeric-typo branch from 8e464b6 to 8bf219d Sep 5, 2018

@mipli

This comment has been minimized.

Contributor

mipli commented Sep 5, 2018

Fixed the things mentioned above, and added test cases for some cases with strange use of underscores.

Rewrote it a bit as well to make the code easier to read. Only issue I see with the rewrite is that we now do a .split_at for each character in the literal (up until the point where it finds an issue), rather than only when we suspect it might be an issue. Considering the speed of the .split_at function I don't think this is a problem, but it's worth being aware of the change.

@mipli mipli force-pushed the mipli:3091-numeric-typo branch from 8bf219d to 38d287f Sep 5, 2018

@flip1995

This comment has been minimized.

Collaborator

flip1995 commented Sep 6, 2018

The suggestions are looking good now!

You could prevent the problem with calling the split_at method at every digit by combining your previous and current solution:

if ... ||
    !float && is_possible_suffix_index(..) {
...
fn is_possible_suffix_index(..) -> bool {
    d == "_" && <length-check> && is_misstyped_suffix(split_at());
}

Thanks to lazy evaluation the split_at method only gets called on the last 2-3 digits.

Also feel free to add as many commits to a PR as you like, you don't have to keep it at one. If necessary, they can be squashed before merging and it is easier for you and for the reviewer to see what changed since opening the PR.

};
if !float && (d == 'i' || d == 'u') ||
float && (d == 'f' || d == 'e' || d == 'E') ||
!float && is_possible_suffix_index(&sans_prefix, suffix_start, len) {

This comment has been minimized.

@flip1995

flip1995 Sep 6, 2018

Collaborator

cargo clippy --all-targets --all-features -- -D clippy::all fails on this:

if !float && A || !float && B

can be simplified to

if !float && (A || B)

https://travis-ci.org/rust-lang-nursery/rust-clippy/jobs/425293764#L1349-L1365

Everything else LGTM! I'm ready to merge this, after this is fixed.

Unrelated: I think the suggestion of the nonminimal-bool lint is wrong or at least way to complicated 🤔 (update: it's right, I just tested it)

This comment has been minimized.

@mipli

mipli Sep 6, 2018

Contributor

The suggested solutions were very complicated I think, but I found another way to simplify the expression that still makes sense to me at least.

This comment has been minimized.

@oli-obk

oli-obk Sep 6, 2018

Collaborator

Yea some parens seem to have gotten lost

This comment has been minimized.

@oli-obk

oli-obk Sep 6, 2018

Collaborator

We might be able to tune the minimization algorithm. I'll open an issue

@mipli

This comment has been minimized.

Contributor

mipli commented Sep 7, 2018

Is anything else required from me for this to be merged now?

The AppVeyor build fails, but I'm not sure why it fails. On my local computer everything passes, so I can't properly debug that issue either.

@oli-obk oli-obk merged commit 63a46b1 into rust-lang:master Sep 7, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Sep 7, 2018

Appveyor is in flux right now, don't worry about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment