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

Add the boxed!() macro to "de-magic" box syntax #3057

Closed
wants to merge 3 commits into from
Closed

Add the boxed!() macro to "de-magic" box syntax #3057

wants to merge 3 commits into from

Conversation

notgull
Copy link

@notgull notgull commented Jan 10, 2021

Box syntax (e.g. box x) is controversial due to its "magic" status in the language. This RFC proposes the addition of a boxed macro that would remove the magic syntax from this feature (e.g. boxed!(x)).

Rendered

@notgull notgull changed the title Adds the boxed!() macro to "de-magic" box syntax Add the boxed!() macro to "de-magic" box syntax Jan 10, 2021
@Havvy
Copy link
Contributor

Havvy commented Jan 10, 2021

Given that box is not stable, and neither would the macro you are proposing, this should be done as a Major Change Proposal.

As somebody who is not part of the compiler team though, the difference between unstable syntax and macros is basically zero, so this just looks like churn for the sake of churn.

@Lokathor
Copy link
Contributor

Doing this as an RFC so that it has a clear path to stability is fine, in terms of process.

However, I also don't see how this fixes much. box <expr> is entirely fine syntax, so we should just stabilize that if we eventually want a stable boxing syntax.

@mark-i-m
Copy link
Member

I would prefer to add magic syntax and remove it in a future edition than add a macro with a magic implementation and be stuck with it in the std library forever.

I do like the motivation of trying to hide some of the magic though.

@clarfonthey
Copy link
Contributor

Personally, I thought that the main reason why box wasn't stabilised wasn't because it was too magic of a syntax, but because we weren't sure how to generalise it in such a way that it would work generically for any kind of heap allocation, rather than just applying to box. And, there was the issue of box patterns, which I think has been resolved and they won't be needed anyway.

The thing I do like about using a boxed! macro instead of a box syntax that gets removed is that we already have this magic macro with vec!, and the precedent would be to add a box! macro like it. (I would call it box! instead of boxed! though, for consistency.) Ultimately, whatever generic solution is used should also work for Vec, and that would also make the vec! macro redundant as well.

@RustyYato
Copy link

@clarfonthey vec! macro isn't magic, it's just a normal macro. It does defer to some implementations that utilize specialization, but specialization that's no different from the rest of std. Funnily enough, vec! does utilize the box syntax internally. Again, utilizing unstable syntax/features within a macro is not a big deal. For an example of a magic macro, look no further than format_args 👀

@clarfonthey
Copy link
Contributor

Oh, I guess I should have clarified -- while vec! isn't strictly magic and mostly doesn't need anything like box to work, it's also essentially the same as Vec::from(box [...]).

@nacaclanga
Copy link

nacaclanga commented Jan 12, 2021

Stabilizing boxed! or r#box! rather them the original box syntax has the advantage that vec::Vec and boxed::Box instances, can be created in very similar fashion, by calling a macro with the same name as the module, they are defined in (or the type name in lowcase in case of r#box!). In addition to not adding new syntax, boxed! or r#box!() can be defined in alloc, whereas the box syntax is awailable even when the boxed::Box type is not available at all. In case of r#box!, if we define it as a syntax extension like format_args!, we can remove the box syntax feature and the box keyword entierly. This would allow the macro to be called simply using box!.

@KodrAus KodrAus added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Jan 12, 2021
The [`vec`](https://doc.rust-lang.org/src/alloc/macros.rs.html#37-47) macro is implemented in terms of it,
and several other parts of the [standard library](https://doc.rust-lang.org/src/alloc/sync.rs.html#314-318)
use it.
[`Box::new`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.new) is the stable alternative to this;
Copy link
Contributor

@KodrAus KodrAus Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we think of any real-world examples that aren't already served by the vec! macro that are likely to be big enough to overflow the stack?

This function for example:

fn big_boxed_slice(init: u8) -> Box<[u8]> {
    Box::new([init; 10485760])
}

allocates a 10MiB array, and (currently) overflows in both debug and release mode.

The alternative with box expr (or boxed!) would be:

fn big_boxed_syntax_slice(init: u8) -> Box<[u8]> {
    boxed!([init; 10485760])
}

which doesn't overflow, but is equivalent to the already stable:

fn big_vec(init: u8) -> Box<[u8]> {
    vec![init; 10485760].into_boxed_slice()
}

Playground link

@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

I would prefer to add magic syntax and remove it in a future edition than add a macro with a magic implementation and be stuck with it in the std library forever.

I think I follow this train of thought that I don't think an intentionally temporary boxed! macro will really move us forwards, but if we did want to commit to a boxed! macro instead of the user-facing language feature to the point of linting against Box::new then I think that could be worth exploring. Once there is a stable way to avoid the stack blowup problem there's really no reason to ever consider the "broken" alternative, right?

Maybe this has come up before but have we ever thought about trying to make Box::new(expr) magically equivalent to the already magic box expr?

@kennytm
Copy link
Member

kennytm commented Jan 13, 2021

We do have #2884 for in-place construction at library level, at the cost of spec-guaranteed RVO.

// the closure's return is guaranteed to be written to box, 
// rather than written to stack then memcpy into the box
let mut hectapus = Box::with(|| Hectapus { arm_lengths: [0; 100] });

(I also think let x: Rc<T> = box T::new() is extremely confusing, since the Box type has already taken the name)

@slightlyoutofphase
Copy link

Maybe this has come up before but have we ever thought about trying to make Box::new(expr) magically equivalent to the already magic box expr?

They are equivalent, aren't they? The source code of Box::new() is this:

pub fn new(x: T) -> Self {
    box x
}

@comex
Copy link

comex commented Mar 29, 2021

Nope, because what's special about box is that it evaluates the initializer expression directly into the destination allocation, rather than first initializing an object on the stack, then allocating, and finally moving the object into the destination allocation. In the case of Box::new, x has to already exist on the stack, and be initialized, before Box::new starts running.

Note that LLVM's optimizer will sometimes optimize away the stack object, making Box::new generate the same assembly as box – but not always, and not when optimizations are disabled.

@slightlyoutofphase
Copy link

slightlyoutofphase commented Mar 30, 2021

Ah, I see what you mean. Like in the sense of x being moved into the function versus not. I was moreso just thinking about it as far as the end result of what they do. One thing I do like box_syntax for in any case is writing tests... doing stuff like [box 1, box 2, box 3, box 4] is a lot nicer to type than [Box::new(1), Box::new(2), Box::new(3), Box::new(4)].

@m-ou-se m-ou-se removed the T-lang Relevant to the language team, which will review and decide on the RFC. label Jun 2, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 2, 2021

The box syntax feature is not blocked on the syntax, but semantics. So whatever is blocking box is also blocking this proposed boxed!().

@rfcbot close

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 2, 2021

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

No concerns currently listed.

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 Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Jun 2, 2021
@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Jun 9, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 9, 2021

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

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Jun 19, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 19, 2021

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC is now closed.

@rfcbot rfcbot added to-announce closed This FCP has been closed (as opposed to postponed) and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Jun 19, 2021
@rfcbot rfcbot closed this Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.