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

Tracking Issue for box_into_inner #80437

Open
1 of 4 tasks
Tracked by #109 ...
crlf0710 opened this issue Dec 28, 2020 · 80 comments
Open
1 of 4 tasks
Tracked by #109 ...

Tracking Issue for box_into_inner #80437

crlf0710 opened this issue Dec 28, 2020 · 80 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. Libs-Tracked Libs issues that are tracked on the team's project board. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@crlf0710
Copy link
Member

crlf0710 commented Dec 28, 2020

Feature gate: #![feature(box_into_inner)]

This is a tracking issue for consuming a Box and returning its wrapped value.

The language actually supports *x as a special case. However it can't be generalized to other types easily. It also doesn't consume the Box value immediately if T is Copy, which is quite a footgun.

An explicit version is added here to make the API more consistent with Pin, Cell, RefCell etc.

Public API

impl<T, A: Allocator> Box<T, A> {
     pub fn into_inner(boxed: Self) -> T;
}

Steps / History

Unresolved Questions

  • None yet.
@crlf0710 crlf0710 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Dec 28, 2020
@camelid camelid added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 29, 2020
@m-ou-se m-ou-se added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jan 6, 2021
@fosskers
Copy link

How come it's a function and not a method?

@crlf0710
Copy link
Member Author

crlf0710 commented Sep 7, 2021

@fosskers https://rust-lang.github.io/api-guidelines/predictability.html#smart-pointers-do-not-add-inherent-methods-c-smart-ptr

@conradludgate
Copy link
Contributor

When looking at how to turn a box back into a stack owned value, I find this function, but in my case the *x method works perfectly fine. It's documented in the box module, but I feel like it should be doubly documented here as a simpler alternative.

@TehPers
Copy link

TehPers commented Apr 9, 2022

Somewhat minor, but I like that Box::into_inner can be used in method chains without requiring a closure. .map(|x| *x) is still shorter than .map(Box::into_inner), but the latter more clearly describes what you're doing in my opinion. It can also help the compiler infer the type in the iterator/Option<T>/whatever at that point in the method chain for what it's worth. Otherwise you'd have to write |x: Box<_>| *x for the same effect.

@BurntSushi
Copy link
Member

Box::into_inner appears to have been around for a while, and I don't see any obvious issues with it? What do others think?

@rfcbot merge

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 13, 2022
@m-ou-se m-ou-se removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 13, 2022
@m-ou-se

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 13, 2022
@m-ou-se

This comment was marked as outdated.

@rfcbot
Copy link

rfcbot commented May 13, 2022

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 13, 2022
@rfcbot
Copy link

rfcbot commented Jun 8, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 8, 2022
@dtolnay
Copy link
Member

dtolnay commented Jun 8, 2022

I think I would want to see better documentation of this before I would be comfortable stabilizing it.

The current example code given for this function is:

let c = Box::new(5);

assert_eq!(Box::into_inner(c), 5);

which is not a thing that we want people to use this function for, right? We would prefer assert_eq!(*c, 5) in that situation still, right?

Ideally the documentation would mention * and try to articulate what the use case for the function, as opposed to the syntax, is. I'd like to see an example code that is best written using Box::into_inner and would not be made better by changing Box::into_inner to *.

As currently written, it's not clear whether this function has been added because we want people to stop writing * as often in favor of some named behavior with better specificity (the way some people feel about as), vs for some other justification.

@rfcbot concern convey the use case in documentation

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 8, 2022
@conradludgate
Copy link
Contributor

If the goal is to work on consistency, I would much rather we focus efforts on a DerevMove trait and not this method. As it stands, the implementation of this function is just *boxed which anyone can write.

pub fn box_into_inner<T>(boxed: Box<T>) -> T {
    *boxed
}

Not as nice as being in std, but it's not much overhead if you really want a function that imitates this behaviour.

@Aandreba
Copy link

Aandreba commented Sep 6, 2022

As it stands, the implementation of this function is just *boxed which anyone can write.

A big part of the standard library are functions that could had been written manually (see Option and Result methods). Also, what downside could stabilizing into_inner have? As far as I see, none, and similar functions exist for other types (like all the cells and atomics).

@BurntSushi
Copy link
Member

Also, what downside could stabilizing into_inner have?

Decision paralysis. There is already a way to utter Box::into_inner in Rust and that's *.

it breaks standard by making an exception for a specific type

I don't think anyone is happy about Box being "special." So I mean, like, yeah, we're all on the same page there. That doesn't mean we should add Box::into_inner.

but we can very much offer a correct alternative.

Are you saying *x where x is a Box<T> is incorrect? I'm confused. I thought we just established that this method was about clarity/intent, but now it seems like you're saying it's about correctness? Can you provide some real code that needs Box::into_inner to be correct?

@Aandreba
Copy link

Aandreba commented Sep 8, 2022

Are you saying *x where x is a Box<T> is incorrect?

Not in the sense that it produces incorrect code.

@Aandreba
Copy link

Aandreba commented Sep 8, 2022

I would much rather we focus efforts on a DerevMove trait and not this method

Why would DerefMove be better than into_inner?

@Aandreba
Copy link

Aandreba commented Sep 8, 2022

Decision paralysis. There is already a way to utter Box::into_inner in Rust and that's *

The same argument could be made for number conversion. Do I use the Into/From/TryInto/TryFrom traits or do I use the as keyword?

@npmccallum
Copy link
Contributor

@Aandreba From and as do different things. From is lossless. as is potentially lossy.

@Aandreba
Copy link

Aandreba commented Sep 8, 2022

From and as do different things. From is lossless. as is potentially lossy.

Not always. If I want to cast from u8 to u16, the result will always be the same, no mater how I casted the numbers. Should we not implement From<u8> for u16?

@BurntSushi
Copy link
Member

The same argument could be made for number conversion. Do I use the Into/From/TryInto/TryFrom traits or do I use the as keyword?

I wasn't making an argument though. You asked for a cost. I gave you one. Do we have other "decision paralysis" costs in the standard library? Yes! Absolutely! There's a difference between the questions "what are the downsides" and "should we do this." You said, "Also, what downside could stabilizing into_inner have?" Which is clearly an instance of the former, not the latter.

Otherwise, this conversation is honestly getting pretty frustrating to participate in, so I'm going to bow out until there's something more significant to respond to. I'll say that I am not against this API landing, but I am not strongly in favor of it either.

@Aandreba
Copy link

Aandreba commented Sep 8, 2022

You said, "Also, what downside could stabilizing into_inner have?" Which is clearly an instance of the former, not the latter.

That's true. The point I'm trying to make is that I don't see how the decision paralysis cost outweighs the benefit in code readability. And it's also a very small decision for which you'll have a goto answer by the second time you encounter it.

@TehPers
Copy link

TehPers commented Sep 10, 2022

I also think that this function is better served in a more generic DerefMove trait, which would allow for a more widely usable version of into_inner. Because of that, I'm not sure that Box::into_inner is any better than, for example, a DerefMove::deref_move function. The biggest reason I want this is for convenience during function chains (in particular, .map(Box::into_inner)), but that is also solved by DerefMove. I would be more in favor of that feature stabilizing, personally.

@Aandreba
Copy link

I see DerefMove as a bad idea. To me it looks like "ups, we added a bad exception for a type, let's solve it by making it possible to apply the same exceptions to other types". I think into_inner is a better alternative.

@TehPers
Copy link

TehPers commented Sep 15, 2022

I'm curious what makes the move-on-derefence behavior bad for Box<T>? To me, it seems like a very useful convenience, given how many functions take self and how often you work with boxed values. On the other hand, generalizing it seems like it would open the possibility for alternate pointer types to take advantage of that behavior. That being said, discussion around DerefMove specifically is probably better suited in the DerefMove/IndexMove/IndexSet issue.

On the topic of this issue, calling Box::into_inner every time I needed to move out of a Box<T> would be quite a bit of syntactic overhead every time I needed to pass a self. While I would like an into_inner-like function, I wouldn't like being forced to use it everytime, especially when Deref and DerefMut already allow you to auto-deref pointer types to call &self/&mut self methods on types behind pointers:

let mut s = Box::new(String::new());
s.push('a'); // The boxed value is automatically dereferenced here with DerefMut's implementation

@Aandreba
Copy link

discussion around DerefMove specifically is probably better suited in the DerefMove/IndexMove/IndexSet issue.

Agreed. But the point was being made that with DerefMove::deref_move, Box::into_inner would be unnecessary (which I agree). My point was "I don't think DerefMove is a good idea, so I hope it never comes to bee. If it doesn't, we'll still have the same problem as now"

calling Box::into_inner every time I needed to move out of a Box would be quite a bit of syntactic overhead every time I needed to pass a self.

Well, I don't think anybody is arguing we should remove the special behavior * has on boxes. The Rust team has made a clear commitment to to backwards compatibility, and that would break it.

I wouldn't like being forced to use it everytime, especially when Deref and DerefMut already allow you to auto-deref pointer types to call &self/&mut self methods on types behind pointers

The problem discussed here is not about taking references of the values inside a box. The Deref/DerefMut API is a well specified and designed one, and yes, 99% of the time you'll work with the value still boxed.

But when time comes to take ownership of the boxed value in the stack, I (and many others) think Box::into_inner is preferable to *.

@conradludgate
Copy link
Contributor

Well, I don't think anybody is arguing we should remove the special behavior * has on boxes. The Rust team has made a clear commitment to to backwards compatibility, and that would break it.

