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

Enable rustdoc to document safe wasm intrinsics #85982

Merged
merged 1 commit into from
Jun 9, 2021
Merged

Enable rustdoc to document safe wasm intrinsics #85982

merged 1 commit into from
Jun 9, 2021

Conversation

alexcrichton
Copy link
Member

This commit fixes an issue not found during #84988 where rustdoc is used
to document cross-platform intrinsics but it was requiring that
functions which use #[target_feature] are unsafe erroneously, even
if they're WebAssembly specific. Rustdoc today, for example, already has
a special case where it enables annotations like
#[target_feature(enable = "simd128")] on platforms other than
WebAssembly. The purpose of this commit is to relax the "require all
#[target_feature] functions are unsafe" requirement for all targets
whenever rustdoc is running, enabling all targets to fully document
other targets, such as WebAssembly, where intrinsics functions aren't
always unsafe.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Jun 4, 2021
@alexcrichton alexcrichton mentioned this pull request Jun 4, 2021
Merged
jyn514
jyn514 reviewed Jun 4, 2021
View reviewed changes
compiler/rustc_typeck/src/collect.rs
//
// Note that this is also allowed if `actually_rustdoc` so
// if a target is documenting some wasm-specific code then
// it's not spuriously denied.
Copy link
Member

Choose a reason for hiding this comment

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

I really am not a fan of continuing to special-case rustdoc ... is there any way to avoid calling codegen_fn_attrs in rustdoc itself? Rustdoc shouldn't care what they are, right?

Copy link
Member

Choose a reason for hiding this comment

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

No, the codegen_fn_attrs query is forced in the check attr pass which ensures that many attributes are valid. Otherwise cargo check would completely ignore it and cargo build would ignore it for functions that aren't codegened.

@jyn514
Copy link
Member

jyn514 commented Jun 4, 2021 •
edited
Loading

Please add a test for this.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Jun 4, 2021
@alexcrichton
Copy link
Member Author

Sure, I've added a test. As for whether we can avoid calling the function, I don't know. I'm trying to basically just continue how target features are already handled differently for rustdoc.

@alexcrichton
Copy link
Member Author

@davidtwco are you ok reviewing this? I can try to find someone else if you're busy?

Or @jyn514 would you be up for reviewing?

@jyn514
Copy link
Member

jyn514 commented Jun 7, 2021

I'm trying to basically just continue how target features are already handled differently for rustdoc.

Do you happen to know where in the codebase that's handled? I'm not familiar with target_feature.

@alexcrichton
Copy link
Member Author

Sure yeah, that happens here --

rust/compiler/rustc_codegen_ssa/src/target_features.rs

Line 251 in f434217

if tcx.sess.opts.actually_rustdoc {

@jyn514
Copy link
Member

jyn514 commented Jun 7, 2021

Thanks! I'm not super happy about that special casing either, but it makes sense to keep them consistent.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2021

📌 Commit 8123570141f7ce1a57e69cd57a5e5c8acbe631d2 has been approved by jyn514

@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 7, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 7, 2021

Oh wait hold on, that test doesn't actually test anything. You need to either add a @Has check (see https://rustc-dev-guide.rust-lang.org/rustdoc-internals.html#dotting-is-and-crossing-ts) or move it to rustdoc-ui with a // check-pass annotation. Maybe there should be a tidy lint for rustdoc tests without @Has checks?

@bors r-

@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 7, 2021
@alexcrichton
Enable rustdoc to document safe wasm intrinsics
aba85ff 
This commit fixes an issue not found during rust-lang#84988 where rustdoc is used
to document cross-platform intrinsics but it was requiring that
functions which use `#[target_feature]` are `unsafe` erroneously, even
if they're WebAssembly specific. Rustdoc today, for example, already has
a special case where it enables annotations like
`#[target_feature(enable = "simd128")]` on platforms other than
WebAssembly. The purpose of this commit is to relax the "require all
`#[target_feature]` functions are `unsafe`" requirement for all targets
whenever rustdoc is running, enabling all targets to fully document
other targets, such as WebAssembly, where intrinsics functions aren't
always `unsafe`.
@alexcrichton
Copy link
Member Author

@bors: r=jyn514

Oh oops, moved. Thanks for reviewing!

@bors
Copy link
Contributor

bors commented Jun 8, 2021

📌 Commit aba85ff has been approved by jyn514

@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 8, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 8, 2021

No problem, thanks for the fix :)

@JohnTitor JohnTitor mentioned this pull request Jun 9, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2021
@bors
Auto merge of rust-lang#86160 - JohnTitor:rollup-8ark9x7, r=JohnTitor
c838138 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#85676 (Fix documentation style inconsistencies for IP addresses)
 - rust-lang#85715 (Document `From` impls in string.rs)
 - rust-lang#85791 (Add `Ipv6Addr::is_unicast`)
 - rust-lang#85957 (Display defaults on const params- rustdoc )
 - rust-lang#85982 (Enable rustdoc to document safe wasm intrinsics)
 - rust-lang#86121 (Forwarding implementation for Seek trait's stream_position method)
 - rust-lang#86124 (Include macro name in 'local ambiguity' error)
 - rust-lang#86128 (Refactor: Extract render_summary from render_impl.)
 - rust-lang#86142 (Simplify proc_macro code using Bound::cloned().)
 - rust-lang#86158 (Update books)
 - rust-lang#86159 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit da1a449 into rust-lang:master Jun 9, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@davidtwco davidtwco

Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/ 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
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
7 participants
@alexcrichton @rust-highfive @jyn514 @bors @bjorn3 @davidtwco @rustbot