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

compiler: allow transmute of ZST arrays with generics #114009

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Jul 24, 2023

Extend the SizeSkeleton evaluator to shortcut zero-sized arrays, thus considering [T; 0] to have a compile-time fixed-size of 0.

The existing evaluator already deals with generic arrays under the feature-guard transmute_const_generics. However, it merely allows comparing fixed-size types with fixed-size types, and generic types with generic types. For generic types, it merely compares whether their arguments match (ordering them first). Even if their exact sizes are not known at compile time, it can ensure that they will eventually be the same.

This patch extends this by shortcutting the size-evaluation of zero sized arrays and thus allowing size comparisons of () with [T; 0], where one contains generics and the other does not.

This code is guarded by transmute_const_generics (#109929), even though it is unclear whether it should be. However, this assumes that a separate stabilization PR is required to move this out of the feature guard.

Initially reported in #98104.

@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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. labels Jul 24, 2023
@dvdhrm
Copy link
Contributor Author

dvdhrm commented Jul 24, 2023

Note that this correctly calculates the size of [T; 0], as well as [[T; 0]; n], etc. However, it does not propagate through structs, newtypes, etc. This would require more changes to SizeSkeleton.

@apiraino
Copy link
Contributor

Based on this comment, I'm switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023
@JulianKnodt
Copy link
Contributor

Since I'm not a maintainer (but have worked on the feature prior), should someone who can approve review @apiraino?

@Dylan-DPC Dylan-DPC 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 Nov 4, 2023
@apiraino
Copy link
Contributor

Hello, checking the current status.

I'm sorry @JulianKnodt somehow I completely missed your question! If you're familiar with the codebase, feel free to comment, it's always helpful. But I think that the final r+ can only be emitted by someone with approval rights.

@dvdhrm was this comment addressed? Thanks for an update.

Rerolling reviewer since the current one is not available at this time

r? compiler

@rustbot rustbot assigned cjgillot and unassigned TaKO8Ki Nov 15, 2023
@cjgillot
Copy link
Contributor

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned cjgillot Nov 25, 2023
@apiraino
Copy link
Contributor

apiraino commented Nov 27, 2023

rerolling because the new assignee is not actively reviewing patches.

EDIT: oops

r? @wesleywiser

@rustbot rustbot assigned b-naber and wesleywiser and unassigned wesleywiser and b-naber Nov 27, 2023
@apiraino
Copy link
Contributor

rolling the dice 🎲 and finding another reviewer

r? compiler

@rustbot rustbot assigned cjgillot and unassigned wesleywiser Feb 29, 2024
@JulianKnodt
Copy link
Contributor

@apiraino can you find another reviewer? cjgillot already was assigned and then reassigned to someone else

@apiraino
Copy link
Contributor

apiraino commented Mar 5, 2024

fair point, sorry I didnt notice. if this dice throwing does not work I'll ask someone to adopt this review. Thank you for your patience @JulianKnodt

r? compiler

@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2024

This code is guarded by transmute_const_generics (#109929), even though it is unclear whether it should be. However, this assumes that a separate stabilization PR is required to move this out of the feature guard.

This code looks fine to me.

I was for a moment a little worried that it might introduce some insta-stable behavior change, but the fact quoted above(that this change is entirely contained under feature-guarded code, which is trivial to verify due to how the code is structured) was the detail that made me confident in an r+ here.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2024

📌 Commit 14be58a has been approved by pnkfelix

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 Mar 5, 2024
@JulianKnodt
Copy link
Contributor

@apiraino thanks for the continued support on this PR
More thanks to @dvdhrm who contributed this!

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 7, 2024
compiler: allow transmute of ZST arrays with generics

Extend the `SizeSkeleton` evaluator to shortcut zero-sized arrays, thus considering `[T; 0]` to have a compile-time fixed-size of 0.

The existing evaluator already deals with generic arrays under the feature-guard `transmute_const_generics`. However, it merely allows comparing fixed-size types with fixed-size types, and generic types with generic types. For generic types, it merely compares whether their arguments match (ordering them first). Even if their exact sizes are not known at compile time, it can ensure that they will eventually be the same.

This patch extends this by shortcutting the size-evaluation of zero sized arrays and thus allowing size comparisons of `()` with `[T; 0]`, where one contains generics and the other does not.

This code is guarded by `transmute_const_generics` (rust-lang#109929), even though it is unclear whether it should be. However, this assumes that a separate stabilization PR is required to move this out of the feature guard.

Initially reported in rust-lang#98104.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
…kingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#113525 (Dynamically size sigaltstk in std)
 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#116793 (Allow targets to override default codegen backend)
 - rust-lang#118623 (Improve std::fs::read_to_string example)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121089 (Remove `feed_local_def_id`)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#122087 (Add missing background color for top-level rust documentation page and increase contrast by setting text color to black)
 - rust-lang#122104 (Rust is a proper name: rust → Rust)
 - rust-lang#122110 (Make `x t miri` respect `MIRI_TEMP`)
 - rust-lang#122114 (Make not finding core a fatal error)
 - rust-lang#122115 (Cancel parsing ever made during recovery)
 - rust-lang#122126 (Fix `tidy --bless` on  ̶X̶e̶n̶i̶x̶ Windows)

r? `@ghost`
`@rustbot` modify labels: rollup
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Need to update ui test flag

tests/ui/transmute/transmute-zst-generics.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@bors r- #122131 (comment)

@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 Mar 7, 2024
Extend the `SizeSkeleton` evaluator to shortcut zero-sized arrays, thus
considering `[T; 0]` to have a compile-time fixed-size of 0.

The existing evaluator already deals with generic arrays under the
feature-guard `transmute_const_generics`. However, it merely allows
comparing fixed-size types with fixed-size types, and generic types with
generic types. For generic types, it merely compares whether their
arguments match (ordering them first). Even if their exact sizes are not
known at compile time, it can ensure that they will eventually be the
same.

This patch extends this by shortcutting the size-evaluation of zero
sized arrays and thus allowing size comparisons of `()` with `[T; 0]`,
where one contains generics and the other does not.

This code is guarded by `transmute_const_generics` (rust-lang#109929), even
though it is unclear whether it should be. However, this assumes that a
separate stabilization PR is required to move this out of the feature
guard.

Initially reported in rust-lang#98104.
@dvdhrm
Copy link
Contributor Author

dvdhrm commented Mar 20, 2024

(force-pushed: updated ui-test-flag and added the previously mentioned tests)

@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 Mar 20, 2024
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2024

📌 Commit 31d23c4 has been approved by pnkfelix

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 Mar 22, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#122195 (Note that the caller chooses a type for type param)
 - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish)
 - rust-lang#122784 (Add `tag_for_variant` query)
 - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`)
 - rust-lang#122873 (Merge my contributor emails into one using mailmap)
 - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups)
 - rust-lang#122888 (add a couple more tests)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#122195 (Note that the caller chooses a type for type param)
 - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish)
 - rust-lang#122784 (Add `tag_for_variant` query)
 - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`)
 - rust-lang#122873 (Merge my contributor emails into one using mailmap)
 - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups)
 - rust-lang#122888 (add a couple more tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 104c4bc into rust-lang:master Mar 23, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
Rollup merge of rust-lang#114009 - dvdhrm:pr/transmzst, r=pnkfelix

compiler: allow transmute of ZST arrays with generics

Extend the `SizeSkeleton` evaluator to shortcut zero-sized arrays, thus considering `[T; 0]` to have a compile-time fixed-size of 0.

The existing evaluator already deals with generic arrays under the feature-guard `transmute_const_generics`. However, it merely allows comparing fixed-size types with fixed-size types, and generic types with generic types. For generic types, it merely compares whether their arguments match (ordering them first). Even if their exact sizes are not known at compile time, it can ensure that they will eventually be the same.

This patch extends this by shortcutting the size-evaluation of zero sized arrays and thus allowing size comparisons of `()` with `[T; 0]`, where one contains generics and the other does not.

This code is guarded by `transmute_const_generics` (rust-lang#109929), even though it is unclear whether it should be. However, this assumes that a separate stabilization PR is required to move this out of the feature guard.

Initially reported in rust-lang#98104.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 4, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#122195 (Note that the caller chooses a type for type param)
 - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish)
 - rust-lang#122784 (Add `tag_for_variant` query)
 - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`)
 - rust-lang#122873 (Merge my contributor emails into one using mailmap)
 - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups)
 - rust-lang#122888 (add a couple more tests)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.