This is just syntax. Syntax can be changed in editions, and I have heard people genuinely suggesting we should remove the * special behaviour.

But when time comes to take ownership of the boxed value in the stack, I (and many others) think Box::into_inner is preferable to *.

Logically, * makes sense for box. We use * to get to the referee. In the case of &T, either we need to copy T or we need to immediately & it again otherwise this is not valid. However, since we have Box<T> as an 'owned pointer', it does make sense that this is not required.

My personal issue is that this behaviour is currently special to Box. I don't mind language features, but we typically have a trait that encodes those behaviours:

  • for is a wrapper around Iterator
  • + is Add
  • .await is Future
  • * is already overloaded with Deref/DerefMut depending on the context of needing a &T/&mut T respectively. I think adding a third trait that we can point to, even if we seal it just for box to implement, is a good thing. Then we can do Box::deref_move instead of Box::into_inner for the same effect.

@LunarLambda
Copy link

Has there been any further discussion about this? I (and people I've taught Rust to) have gotten quite surprised more than once at *box being the way to move out of a box.

@EndilWayfare
Copy link

Just to add my two cents, I just today found out about how "unbox" has to be spelled *x, despite working pretty heavily in Rust for almost 5 years.

For some reason, in all my use cases, I've never had a reason to reach for a Box in situations where ownership transferred. I've used other smart-pointer/cell types extensively, because most of the time when I need a heap allocation that isn't a standard collection or a foreign crate import, the reason I'm doing so is better served by an Rc or Arc, with or without a RefCell/Mutex. I've even had reasons to manually, explicitly use Pin. I knew that Box has some special-casing in rustc.

Given that prior context, my immediate thought was "oh, obviously Box::into_inner." But it wasn't. And I had to look it up.

I wouldn't consider myself a Rust "expert". But, my point is, I'm certainly not a noob at this point, either, and I found this behavior surprising. In fact, surprisingly surprising. One of my (many) favorite parts about Rust is the consistency, coherence, and wide-adoption of naming convention patterns. They're short, sweet, descriptive, readable, say-able, and practically self-documentation in many cases. That's probably the main reason why I expected Box::into_inner to be the idiom, and expected pretty confidently at that.

Still anecdotal/personal-preference, but I figured it might be relevant that the issue isn't unique or specific to newcomers.

@sylv256
Copy link

sylv256 commented May 9, 2024

I wouldn't consider myself a Rust "expert". But, my point is, I'm certainly not a noob at this point, either, and I found this behavior surprising. In fact, surprisingly surprising. One of my (many) favorite parts about Rust is the consistency, coherence, and wide-adoption of naming convention patterns. They're short, sweet, descriptive, readable, say-able, and practically self-documentation in many cases. That's probably the main reason why I expected Box::into_inner to be the idiom, and expected pretty confidently at that.

I agree with all of what you said. I never thought unboxing a Box would be *boxed, so I think this should be—at the very least—locked behind an #[allow()] in the next Rust edition, if not entirely removed.

@ais523
Copy link

ais523 commented Jun 13, 2024

Currently, the *boxed method of converting a box into its inner value is underdocumented and I learned about it only today, from this bug report (which I found by searching Box::into_inner() in the documentation), despite having been using Rust for over 6 years. It is not listed in the documentation for std::boxed or std::boxed::Boxed. It is not listed in the section of the Rust Reference that covers a DereferenceExpression. "The Rust Programming Language" not only does not mention that a Box can be used like that, it actively denies it (there is an entire section on how calling * on a box uses the Deref trait and how you need to implement the Deref trait yourself to get the same behaviour on your own smart pointers, and a statement that "Boxes provide only the indirection and heap allocation; they don’t have any other special capabilities, like those we’ll see with the other smart pointer types.").

If the intended way to convert a box into its inner value is *boxed, then this urgently needs to be documented somewhere people will find it – it is possible that it is documented somewhere, but I didn't find it documented anywhere other than this bug report. It would make a lot of sense to at least, in the documentation of the unstable box_into_inner, to say "You can also convert a box into its inner value by doing *boxed" and giving an example of that – the documentation of Box is an obvious place to look for someone who's trying to do that in their code, and into_inner is the obvious method to look at. I got here because, finding no stable way to do the conversion, I decided to check the bug report to see how close it was to stabilisation.

Note that updating the documentation for into_inner to list the currently stable way to do things is something that doesn't require it to actually be stabilised to have benefits; even when people use stable versions of Rust, they use documentation that lists the unstable methods, so that would be a good way to have discoverability for the feature in question (and there is no reason to not make it discoverable even if planning to remove it in an edition – you would need to maintain backwards compatibility for code that's already using it anyway). As such, it could be done regardless of the conclusions in this issue as to which of the two syntaxes is preferable.

@BurntSushi
Copy link
Member

@ais523 The second example in the std::boxed module docs specifically calls out the use of * to dereference a Box<T>: https://doc.rust-lang.org/std/boxed/index.html#examples

@ais523
Copy link

ais523 commented Jun 13, 2024

@BurntSushi: The example given actually copies a value out of the box back to the stack, using the Box as Deref trait implementation, because u8: Copy. For example, this works (and the first two lines are the given example):

let boxed: Box<u8> = Box::new(5);
let val: u8 = *boxed; 
println!("{}", val);
println!("{}", boxed);

Playground link

If the Box were consumed, this wouldn't compile (because a Box<u8> is not Copy even though a u8 is).

As such, the example isn't very instructive to people who are trying to learn how Box works because they just assume it's a demonstration of Box supporting Deref, something which is comprehensively discussed in the documentation. (Yes, the introduction to the example says "moved" – but that's inconsistent with the actual example, which is a demonstration of Deref and not DerefMove.) In particular, it doesn't make it clear what would happen with a Box that boxes a non-Copy type, and as such, isn't teaching people about the special case that moves the contents.

@BurntSushi
Copy link
Member

@ais523 Sorry, I'm not understanding the problem here. The code example in your comment doesn't match what's in the docs. The example in the docs works regardless of whether the boxed type is Copy or not.

I think this sort of feedback is more helpful in the form of a PR with edits to the status quo. That will make your feedback extremely concrete, and there will be something specific to compare with the status quo.

@ais523
Copy link

ais523 commented Jun 13, 2024

I wonder if we're looking at different documentation? I'm looking at https://doc.rust-lang.org/std/boxed/index.html in which the example in question is

let boxed: Box<u8> = Box::new(5);
let val: u8 = *boxed;

It is possible that this has already been fixed recently, and the fix simply hasn't reached the public documentation yet.

(I also can't submit pull requests via Github because doing so would force my account to use two-factor authentication, and I don't have a smartphone or other similar device to use as the second factor.)

@BurntSushi
Copy link
Member

Yes. That's the same example I'm looking at. You can replace u8 with a non-Copy type and it still works.

@ais523
Copy link

ais523 commented Jun 13, 2024

So the problem is that the example doesn't convey the information that it would still work after replacing u8 with a non-Copy type. Someone who had read most of the relevant documentation (e.g. The Rust Programming Language and the Rust Reference) would be under the impression that the reason the example worked was that * is calling the Deref implementation on Box to obtain an &u8, and that it is possible to copy out of an &u8. It still works if you replace the Box with something that doesn't have the special case:

use std::rc::Rc;
let boxed: Rc<u8> = Rc::new(5);
let val: u8 = *boxed;

Playground link

As such, the example fails to inform readers that the special case exists – because it would work even without it (and in fact doesn't make use of it).

@BurntSushi
Copy link
Member

OK, I'm not sure I really agree with that. But I don't really know how to engage with you further without looking at a concrete set of edits to the status quo. I don't know how to resolve your issue with PRs. If you can't create one, perhaps you can find someone to do it on your behalf.

I also think this is meandering into off-topic territory for this specific issue. I might suggest creating a new one because "documentation for Box is not as good as it could be" and "should we stabilize Box::into_inner" are, while related, two distinct things.

@ais523
Copy link

ais523 commented Jun 13, 2024

I am trying to think of a concrete set of fixes, but the problem is actually the same one that blocked stablisation of this issue earlier: Box::into_inner (and the competing alternative Box as DerefMove) aren't actually useful very often, which is why people can spend so long before coming across the special case (I've been programing Rust for years without knowing how to move out of a Box), and why it's hard to create a simple example for use in documentation. Normally, once a value has been placed in a Box, it makes sense to keep it there and not move the value further. (Likewise, although self receivers are fairly common, they're most commonly used either on types that are Copy and can be copied out of a box normally, or on container types that already store their data on the heap and thus don't need a box.)

In case it helps someone: the situation in which I came across this was that I was trying to convert a Vec of a non-Copy type into an array in stable Rust, and came across the sequence "convert the Vec into a boxed slice, then use the TryFrom implementation between a boxed slice and box array, then unbox the array". Unfortunately, that's probably too complex for use in a simple example.

@scottmcm
Copy link
Member

Unfortunately, that's probably too complex for use in a simple example.

Also there's been [T; N]: TryFrom<Vec<T>> since 1.48 (#76310), to do the whole thing in one step.

@dtolnay
Copy link
Member

dtolnay commented Aug 6, 2024

We discussed this tracking issue in today's standard library API team meeting. The thing it's stuck on is still a PR for #80437 (comment). Some of the comments following that one have some additional suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. Libs-Tracked Libs issues that are tracked on the team's project board. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. 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