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

Don't mark "safe" intrinsics as unsafe #86327

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jun 15, 2021

A good example of this is intrinsics::abort.

Before:

Screenshot from 2021-06-15 14-58-42

After:

Screenshot from 2021-06-15 14-59-22

cc @jyn514
r? @lqd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 15, 2021

Can you add a test for this?

@GuillaumeGomez
Copy link
Member Author

Well, I have no idea how to do that... Only a few specific intrinsics listed in the typeck function are safe to call, and we don't run DOM tests on std/core. If you have an idea by any chance?

@jyn514
Copy link
Member

jyn514 commented Jun 15, 2021

I thought intrinsics were part of the compiler, not the standard library?

Anyway, you can replicate however the standard library does it in a standalone file with #![no_core] and then test that.

@GuillaumeGomez
Copy link
Member Author

Oh good idea!

@GuillaumeGomez
Copy link
Member Author

Done, thanks again for the tip!

@lqd
Copy link
Member

lqd commented Jun 15, 2021

Nicely done. The test is exactly what I was hoping for (thanks @jyn514).

Thanks for taking care of it so quickly !
@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2021

📌 Commit 58617eb492a72efb581884f48565a2e7e8c2dcfe has been approved by lqd

@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 15, 2021
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@lqd
Copy link
Member

lqd commented Jun 15, 2021

@bors r- until we check whether this is applied to all hir::ForeignItem and not just the intrinsics ?

@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 Jun 15, 2021
@GuillaumeGomez
Copy link
Member Author

I added the abi check.

@lqd
Copy link
Member

lqd commented Jun 15, 2021

Could we add something like this to the test ? core::intrinsics::needs_drop is safe as well

extern "C" {
    // @has 'foo/fn.needs_drop.html'
    // @has - '//pre[@class="rust fn"]' 'pub unsafe extern "C" fn needs_drop() -> !'
    pub fn needs_drop() -> !;
}

@GuillaumeGomez
Copy link
Member Author

@lqd Added your test as well!

@lqd
Copy link
Member

lqd commented Jun 15, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2021

📌 Commit f683040 has been approved by lqd

@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 15, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#85283 (Avoid possible filename collision in coverage tests)
 - rust-lang#86200 (Updates `Clone` docs for `Copy` comparison.)
 - rust-lang#86209 (fix minor wording/typo issues in core::option docs)
 - rust-lang#86242 (rustdoc- dont ICE on `ConstEvaluatable` predicates)
 - rust-lang#86280 (Add a regression test for issue-76510)
 - rust-lang#86293 (Allow to run only a few GUI tests)
 - rust-lang#86327 (Don't mark "safe" intrinsics as unsafe)
 - rust-lang#86345 (Remove some duplicate `char` assoc items on RELEASES.md)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 98d5842 into rust-lang:master Jun 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 16, 2021
@GuillaumeGomez GuillaumeGomez deleted the safe-intrinsics branch June 16, 2021 08:31
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants