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

Check that a box expression's type is Sized #88087

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

jesyspa
Copy link
Contributor

@jesyspa jesyspa commented Aug 16, 2021

This resolves issue 87935.

This makes E0161 (move from an unsized rvalue) much less common. I've replaced the test to use this case, when a boxed dyn trait is passed by value, but that isn't an error when unsized_locals is enabled. I think it may be possible to get rid of E0161 entirely by checking that case earlier, but I'm not sure if that's desirable?

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2021
@jesyspa jesyspa marked this pull request as ready for review August 16, 2021 17:14
@rust-log-analyzer

This comment has been minimized.

@jesyspa
Copy link
Contributor Author

jesyspa commented Aug 17, 2021

There's another possible implementation, where we issue E0161 for unsized box expressions even when unsized_locals are disabled. That approach keeps the checking in rustc_mir, but it currently emits duplicate errors for cases like rust fn foo(x: Box<[i32]>) { box *x; }

Given that the implementation in this PR gives a clearer error message, I think it's probably the better option.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

Why change the E0161 output?

The code will not emit this warning once box expressions require a sized
type (since that error is emitted earlier in the flow).
New tests also check that we're not triggering this error
over-zealously.
@jackh726
Copy link
Member

Thanks! Going to @bors rollup=never in case there are any subtle changes here that need to be bisected.

@bors r+

@jackh726 jackh726 closed this Aug 20, 2021
@jackh726 jackh726 reopened this Aug 20, 2021
@jackh726
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 20, 2021

📌 Commit c75a930 has been approved by jackh726

@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 Aug 20, 2021
@nbdd0121
Copy link
Contributor

I wonder if it's worth a new obligation cause code if we want to remove box syntax eventually anyway 🤔

@jackh726
Copy link
Member

Those are cheap :)

@bors
Copy link
Contributor

bors commented Aug 20, 2021

⌛ Testing commit c75a930 with merge 1e3d632...

@bors
Copy link
Contributor

bors commented Aug 21, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 1e3d632 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 21, 2021
@bors bors merged commit 1e3d632 into rust-lang:master Aug 21, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 21, 2021
@jesyspa
Copy link
Contributor Author

jesyspa commented Aug 21, 2021

Thanks for the review!

I wonder if it's worth a new obligation cause code if we want to remove box syntax eventually anyway thinking

I don't think this will complicate it, since check_box can be removed in its entirety when the syntax goes away. The cost should only be paid by code that uses box expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE with box syntax and unsized types
7 participants