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

rustdoc: Document lack of object safety on affected traits #113241

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

poliorcetics
Copy link
Contributor

Closes #85138

I saw the issue didn't have any recent activity, if there is another MR for it I missed it.

I want the issue to move forward so here is my proposition.

It takes some space just before the "Implementors" section and only if the trait is not object
safe since it is the only case where special care must be taken in some cases and this has the
benefit of avoiding generation of HTML in (I hope) the common case.

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@poliorcetics poliorcetics force-pushed the 85138-doc-object-safety branch from 9170170 to a5a5756 Compare July 1, 2023 17:18
@poliorcetics
Copy link
Contributor Author

Screenshot of the new 'Object Safety' section just above the 'Implementors' one on the Default trait, also showing the link to the section in the sidebar

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks! Please add object-safety to the list of IDs in file src/librustdoc/html/markdown.rs, function init_id_map.

src/librustdoc/html/render/print_item.rs Outdated Show resolved Hide resolved
@poliorcetics poliorcetics force-pushed the 85138-doc-object-safety branch from a5a5756 to bb5801e Compare July 8, 2023 15:36
@rust-log-analyzer

This comment has been minimized.

@poliorcetics poliorcetics force-pushed the 85138-doc-object-safety branch from bb5801e to 41b9649 Compare July 8, 2023 15:47
@GuillaumeGomez
Copy link
Member

Since it's making UI changes, let's start an FCP.

@rfcbot fcp start

@GuillaumeGomez
Copy link
Member

Wrong command...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 10, 2023

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

No concerns currently listed.

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 Jul 10, 2023
@poliorcetics
Copy link
Contributor Author

By the way if someone better versed in it can confirm I made the section & link accessible it would be nice, I don't know what is needed for this

@poliorcetics
Copy link
Contributor Author

@Nemo157 I think this PR is only waiting for review/approval, if you have the time

@GuillaumeGomez
Copy link
Member

Soory, I completely missed this FCP when I was filling the agenda. I added it in the rustdoc agenda to be discussed next meeting if it hasn't been approved until then.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 20, 2023
@poliorcetics poliorcetics force-pushed the 85138-doc-object-safety branch from 70aa87b to a119158 Compare October 29, 2023 21:57
@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 Oct 30, 2023
@rfcbot
Copy link

rfcbot commented Oct 30, 2023

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.

@GuillaumeGomez
Copy link
Member

Thanks for the improvement!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 31, 2023

📌 Commit a119158 has been approved by GuillaumeGomez

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 Oct 31, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#113241 (rustdoc: Document lack of object safety on affected traits)
 - rust-lang#117388 (Turn const_caller_location from a query to a hook)
 - rust-lang#117417 (Add a stable MIR visitor)
 - rust-lang#117439 (prepopulate opaque ty storage before using it)
 - rust-lang#117451 (Add support for pre-unix-epoch file dates on Apple platforms (rust-lang#108277))

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#113241 (rustdoc: Document lack of object safety on affected traits)
 - rust-lang#117388 (Turn const_caller_location from a query to a hook)
 - rust-lang#117417 (Add a stable MIR visitor)
 - rust-lang#117439 (prepopulate opaque ty storage before using it)
 - rust-lang#117451 (Add support for pre-unix-epoch file dates on Apple platforms (rust-lang#108277))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 51b275b into rust-lang:master Nov 1, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 1, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2023
Rollup merge of rust-lang#113241 - poliorcetics:85138-doc-object-safety, r=GuillaumeGomez

rustdoc: Document lack of object safety on affected traits

Closes rust-lang#85138

I saw the issue didn't have any recent activity, if there is another MR for it I missed it.

I want the issue to move forward so here is my proposition.

It takes some space just before the "Implementors" section and only if the trait is **not** object
safe since it is the only case where special care must be taken in some cases and this has the
benefit of avoiding generation of HTML in (I hope) the common case.
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 1, 2023
78: Automated pull from upstream `master` r=tshepang a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#113970
* rust-lang/rust#117459
  * rust-lang/rust#117451
  * rust-lang/rust#117439
  * rust-lang/rust#117417
  * rust-lang/rust#117388
  * rust-lang/rust#113241
* rust-lang/rust#117462
* rust-lang/rust#117450
* rust-lang/rust#117407
* rust-lang/rust#117444
  * rust-lang/rust#117438
  * rust-lang/rust#117421
  * rust-lang/rust#117416
  * rust-lang/rust#116712
  * rust-lang/rust#116267
* rust-lang/rust#117377
* rust-lang/rust#117419



Co-authored-by: Alexis (Poliorcetics) Bourget <[email protected]>
Co-authored-by: Esteban Küber <[email protected]>
Co-authored-by: David Tolnay <[email protected]>
Co-authored-by: Celina G. Val <[email protected]>
Co-authored-by: Michael Goulet <[email protected]>
Co-authored-by: bors <[email protected]>
Co-authored-by: Camille GILLOT <[email protected]>
Co-authored-by: lcnr <[email protected]>
Co-authored-by: Zalathar <[email protected]>
Co-authored-by: Oli Scherer <[email protected]>
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
@obi1kenobi
Copy link
Member

Would it be possible to expose this data in rustdoc JSON as well?

It would be very useful to cargo-semver-checks, since object safety is public API and upholding it is a tricky semver hazard.

@GuillaumeGomez
Copy link
Member

Sure.

@obi1kenobi
Copy link
Member

Thank you! 🚀

Another thing that would be super useful to have (it would unlock many new lints) is knowing whether a trait is sealed or not. This is not currently shown in either HTML or JSON output. In theory it is possible to compute given sufficient rustdoc JSONs (including the stdlib), but it's quite hard to do since we don't have all of the compiler's infrastructure available and there are so many different ways to seal a trait (partial list here).

@GuillaumeGomez
Copy link
Member

So a trait that has a requirement for a local trait (based on this blog post). One question I have is: if the required private trait has a blanket impl, is the trait still sealed?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 23, 2023
…e-json, r=aDotInTheVoid

[rustdoc] Add `is_object_safe` information for traits in JSON output

As asked by `@obi1kenobi` [here](rust-lang#113241 (comment)).

cc `@aDotInTheVoid`
r? `@notriddle`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2023
Rollup merge of rust-lang#119246 - GuillaumeGomez:trait-is_object_safe-json, r=aDotInTheVoid

[rustdoc] Add `is_object_safe` information for traits in JSON output

As asked by `@obi1kenobi` [here](rust-lang#113241 (comment)).

cc `@aDotInTheVoid`
r? `@notriddle`
@obi1kenobi
Copy link
Member

One question I have is: if the required private trait has a blanket impl, is the trait still sealed?

The trait is still sealed if the blanket impl has a bound on a trait that is itself sealed. Otherwise, it is not sealed -- it can be impl'd outside its defining crate.

So a trait that has a requirement for a local trait (based on this blog post).

There are a few more ways to make a trait not implementable outside its crate. I believe this to be an exhaustive list:

  • if the trait has a sealed supertrait (sealed in any way, not just pub-in-priv)
  • if the trait has a "sealed method" without a default impl, in any of the ways that allow sealing a method:
    • a pub-in-priv type as a parameter or return type (described here)
    • a sealed trait as a bound (described here)
  • if the trait has an associated type with a sealed trait as a bound
  • if the trait has an associated const of a pub-in-priv type that doesn't have a default

This is why it's so hard to determine whether a trait is sealed or not without all the compiler machinery being present 😅

@GuillaumeGomez
Copy link
Member

Sounds like it would be quite tricky to check. However I think the information would be important to have. Can you open a new issue so the rustdoc team can discuss about it please? (With all the explanations too :) )

@Nemo157
Copy link
Member

Nemo157 commented Dec 24, 2023

https://rust-lang.github.io/rfcs/3323-restrictions.html adds native syntax for sealed traits, it might be better to only support that (which will presumably be carried into the rustdoc-json somehow) and just require users of the existing hacks to migrate (which should be non-breaking, other than MSRV).

@obi1kenobi
Copy link
Member

Opened #119280 with more precise descriptions of when a trait is sealed.

To be honest, even if that RFC were to become available on stable tomorrow, I don't expect users of the existing approaches will migrate anytime soon. The linked issue has details, but TL;DR maintainers have minimal incentive to change code that from their perspective 100% works fine just to adopt a new syntax.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2024
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 16, in accordance
   with the upstream changes.
 * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported.
 * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded
   LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it,
   resulting in an illegal instruction fault during the build.
   Ref. rust-lang/rust#117231

Upstream changes:

Version 1.75.0 (2023-12-28)
==========================

- [Stabilize `async fn` and return-position `impl Trait` in traits.]
  (rust-lang/rust#115822)
- [Allow function pointer signatures containing `&mut T` in `const` contexts.]
  (rust-lang/rust#116015)
- [Match `usize`/`isize` exhaustively with half-open ranges.]
  (rust-lang/rust#116692)
- [Guarantee that `char` has the same size and alignment as `u32`.]
  (rust-lang/rust#116894)
- [Document that the null pointer has the 0 address.]
  (rust-lang/rust#116988)
- [Allow partially moved values in `match`.]
  (rust-lang/rust#103208)
- [Add notes about non-compliant FP behavior on 32bit x86 targets.]
  (rust-lang/rust#113053)
- [Stabilize ratified RISC-V target features.]
  (rust-lang/rust#116485)

Compiler
--------

- [Rework negative coherence to properly consider impls that only
  partly overlap.] (rust-lang/rust#112875)
- [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.]
  (rust-lang/rust#116493)
- [Consider alias bounds when computing liveness in NLL.]
  (rust-lang/rust#116733)
- [Add the V (vector) extension to the `riscv64-linux-android` target spec.]
  (rust-lang/rust#116618)
- [Automatically enable cross-crate inlining for small functions]
  (rust-lang/rust#116505)
- Add several new tier 3 targets:
    - [`csky-unknown-linux-gnuabiv2hf`]
      (rust-lang/rust#117049)
    - [`i586-unknown-netbsd`]
      (rust-lang/rust#117170)
    - [`mipsel-unknown-netbsd`]
      (rust-lang/rust#117356)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.]
  (rust-lang/rust#96979)
- [Implement `BufRead` for `VecDeque<u8>`.]
  (rust-lang/rust#110604)
- [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.]
  (rust-lang/rust#110729)
- [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.]
  (rust-lang/rust#113747)
- [Implement `Default` for `ExitCode`.]
  (rust-lang/rust#114589)
- [Guarantee representation of None in NPO]
  (rust-lang/rust#115333)
- [Document when atomic loads are guaranteed read-only.]
  (rust-lang/rust#115577)
- [Broaden the consequences of recursive TLS initialization.]
  (rust-lang/rust#116172)
- [Windows: Support sub-millisecond sleep.]
  (rust-lang/rust#116461)
- [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl]
  (rust-lang/rust#100806)
- [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.]
  (rust-lang/rust#115108)

Stabilized APIs
---------------

- [`Atomic*::from_ptr`]
  (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr)
- [`FileTimes`]
  (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html)
- [`FileTimesExt`]
  (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html)
- [`File::set_modified`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified)
- [`File::set_times`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times)
- [`IpAddr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical)
- [`Ipv6Addr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical)
- [`Option::as_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice)
- [`Option::as_mut_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice)
- [`pointer::byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add)
- [`pointer::byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset)
- [`pointer::byte_offset_from`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from)
- [`pointer::byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub)
- [`pointer::wrapping_byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add)
- [`pointer::wrapping_byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset)
- [`pointer::wrapping_byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub)

These APIs are now stable in const contexts:

- [`Ipv6Addr::to_ipv4_mapped`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped)
- [`MaybeUninit::assume_init_read`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read)
- [`MaybeUninit::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed)
- [`mem::discriminant`]
  (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html)
- [`mem::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html)

Cargo
-----

- [Add new packages to `[workspace.members]` automatically.]
  (rust-lang/cargo#12779)
- [Allow version-less `Cargo.toml` manifests.]
  (rust-lang/cargo#12786)
- [Make browser links out of HTML file paths.]
  (rust-lang/cargo#12889)

Rustdoc
-------

- [Accept less invalid Rust in rustdoc.]
  (rust-lang/rust#117450)
- [Document lack of object safety on affected traits.]
  (rust-lang/rust#113241)
- [Hide `#[repr(transparent)]` if it isn't part of the public ABI.]
  (rust-lang/rust#115439)
- [Show enum discriminant if it is a C-like variant.]
  (rust-lang/rust#116142)

Compatibility Notes
-------------------

- [FreeBSD targets now require at least version 12.]
  (rust-lang/rust#114521)
- [Formally demote tier 2 MIPS targets to tier 3.]
  (rust-lang/rust#115238)
- [Make misalignment a hard error in `const` contexts.]
  (rust-lang/rust#115524)
- [Fix detecting references to packed unsized fields.]
  (rust-lang/rust#115583)
- [Remove support for compiler plugins.]
  (rust-lang/rust#116412)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

Show whether a trait is object-safe on its document page