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

Specify that packed types must derive, not implement, Copy #51143

Merged
merged 1 commit into from
Jun 3, 2018

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented May 28, 2018

No description provided.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 May 28, 2018
@cramertj
Copy link
Member

Are all of the autogenerated derive impls already smart enough to Copy out fields of packed Copy types when they use them? I would have assumed they'd still attempt to create references to the fields, which is unsound.

@Mark-Simulacrum
Copy link
Member Author

I'm not sure; the error message though somewhat indicates that they should be... it looks like there's some code in libsyntax_ext which does something to take care of it, but I don't follow it well enough to be sure. It seems somewhat unrelated to this change though.

@cramertj
Copy link
Member

I just checked the behavior with --pretty-expanded and it looks like adding Copy causes the compiler to omit references to fields. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 29, 2018

📌 Commit c604df6 has been approved by cramertj

@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 May 29, 2018
@cramertj
Copy link
Member

Ah, wait no-- it only omits the reference if the Copy impl was in derive-- it doesn't omit it in the case of manual Copy impls. Either that should be fixed (hard, I think) or this PR should be amended to check for a derive of Copy (rather than that the type implements Copy).

@bors r-

@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 May 29, 2018
@Mark-Simulacrum Mark-Simulacrum changed the title Don't warn on repr(packed) types that implement Copy Specify that packed types must derive, not implement, Copy May 30, 2018
@Mark-Simulacrum
Copy link
Member Author

There's actually not much point then -- the way derive is structured, if the type derives Copy, the packed derive will be "safe" and as such not generate this warning. I've updated the PR to change the warning message to indicate that the type must derive Copy, not just implement it; and reverted the other changes.

The long-term solution here is probably to drop the requirement that we "see" a Copy derive and generate code as-if the struct is Copy when we see that it's packed. Then we'd somehow ensure that structs who aren't Copy are errored early enough (before borrowck?) that the errors about moving out of a borrowed field don't get emitted. This is far harder though, and I don't have confidence in the correct approach, so I'm not going to work on that in this PR.

@cramertj
Copy link
Member

Sounds good to me. Thanks for the updates! r=me with travis passing.

@Mark-Simulacrum
Copy link
Member Author

@bors r=cramertj

@bors
Copy link
Contributor

bors commented May 30, 2018

📌 Commit 033d75d has been approved by cramertj

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2018
@bors
Copy link
Contributor

bors commented May 31, 2018

🔒 Merge conflict

@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 May 31, 2018
@Mark-Simulacrum
Copy link
Member Author

@bors r=cramertj

@bors
Copy link
Contributor

bors commented Jun 2, 2018

📌 Commit 5c37473 has been approved by cramertj

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 2, 2018
@bors
Copy link
Contributor

bors commented Jun 2, 2018

⌛ Testing commit 5c37473 with merge 0bdd29c7e43d60ce5e4e37ee7ce99770a057de65...

@bors
Copy link
Contributor

bors commented Jun 2, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 2, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-distcheck of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[02:58:28] Documenting stage2 std (x86_64-unknown-linux-gnu)
[02:58:28]     Checking core v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libcore)
[02:58:28]  Documenting core v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libcore)
[02:58:28]    Compiling std v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libstd)
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

1 similar comment
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-distcheck of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[02:58:28] Documenting stage2 std (x86_64-unknown-linux-gnu)
[02:58:28]     Checking core v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libcore)
[02:58:28]  Documenting core v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libcore)
[02:58:28]    Compiling std v0.0.0 (file:///checkout/obj/build/tmp/distcheck/src/libstd)
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Mark-Simulacrum
Copy link
Member Author

@bors retry - timeout

@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 Jun 2, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 3, 2018
…ertj

Specify that packed types must derive, not implement, Copy
bors added a commit that referenced this pull request Jun 3, 2018
Rollup of 6 pull requests

Successful merges:

 - #51143 (Specify that packed types must derive, not implement, Copy)
 - #51226 (Make Layout's align a NonZeroUsize)
 - #51297 (Fix run button style)
 - #51306 (impl Default for &mut str)
 - #51312 (Clarify the difference between get_mut and into_mut for OccupiedEntry)
 - #51313 (use type name in E0599 enum variant suggestion)

Failed merges:
@bors
Copy link
Contributor

bors commented Jun 3, 2018

⌛ Testing commit 5c37473 with merge 3575be6...

@bors bors merged commit 5c37473 into rust-lang:master Jun 3, 2018
@bors
Copy link
Contributor

bors commented Jun 3, 2018

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2018
@Mark-Simulacrum Mark-Simulacrum deleted the issue-50826 branch June 12, 2018 18:19
@RalfJung
Copy link
Member

Per @retep998's comment at #46043 (comment), this breaks WinAPI.

@retep998
Copy link
Member

It doesn't break winapi so much as simply document that things actually are broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants