Extend from_str_radix_10 to NonZero types by SomaFon · Pull Request #17715 · rust-lang/rust-clippy

SomaFon · 2026-09-10T01:23:08Z

closes #17712

The from_str_radix_10 lint now covers NonZero* types, which gained from_str_radix in Rust 1.98.

Uses is_diag_item(sym::NonZero) for detection and guards the suggestion with MSRV 1.98.

changelog: [from_str_radix_10]: now lints on NonZero* types

rustbot · 2026-09-10T01:23:12Z

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

CommanderStorm

LGTM assuming that you move the test down and add an comment why this is MaybeIncorrect

View changes since this review

CommanderStorm · 2026-09-10T06:36:30Z


+    NonZeroU16::from_str_radix("8", 10)?;
+    //~^ from_str_radix_10
+
+    std::num::NonZeroI32::from_str_radix("42", 10)?;
+    //~^ from_str_radix_10
+
+    MyNonZero::from_str_radix("11", 10)?;
+    //~^ from_str_radix_10


Please move to fn issue_XYZ like the rest.
fn main is for the initial impl.

Done in 8084d7a. I moved the NonZero cases into issue_17712; main remains for the initial implementation.

CommanderStorm · 2026-09-10T07:58:12Z

+                    ty.span,
+                    exp.span.ctxt(),
+                    "<integer>",
+                    &mut Applicability::MaybeIncorrect,


could you add an comment why this may be incorrect? this is otherwise a bit confusing for users

Done in 167820b. I added a comment explaining why this suggestion remains MaybeIncorrect.

CommanderStorm · 2026-09-10T08:00:15Z

-            && let def::Res::PrimTy(prim_ty) = ty_res
-            && matches!(prim_ty, PrimTy::Int(_) | PrimTy::Uint(_))
+            && let Some(integer_ty) = get_integer_ty(cx, ty, ty_qpath)
+            && (!integer_ty.is_nonzero() || self.msrv.meets(cx, msrvs::NONZERO_FROM_STR_RADIX))


How about we inline this? The negation made me do a bit of an double take.

Suggested change

&& (!integer_ty.is_nonzero() || self.msrv.meets(cx, msrvs::NONZERO_FROM_STR_RADIX))

&& (matches!(integer_ty, IntegerTy::Primitive(_)) || self.msrv.meets(cx, msrvs::NONZERO_FROM_STR_RADIX))

Done in 8084d7a. I inlined the MSRV check as suggested.

SomaFon · 2026-09-11T01:28:45Z

Thanks for the review. I addressed all three comments, and all CI checks are passing. Could you take another look?

CommanderStorm

I guess LGTM.
I still don't quite understand why you marked this MaybeIncorrect, but I don't want to be a blocker to you.

Make sure to squash your changes though.

View changes since this review

CommanderStorm · 2026-09-11T20:57:44Z

+                    // Preserve aliases and paths in the suggestion. This makes it impossible
+                    // to guarantee that the replacement compiles in every context.
+                    snippet_with_context(
+                        cx,
+                        ty.span,
+                        exp.span.ctxt(),
+                        "<integer>",
+                        &mut Applicability::MaybeIncorrect,


I still don't quite understand what kind of code you are concerned about exactly tbh...
Could you given an code-snippet of what you are concearned about?

The reason I am questioning this is to know why here and not in the IntegerTy::Primitive case too?

You're right — the comment was misleading. MaybeIncorrect is used for the final suggestion for both primitive and NonZero types; it isn't specific to the source snippet.

The snippet is only used for NonZero so aliases and qualified paths are preserved, while primitives use PrimTy::name_str(). I've changed the snippet lookup to use Applicability::Unspecified and shortened the comment.

Shortenign the comment to Preserve aliases and paths in the suggestion does not tell me WHY you did this.
WHich aliases and paths?

No, actually tell me which aliases and paths you are preserving.
Give me an syntax example (here on github is fine) so that we have this kind of shared understanding.

Also, why Applicability::Unspecified? I don't really think this makes sense, or?

rustbot · 2026-09-12T05:08:50Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

CommanderStorm

Community Review:
Actually, I think the MaybeIncorrect is a mistake or at least somethign that needs to be better explained why it is this way.
I am specifically asking because this seems to work as expected and prints

mod shadowed {
    pub type NonZeroU8 = std::num::NonZeroU16;
}
use shadowed::NonZeroU8;

fn similarly_named_type() {    
    let _ = NonZeroU8::from_str_radix("300", 10);
    //~^ from_str_radix_10 -> "300".parse::<NonZeroU8>()

    let _ = shadowed::NonZeroU8::from_str_radix("300", 10);
    //~^ from_str_radix_10 -> "300".parse::<shadowed::NonZeroU8>()

    let _ = std::num::NonZeroU8::from_str_radix("30", 10);
    //~^ from_str_radix_10 -> "30".parse::<std::num::NonZeroU8>(
}

Where you are semi-right (I don't think you meant this) is that there is an bug that macro generated code is not properly ignored.
So please add the respective guard to the span if it is from an expansion.
That can arguably be a differnt PR though given that the existing lint also does not guard against this, so you don't have to do this if you don't want 😉..

macro_rules! parse_radix_10 {
    ($t:ty, $s:expr) => {
        <$t>::from_str_radix($s, 10)
    };
}

fn macro_argument_type() {
    let _ = parse_radix_10!(std::num::NonZeroU32, "9");
}

generates the follwing:

error: this call to `from_str_radix` can be replaced with a call to `str::parse`
  --> tests/ui/from_str_radix_10.rs:134:9
   |
LL |         <$t>::from_str_radix($s, 10)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"9".parse::<std::num::NonZeroU32>()`
...
LL |     let _ = parse_radix_10!(std::num::NonZeroU32, "9");
   |             ------------------------------------------ in this macro invocation
   |
   = note: this error originates in the macro `parse_radix_10` (in Nightly builds, run with -Z macro-backtrace for more info)

View changes since this review

CommanderStorm

Community review: LGTM, thanks

View changes since this review

CommanderStorm

Community review: I already aproved above, please see #17715 (comment)

View changes since this review

hashcatHitman

Looks good for the most part, but I have some concerns about the test coverage.

View changes since this review

hashcatHitman · 2026-09-17T19:31:26Z

If you add these test cases:

use std::num::NonZero; trait FakeFromStr: Sized { fn from_str_radix(src: &str, radix: u32) -> Result<Self, std::num::ParseIntError>; } impl FakeFromStr for char { fn from_str_radix(src: &str, radix: u32) -> Result<Self, std::num::ParseIntError> { Ok('a') } } impl FakeFromStr for NonZero<char> { fn from_str_radix(src: &str, radix: u32) -> Result<Self, std::num::ParseIntError> { Ok(NonZero::new('a').unwrap()) } } fn char_cannot_str_from_radix() -> Result<(), Box<dyn std::error::Error>> { char::from_str_radix("1", 10)?; NonZero::<char>::from_str_radix("1", 10)?; }

The original lint correctly ignores char, but NonZero<char> is incorrectly flagged by the updated lint.

Good catch. I restricted the NonZero check to cases where its type parameter is an integer, and added this as a regression test. NonZero<char> is no longer linted.

hashcatHitman · 2026-09-17T19:34:36Z

+    }
+
+    NonZeroU16::from_str_radix("8", 10)?;


All your tests are for type aliases, it'd be nice to also test a NonZero that isn't a type alias. (You'll need to import std::num::NonZero)

Suggested change

}

NonZeroU16::from_str_radix("8", 10)?;

}

NonZero::<usize>::from_str_radix("16", 10)?;

//~^ from_str_radix_10

NonZeroU16::from_str_radix("8", 10)?;

Added NonZero::<usize>::from_str_radix to the test cases as suggested.

changelog: [`from_str_radix_10`]: support `NonZero` integer types

hashcatHitman

community review: Looks good to me, thanks! I left a small nitpick, but you can leave it as is for now. I'd wait to see what the t-clippy reviewer says about it (if anything).

View changes since this review

hashcatHitman · 2026-09-17T20:19:54Z

+    }
+}
+
+fn issue_17715() -> Result<(), Box<dyn std::error::Error>> {


nit: I'm not sure if we would usually use the issue_# naming convention for something brought up in review? It is probably fine since issues and pull requests can't have matching numbers. Closest I found was a small precedence to leave a comment linking to the relevant review (done for #9136 (review) and #8737 (review)).

It doesn't really change whether the tests work or not, so I'd defer to whatever t-clippy reviewer gets assigned to the PR.

rustbot · 2026-09-17T20:24:42Z

r? @llogiq

rustbot has assigned @llogiq for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: 9 candidates
9 candidates expanded to 9 candidates
Random selection from 6 candidates

rustbot added S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 10, 2026

CommanderStorm suggested changes Sep 10, 2026

View reviewed changes

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch 4 times, most recently from e16f56a to 8084d7a Compare September 10, 2026 18:06

SomaFon requested a review from CommanderStorm September 11, 2026 01:32

This comment has been minimized.

Sign in to view

CommanderStorm approved these changes Sep 11, 2026

View reviewed changes

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from 167820b to dad311d Compare September 12, 2026 05:08

This comment has been minimized.

Sign in to view

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch 2 times, most recently from ea8af18 to ad3648c Compare September 12, 2026 12:31

CommanderStorm suggested changes Sep 12, 2026

View reviewed changes

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from ad3648c to dca676a Compare September 12, 2026 19:11

CommanderStorm approved these changes Sep 12, 2026

View reviewed changes

SomaFon requested a review from CommanderStorm September 13, 2026 03:37

CommanderStorm approved these changes Sep 13, 2026

View reviewed changes

hashcatHitman suggested changes Sep 17, 2026

View reviewed changes

Support NonZero types in from_str_radix_10 lint

1204021

changelog: [`from_str_radix_10`]: support `NonZero` integer types

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from dca676a to 1204021 Compare September 17, 2026 19:57

hashcatHitman approved these changes Sep 17, 2026

View reviewed changes

rustbot removed the S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. label Sep 17, 2026

rustbot assigned llogiq Sep 17, 2026

	&& (!integer_ty.is_nonzero() \|\| self.msrv.meets(cx, msrvs::NONZERO_FROM_STR_RADIX))
	&& (matches!(integer_ty, IntegerTy::Primitive(_)) \|\| self.msrv.meets(cx, msrvs::NONZERO_FROM_STR_RADIX))

Uh oh!

Conversation

SomaFon commented Sep 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 10, 2026

Uh oh!

CommanderStorm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomaFon commented Sep 11, 2026

Uh oh!

This comment has been minimized.

CommanderStorm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 12, 2026

Uh oh!

This comment has been minimized.

CommanderStorm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CommanderStorm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CommanderStorm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashcatHitman left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashcatHitman left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

SomaFon commented Sep 10, 2026 •

edited by rustbot

Loading

CommanderStorm left a comment •

edited by rustbot

Loading

CommanderStorm left a comment •

edited by rustbot

Loading

CommanderStorm Sep 12, 2026 •

edited

Loading

CommanderStorm left a comment •

edited by rustbot

Loading

CommanderStorm left a comment •

edited by rustbot

Loading

CommanderStorm left a comment •

edited by rustbot

Loading

hashcatHitman left a comment •

edited by rustbot

Loading

hashcatHitman left a comment •

edited by rustbot

Loading