Skip to content

Lint suggesting use of TryFrom for checked integer conversion #3947

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
sfackler opened this issue Apr 12, 2019 · 1 comment
Open

Lint suggesting use of TryFrom for checked integer conversion #3947

sfackler opened this issue Apr 12, 2019 · 1 comment
Labels
A-lint Area: New lints L-complexity Lint: Belongs in the complexity lint group L-style Lint: Belongs in the style lint group

Comments

@sfackler
Copy link
Member

sfackler commented Apr 12, 2019

Checked integer conversion code has historically looked something like this:

if foo <= i32::max_value() as usize {
    thing_a
} else {
    thing_b
}

But now that TryFrom is stable, it can be done in a more foolproof way like this:

match i32::try_from(foo) {
    Ok(foo) => thing_a,
    Err(_) => thing_b,
}

It'd be cool if there was a lint guiding users to migrate their code.

@flip1995 flip1995 added L-style Lint: Belongs in the style lint group A-lint Area: New lints L-complexity Lint: Belongs in the complexity lint group labels Apr 12, 2019
pJunger added a commit to pJunger/rust-clippy that referenced this issue May 4, 2019
pJunger added a commit to pJunger/rust-clippy that referenced this issue May 12, 2019
pJunger added a commit to pJunger/rust-clippy that referenced this issue May 14, 2019
bors added a commit that referenced this issue May 17, 2019
Added lint for TryFrom for checked integer conversion.

works towards #3947

Added lint for try_from for checked integer conversion.
Should recognize simple & straight-forward checked integer conversions.
pJunger added a commit to pJunger/rust-clippy that referenced this issue May 18, 2019
bors added a commit that referenced this issue May 20, 2019
Added lint for TryFrom for checked integer conversion.

works towards #3947

Added lint for try_from for checked integer conversion.
Should recognize simple & straight-forward checked integer conversions.
@pJunger
Copy link
Contributor

pJunger commented May 21, 2019

So now that the simple check is merged, the next steps that I can see would be something like this:

  1. Further transformations

    1. Transform simple casts
    2. Transform more involved cases?
  2. Detect (probably) incorrect casts like (i < ITYPE::max_value()) && (i >= ITYPE::min_value()) ?

Regarding 1:

Simple transformations:

if (i <= ITYPE::max_value()) && (i >= ITYPE::min_value()) {
    ...(i as ITYPE)... i
}

or a transformation for the current rustfix solution

if ITYPE::try_from(i).is_ok()  {
    ...(i as ITYPE)... i
}

into

if let Some(i_as_ITYPE) = ITYPE::try_from(i) {
    ...(i_as_ITYPE)... i
}

Questions:

  • Is there a way to get all usages of i that are in scope?
  • How can you emit multiple related findings (that are resolved by rustfix in one step?)

Regarding 2:

Questions:

  • Do we want a lint that will emit false positives?
  • Or should code like this be transformed into something with more intent like:
match ITYPE::try_from(i) {
    Some(i_as_ITYPE) if i_as_ITYPE != ITYPE::max_value() => ...
    _ => {}

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints L-complexity Lint: Belongs in the complexity lint group L-style Lint: Belongs in the style lint group
Projects
None yet
Development

No branches or pull requests

3 participants