unwrap_or_default: respect MSRV for raw-pointer Default impls

[uditjainstjis]

or_fun_call (through its unwrap_or_default suggestion) rewrites opt.unwrap_or(core::ptr::null()) to opt.unwrap_or_default(). The Default impls for *const T / *mut T were only stabilized in 1.88.0, so under a lower MSRV the suggested code does not compile. This was hit in the Linux kernel (msrv = "1.85", built with -Copt-level=s) — see the report linked from #17379.

This adds a RAW_PTR_DEFAULT (1.88.0) MSRV alias and bails out of the unwrap_or_default suggestion when the default value is a raw pointer and the MSRV is not met.

A note on testing

I could not reproduce the false positive in the UI test harness. The lint only recognises core::ptr::null() as a default value through is_default_equivalent's MIR comparison against <*const T as Default>::default(), which needs that impl's MIR to be available. That happens with -Zbuild-std (as in the kernel build), but with the precompiled std the harness uses, is_mir_available is false, so the lint never fires for raw pointers and a #[clippy::msrv] regression test can't trigger it. The reproduction is therefore the linked kernel report. Happy to add a test if there's a preferred way to exercise the build-std path, or to gate this differently.

fixes #17379

changelog: [unwrap_or_default]: don't suggest unwrap_or_default() for a raw-pointer value below the 1.88 MSRV that stabilized Default for raw pointers

[rustbot]

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. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

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.

[DanielEScherzer]

Community review: makes sense to me
But, since you couldn't add a test for this (with a reasonable explanation for why) can you please

• document with an inline comment the situations in which this is actually triggerable (since people might see that removing that code doesn't break any tests)
• test locally and confirm that this no longer fires for the std build?

View changes since this review

[uditjainstjis]

Both done.

1. Inline comment. Amended into the commit, so the "changes since review" link won't help — here it is verbatim:

    // The `Default` implementations for raw pointers (`*const T`, `*mut T`) were only stabilized in
    // 1.88.0. Suggesting `unwrap_or_default()` below that MSRV would produce code that does not
    // compile, so bail out in that case (see #17379).
    //
    // This is not covered by a UI test, so to be clear about when it is reachable: the value has to
    // be recognized as a default one by `is_default_equivalent`, which for `ptr::null()` compares
    // MIR against `<*const T as Default>::default()`. In practice that only happens under
    // `-Zbuild-std`; against the precompiled `std` the UI tests run on, the lint never reaches here
    // for raw pointers, which is why this cannot be covered by a test. So removing this check
    // breaks no test, but it does reintroduce the false positive on
    // `opt.unwrap_or(core::ptr::null())` for build-std users such as the Linux kernel.

2. Tested locally. I read "the std build" as building std from source, since that is the configuration the kernel report hits — say the word if you meant running clippy over the library itself instead and I'll do that.

Test crate:

#![feature(custom_inner_attributes)]
#![warn(clippy::unwrap_or_default)]
#![clippy::msrv = "1.85"]

pub fn f(o: Option<*const u8>) -> *const u8 { o.unwrap_or(core::ptr::null()) }
pub fn g(o: Option<*mut u8>) -> *mut u8 { o.unwrap_or(core::ptr::null_mut()) }
pub fn h(o: Option<*const u8>) -> *const u8 { o.unwrap_or_else(core::ptr::null) }

run against a locally built clippy with cargo clippy -Zbuild-std --target aarch64-apple-darwin:

build	msrv	unwrap_or_default fires
this branch	1.85	no
this branch	1.88	yes, on f and g
this branch, bail-out disabled	1.85	yes, on f and g — the #17379 false positive

So it suppresses exactly the below-MSRV case and nothing more.

Two things that came out of it:

• h never fires, even with the bail-out disabled — unwrap_or_else(ptr::null) goes through the call_expr == None path and is_default_equivalent_call doesn't treat ptr::null as default-equivalent. So the if let Some(call_expr) guard isn't leaving a hole.
• Without -Zbuild-std nothing fires at all, at any MSRV, which is the "can't write a UI test for it" part.

[DanielEScherzer]

Both done.

1. Inline comment. Amended into the commit, so the "changes since review" link won't help — here it is verbatim:

The triagebot link does show the changed comment, https://triagebot.infra.rust-lang.org/gh-range-diff/rust-lang/rust-clippy/a63d11b92b45931a5e789c56e01e772e17c278fa..3532097ae2ab397c13d142927b9727f0e1ee80bf/84544006d21b57688f3e92af1683df923868cd51..2d360f53d61124b3bd61341e392b7657b3438a95

2. Tested locally. I read "the std build" as building std from source, since that is the configuration the kernel report hits — say the word if you meant running clippy over the library itself instead and I'll do that.

I just wanted to confirm that this was tested to fix the bug that led to the report being filed, and you seem to have indeed tested this fully

View changes since this review

[rustbot]

r? @dswij

rustbot has assigned @dswij 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: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, dswij, llogiq

[CommanderStorm]

LGTM, but needs a bit of "de-vibing" in he comments 😆

View changes since this review

[CommanderStorm]

nit: seems overburdening. a shorter one likely also suffices.

Also might need an format, I don't know the line length by heart

Suggested change

	// The `Default` implementations for raw pointers (`*const T`, `*mut T`) were only stabilized in
	// 1.88.0. Suggesting `unwrap_or_default()` below that MSRV would produce code that does not
	// compile, so bail out in that case (see #17379).
	//
	// This is not covered by a UI test, so to be clear about when it is reachable: the value has to
	// be recognized as a default one by `is_default_equivalent`, which for `ptr::null()` compares
	// MIR against `<*const T as Default>::default()`. In practice that only happens under
	// `-Zbuild-std`; against the precompiled `std` the UI tests run on, the lint never reaches here
	// for raw pointers, which is why this cannot be covered by a test. So removing this check
	// breaks no test, but it does reintroduce the false positive on
	// `opt.unwrap_or(core::ptr::null())` for build-std users such as the Linux kernel.
	// Without stable `Default` for raw pointers (1.88.0), this is a compilation error
	// In practice that only happens under `-Zbuild-std`, so not covereded by our ui tests under precompiled `std`.

[uditjainstjis]

Fair 😄 — cut it down to four lines, keeping only the bit that stops someone deleting the check because no test covers it:

    // `Default` for raw pointers is only stable since 1.88.0, so the suggestion would not compile
    // below that MSRV (#17379). Only reachable under `-Zbuild-std`, hence not covered by our UI
    // tests against a precompiled `std` — removing this breaks no test, but does bring the false
    // positive back for build-std users.

cargo fmt --check is clean. Amended into the same commit (e0a5c4a).