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

coord: ranged1d: fix irrefutable if let warning #667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

10ne1
Copy link
Contributor

@10ne1 10ne1 commented Dec 21, 2024

I keep getting this annoying warning with rust 1.83:

warning: irrefutable if let pattern
--> plotters/src/coord/ranged1d/types/numeric.rs:29:20
|
29 | if let Ok(index) = Self::ValueType::try_from(index) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
365 | impl_discrete_trait!(RangedCoordusize);
| -------------------------------------- in this macro invocation
|
= note: this pattern will always match, so the if let is useless
= help: consider replacing the if let with a let
= note: #[warn(irrefutable_let_patterns)] on by default
= note: this warning originates in the macro impl_discrete_trait (in Nightly builds, run with -Z macro-backtrace for more info)

This is because the conversion of usize to ValueType always passes, so we don't need the try_from().

The problem with this operation is not the conversion which always succeeds, instead it is the potential overflows, so we use checked_add() which returns None if the addition fails.

@booti386
Copy link
Contributor

booti386 commented Mar 7, 2025

Thanks for your PR.
However, i will complete your analysis. The macro impl_discrete_trait that you patched is used several times at the bottom of the file, to implement DiscreteRanged for all the different RangedCoord* types.
The specific call that produces the warning that you're citing is impl_discrete_trait!(RangedCoordusize);, because we convert usize to usize, which always succeed. However it is not always true for the other calls, e.g. impl_discrete_trait!(RangedCoordu32); when usize is bigger than u32 on the target architecture.
So I suggest you modify your patch slightly (and don't bother adding me as a co-author, I don't care):

            fn from_index(&self, index: usize) -> Option<Self::ValueType> {
                Self::ValueType::try_from(index)
                    .ok()
                    .and_then(|index| self.0.checked_add(index))
            }

I keep getting this annoying warning with rust 1.83:

warning: irrefutable `if let` pattern
   --> plotters/src/coord/ranged1d/types/numeric.rs:29:20
    |
29  |                 if let Ok(index) = Self::ValueType::try_from(index) {
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
365 | impl_discrete_trait!(RangedCoordusize);
    | -------------------------------------- in this macro invocation
    |
    = note: this pattern will always match, so the `if let` is useless
    = help: consider replacing the `if let` with a `let`
    = note: `#[warn(irrefutable_let_patterns)]` on by default
    = note: this warning originates in the macro `impl_discrete_trait` (in Nightly builds, run with -Z macro-backtrace for more info)

This is because the conversion of usize to ValueType always
passes, so we don't need the try_from() in all cases.

An additional problem are potential overflows so we use
checked_add() which returns None if the addition fails,
thus also avoiding the irrefutable let pattern while also
protecting against overflows.

Signed-off-by: Adrian Ratiu <[email protected]>
@10ne1 10ne1 force-pushed the dev/aratiu/fix-irrefutable-let-warn branch from 43990c4 to dd806bd Compare March 14, 2025 11:39
@10ne1
Copy link
Contributor Author

10ne1 commented Mar 14, 2025

Thanks @booti386 I've updated the PR with your suggestion.

@AaronErhardt Can we please land this? It's been waiting for some while now. The failing checks seem to be unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants