Skip to content

Unimplement unsized_locals #141811

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Unimplement unsized_locals #141811

wants to merge 1 commit into from

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented May 31, 2025

Implements rust-lang/compiler-team#630

Tracking issue here: #111942

Note that this just removes the feature, not the implementation, and does not touch unsized_fn_params. This is because it is required to support Box<dyn FnOnce()>: FnOnce().

There may be more that should be removed (possibly in follow up prs)

  • the forget_unsized function and forget intrinsic.
  • the unsized_locals test directory; I've just fixed up the tests for now
  • various codegen support for unsized values and allocas

cc @JakobDegen @oli-obk @Noratrieb @programmerjake @bjorn3

@rustbot label F-unsized_locals

Fixes #79409

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2025

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 31, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 31, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

@rustbot rustbot added the F-unsized_locals `#![feature(unsized_locals)]` label May 31, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -1951,7 +1950,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
);
}

// When `unsized_fn_params` and `unsized_locals` are both not enabled,
// When `unsized_fn_params` is both not enabled,

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@mejrs mejrs force-pushed the bye_locals branch 2 times, most recently from 50c26a1 to 8f1e09e Compare May 31, 2025 20:30
@RalfJung
Copy link
Member

RalfJung commented May 31, 2025

Implements rust-lang/compiler-team#630

That's 2 years ago and so it's not indicative of the team position today. Some stuff happened since then since some people want to preserve the feature.

This will need a new MCP I think.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

I don't think anything changed about the biggest issues (MIR support being lacking), the unsoundness around alignment was just one small factor I'd say. No one has stepped up to actually resolve any of these issues in two years, which doesn't indicate a lot of interest in the feature to me. That said, if you think a re-MCP should be done, I'd be happy to help drafting that up (I imagine it would mostly just copy from the old MCP).
To me, two years is not that long in this case and I'm not aware of any significant changes in any area that would make the result different now (other than the one fix), so I don't think it warrants a new MCP.

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2025

The MIR representation of unsized locals still needs to be re-done from scratch, that is true.

@@ -247,6 +247,8 @@ declare_features! (
/// Allows unnamed fields of struct and union type
(removed, unnamed_fields, "1.83.0", Some(49804), Some("feature needs redesign")),
(removed, unsafe_no_drop_flag, "1.0.0", None, None),
/// Allows unsized rvalues at arguments and parameters.
(removed, unsized_locals, "CURRENT_RUSTC_VERSION", Some(48055), Some("removed due to soundness issues; see https://github.com/rust-lang/rust/issues/111942")),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(removed, unsized_locals, "CURRENT_RUSTC_VERSION", Some(48055), Some("removed due to soundness issues; see https://github.com/rust-lang/rust/issues/111942")),
(removed, unsized_locals, "CURRENT_RUSTC_VERSION", Some(48055), Some("removed due to implementation concerns; see https://github.com/rust-lang/rust/issues/111942")),

As pointed out by Ralf, it should be sound, just not sufficiently implemented

Copy link
Member

Choose a reason for hiding this comment

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

Wait yeah, you should apply this change.

@scottmcm
Copy link
Member

scottmcm commented Jun 1, 2025

Note that this just removes the feature, not the implementation

Maybe a dumb question, but why not remove the implementation? If the implementation is going to be there anyway, feels like having the feature is fine.

(My preference would be to remove the implementation too.)

@Noratrieb
Copy link
Member

The implementation should be removed too but not in this PR^^

@SparrowLii
Copy link
Member

This change makes sense. I know little about unsized locals, so I'd like to have someone more experienced take a look.
r? compiler

@compiler-errors
Copy link
Member

Yippee

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 11, 2025

📌 Commit 9376cd8 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2025
@@ -247,6 +247,8 @@ declare_features! (
/// Allows unnamed fields of struct and union type
(removed, unnamed_fields, "1.83.0", Some(49804), Some("feature needs redesign")),
(removed, unsafe_no_drop_flag, "1.0.0", None, None),
/// Allows unsized rvalues at arguments and parameters.
(removed, unsized_locals, "CURRENT_RUSTC_VERSION", Some(48055), Some("removed due to soundness issues; see https://github.com/rust-lang/rust/issues/111942")),
Copy link
Member

Choose a reason for hiding this comment

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

Wait yeah, you should apply this change.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 11, 2025
@compiler-errors
Copy link
Member

@bors r-

@compiler-errors
Copy link
Member

r=me after

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 11, 2025
@mejrs
Copy link
Contributor Author

mejrs commented Jun 11, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 11, 2025
@compiler-errors
Copy link
Member

ty

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 11, 2025

📌 Commit c821423 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2025
@rust-log-analyzer

This comment has been minimized.

@mejrs
Copy link
Contributor Author

mejrs commented Jun 12, 2025

Woops lol. I'll bless it tomorrow.

@compiler-errors
Copy link
Member

yeah, pls rebase

@bors r- delegate+

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 12, 2025
@bors
Copy link
Collaborator

bors commented Jun 12, 2025

✌️ @mejrs, you can now approve this pull request!

If @compiler-errors told you to "r=me" after making some further change, please make that change, then do @bors r=@compiler-errors

@rust-log-analyzer

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide F-unsized_locals `#![feature(unsized_locals)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE with unsizing an extern type