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

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

View all comments

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?

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.

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.

Yeah, and if it doesn't really require background context, no need to add link or special names.

I think we could use some re-wording here, as indeed having the PR that introduced the test as the test name does not tell us much.

Suggested change

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

fn with_non_integer_nonzero() -> Result<(), Box<dyn std::error::Error>> {

Renamed it to with_non_integer_nonzero in 6498b7b and updated the .fixed file too. Thanks!

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 · 2026-09-25T08:41:07Z

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.

SomaFon · 2026-09-29T06:18:50Z

Hi @llogiq, just checking in on this PR when you have a chance. The community reviews are complete, and the merge conflict has been resolved. Please let me know if there is anything else I should change. Thanks!

Gri-ffin · 2026-09-29T13:24:06Z

-            let sugg =
-                Sugg::hir_with_applicability(cx, expr, "<string>", &mut Applicability::MachineApplicable).maybe_paren();
+            let type_name = match integer_ty {
+                IntegerTy::Primitive(prim_ty) => prim_ty.name_str().to_owned(),
+                IntegerTy::NonZero(ty) => {
+                    // Keep the source spelling, including aliases like `MyNonZero` and
+                    // qualified paths like `std::num::NonZeroU8`.
+                    snippet(cx, ty.span, "<integer>").into_owned()
+                },
+            };
+            let sugg = Sugg::hir(cx, expr, "<string>").maybe_paren();


This would change the applicability of the entire lint we should keep using hir_with_applicability for primitives and maybe use HasPlaceholders or MaybeIncorrect if the extraction fails for NonZero types. Or use snippet_with_applicability.

I didn't see that MaybeIncorrect was hardcoded..., but handling NonZero separately is good too.

View changes since the review

Thanks for taking a look! In e3ddcb2, I restored Sugg::hir_with_applicability and changed the NonZero type lookup to snippet_opt. If the type snippet is unavailable, the lint is skipped rather than suggesting a placeholder. The final applicability is unchanged, and CI is passing.

This is fixed now, right?

SomaFon · 2026-10-05T18:13:53Z

Hi @llogiq, just a gentle follow-up on this PR when you have a chance. All CI checks are passing and the branch is up to date. The latest review discussion clarified the applicability handling, and I am happy to make any further changes if needed. Thanks for your time!

SomaFon · 2026-10-05T18:18:34Z

@rustbot ready

CommanderStorm · 2026-10-12T12:58:36Z

@rustbot reroll

blyxyas

It's looking good! @SomaFon please look into my review comment and I think we'll be able to move forward with this PR.

Thank you for the contribution!

View changes since this review

rustbot · 2026-10-18T17:16:43Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

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

SomaFon · 2026-10-19T13:38:38Z

@rustbot ready

blyxyas

LGTM, thanks for addressing this issue! ❤️ ฅ•ω•ฅ

View changes since this review

Gri-ffin

LGTM :)

View changes since this review

blyxyas · 2026-10-19T20:50:42Z

@rustbot merge

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

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

This comment has been minimized.

Sign in to view

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from 1204021 to 29dc49e Compare September 25, 2026 08:41

Gri-ffin suggested changes Sep 29, 2026

View reviewed changes

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from 29dc49e to e3ddcb2 Compare September 29, 2026 16:48

rustbot assigned dswij and unassigned llogiq Oct 12, 2026

blyxyas requested changes Oct 18, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 18, 2026

Support NonZero types in from_str_radix_10 lint

6498b7b

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

SomaFon force-pushed the fix-17712-nonzero-from-str-radix branch from e3ddcb2 to 6498b7b Compare October 19, 2026 13:37

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 19, 2026

blyxyas approved these changes Oct 19, 2026

View reviewed changes

blyxyas requested a review from Gri-ffin October 19, 2026 20:36

Gri-ffin approved these changes Oct 19, 2026

View reviewed changes

rustbot added this pull request to the merge queue Oct 19, 2026

Merged via the queue into rust-lang:master with commit 3583eb8 Oct 19, 2026
13 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 19, 2026

rustbot mentioned this pull request Oct 19, 2026

Unify impl pass and declare lint pass macros #17897

Open

	&& (!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))

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

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!

This comment has been minimized.

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!

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!

rustbot commented Sep 17, 2026

Uh oh!

This comment has been minimized.

rustbot commented Sep 25, 2026

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

Gri-ffin Sep 29, 2026 •

edited

Loading

blyxyas Oct 19, 2026 •

edited

Loading

blyxyas left a comment •

edited by rustbot

Loading

blyxyas left a comment •

edited

Loading

Gri-ffin left a comment •

edited by rustbot

Loading