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.

Sign up for GitHub

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

Jump to bottom

Do not implement unsafe auto traits for types with unsafe fields #133934

Merged
merged 1 commit into from
Dec 7, 2024
Merged

Do not implement unsafe auto traits for types with unsafe fields #133934

merged 1 commit into from
Dec 7, 2024

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Dec 5, 2024

If a type has unsafe fields, its safety invariants are not simply the conjunction of its field types' safety invariants. Consequently, it's invalid to reason about the safety properties of these types in a purely structural manner — i.e., the manner in which auto traits are implemented. Consequently, auto implementations of unsafe auto traits should not be generated for types with unsafe fields.

Tracking: #132922

r? @compiler-errors

@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 Dec 5, 2024
compiler-errors
compiler-errors requested changes Dec 5, 2024
View reviewed changes
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.

Please adjust this logic in the new trait solver too. Also, there's no test as far as I can tell.

@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 Dec 5, 2024
@compiler-errors
Copy link
Member

Also make sure the test exercises the behavior in both the old and new solvers. You can do something like:

//@ revisions: current next
//@[next] compile-flags: -Znext-solver

@jswrenn jswrenn force-pushed the unsafe-fields-auto-traits branch from e6e322f to baec50e Compare December 5, 2024 20:28
@jswrenn
Copy link
Member Author

jswrenn commented Dec 5, 2024

Whoops, somehow failed to push the test I wrote. It's here: https://github.com/rust-lang/rust/compare/e6e322f79713b156d43d77be6135463ec2d4b6e7..baec50ee0b8afc7433c3bd2f2b862a21addd2f57

compiler-errors
compiler-errors reviewed Dec 5, 2024
View reviewed changes
tests/ui/unsafe-fields/auto-traits.rs Outdated

#![feature(auto_traits)]
#![feature(unsafe_fields)]
#![allow(dead_code, incomplete_features, unconditional_recursion)]
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the unconditional_recursion. No reason not to just split this out into a fn main() {} or something that just calls both of the functions.

Copy link
Member

Choose a reason for hiding this comment

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

No need for dead_code, that's enabled in the UI test suite unconditionally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jswrenn jswrenn force-pushed the unsafe-fields-auto-traits branch from baec50e to 9d604ce Compare December 5, 2024 22:44
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Dec 5, 2024
@jswrenn
Copy link
Member Author

jswrenn commented Dec 5, 2024

Please adjust this logic in the new trait solver too.

Done

Also make sure the test exercises the behavior in both the old and new solvers. You can do something like:

//@ revisions: current next
//@[next] compile-flags: -Znext-solver

Done

@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 Dec 5, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jswrenn
Copy link
Member Author

jswrenn commented Dec 5, 2024 •
edited
Loading

Yuck, look like I accidentally committed some things... Fixed!

@jswrenn jswrenn force-pushed the unsafe-fields-auto-traits branch from 9d604ce to 9ccf285 Compare December 5, 2024 23:18
compiler-errors
compiler-errors approved these changes Dec 5, 2024
View reviewed changes
compiler/rustc_type_ir/src/inherent.rs Outdated
@@ -136,6 +136,9 @@ pub trait Ty<I: Interner<Ty = Self>>:
matches!(self.kind(), ty::FnPtr(..))
}

/// Checks whether this type directly contains unsafe fields.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Checks whether this type directly contains unsafe fields.
/// Checks whether this type is an ADT that has unsafe fields.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2024

📌 Commit 9ccf285 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 Dec 5, 2024
@jswrenn
do not implement unsafe auto traits for types with unsafe fields
a122dde 
If a type has unsafe fields, its safety invariants are not simply
the conjunction of its field types' safety invariants. Consequently,
it's invalid to reason about the safety properties of these types
in a purely structural manner — i.e., the manner in which `auto`
traits are implemented.

Makes progress towards rust-lang#132922.
@jswrenn jswrenn force-pushed the unsafe-fields-auto-traits branch from 9ccf285 to a122dde Compare December 5, 2024 23:52
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 Commit a122dde has been approved by compiler-errors

It is now in the queue for this repository.

@jswrenn
Copy link
Member Author

jswrenn commented Dec 6, 2024

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Dec 6, 2024

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 Commit a122dde has been approved by compiler-errors

It is now in the queue for this repository.

@matthiaskrgr matthiaskrgr mentioned this pull request Dec 6, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133978 - matthiaskrgr:rollup-6gh1iho, r=matth…
86b922d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#130209 (Stabilize `std::io::ErrorKind::CrossesDevices`)
 - rust-lang#130254 (Stabilize `std::io::ErrorKind::QuotaExceeded`)
 - rust-lang#132187 (Add Extend impls for tuples of arity 1 through 12)
 - rust-lang#133875 (handle `--json-output` properly)
 - rust-lang#133934 (Do not implement unsafe auto traits for types with unsafe fields)
 - rust-lang#133954 (Hide errors whose suggestions would contain error constants or types)
 - rust-lang#133960 (rustdoc: remove eq for clean::Attributes)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
@bors
Auto merge of rust-lang#133978 - matthiaskrgr:rollup-6gh1iho, r=matth…
d79642c 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#130209 (Stabilize `std::io::ErrorKind::CrossesDevices`)
 - rust-lang#130254 (Stabilize `std::io::ErrorKind::QuotaExceeded`)
 - rust-lang#132187 (Add Extend impls for tuples of arity 1 through 12)
 - rust-lang#133875 (handle `--json-output` properly)
 - rust-lang#133934 (Do not implement unsafe auto traits for types with unsafe fields)
 - rust-lang#133954 (Hide errors whose suggestions would contain error constants or types)
 - rust-lang#133960 (rustdoc: remove eq for clean::Attributes)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
@bors
Auto merge of rust-lang#133978 - matthiaskrgr:rollup-6gh1iho, r=matth…
9c707a8 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#130209 (Stabilize `std::io::ErrorKind::CrossesDevices`)
 - rust-lang#130254 (Stabilize `std::io::ErrorKind::QuotaExceeded`)
 - rust-lang#132187 (Add Extend impls for tuples of arity 1 through 12)
 - rust-lang#133875 (handle `--json-output` properly)
 - rust-lang#133934 (Do not implement unsafe auto traits for types with unsafe fields)
 - rust-lang#133954 (Hide errors whose suggestions would contain error constants or types)
 - rust-lang#133960 (rustdoc: remove eq for clean::Attributes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6457761 into rust-lang:master Dec 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
@rust-timer
Unrolled build for rust-lang#133934
72e92c3 
Rollup merge of rust-lang#133934 - jswrenn:unsafe-fields-auto-traits, r=compiler-errors

Do not implement unsafe auto traits for types with unsafe fields

If a type has unsafe fields, its safety invariants are not simply the conjunction of its field types' safety invariants. Consequently, it's invalid to reason about the safety properties of these types in a purely structural manner — i.e., the manner in which `auto` traits are implemented. Consequently, auto implementations of unsafe auto traits should not be generated for types with unsafe fields.

Tracking: rust-lang#132922

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jswrenn @compiler-errors @rust-log-analyzer @bors @rustbot