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

More deriving on packed structs #104429

Merged
merged 1 commit into from Jan 30, 2023

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Nov 15, 2022

See here for the t-lang nomination summary, and here for the approval.

r? @RalfJung

@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 Nov 15, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 15, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@nnethercote
Copy link
Contributor Author

Note that the first three commits are from #104391, and have already been approved.

@RalfJung: even though you don't know the deriving code, there is enough stuff in this PR that you do know about (e.g. unsafe_derive_on_repr_packed) that I would like your review. If you want another co-reviewer as well I think that's fine.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 15, 2022

⌛ Trying commit 1e84976f23459b2afa61c169c440b98637d8f747 with merge a31ace633f6135f7232aa68f2493896256980c6a...

@bors
Copy link
Contributor

bors commented Nov 15, 2022

☀️ Try build successful - checks-actions
Build commit: a31ace633f6135f7232aa68f2493896256980c6a (a31ace633f6135f7232aa68f2493896256980c6a)

// This one is fine because it's not packed.
#[derive(Debug, Default)]
struct Y(usize);

// This one has an error because `Y` doesn't impl `Copy`.
// Note: there is room for improvement in the error message.
Copy link
Member

Choose a reason for hiding this comment

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

Yes there is indeed.^^ I noticed some other derive get really nice error messages but I have no idea how they do that. I suspect it is related to these AssertParamIs things?

// This one is an error because `Box<u32>` doesn't impl `Copy`.
let x: Foo<NonCopy> = Foo(NonCopy, NonCopy, NonCopy);
_ = x.clone();
//~^ ERROR the method `clone` exists for struct `Foo<NonCopy>`, but its trait bounds were not satisfied
Copy link
Member

Choose a reason for hiding this comment

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

This is where I wonder how we can best document this. Users might be surprised here that Clone doesn't work since they did derive(Clone) and all fields are Clone...

@RalfJung
Copy link
Member

I looked at the testcases only; there are some more that should be added:

  • non-generic with Copy+Clone (should get simple clone)
  • non-generic with only Clone, no Copy (should work -- just to make sure this kicks in even when there are no generics)
  • generic using an associated type (needs the extra bound on the where)

Looks like we don't even have a test for generic+associated without packed; that would be good to have, too.

The changes in check_packed_ref are fine, too.

I'll hand over review of the actual implementation to someone else, don't have the time to dig into the derive code currently, sorry. r? compiler

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 16, 2022

⌛ Trying commit f23921e2784a92f203f389cebc75bc7656d86124 with merge b2686d78d0034bb3435b348d5a63910b074a1990...

@bors
Copy link
Contributor

bors commented Nov 16, 2022

☀️ Try build successful - checks-actions
Build commit: b2686d78d0034bb3435b348d5a63910b074a1990 (b2686d78d0034bb3435b348d5a63910b074a1990)

@nnethercote
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-104429 created and queued.
🤖 Automatically detected try build b2686d78d0034bb3435b348d5a63910b074a1990
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-104429 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-104429 is completed!
📊 70 regressed and 15 fixed (248247 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors
Copy link
Contributor

bors commented Jan 30, 2023

⌛ Testing commit 2e93f2c with merge 3f25e56...

@bors
Copy link
Contributor

bors commented Jan 30, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 3f25e56 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 30, 2023
@bors bors merged commit 3f25e56 into rust-lang:master Jan 30, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 30, 2023
@bors bors mentioned this pull request Jan 30, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3f25e56): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.8% [-1.3%, -0.3%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.5% [3.2%, 3.8%] 2
Regressions ❌
(secondary)
3.7% [2.6%, 5.4%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.9% [-2.5%, -1.3%] 2
All ❌✅ (primary) 3.5% [3.2%, 3.8%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@Manishearth
Copy link
Member

@RalfJung FWIW I was under the impression we'd found all the cases where zerovec relies on this, but turns out we were producing this in a custom derive as well (unicode-org/icu4x#3069).

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2023

Ouch, strange that crater missed that.

The issue unicode-org/icu4x#3060 mentions str. We could probably treat that like [u8] and make it a future-compat warning instead of a hard error. Would that help?

@Manishearth
Copy link
Member

That would be great, yeah. It's already fixed on our end so it's not a huge deal, but I'm surprised this didn't crop up in future-incompat warnings at all.

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2023

Ah no there's also other types like MultiFieldsULE. I don't see a good way that we can detect those... other than the crude hack of just checking the name of the type (and of the crate it is defined in).

Do I understand correctly that right now the [u8] hack we added is useless because icu4x fails to build anyway?

I'm surprised this didn't crop up in future-incompat warnings at all.

We didn't find a way to have future-incompat warnings for this deriving change.

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2023

Specifically I think this is the list of types we'd have to treat like [u8]:

  • str
  • VarZeroSlice<str>
  • MultiFieldsULE

I think this is pre-name-resolution so we can only go by their name, we can't query their alignment or anything like that. Is there precedent for carrying hacks for specific type / crate names to ease a transition?

@Manishearth
Copy link
Member

Do I understand correctly that right now the [u8] hack we added is useless because icu4x fails to build anyway?

Yeah we use other types at the end; we have a ZeroSlice and VarZeroSlice and MultiFieldsULE in use in ICU4X right now I think.

I think it's still potentially useful for others though!

Is there precedent for carrying hacks for specific type / crate names to ease a transition?

I don't think so?

But again I've already fixed it in zerovec (pending PR merge) and it's compatible with the zerovec version used in ICU4X 1.0, so users canjust update.

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2023

I think it's still potentially useful for others though!

Ah, because it's a macro that could be used elsewhere?

Just adding str to the exception should be easy. @nnethercote do you have time to look into this?

@Manishearth
Copy link
Member

Ah, because it's a macro that could be used elsewhere?

Yep! The most common things to use it with are string and slice types.

@nnethercote
Copy link
Contributor Author

Yeah, I can add str.

@nnethercote
Copy link
Contributor Author

Yeah, I can add str.

#107713

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Apr 24, 2023
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * Sadly, the patch to reduce the cargo verbosity no longer applies,
   so I've asked upstream about the proper way to get the old result.
   (so the build log becomes Quite Bloated for now).

Upstream changes:

Version 1.69.0 (2023-04-20)
==========================

Language
--------

- [Deriving built-in traits on packed structs works with `Copy` fields.]
  (rust-lang/rust#104429)
- [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.]
  (rust-lang/rust#106774)
- [Improve analysis of trait bounds for associated types.]
  (rust-lang/rust#103695)
- [Allow associated types to be used as union fields.]
  (rust-lang/rust#106938)
- [Allow `Self: Autotrait` bounds on dyn-safe trait methods.]
  (rust-lang/rust#107082)
- [Treat `str` as containing `[u8]` for auto trait purposes.]
  (rust-lang/rust#107941)

Compiler
--------

- [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.]
  (rust-lang/rust#100178)
- [Rework min_choice algorithm of member constraints.]
  (rust-lang/rust#105300)
- [Support `true` and `false` as boolean flags in compiler arguments.]
  (rust-lang/rust#107043)
- [Default `repr(C)` enums to `c_int` size.]
  (rust-lang/rust#107592)

Libraries
---------

- [Implement the unstable `DispatchFromDyn` for cell types, allowing
  downstream experimentation with custom method receivers.]
  (rust-lang/rust#97373)
- [Document that `fmt::Arguments::as_str()` may return `Some(_)`
  in more cases after optimization, subject to change.]
  (rust-lang/rust#106823)
- [Implement `AsFd` and `AsRawFd` for `Rc`.]
  (rust-lang/rust#107317)

Stabilized APIs
---------------

- [`CStr::from_bytes_until_nul`]
  (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul)
- [`core::ffi::FromBytesUntilNulError`]
  (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html)

These APIs are now stable in const contexts:

- [`SocketAddr::new`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new)
- [`SocketAddr::ip`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip)
- [`SocketAddr::port`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port)
- [`SocketAddr::is_ipv4`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4)
- [`SocketAddr::is_ipv6`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6)
- [`SocketAddrV4::new`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new)
- [`SocketAddrV4::ip`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip)
- [`SocketAddrV4::port`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port)
- [`SocketAddrV6::new`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new)
- [`SocketAddrV6::ip`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip)
- [`SocketAddrV6::port`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port)
- [`SocketAddrV6::flowinfo`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo)
- [`SocketAddrV6::scope_id`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id)

Cargo
-----

- [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.]
  (rust-lang/cargo#11558)
- [Cargo now suggests `cargo add` if you try to install a library crate.]
  (rust-lang/cargo#11410)
- [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.]
  (rust-lang/cargo#11705)

Rustdoc
-----

- [Vertically compact trait bound formatting.]
  (rust-lang/rust#102842)
- [Only include stable lints in `rustdoc::all` group.]
  (rust-lang/rust#106316)
- [Compute maximum Levenshtein distance based on the query.]
  (rust-lang/rust#107141)
- [Remove inconsistently-present sidebar tooltips.]
  (rust-lang/rust#107490)
- [Search by macro when query ends with `!`.]
  (rust-lang/rust#108143)

Compatibility Notes
-------------------

- [The `rust-analysis` component from `rustup` now only contains
  a warning placeholder.]
  (rust-lang/rust#101841) This was primarily
  intended for RLS, and the corresponding `-Zsave-analysis` flag
  has been removed from the compiler as well.
- [Unaligned references to packed fields are now a hard error.]
  (rust-lang/rust#102513) This has been
  a warning since 1.53, and denied by default with a future-compatibility
  warning since 1.62.
- [Update the minimum external LLVM to 14.]
  (rust-lang/rust#107573)
- [Cargo now emits errors on invalid characters in a registry token.]
  (rust-lang/cargo#11600)
- [When `default-features` is set to false of a workspace dependency,
  and an inherited dependency of a member has `default-features =
  true`, Cargo will enable default features of that dependency.]
  (rust-lang/cargo#11409)
- [Cargo denies `CARGO_HOME` in the `[env]` configuration table.
  Cargo itself doesn't pick up this value, but recursive calls to
  cargo would, which was not intended.]
  (rust-lang/cargo#11644)
- [Debuginfo for build dependencies is now off if not explicitly
  set. This is expected to improve the overall build time.]

  (rust-lang/cargo#11252)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

- [Move `format_args!()` into AST (and expand it during AST lowering)]
  (rust-lang/rust#106745)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 4, 2023
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.

Upstream changes:

Version 1.69.0 (2023-04-20)
==========================

Language
--------

- [Deriving built-in traits on packed structs works with `Copy` fields.]
  (rust-lang/rust#104429)
- [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.]
  (rust-lang/rust#106774)
- [Improve analysis of trait bounds for associated types.]
  (rust-lang/rust#103695)
- [Allow associated types to be used as union fields.]
  (rust-lang/rust#106938)
- [Allow `Self: Autotrait` bounds on dyn-safe trait methods.]
  (rust-lang/rust#107082)
- [Treat `str` as containing `[u8]` for auto trait purposes.]
  (rust-lang/rust#107941)

Compiler
--------

- [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.]
  (rust-lang/rust#100178)
- [Rework min_choice algorithm of member constraints.]
  (rust-lang/rust#105300)
- [Support `true` and `false` as boolean flags in compiler arguments.]
  (rust-lang/rust#107043)
- [Default `repr(C)` enums to `c_int` size.]
  (rust-lang/rust#107592)

Libraries
---------

- [Implement the unstable `DispatchFromDyn` for cell types, allowing
  downstream experimentation with custom method receivers.]
  (rust-lang/rust#97373)
- [Document that `fmt::Arguments::as_str()` may return `Some(_)`
  in more cases after optimization, subject to change.]
  (rust-lang/rust#106823)
- [Implement `AsFd` and `AsRawFd` for `Rc`.]
  (rust-lang/rust#107317)

Stabilized APIs
---------------

- [`CStr::from_bytes_until_nul`]
  (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul)
- [`core::ffi::FromBytesUntilNulError`]
  (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html)

These APIs are now stable in const contexts:

- [`SocketAddr::new`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new)
- [`SocketAddr::ip`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip)
- [`SocketAddr::port`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port)
- [`SocketAddr::is_ipv4`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4)
- [`SocketAddr::is_ipv6`]
  (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6)
- [`SocketAddrV4::new`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new)
- [`SocketAddrV4::ip`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip)
- [`SocketAddrV4::port`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port)
- [`SocketAddrV6::new`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new)
- [`SocketAddrV6::ip`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip)
- [`SocketAddrV6::port`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port)
- [`SocketAddrV6::flowinfo`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo)
- [`SocketAddrV6::scope_id`]
  (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id)

Cargo
-----

- [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.]
  (rust-lang/cargo#11558)
- [Cargo now suggests `cargo add` if you try to install a library crate.]
  (rust-lang/cargo#11410)
- [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.]
  (rust-lang/cargo#11705)

Rustdoc
-----

- [Vertically compact trait bound formatting.]
  (rust-lang/rust#102842)
- [Only include stable lints in `rustdoc::all` group.]
  (rust-lang/rust#106316)
- [Compute maximum Levenshtein distance based on the query.]
  (rust-lang/rust#107141)
- [Remove inconsistently-present sidebar tooltips.]
  (rust-lang/rust#107490)
- [Search by macro when query ends with `!`.]
  (rust-lang/rust#108143)

Compatibility Notes
-------------------

- [The `rust-analysis` component from `rustup` now only contains
  a warning placeholder.]
  (rust-lang/rust#101841) This was primarily
  intended for RLS, and the corresponding `-Zsave-analysis` flag
  has been removed from the compiler as well.
- [Unaligned references to packed fields are now a hard error.]
  (rust-lang/rust#102513) This has been
  a warning since 1.53, and denied by default with a future-compatibility
  warning since 1.62.
- [Update the minimum external LLVM to 14.]
  (rust-lang/rust#107573)
- [Cargo now emits errors on invalid characters in a registry token.]
  (rust-lang/cargo#11600)
- [When `default-features` is set to false of a workspace dependency,
  and an inherited dependency of a member has `default-features =
  true`, Cargo will enable default features of that dependency.]
  (rust-lang/cargo#11409)
- [Cargo denies `CARGO_HOME` in the `[env]` configuration table.
  Cargo itself doesn't pick up this value, but recursive calls to
  cargo would, which was not intended.]
  (rust-lang/cargo#11644)
- [Debuginfo for build dependencies is now off if not explicitly
  set. This is expected to improve the overall build time.]

  (rust-lang/cargo#11252)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

- [Move `format_args!()` into AST (and expand it during AST lowering)]
  (rust-lang/rust#106745)
bors added a commit to rust-lang/crater that referenced this pull request Sep 4, 2023
add another exception

This failed with the same error in both runs in rust-lang/rust#104429.

Doesn't crater run both builds in the same clean environment? We seem to have a bunch of cases of "baseline always works but try build always fails". It almost seems like the 'try' run can see artifacts created by the baseline, and that breaks quite a few poorly written build scripts out there.
bors added a commit to rust-lang/crater that referenced this pull request Oct 27, 2023
add another exception

This failed with the same error in both runs in rust-lang/rust#104429.

Doesn't crater run both builds in the same clean environment? We seem to have a bunch of cases of "baseline always works but try build always fails". It almost seems like the 'try' run can see artifacts created by the baseline, and that breaks quite a few poorly written build scripts out there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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.

None yet