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

relax adt unsizing requirements #80726

Merged
merged 2 commits into from
Feb 5, 2021
Merged

relax adt unsizing requirements #80726

merged 2 commits into from
Feb 5, 2021

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jan 5, 2021

Changes unsizing of structs in case the last struct field shares generic params with other adt fields which do not change.
This change is currently insta stable and changes the language, so it at least requires a lang fcp. I feel like the current state is fairly unintuitive.

An example for what's now allowed would be https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6dd331d23f5c9ffc8c978175aae2e967

struct A<T, U: ?Sized>(T, B<T, U>); // previously ERR
// struct A<T, U: ?Sized>(T, B<[u32; 1], U>); // ok
struct B<T, U: ?Sized>(T, U);

fn main() {
    let x = A([0; 1], B([0; 1], [0; 1]));
    let y: &A<[u32; 1], [u32]> = &x;
    assert_eq!(y.1.1.len(), 1);
}

@lcnr lcnr added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jan 5, 2021
@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Jan 5, 2021
@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 5, 2021
@joshtriplett
Copy link
Member

We discussed this in the @rust-lang/lang meeting today, but didn't come to any conclusions.

We felt that it would help to have a little more information about what exactly this is changing, beyond an example. The example helped, but some background on what exactly was allowed before and what is allowed now (rather than just the delta from the former to the latter) would make us much more confident making this change.

@lcnr
Copy link
Contributor Author

lcnr commented Jan 18, 2021

wasn't able to get to summarize the current status here, removing nomination and renominating after I finish the summary

@lcnr lcnr removed the I-nominated label Jan 18, 2021
@lcnr
Copy link
Contributor Author

lcnr commented Jan 23, 2021

The only ADT we implement Unsize for are structs. All other coercion remain the same so I am going to focus on that here.

Current implementation

A generic struct implements Unsize if and only if:

  • the tail field depends on at least one type or const parameter
  • all type and const parameters used in the last field must only be used in the last field
  • the target struct can be created from the source by replacing only the parameters found in the last struct field
  • the tail field implements Unsize from source to target

This PR

A generic struct implements Unsize if and only if:

  • the tail field depends on at least one type or const parameter not used in any other field
  • the target struct can be created from the source by replacing only the parameters only found in the last struct field
  • the tail field implements Unsize from source to target

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2021

Thanks for the write-up, @lcnr . The T-lang design team discussed this in our meeting today.

We're generally in favor of this change, but we wanted to push back on it being insta-stable. From skimming the PR, it seemed to us like there might be reasonable strategies for feature-gating this change. (Either by running old and new copies of the algorithm, or potentially switching from a boolean flag to a tri-state one when determining whether Unsize can be applied.)

Can you look into putting a feature gate in here?

@lcnr
Copy link
Contributor Author

lcnr commented Feb 3, 2021

added a feature gate, note that this means that activating feature(relaxed_struct_unsize) changes which trait impls exist which afaik is generally unsound to do.

I think this should mostly be a theoretical concern as Unsize isn't really used like an ordinary trait so that unsoundness would have to be intentionally caused. Or maybe we don't even care as one has to explicitly enable a feature idk 🤷

@oli-obk
Copy link
Contributor

oli-obk commented Feb 4, 2021

@bors r+

seems fine to me.

@bors
Copy link
Contributor

bors commented Feb 4, 2021

📌 Commit 031cce8 has been approved by oli-obk

@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 Feb 4, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 5, 2021
relax adt unsizing requirements

Changes unsizing of structs in case the last struct field shares generic params with other adt fields which do not change.
This change is currently insta stable and changes the language, so it at least requires a lang fcp. I feel like the current state is fairly unintuitive.

An example for what's now allowed would be https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6dd331d23f5c9ffc8c978175aae2e967
```rust
struct A<T, U: ?Sized>(T, B<T, U>); // previously ERR
// struct A<T, U: ?Sized>(T, B<[u32; 1], U>); // ok
struct B<T, U: ?Sized>(T, U);

fn main() {
    let x = A([0; 1], B([0; 1], [0; 1]));
    let y: &A<[u32; 1], [u32]> = &x;
    assert_eq!(y.1.1.len(), 1);
}
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2021
Rollup of 15 pull requests

Successful merges:

 - rust-lang#79554 (Generic associated types in trait paths)
 - rust-lang#80726 (relax adt unsizing requirements)
 - rust-lang#81307 (Handle `Span`s for byte and raw strings and add more detail )
 - rust-lang#81318 (rustdoc-json: Fix has_body)
 - rust-lang#81456 (Make remote-test-server easier to use with new targets)
 - rust-lang#81497 (rustdoc: Move `display_fn` struct inside `display_fn`)
 - rust-lang#81500 (Remove struct_type from union output)
 - rust-lang#81542 (Expose correct symlink API on WASI)
 - rust-lang#81676 (Add more information to the error code for 'crate not found')
 - rust-lang#81682 (Add additional bitset benchmarks)
 - rust-lang#81730 (Make `Allocator` object-safe)
 - rust-lang#81763 (Cleanup rustdoc pass descriptions a bit)
 - rust-lang#81767 (Update LayoutError/LayoutErr stability attributes)
 - rust-lang#81771 (Indicate change in RSS from start to end of pass in time-passes output)
 - rust-lang#81781 (Fix `install-awscli.sh` error in CI)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 676ff77 into rust-lang:master Feb 5, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 5, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
…sue, r=camelid

update tracking issue for `relaxed_struct_unsize`

forgot to do this before rust-lang#80726 got merged. The tracking issue is rust-lang#81793
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
…sue, r=camelid

update tracking issue for `relaxed_struct_unsize`

forgot to do this before rust-lang#80726 got merged. The tracking issue is rust-lang#81793
@lcnr lcnr added the F-relaxed_struct_unsize `#![feature(relaxed_struct_unsize)]` label Aug 26, 2021
@lcnr lcnr deleted the unsize-query branch August 26, 2021 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-relaxed_struct_unsize `#![feature(relaxed_struct_unsize)]` needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants