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

safe transmute: use Assume struct to provide analysis options #100726

Merged
merged 3 commits into from
Sep 4, 2022

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Aug 18, 2022

This task was left as a TODO in #92268; resolving it brings BikeshedIntrinsicFrom more in line with the API defined in MCP411.

Before:

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:

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:

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 18, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@jswrenn
Copy link
Member Author

jswrenn commented Aug 18, 2022

I also attempted to implement Add and Sub for Assume (another part of MCP411):

#[unstable(feature = "transmutability", issue = "99571")]
#[rustc_const_unstable(feature = "transmutability", issue = "99571")]
impl const core::ops::Add for Assume {
    type Output = Assume;

    fn add(self, rhs: Assume) -> Assume {
        Assume {
            alignment: self.alignment || rhs.alignment,
            lifetimes: self.lifetimes || rhs.lifetimes,
            safety: self.safety || rhs.safety,
            validity: self.validity || rhs.validity,
        }
    }
}

#[unstable(feature = "transmutability", issue = "99571")]
#[rustc_const_unstable(feature = "transmutability", issue = "99571")]
impl const core::ops::Sub for Assume {
    type Output = Assume;

    fn sub(self, rhs: Assume) -> Assume {
        Assume {
            alignment: self.alignment && !rhs.alignment,
            lifetimes: self.lifetimes && !rhs.lifetimes,
            safety: self.safety && !rhs.safety,
            validity: self.validity && !rhs.validity,
        }
    }
}

...but attempting to use these impls in the UI tests results in errors; e.g.:

error[E0015]: cannot call non-const operator in constants
  --> /home/ubuntu/projects/rust/src/test/ui/transmutability/arrays/should_have_correct_length.rs:14:52
   |
LL |         Dst: BikeshedIntrinsicFrom<Src, Context, { Assume::SAFETY + Assume::VALIDITY }>
   |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: impl defined here, but it is not `const`
  --> /home/ubuntu/projects/rust/library/core/src/mem/transmutability.rs:65:1
   |
LL | impl const core::ops::Add for Assume {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: calls in constants are limited to constant functions, tuple structs and tuple variants

Anyone know what's going on here? The error message seems non-sensical.

@bors
Copy link
Contributor

bors commented Aug 22, 2022

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

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
resolves query instability issues, and probably better for performance
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.

small suggestion for the future: your first commit did 3 different things at once, it can be easier to review if they are done in separate commits.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit fbcc038 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 Sep 1, 2022
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
Copy link
Contributor

bors commented Sep 4, 2022

⌛ Testing commit fbcc038 with merge 8521a8c...

@bors
Copy link
Contributor

bors commented Sep 4, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 4, 2022
@bors bors merged commit 8521a8c into rust-lang:master Sep 4, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 4, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8521a8c): comparison URL.

Overall result: ❌✅ regressions and 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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.6% [1.0%, 2.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
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.

mean1 range count2
Regressions ❌
(primary)
2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)
4.0% [1.3%, 6.8%] 5
Improvements ✅
(primary)
-3.1% [-3.1%, -3.1%] 1
Improvements ✅
(secondary)
-3.4% [-9.8%, -1.4%] 7
All ❌✅ (primary) -0.5% [-3.1%, 2.0%] 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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.5% [-2.5%, -2.5%] 1

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@@ -93,6 +93,7 @@
#![warn(missing_debug_implementations)]
#![warn(missing_docs)]
#![allow(explicit_outlives_requirements)]
#![allow(incomplete_features)]
Copy link
Member

@RalfJung RalfJung Apr 5, 2024

Choose a reason for hiding this comment

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

I am quite concerned about this allow. That makes it way too easy to use features such as generic_const_exprs that are just not ready yet for use in the standard library.

This PR should at least have consulted t-types to ensure that adt_const_params is sufficiently stable that it can be used internally in the standard library. Cc @lcnr

But even then I think we actually want forbid(incomplete_features) here to avoid people just adding feature gates when it seems convenient. Or even better, set some flag in bootstrap which ensures that the entire sysroot is built without incomplete features.

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants