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

Initial implementation of transmutability trait. #92268

Merged
merged 13 commits into from
Aug 3, 2022

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Dec 25, 2021

T'was the night before Christmas and all through the codebase, not a miri was stirring — no hint of unsafe!

This PR provides an initial, incomplete implementation of MCP 411: Lang Item for Transmutability. The core::mem::BikeshedIntrinsicFrom trait provided by this PR is implemented on-the-fly by the compiler for types Src and Dst when the bits of all possible values of type Src are safely reinterpretable as a value of type Dst.

What this PR provides is:

What isn't yet implemented:

These features will be implemented in future PRs.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 25, 2021
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Dec 25, 2021
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I did not review the new crate at all yet

Comment on lines 888 to 889
if obligation.potentially_has_param_types_or_consts() {
return candidates.ambiguous = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here? Why can we set the ambiguity to false if there are generics involved? Because we know that all impls must be from trait bounds? Not sure that is true. There may be user impls on generic types giving us an ambiguous result, and now you're overwriting it as unambiguous. I don't know if this can happen or if it's fine as we'll later error out due to ambiguity, but this early return should be documented

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also deeply unsure about this code. If I comment it out, the compiler panics here while building stage 1 artifacts. I think I recall putting this check here after discovering that the trait definition itself was causing problems.

Really, I just stumbled my way through assemble_candidates_for_transmutability and would be very grateful for insight of what should go there.

@petrochenkov
Copy link
Contributor

r? @oli-obk

Yes,

/// `Src` is NOT transmutable into `Dst`.
No,
Copy link
Member Author

Choose a reason for hiding this comment

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

Re. better diagnostics: I suspect eventually we'll make this No carry information about the incompatibility that makes the transmute invalid. The challenge with this is going to be ensure that the single counter-example that's eventually surfaced in the diagnostic is the most useful one.

@jswrenn jswrenn force-pushed the transmute branch 2 times, most recently from 1d8176e to f848e36 Compare January 13, 2022 19:18
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 15, 2022

☔ The latest upstream changes (presumably #92915) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2022
@apiraino
Copy link
Contributor

apiraino commented Mar 3, 2022

Discussed in T-compiler meeting (on zulip).

I'm flipping the switch as it seems this PR is still a bit in progress (but don't hesitate to ask for comments or suggestions).

@rustbot author

@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 Mar 3, 2022
@JohnCSimon
Copy link
Member

@jswrenn

Ping from triage: Can you please post the status of this PR?
Thank you.

@jswrenn
Copy link
Member Author

jswrenn commented Jun 14, 2022

Hey @JohnCSimon, so sorry for missing your comment (I had just defended my thesis, went offline for a bit, and evidently missed a lot of notifications). This PR needs some minor work before it's ready to graduate out of draft status, namely: implementing the NFA lowering for enums and unions. I've budgeted that work for this Thursday. Basic support for references, error messages, and recursive types will come in follow-up PRs.

@rust-log-analyzer

This comment has been minimized.

@jswrenn
Copy link
Member Author

jswrenn commented Jun 16, 2022

Hm, I don't get the diagnostics about TyKind when I build locally. Anyone know why that might be?


/// Constructs an `Nfa` that describes the layout of the given discriminant.
pub fn from_disr(discr: Discr<'tcx>, tcx: TyCtxt<'tcx>) -> Self {
// FIXME(@jswrenn): I'm certain this is missing needed endian nuance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rustc_middle::mir::interpret::write_target_uint?

}
AdtKind::Enum => {
// is the layout well-defined?
if !(adt_def.repr().c() || adt_def.repr().int.is_some()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be && here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't this have a well-defined layout?

#[repr(u8)]
enum Foo { A, B, C }

Copy link
Contributor

@m1el m1el Jun 17, 2022

Choose a reason for hiding this comment

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

Yes, it does.
However, #[repr(u8)] enum Foo { A(u8,u32,u8) } does not
has a different representation from #[repr(u8, C)].

Edit: It was wrong

@jswrenn
Copy link
Member Author

jswrenn commented Jul 28, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit e8a1925 has been approved by oli-obk

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 Jul 28, 2022
@bors
Copy link
Contributor

bors commented Jul 30, 2022

⌛ Testing commit e8a1925 with merge 6ac6e3903baf610c3dfe8071e0919b0e5c1a480d...

@bors
Copy link
Contributor

bors commented Jul 30, 2022

💔 Test failed - checks-actions

@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 Jul 30, 2022
@rust-log-analyzer

This comment has been minimized.

@jswrenn
Copy link
Member Author

jswrenn commented Aug 2, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 2, 2022

📌 Commit 965ffb0 has been approved by oli-obk

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 Aug 2, 2022
@matthiaskrgr
Copy link
Member

@bors p=15 (moving before rollup, also pr is older than half a year already)

@bors
Copy link
Contributor

bors commented Aug 2, 2022

⌛ Testing commit 965ffb0 with merge e4417cf...

@bors
Copy link
Contributor

bors commented Aug 3, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e4417cf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2022
@bors bors merged commit e4417cf into rust-lang:master Aug 3, 2022
@rustbot rustbot added this to the 1.64.0 milestone Aug 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e4417cf): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
0.6% 1.4% 4
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
4.0% 4.8% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-2.8% -2.8% 1
Improvements 🎉
(secondary)
-4.1% -4.1% 1
All 😿🎉 (primary) -2.8% -2.8% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

jswrenn added a commit to jswrenn/rust that referenced this pull request Aug 22, 2022
This was left as a TODO in rust-lang#92268, and brings the trait more in
line with what was defined in MCP411.

`Assume::visibility` has been renamed to `Assume::safety`, as
library safety is what's actually being assumed; visibility is
just the mechanism by which it is currently checked (this may
change).

ref: rust-lang/compiler-team#411
ref: rust-lang#99571
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 3, 2022
safe transmute: use `Assume` struct to provide analysis options

This task was left as a TODO in rust-lang#92268; resolving it brings [`BikeshedIntrinsicFrom`](https://doc.rust-lang.org/nightly/core/mem/trait.BikeshedIntrinsicFrom.html) more in line with the API defined in [MCP411](rust-lang/compiler-team#411).

**Before:**
```rust
pub unsafe trait BikeshedIntrinsicFrom<
    Src,
    Context,
    const ASSUME_ALIGNMENT: bool,
    const ASSUME_LIFETIMES: bool,
    const ASSUME_VALIDITY: bool,
    const ASSUME_VISIBILITY: bool,
> where
    Src: ?Sized,
{}
```
**After:**
```rust
pub unsafe trait BikeshedIntrinsicFrom<Src, Context, const ASSUME: Assume = { Assume::NOTHING }>
where
    Src: ?Sized,
{}
```

`Assume::visibility` has also been renamed to `Assume::safety`, as library safety invariants are what's actually being assumed; visibility is just the mechanism by which it is currently checked (and that may change).

r? `@oli-obk`

---

Related:
- rust-lang/compiler-team#411
- rust-lang#99571
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2022
safe transmute: use `Assume` struct to provide analysis options

This task was left as a TODO in rust-lang#92268; resolving it brings [`BikeshedIntrinsicFrom`](https://doc.rust-lang.org/nightly/core/mem/trait.BikeshedIntrinsicFrom.html) more in line with the API defined in [MCP411](rust-lang/compiler-team#411).

**Before:**
```rust
pub unsafe trait BikeshedIntrinsicFrom<
    Src,
    Context,
    const ASSUME_ALIGNMENT: bool,
    const ASSUME_LIFETIMES: bool,
    const ASSUME_VALIDITY: bool,
    const ASSUME_VISIBILITY: bool,
> where
    Src: ?Sized,
{}
```
**After:**
```rust
pub unsafe trait BikeshedIntrinsicFrom<Src, Context, const ASSUME: Assume = { Assume::NOTHING }>
where
    Src: ?Sized,
{}
```

`Assume::visibility` has also been renamed to `Assume::safety`, as library safety invariants are what's actually being assumed; visibility is just the mechanism by which it is currently checked (and that may change).

r? `@oli-obk`

---

Related:
- rust-lang/compiler-team#411
- rust-lang#99571
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.