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

Support T: ?Sized in Weak::new() #50513

Closed
dtolnay opened this issue May 7, 2018 · 13 comments
Closed

Support T: ?Sized in Weak::new() #50513

dtolnay opened this issue May 7, 2018 · 13 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented May 7, 2018

The std::rc::Weak::new and std::sync::Weak::new constructors added in #30467 should be able to work with T: ?Sized.

let _: Weak<str> = Weak::new();
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 7, 2018
@SimonSapin
Copy link
Contributor

I don’t see why not. I’d r+ a PR.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jun 27, 2018

I don't see how that's possible for arbitrary DSTs. Constructing the Weak<DST> implies constructing a fat pointer for the DST, which means you have to come up with a valid metadata, and you also have to make an allocation matching that metadata (so that the deallocation part, which gets the size/align from the metadata, knows how much to allocate). This is extremely difficult for trait objects (AFAICT you'd have to fake a vtable, or somehow get at a Sized type implementing the trait and use its size/align to make the allocation), and becomes completely impossible under any custom DST proposal I'm aware of.

Of course, it could easily be done for [T] (and newtypes around it, like str) by just setting len = 0.

@dtolnay
Copy link
Member Author

dtolnay commented Jun 27, 2018

@rkruppe this is Weak::new, not Rc::new. Weak::new makes a weak reference that does not refer to any T. So there should not need to exist an actual T or vtable.

@hanna-kruppe
Copy link
Contributor

Weak refers to an RcBox though, and so Weak::new allocates a dummy one to refer to it. I believe this could be changed to make it store an Option<NonNull<RcBox<T>>>, but then Weak itself would no longer be non-null (i.e., Option<Weak<T>> wouldn't be pointer sized any more) and we'd need branches in any method that accesses the RcBox.

@hanna-kruppe
Copy link
Contributor

Additionally, Weak<DST> is a fat pointer even if we changed it so there's no actual underlying allocation, so the problem about synthesizing valid metadata is still an issue (it might be solvable for vtables, but still intractable for custom DSTs).

@SimonSapin
Copy link
Contributor

This is a good point @rkuppe, I should have thought of it :) Weak<dyn Trait> needs to store a vtable pointer inline even if there is no real dyn Trait value being pointed to.

@dtolnay
Copy link
Member Author

dtolnay commented Jun 27, 2018

Yep you are right.

If custom DSTs were required a way to synthesize an arbitrary but unusable metadata, that would be useful both here and for extending ptr::null to T: ?Sized.

@hanna-kruppe
Copy link
Contributor

That sort of operation seems dubious to me. For example, if we ever add any sort of reflection based on vtable contents (even if opt in), the choice of "arbitrary" vtable will leak to users. Similarly, I suspect there might be reasonable custom DSTs that can't really oblige this requirement (e.g. if the metadata is a handle into some out-of-line data structure).

@dtolnay
Copy link
Member Author

dtolnay commented Jun 27, 2018

Okay I buy that. I think that means I give up on this issue. 😸 Thanks for the justifications!

@dtolnay dtolnay closed this as completed Jun 27, 2018
@foriequal0
Copy link

foriequal0 commented Jun 12, 2020

Is this issue still valid after #51901 and #52637 ?
edit Never mind. I didn't fully understand what DST means. It seems still hard to implement.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 7, 2020

I think this should be revisited: I don't buy @hanna-kruppe's argument about reflection, because IMO, any reflection machinery which allows you to obtain metadata from a de-allocated Weak pointer is broken. Weak should be treated like an Option: when the target is deallocated (or when constructed via Weak::new) it doesn't make sense to talk about what it refers to anymore.

This is discussed further in the UCG repo but the consensus seems to be that at the moment the only validity requirement on the vtable pointer is that it be non-null. Therefore, it would be valid to implement this method by storing any non-zero value in the vtable portion of the pointer.

@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2020

Note that #73845 by @CAD97 relies on unsized Weak never dangling for soundness (see the comments added there).

This is discussed further in the UCG repo but the consensus seems to be that at the moment the only validity requirement on the vtable pointer is that it be non-null.

Yes, but that is a "what rustc happens to do" kind of situation. Before we rely on it for a stable API, we should have it RFC'd or FCP'd by the lang team at least.

@CAD97
Copy link
Contributor

CAD97 commented Jul 8, 2020

The functionality doesn't strictly rely on unsized Weak never dangling, just that the pointer used for unsized Weak always works in size_of_val_raw. Saying unsized Weak never dangle is just the easiest way to meet this requirement.

It does, however, require that the metadata is "real" for DSTs, so a dangling vtable would be UB in those methods as implemented. They could always be adjusted to explicitly check for the sentinel value, however, and avoid using the metadata in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants