Skip to content

Don't mark #[target_feature] safe fns as unsafe in rustdoc JSON. #143555

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obi1kenobi
Copy link
Member

Fixes #142655 by explicitly checking whether functions are safe but using #[target_feature], instead of relying on the FnHeader::is_unsafe() method which considers such functions unsafe.

I don't believe this merits a bump of the rustdoc JSON FORMAT_VERSION constant, since the format is unchanged and this is just a small bugfix.

r? aDotInTheVoid

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2025

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

  • It's intentional.
  • The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

@obi1kenobi
Copy link
Member Author

While reading up on the overall context, I noticed that @oli-obk had considered changing the Safety enum to express the #[target_feature] complexity in #134317 and appears to have decided in favor of a smaller change in #134353.

I'm not qualified to decide whether refactoring Safety or FnHeader to fully capture #[target_feature]'s complexity is worth it, nor what the best way to do it is. I just wanted to raise the fact that we now know the fix from #134353 isn't entirely without its own "easy to get wrong" aspects, so it may be worth another look.

Comment on lines 391 to 392
let is_unsafe =
header.is_unsafe() && !matches!(header.safety, HeaderSafety::SafeTargetFeatures);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this an exhaustive match on header.safety (not not header.safety()!).

Tho long term I think we'll want to change the rustdoc::json::FunctionHeader to just fully have the header safety information. Even normal rustdoc rendering should probably distinguish between safe functions and safe-in-some-contexts functions and have appropriate hints somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, thanks Oli!

// The type system's internal implementation details consider
// safe functions with the `#[target_feature]` attribute as analogous to unsafe functions.
// `header.is_unsafe()` returns `true` for them. But that's not the right decision
// for rustdoc, so we specifically exclude that case.
Copy link
Member

Choose a reason for hiding this comment

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

NIT: It might be nice to link back to #142655 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.

Done inside the exhaustive match's HeaderSafety::SafeTargetFeatures arm.

@aDotInTheVoid
Copy link
Member

Thanks!

r=me with oli's match thing changed. (You don't need to handle my nit).

@bors delegate+

@bors
Copy link
Collaborator

bors commented Jul 7, 2025

✌️ @obi1kenobi, you can now approve this pull request!

If @aDotInTheVoid told you to "r=me" after making some further change, please make that change, then do @bors r=@aDotInTheVoid

@obi1kenobi obi1kenobi force-pushed the pg/target-feature-not-unsafe-rustdoc-json branch from 239f472 to 7170412 Compare July 8, 2025 02:03
@obi1kenobi
Copy link
Member Author

@bors r=aDotInTheVoid

@bors
Copy link
Collaborator

bors commented Jul 8, 2025

📌 Commit 7170412 has been approved by aDotInTheVoid

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 8, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 8, 2025
…nsafe-rustdoc-json, r=aDotInTheVoid

Don't mark `#[target_feature]` safe fns as unsafe in rustdoc JSON.

Fixes rust-lang#142655 by explicitly checking whether functions are safe but using `#[target_feature]`, instead of relying on the `FnHeader::is_unsafe()` method which considers such functions unsafe.

I don't believe this merits a bump of the rustdoc JSON `FORMAT_VERSION` constant, since the format is unchanged and this is just a small bugfix.

r? aDotInTheVoid
bors added a commit that referenced this pull request Jul 8, 2025
Rollup of 6 pull requests

Successful merges:

 - #134628 (Make `Default` const and add some `const Default` impls)
 - #143555 (Don't mark `#[target_feature]` safe fns as unsafe in rustdoc JSON.)
 - #143600 (Update intro blurb in `wasm32-wasip1` docs)
 - #143603 (Clarify the meaning of `AttributeOrder::KeepFirst` and `AttributeOrder::KeepLast`)
 - #143620 (fix: Remove newline from multiple crate versions note)
 - #143622 (Add target maintainer information for mips64-unknown-linux-muslabi64)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend 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.

rustdoc-json: #[target_feature] incorrectly causes safe functions to be listed as unsafe
5 participants