-
Notifications
You must be signed in to change notification settings - Fork 1.6k
de-RFC: Remove unsized_locals #3829
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
base: master
Are you sure you want to change the base?
Conversation
f44fab9
to
f951ce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos.
I hope that this won't affect feasibility of #2884 (basically unsized_return). |
Co-authored-by: Jubilee <[email protected]> Co-authored-by: kennytm <[email protected]>
As previously mentioned, dynamic stack allocation can be dangerous and should not be used lightly. | ||
It's an advanced optimization feature that is best left untouched by default. | ||
As such, it behaves similarly to `unsafe` (but is not actually `unsafe`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this exaggerates the danger. For example, maybe you are writing code for an embedded system with a low amount of memory. You want to use trait objects to save on code size, but also don’t want to pull in an allocator. In this case, dynamic alloca could be very helpful for reducing memory usage.
And, as others have mentioned, there are many other safe and easy ways of overflowing the stack in Rust. We don’t generally call e.g. recursion an “advanced optimization feature” that’s so scary it’s “similar to unsafe
”.
|
||
Dynamic stack allocation also has its upsides. | ||
It is generally faster than heap allocation and can therefore improve performance in cases where the previously mentioned downsides are not a concern. | ||
Therefore, this RFC is not necessarily a rejection of the idea of dynamic stack allocation, but merely the way the `unsized_locals` feature exposes it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we still need to figure out the best way to expose the feature, but I think that the best way to do that is to implement it properly on nightly and enable experimentation. Right now, we have vague concerns about footguns and blowing the stack, but without concrete user feedback, it’s impossible to determine how severe the issue is or how the problem could be mitigated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. maybe lints could address the danger. Or maybe all unsized locals should be annotated in some way (e.g. let #[unsized] foo = …
). We can’t try those things out unless we have a proper implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then go implement it if you think it's such a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is, it’s hard to experiment with the frontend/syntax parts of the feature, if the codegen/backend is missing/broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No significant experimentation has happened for the past eight years. I wouldn't hold my beer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's actually the middle-end that we're missing, I think? Because MIR is kinda its own beast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No significant experimentation has happened for the past eight years.
Yes, because the middle/back-end implementation is broken, scaring nightly users away. If the compiler team would rather strip out that broken implementation than continue to let it rot, I have no objection. But rust-lang/compiler-team#630 is enough for that. IIUC this de-RFC would require anyone motivated to implement the feature properly, to go through the full RFC process first. I think that’s going too far. My preference would be to “downgrade” #1909 to an eRFC, instead of unaccepting it completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to an "experimental" RFC will not change that right now it is a gravestone of a feature that no one wants to proceed on. Yes, probably someone should go through the RFC process again, because right now there's not even a draft of a proposal for how it may be implemented in rustc's semantic layers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a gravestone of a feature that no one wants to proceed on
I wouldn’t say that. For example, someone recently put in the effort to fix the longstanding issue with unsized locals being misaligned. Clearly there are some people still interested in this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that what @tmiasko was touching on there involved fixes to parts of the code that were shared between unsized_locals
and unsized_fn_params
, because code that accepts unsized_fn_params
sometimes, due to the way codegen works, creates unsized temporaries, i.e. locals. That then requires handling that for unsized_fn_params
. Our current rude hack that addresses this involves blocking inlining for unsized_fn_params
, since that's one of the conditions to reach that.
Also see:
It'd be cool if unsized_return could return into some future unsized_local, which sounds impossible with the alloca crate. An unsized_local that's less natural feeling sounds reasonable though. Anyways, the alloca crate has only three dependents, so maybe nobody cares enough. |
text/3829-de-rfc-unsized-locals.md
Outdated
It is implemented in the type checker and codegen, but lacking MIR semantics and therefore unimplemented in the compile time function evaluator. | ||
This is very significant, as MIR semantics govern how the feature should behave precisely in the first place. | ||
Without them, they cannot work in `const` and optimizations are likely broken around them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to understand the “MIR semantics” issues in more detail. What exactly would the compiler team need to see before it would become possible to move forward? rust-lang/compiler-team#630 says:
The [MIR semantics] that const eval used to implement - before support was removed - were known to be awkward and surprising.
But doesn’t link to any of that discussion. It’s hard for people to step up and address the problems, without a clear summary of what they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even then, a detailed summary of the problems would also have to be updated for changes in MIR semantics since then, no? Which may be potentially quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found rust-lang/rust#48055 (comment) which seems to be what I was looking for, or at least a start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a link to the comment in 5bc1184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started a discussion about fixing the MIR on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/MIR.20semantics.20for.20unsized.20locals
I think de-RFCing the local variables part of this makes sense. The original FCP was
But it seems like what we'd ended up hitting is that we really do need a fairly drastic re-work of how this works internally (since we don't have a MIR semantic for it) and plausibly also a re-work of how the syntax should behave (because we don't necessarily want this to silently happen). Notably, like how the type ascription de-RFC wasn't a "we don't want type ascription at all" statement, I don't see this de-RFC as a statement that "we don't want allocas ever". If a feature with similar motivation comes back that's fine; this is just recognizing that the current state here doesn't seem to be stabilization-track and probably getting there deserves a new RFC. My understanding is that this de-RFC is intentionally not touching @rfcbot fcp merge |
Team member @scottmcm 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! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
Co-authored-by: scottmcm <[email protected]>
Co-authored-by: Josh Triplett <[email protected]>
When recursion is used, it is usually used on purpose, and while sometimes the potential for stack overflows is overlooked, the general feature is usually used on purpose. | ||
That said, recursion can certainly be dangerous in some contexts, but prior existing features are not a good reason to introduce more ways to blow the stack. | ||
|
||
The Linux kernel has spent a lot of time on getting VLAs (C's cousin to `unsized_locals`) [removed from the codebase](https://www.phoronix.com/news/Linux-Kills-The-VLA). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reiterating the point I made here in a now-resolved thread:
Rust has two main kinds of unsized types: slices and trait objects. Putting unsized slices on the stack is analogous to VLAs in C, and is footgunny and useless to about the same degree. However, putting trait objects on the stack—something which has no analogue in C—is much less likely to cause an overflow, and also offers far more utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably depends greatly on the trait involved. Trivially dyn AsRef<[u8]>
is not materially better than [u8]
on the stack, and dyn coroutines of various types can also be much too large to be good on the stack.
Though I do agree that dyn SqlBackend
or similar might have much more of a reasonable size expectation that might make putting it on the stack less worrisome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivially
dyn AsRef<[u8]>
is not materially better than[u8]
on the stack
And impl AsRef<[u8]>
is also going to be equally bad, but we allow that today. #3829 (comment)
While I'm OK with de-RFCing this, as probably I do want someone to write a fresh RFC, if we were to actually do this1, that made the case for how this should be done in the context of Rust as it is today, I don't want to commit to the following as a normative position:
Maybe that's correct, maybe it's not (as @Jules-Bertholet is arguing above). Probably I'd want to see an RFC arguing the case for what the right approach is (and read through the threads of all the prior RFCs), and I don't want to prejudge that. In accepting an RFC, we generally aren't necessarily accepting the motivation or other non-normative sections as the position of the team, so perhaps I could check a box anyway. But there's a lot of normative-sounding language in this RFC, e.g.,
and it's not clear to me what the intended normative sections are. If this can adjusted a bit so that it's clear that this RFC isn't committing normatively to this, but is rather raising this as one item that a future RFC should clearly speak to, and that the position taken here is the author's only, then I'll happily check a box. I'd be happy both to do that and for @Jules-Bertholet or others to pursue an experiment to see if dynamic stack allocation can be done correctly in Rust, and to explore the best way to expose that in the language. Footnotes
|
@rfcbot concern clarify-what-is-normative As mentioned, I'd like to avoid taking a normative position on how this might be done in the future, but in any event, we should clarify what exactly is normative here and therefore what our proposed FCP represents, so I'll file a concern to that effect. It'd be OK, I think, if the only normative aspect of this RFC is that, by virtue of this de-RFC, someone needs to write a new RFC in order to eventually stabilize this. |
To be clear, though I think the RFC’s language on this is too strong, I do agree that there is a real and serious concern here, and it would have to be addressed before stabilization.
I fully agree with this, this would address all my objections. |
|
||
[RFC 1808] proposed an `alloca` function, which was rejected because `alloca` does not really behave like a function. | ||
|
||
[RFC 1808] then changed to propose the the VLA syntax instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[RFC 1808] then changed to propose the the VLA syntax instead. | |
[RFC 1808] then changed to propose the VLA syntax instead. |
[posterior-art]: #posterior- | ||
|
||
The best prior art for this removal is of course the inspiration for the de-RFC format, the [type ascription de-RFC]. | ||
[MCP 630] can also be seen as prior art. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a reference to this de-RFC to the type ascription de-RFC as posterior art?
Original RFC
New RFC