Weak::from_raw is practically unusable

[CAD97]

... as documented.

The problem: this line from the documentation:

It is allowed for the strong count to be 0 at the time of calling this, but the weak count must be non-zero or the pointer must have originated from a dangling Weak<T> (one created by new).

The weak count as reported by Weak::weak_count is 0 when the strong count is 0, even if there are still outstanding weak references:

pub fn weak_count(&self) -> usize

Gets the number of Weak pointers pointing to this allocation.

If no strong pointers remain, this will return zero.

The combination of these two means that getting a pointer from Weak::into_raw is not enough to guarantee that it's safe to use in Weak::from_raw. Instead, you have to know that (if it originated from a strong reference) you still have an outstanding strong reference to the allocation, because otherwise the (exposed) weak count is 0, and thus calling from_raw is "documented UB".

To properly document when this is intuitively allowed (i.e. it's allowed because I got the pointer from into_raw and have called into_raw more than from_raw, so there are still "unowned" raw weak references to claim) (which lines up with what the implementation allows), we need to expose (probably docs only) the fact that there is still being a weak reference count behind the hood being tracked until all of the Weaks have been dropped.

This will probably also require guaranteeing that Weak doesn't drop its weak reference and become dangling "early" (e.g. note that upgrade failed, decrement the (real) weak count (potentially deallocating the place), and become a dangling weak via internal mutability) and that Weak::clone on a "zombie" Weak increments the (real) weak count and creates a new "zombie" Weak, not a dangling Weak.

[CAD97]

(I don't know if C-bug is the correct label for a "documentation bug," but it was the closest template to correct.)

[nbdd0121]

I guess that weak count must be non-zero means the internal weak count rather than the user-visible weak_count().

[CAD97]

That's obviously the intent and what the implementation shows is the case, but when considering documented properties of an API, you have to only consider the documented API.

A correct resolution to the issue would be to adjust the documentation to clarify that the weak count is referring to the hidden internal weak count that's not exposed (except conceptually for the safety of into/from_raw). cc @vorner who wrote these methods and docs

[vorner]

Yes, it's meant as the actual number of weak pointers pointing there (including the ones „frozen“ to raw pointers), not what some method returns (if it's currently in the form of the raw pointer, you really can't even call the method, so I'm not sure if that part can even apply). I agree it could probably be documented better.

I'd try to fix the docs, but considering I've already updated them like 3 times during the review and they still haven't ended up entirely correct, I'll probably ask if you want to give it a go first, as obviously the docs aren't my strong ability :-).

[CAD97]

The main thing this requires is actually specifying how the "internal" weak count works, and guarnateeing that we don't do "clever" things with converting "zombie Weak"s into dangling Weaks. I really don't know the best way to do this or even where to do this, since it basically only impacts how and when from_raw is allowed to be called.

[vorner]

🤔 Maybe just getting rid of the „weak count“ term altogether and try to describe it in a way that the into_raw pointer preserves the ownership of the Weak, or that it's an alternative form of Weak and it can be turned back if it still has the ownership.

Or maybe just point out that the „weak count“ is meant in its natural meaning, not referring to the method.

[vorner]

Ok, looking into this, how about wording like this?

On the into_raw:

This call effectively „freezes“ the Weak into a raw pointer, while still preserving the ownership.

And on the from_raw:

It is allowed for the strong count to be 0 at the time of calling this. Nevertheless, this turns one conceptual „frozen“ Weak in the form of raw pointer to a concrete Weak and therefore must be paired with previous call to into_raw.

Do you think this would be better?

[CAD97]

Yeah, I think that wording captures the semantics properly.

[vorner]

Thanks. I'll get around to submitting the changes some time soonish.