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

Tracking Issue for hint::assert_unchecked #119131

Closed
3 of 5 tasks
scottmcm opened this issue Dec 19, 2023 · 18 comments
Closed
3 of 5 tasks

Tracking Issue for hint::assert_unchecked #119131

scottmcm opened this issue Dec 19, 2023 · 18 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@scottmcm
Copy link
Member

scottmcm commented Dec 19, 2023

Feature gate: #![feature(hint_assert_unchecked)]

This is a tracking issue for ...

Public API

// core::hint

pub const unsafe fn assert_unchecked(p: bool);

Steps / History

Unresolved Questions

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Dec 19, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 26, 2023
Add `hint::assert_unchecked`

Libs-API expressed interest, modulo bikeshedding, in rust-lang/libs-team#315 (comment)

I think that means this is good for nightly, since we can always rename it before stabilization.

Tracking issue: rust-lang#119131
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 31, 2023
Add `hint::assert_unchecked`

Libs-API expressed interest, modulo bikeshedding, in rust-lang/libs-team#315 (comment)

I think that means this is good for nightly, since we can always rename it before stabilization.

Tracking issue: rust-lang/rust#119131
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 17, 2024
…<try>

Use `assert_unchecked` instead of `assume` intrinsic in the standard library

Now that a public wrapper for the `assume` intrinsic exists, we can use it in the standard library.

CC rust-lang#119131
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 19, 2024
…Nilstrieb

Use `assert_unchecked` instead of `assume` intrinsic in the standard library

Now that a public wrapper for the `assume` intrinsic exists, we can use it in the standard library.

CC rust-lang#119131
@scottmcm
Copy link
Member Author

Hello libs-api folks! I'm nominating this (at low priority) because when the open issue is the name, I think it mostly needs your input to make progress.

@scottmcm scottmcm added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 23, 2024
@bend-n
Copy link
Contributor

bend-n commented Jan 23, 2024

What if the name was safety_invariant?

    unsafe fn get_unchecked(self, slice: *const [T]) -> *const T {
        unsafe { slice.as_ptr().add(self) }
        unsafe {
            safety_invariant(self < slice.len());
            slice.as_ptr().add(self)
        }
    }

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 23, 2024
…Nilstrieb,cuviper

Use `assert_unchecked` instead of `assume` intrinsic in the standard library

Now that a public wrapper for the `assume` intrinsic exists, we can use it in the standard library.

CC rust-lang#119131
@Amanieu
Copy link
Member

Amanieu commented Jan 23, 2024

We discussed this in the libs-api meeting and we're happy with assert_unchecked because it follows the existing pattern of _unchecked functions in the standard library.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 23, 2024

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 23, 2024
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 30, 2024
@tgross35
Copy link
Contributor

tgross35 commented Feb 29, 2024

Todo list: docs could use an example

(+1 for the name assert_unchecked since it likely enables the same optimizations as assert)

@BurntSushi
Copy link
Member

@rfcbot concern doc-example

I agree that we should have a doc example showing when someone might want to use this method. The docs otherwise look good as written!

@bend-n
Copy link
Contributor

bend-n commented Mar 1, 2024

Suggestion for doc example: show example implementation of [T]::slice_unchecked?

@tgross35
Copy link
Contributor

tgross35 commented Apr 7, 2024

Could assert_unsafe_precondition! emit assert_unchecked when debug assertions are disabled? #123561 looks like a case where assuming the unsafe preconditions would have led to more efficient codegen, I am sure there are others.

Edit: I guess assert_unchecked calls assert_unsafe_precondition so calling intrinsics::assume would be correct.

@tgross35
Copy link
Contributor

tgross35 commented Apr 7, 2024

I am unsure whether the FCP at #119131 (comment) was purely for the unstable name change or an intent to stabilize; assuming the latter, I created a stabilization PR at #123588.

That PR should also resolve the docs example concern.

lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Add `hint::assert_unchecked`

Libs-API expressed interest, modulo bikeshedding, in rust-lang/libs-team#315 (comment)

I think that means this is good for nightly, since we can always rename it before stabilization.

Tracking issue: rust-lang/rust#119131
@scottmcm
Copy link
Member Author

scottmcm commented Apr 7, 2024

Could assert_unsafe_precondition! emit assert_unchecked when debug assertions are disabled?

The problem is that all those extra assumes in codegen have pretty high compile-time cost, and because they change inlining metrics can actually make runtime performance worse.

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add `hint::assert_unchecked`

Libs-API expressed interest, modulo bikeshedding, in rust-lang/libs-team#315 (comment)

I think that means this is good for nightly, since we can always rename it before stabilization.

Tracking issue: rust-lang/rust#119131
@BurntSushi
Copy link
Member

@rfcbot resolve doc-example

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 9, 2024
@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 9, 2024
@rfcbot
Copy link

rfcbot commented Jun 9, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 19, 2024
@rfcbot
Copy link

rfcbot commented Jun 19, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@FeldrinH
Copy link

What happens if the expression passed to assert_unchecked has a side effect? I could not find a clear answer in the current documentation.

@scottmcm
Copy link
Member Author

assert_unchecked is an ordinary function, not a macro. There's only a value passed to it; it works like a call to any other function.

@FeldrinH
Copy link

FeldrinH commented Jun 22, 2024

In that case I think the documentation could use a clarification on a few related points:

  • The documentation for __builtin_assume explicitly states that side effects of the expression passed to it are discarded. If this is not the case for assert_unchecked then that difference should probably be highlighted, since the documentation for assert_unchecked implies that assert_unchecked and __builtin_assume are equivalent.
  • Is the expression passed to assert_unchecked likely/guaranteed to be optimized out in a release compile if it has no side effects? This could matter quite a bit for microoptimizations.

@the8472
Copy link
Member

the8472 commented Jun 29, 2024

Is the expression passed to assert_unchecked likely/guaranteed to be optimized out in a release compile if it has no side effects?

If there are no other uses? Probably, at some point in the optimization pipeline. But up to that point the expression is preserved which can affect earlier optimizations. Which is one of the reasons why the docs say

[... ]but it might also make the generated code slower

The usual approach is to benchmark, look at the assembly or look at the LLVM IR after optimizations to see if it's worthwhile. Blind applications of the hint are discouraged.

jhpratt added a commit to jhpratt/rust that referenced this issue Jul 3, 2024
…d, r=dtolnay

Stabilize `hint::assert_unchecked`

Make the following API stable, including const:

```rust
// core::hint, std::hint

pub const unsafe fn assert_unchecked(p: bool);
```

This PR also reworks some of the documentation and adds an example.

Tracking issue: rust-lang#119131
FCP: rust-lang#119131 (comment). The docs update should resolve the remaining concern.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 3, 2024
Rollup merge of rust-lang#123588 - tgross35:stabilize-assert_unchecked, r=dtolnay

Stabilize `hint::assert_unchecked`

Make the following API stable, including const:

```rust
// core::hint, std::hint

pub const unsafe fn assert_unchecked(p: bool);
```

This PR also reworks some of the documentation and adds an example.

Tracking issue: rust-lang#119131
FCP: rust-lang#119131 (comment). The docs update should resolve the remaining concern.
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 4, 2024
@jhpratt
Copy link
Member

jhpratt commented Jul 19, 2024

Closing as completed by #123588.

@jhpratt jhpratt closed this as completed Jul 19, 2024
@tgross35 tgross35 added this to the 1.81.0 milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants