Skip to content

Emit explanatory note for move errors in packed struct derives #117511

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Nov 2, 2023

Derive expansions for packed structs with non-Copy fields cause move errors because they prefer copying over borrowing since borrowing the fields of a packed struct can result in unaligned access.

This underlying cause of the errors, however, is not apparent to the user. This PR adds a diagnostic note to make it clear to the user (the new note is on the second last line):

tests/ui/derives/deriving-with-repr-packed-move-errors.rs:13:16
   |
12 | #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Default)]
   |          ----- in this derive macro expansion
13 | struct StructA(String);
   |                ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
   |
   = note: `#[derive(Debug)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
   = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

Fixes #117406

Partially addresses #110777

@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2023

r? @oli-obk

(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 Nov 2, 2023
@gurry gurry force-pushed the 117406-err-packed-structs branch from b6c2cb3 to 8370eee Compare November 2, 2023 11:08
@gurry gurry force-pushed the 117406-err-packed-structs branch from 8370eee to 87b5e40 Compare November 3, 2023 01:49
Derive expansions for packed structs cause move errors because
they prefer copying over borrowing since borrowing the fields of a
packed struct can result in unaligned access and therefore undefined
behaviour.

This underlying cause of the errors, however, is not apparent
to the user. We add a diagnostic note here to remedy that.
@gurry gurry force-pushed the 117406-err-packed-structs branch from 87b5e40 to 4b3ece4 Compare November 3, 2023 02:02
@gurry
Copy link
Contributor Author

gurry commented Nov 3, 2023

@compiler-errors In your opinion, should this PR also close #110777? It is pretty much the same issue as #117406 but I was slightly hesitant to close it given the direction of the discussion in it.

@compiler-errors
Copy link
Member

@gurry does this PR add the note to #110777 as well? If so, then I consider that to be sufficient.

@gurry
Copy link
Contributor Author

gurry commented Nov 4, 2023

@gurry does this PR add the note to #110777 as well? If so, then I consider that to be sufficient.

Yeah, the two issues are reporting the same problem and therefore the additional note emitted through this PR will work for #110777 as well.

I'll leave #110777 open for now as per your suggestion.

Please review.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 6, 2023

📌 Commit 4b3ece4 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 Nov 6, 2023
@bors
Copy link
Collaborator

bors commented Nov 7, 2023

⌛ Testing commit 4b3ece4 with merge 7b97a5c...

@bors
Copy link
Collaborator

bors commented Nov 7, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 7b97a5c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 7, 2023
@bors bors merged commit 7b97a5c into rust-lang:master Nov 7, 2023
@rustbot rustbot added this to the 1.75.0 milestone Nov 7, 2023
@gurry gurry deleted the 117406-err-packed-structs branch November 7, 2023 07:30
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 7, 2023
89: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117006
* rust-lang/rust#117511
* rust-lang/rust#117641
  * rust-lang/rust#117637
  * rust-lang/rust#117631
  * rust-lang/rust#117516
  * rust-lang/rust#117190
* rust-lang/rust#117292
* rust-lang/rust#117603
* rust-lang/rust#116988
* rust-lang/rust#117630
  * rust-lang/rust#117615
  * rust-lang/rust#117613
  * rust-lang/rust#117592
* rust-lang/rust#117578
* rust-lang/rust#117435
* rust-lang/rust#117607



90: bump serde and serde_derive r=tshepang a=Dajamante

Trying to get around the failure seen in #86 

Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: Esteban Küber <esteban@kuber.com.ar>
Co-authored-by: SparrowLii <liyuan179@huawei.com>
Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de>
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com>
Co-authored-by: Michael Goulet <michael@errs.io>
Co-authored-by: Thom Chiovoloni <thom@shift.click>
Co-authored-by: klensy <klensy@users.noreply.github.com>
Co-authored-by: Jack Huey <31162821+jackh726@users.noreply.github.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Co-authored-by: hkalbasi <hamidrezakalbasi@protonmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Sven Marnach <sven@mozilla.com>
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>
Co-authored-by: aissata <aimaiga2@gmail.com>
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7b97a5c): comparison URL.

Overall result: ❌ regressions - 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.9% [0.9%, 0.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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)
0.9% [0.5%, 1.4%] 4
Regressions ❌
(secondary)
4.8% [4.6%, 5.0%] 2
Improvements ✅
(primary)
-0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
-1.0% [-1.0%, -1.0%] 1
All ❌✅ (primary) 0.6% [-0.6%, 1.4%] 5

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.6% [0.4%, 0.9%] 4
Regressions ❌
(secondary)
0.8% [0.8%, 0.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [0.4%, 0.9%] 4

Binary size

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)
0.0% [0.0%, 0.0%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 662.837s -> 662.627s (-0.03%)
Artifact size: 308.90 MiB -> 308.96 MiB (0.02%)

celinval added a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
Update Rust toolchain from nightly-2023-11-07 to nightly-2023-11-08
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@189d6c7
up to
rust-lang@7adc89b.
The log for this commit range is:
rust-lang@7adc89b69b Auto merge of
rust-lang#117680 - matthiaskrgr:rollup-kgaa4ma, r=matthiaskrgr
rust-lang@518fe492f1 Rollup merge of
rust-lang#117675 - zmodem:vectorize_h, r=durin42
rust-lang@f6f6fd1d1a Rollup merge of
rust-lang#117639 - rustbot:docs-update, r=ehuss
rust-lang@f8c67704f2 Rollup merge of
rust-lang#117616 - RalfJung:unstable-target-features, r=compiler-errors
rust-lang@cd5b5e08fe Rollup merge of
rust-lang#115485 - DaniPopes:rustdoc-macro-consts, r=jackh726,fmease
rust-lang@118a2deea5 Auto merge of
rust-lang#117617 - Urgau:bump-libc-0.2.150, r=Mark-Simulacrum
rust-lang@84abf837b8 manually bless a
wasm-only test
rust-lang@752a6132e5 llvm-wrapper: Remove
include of non-existant Vectorize.h
rust-lang@9bd71afb90 Auto merge of
rust-lang#115904 - notriddle:notriddle/preload-bold, r=GuillaumeGomez
rust-lang@187d1afa9d Auto merge of
rust-lang#117297 - clubby789:fn-trait-missing-paren, r=TaKO8Ki
rust-lang@61a3eea804 Auto merge of
rust-lang#117229 - matthewjasper:thir-unsafeck-fixes, r=cjgillot
rust-lang@114f1f6838 Auto merge of
rust-lang#117610 - compiler-errors:object-hmm, r=aliemjay
rust-lang@504f63efb0 Auto merge of
rust-lang#117418 - compiler-errors:better_error_body, r=oli-obk
rust-lang@4e0fb98a5c Auto merge of
rust-lang#117006 - estebank:issue-69512, r=compiler-errors
rust-lang@f926031ea5 When not finding
assoc fn on type, look for builder fn
rust-lang@7b97a5ca84 Auto merge of
rust-lang#117511 - gurry:117406-err-packed-structs, r=compiler-errors
rust-lang@5a9f07cc97 Build a better MIR
body when errors are encountered
rust-lang@171d5587ca Don't instantiate
the binder twice when assembling object candidate
rust-lang@24e14dd8b4 Only check
predicates for late-bound non-lifetime vars in object candidate assembly
rust-lang@bf65e3bddb Update books
rust-lang@868de8e76b Visit patterns in
THIR let expressions
rust-lang@2b59992736 Add suggestion to
THIR unsafe_op_in_unsafe_fn lint
rust-lang@2b2c0f9886 Allow tests with
rust-rustfix and revisions
rust-lang@931692fa13 Recognise thread
local statics in THIR unsafeck
rust-lang@b85c6835d0 warn when using an
unstable feature with -Ctarget-feature
rust-lang@15719a8c1d libc: bump
dependency to 0.2.150
rust-lang@4b3ece475d Emit explanatory
note for move errors in packed struct derives
rust-lang@904aceec7d Give a better
diagnostic for missing parens in Fn* bounds
rust-lang@2b858b7eb8 Format macro const
literals with pretty printer
rust-lang@2a92981301 rustdoc: stop
preloading Source Serif 4 Bold

Co-authored-by: celinval <celinval@users.noreply.github.com>
Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # 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. 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.

Debug on packed structs causes error on non-Copy fields
6 participants