Change NonNull::as_uninit_* to take self by value (as opposed to reference), matching primitive pointers.

[Raekye]

Copied from my comment on #75402:

I noticed that as_uninit_* on pointers take self by value (and pointers are Copy), e.g. see as_uninit_mut.

However, on NonNull, these functions take self by reference, e.g. see the function with the same name by for NonNull: as_uninit_mut takes self by mutable reference. Even more inconsistent, as_uninit_slice_mut returns a mutable reference, but takes self by immutable reference.

I think these methods should take self by value for consistency. The returned lifetime is unbounded anyways and not tied to the pointer/NonNull value anyways

I realized the change is trivial (if desired) so here I am creating my first PR. I think it's not a breaking change since (it's on nightly and) NonNull is Copy; all previous usages of these methods taking self by reference should continue to compile. However, it might cause warnings to appear on usages of NonNull::as_uninit_mut, which used to require the the NonNull variable be declared mut, but now it's not necessary.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information.

[thomcc]

I'm in favor (because using &self here can lead to having to cast away lifetimes for no reason), but this is an API change, so

r? rust-lang/libs-api @rustbot label +T-libs-api -T-lib

[Raekye]

I just noticed NonNull::as_mut and NonNull::as_ref also take self by reference. Unfortunately those are already stable methods... can anything be done about them? (Note that the methods of the same name on primitive pointers take the pointer self by value: as_mut and as_ref.

[thomcc]

Yes, I don't think anything can be done there. In #80771 I made them return unbounded lifetimes which avoids most of the issues, but going further than that can't be done without breaking code.

[Raekye]

Personally, I think it's better still for NonNull::as_uninit_* to take self by value. But maybe it's better to leave it consistent with the other NonNull methods. Otherwise you would get this:

let mut ptr = NonNull::new(...).unwrap(); // the mut is required
unsafe {
    *ptr.as_mut() = ...; // as_mut is a mutable borrow of ptr
}

let mut ptr = NonNull::new(...).unwrap(); // the mut is unnecessary and will trigger a lint
unsafe {
    *ptr.as_uninit_mut().write(...); // no borrow of ptr
}

But in that case, I think NonNull::as_uninit_slice_mut (which currently takes an immutable reference) should be made consistent with NonNull::as_uninit_mut which takes a mutable reference

[thomcc]

I think it's better to take by value. If we could change the others compatibly, I think we would. The fact that it will lead to an inconsistency in lints seems fairly minor, I doubt anybody would notice.

[RalfJung]

Personally I am leaning slightly towards consistency with other NonNull methods, but don't have a strong opinion. Let's see what @rust-lang/libs-api says.

[Raekye]

Changing NonNull::as_ref/mut to take self by value would add warnings to code with #![warn(unused_mut)] (which is on by default), and likely break code with #![deny(unused_mut)] (unless the non-null was actually reassigned).

Aside from that, is there/what code actually breaks specifically because of the API change? (Is there a chance all the methods can be made consistent in a new edition? I can try to do a crater run)

[thomcc]

I don't think the change can be made. It would also break code that tries to pass NonNull::as_ref as a function pointer. I can think of a lot of cases where changing this would break code, and it doesn't seem worth it.

[Raekye]

I see, I didn't think about function pointers. That's fair enough

[Raekye]

Actually thinking about it more, I think it's better for NonNull to be locally consistent, rather than having NonNull::as_uninit_* be consistent with primitive pointers but diverge from NonNull::_as_ref/mut. In which case, I think NonNull::as_uninit_slice_mut (which currently takes an immutable reference and returns a mutable reference) should be made consistent with NonNull::as_uninit_mut which takes a mutable reference (and returns a mutable reference). (Just my two cents)

[RalfJung]

I think NonNull::as_uninit_slice_mut (which currently takes an immutable reference and returns a mutable reference) should be made consistent with NonNull::as_uninit_mut which takes a mutable reference (and returns a mutable reference).

I agree. That's also consistent with NonNull::as_mut.

[thomcc]

Right, but these are not consistent with the methods on pointer types, e.g. https://doc.rust-lang.org/nightly/core/primitive.pointer.html#method.as_uninit_mut

So I don't know that consistency is an unambiguous argument.

[yaahc]

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs-api

[dtolnay]

I prefer the signatures in this PR. I think it's fine to leave as_ref/as_mut alone and just change these.

Thanks @Raekye!

[dtolnay]

@bors r+

[bors]

📌 Commit d5f96e6 has been approved by dtolnay

[bors]

⌛ Testing commit d5f96e6 with merge 03c8b0b...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 03c8b0b to master...

[rust-timer]

Finished benchmarking commit (03c8b0b): comparison url.

Instruction count

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	0	1	0	1
mean2	N/A	N/A	-0.7%	N/A	-0.7%
max	N/A	N/A	-0.7%	N/A	-0.7%

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	3	0	3	0
mean2	N/A	1.9%	N/A	-1.7%	N/A
max	N/A	2.6%	N/A	-3.2%	N/A

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 😿 relevant regression found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	1	1	0	2
mean2	2.1%	1.2%	-1.2%	N/A	0.4%
max	2.1%	1.2%	-1.2%	N/A	2.1%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩ ↩² ↩³

2. the arithmetic mean of the percent change ↩ ↩² ↩³