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

Can Weak be null? #114517

Open
est31 opened this issue Aug 5, 2023 · 5 comments
Open

Can Weak be null? #114517

est31 opened this issue Aug 5, 2023 · 5 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@est31
Copy link
Member

est31 commented Aug 5, 2023

There is a discrepancy between the docs of Weak and its implementation, originally found by @Urgau in #114494 (comment) .

On one hand, the docs of Weak::as_ptr state:

The pointer is valid only if there are some strong references. The pointer may be dangling, unaligned or even null otherwise.

On the other hand, the implementation of Weak uses NonNull:

rust/library/alloc/src/rc.rs

Lines 2681 to 2693 in 1cabb8e

pub struct Weak<
T: ?Sized,
#[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global,
> {
// This is a `NonNull` to allow optimizing the size of this type in enums,
// but it is not necessarily a valid pointer.
// `Weak::new` sets this to `usize::MAX` so that it doesn’t need
// to allocate space on the heap. That's not a value a real pointer
// will ever have because RcBox has alignment at least 2.
// This is only possible when `T: Sized`; unsized `T` never dangle.
ptr: NonNull<RcBox<T>>,
alloc: A,
}

From what I know, it is okay for NonNull pointers to be dangling or unaligned, but, staying true to their name, they are never allowed to be null pointers.

I think this discrepancy should be resolved, but I wonder, which way? Should the docs be adjusted, or the implementation?

The issue exists both with alloc::sync::Weak as well as alloc::rc::Weak.

@est31 est31 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-opsem Relevant to the opsem team labels Aug 5, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 5, 2023
@est31 est31 removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 5, 2023
@est31
Copy link
Member Author

est31 commented Aug 5, 2023

Some research on the history: Weak::as_ptr was introduced by #60766 (under a different name), tracking issue #60728. That PR already included the note that the pointer can't be null, even though already back then, Weak was using a NonNull internally.

@Zalathar
Copy link
Contributor

Zalathar commented Aug 5, 2023

I don't think there's a discrepancy here?

The documentation says that the method isn't guaranteed to return a non-null pointer.

The current implementation happens to never return a null pointer due to an internal detail (using NonNull), but that detail isn't a documented guarantee, so callers can't rely on it.

(For example, changing the implementation to return a null pointer in some circumstances would not be a breaking change, because the documented behaviour allows that possibility, even though the current implementation doesn't exercise it.)

@RalfJung
Copy link
Member

RalfJung commented Aug 5, 2023

Yeah, the key point is that from_raw does not allow null pointers.

@est31
Copy link
Member Author

est31 commented Aug 5, 2023

but that detail isn't a documented guarantee, so callers can't rely on it.

Good point, makes sense.

from_raw does not allow null pointers.

This is not written anywhere in the docs of from_raw. It only says that the weak has to come from into_raw. The obvious alternatives like as_ptr plus forget (which from_raw is a wrapper of), or into_raw_and_alloc are missing. into_raw mentions nowhere that it can return a null pointer either.

I think there is a small, but non zero chance that when people read that the output of as_ptr can be a null pointer, they could assume that from_raw can take the input of as_ptr as well, and therefore, also a null pointer.

I'll make a PR to improve the docs.

@est31
Copy link
Member Author

est31 commented Aug 5, 2023

PR filed: #114525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants