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

rustdoc: use stability, instead of features, to decide what to show #124864

Merged
merged 1 commit into from
May 8, 2024
Merged

rustdoc: use stability, instead of features, to decide what to show #124864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 19 additions & 28 deletions src/librustdoc/clean/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use rustc_hir::Mutability;
use rustc_metadata::creader::{CStore, LoadedMacro};
use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::LOCAL_CRATE;
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Symbol};

Expand Down Expand Up @@ -444,24 +445,24 @@ pub(crate) fn build_impl(

let associated_trait = tcx.impl_trait_ref(did).map(ty::EarlyBinder::skip_binder);

// Do not inline compiler-internal items unless we're a compiler-internal crate.
let is_compiler_internal = |did| {
tcx.lookup_stability(did)
.is_some_and(|stab| stab.is_unstable() && stab.feature == sym::rustc_private)
};
let document_compiler_internal = is_compiler_internal(LOCAL_CRATE.as_def_id());
let is_directly_public = |cx: &mut DocContext<'_>, did| {
cx.cache.effective_visibilities.is_directly_public(tcx, did)
&& (document_compiler_internal || !is_compiler_internal(did))
};

// Only inline impl if the implemented trait is
// reachable in rustdoc generated documentation
if !did.is_local()
&& let Some(traitref) = associated_trait
&& !is_directly_public(cx, traitref.def_id)
{
let did = traitref.def_id;
if !cx.cache.effective_visibilities.is_directly_public(tcx, did) {
return;
}

if !tcx.features().rustc_private && !cx.render_options.force_unstable_if_unmarked {
Copy link
Member

Choose a reason for hiding this comment

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

quick question, why wouldn't the following be a potential solution:

if !cx.render_options.force_unstable_if_unmarked {

(just removing the F-rustc_private check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a potential solution, but it requires more code than is_compiler_internal(LOCAL_CRATE.as_def_id()). I'm trying to simplify this code, because I had trouble understanding the old version.

if let Some(stab) = tcx.lookup_stability(did)
&& stab.is_unstable()
&& stab.feature == sym::rustc_private
{
return;
}
}
return;
}

let impl_item = match did.as_local() {
Expand All @@ -484,21 +485,11 @@ pub(crate) fn build_impl(

// Only inline impl if the implementing type is
// reachable in rustdoc generated documentation
if !did.is_local() {
if let Some(did) = for_.def_id(&cx.cache) {
if !cx.cache.effective_visibilities.is_directly_public(tcx, did) {
return;
}

if !tcx.features().rustc_private && !cx.render_options.force_unstable_if_unmarked {
if let Some(stab) = tcx.lookup_stability(did)
&& stab.is_unstable()
&& stab.feature == sym::rustc_private
{
return;
}
}
}
if !did.is_local()
&& let Some(did) = for_.def_id(&cx.cache)
&& !is_directly_public(cx, did)
{
return;
}

let document_hidden = cx.render_options.document_hidden;
Expand Down
4 changes: 0 additions & 4 deletions src/librustdoc/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,6 @@ pub(crate) struct RenderOptions {
pub(crate) no_emit_shared: bool,
/// If `true`, HTML source code pages won't be generated.
pub(crate) html_no_source: bool,
/// Whether `-Zforce-unstable-if-unmarked` unstable option is set
pub(crate) force_unstable_if_unmarked: bool,
}

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -353,7 +351,6 @@ impl Options {

let codegen_options = CodegenOptions::build(early_dcx, matches);
let unstable_opts = UnstableOptions::build(early_dcx, matches);
let force_unstable_if_unmarked = unstable_opts.force_unstable_if_unmarked;

let dcx = new_dcx(error_format, None, diagnostic_width, &unstable_opts);

Expand Down Expand Up @@ -790,7 +787,6 @@ impl Options {
call_locations,
no_emit_shared: false,
html_no_source,
force_unstable_if_unmarked,
};
Some((options, render_options))
}
Expand Down
6 changes: 4 additions & 2 deletions tests/rustdoc/inline_cross/issue-76736-2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
//@ aux-build:issue-76736-1.rs
//@ aux-build:issue-76736-2.rs

// https://github.com/rust-lang/rust/issues/124635

#![crate_name = "foo"]
#![feature(rustc_private)]

extern crate issue_76736_1;
extern crate issue_76736_2;

// @has foo/struct.Foo.html
// @has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
// @!has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
Copy link
Member

@fmease fmease May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know, couldn't this impl be useful? The user is explicitly opting into using internal APIs with #![feature(rustc_private)], then why should we refuse them access to potential useful information (here: “impl MaybeResult for Foo” via the upstream blanket impl, which they can indeed make use of)

I feel like I'm missing something obvious. If I'm not mistaken the suggestion from my review comment one above wouldn't throw this impl out and would still fix the linked issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“The user,” in this case, is the standard library. It uses the feature rudtc_private, but doesn’t want private items to show up in its docs.

Copy link
Member

@fmease fmease May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, the std library doesn't contain #[feature(rustc_internal)] directly as you probably know but yeah.

There are other users of rustc_internal. I' on mobile rn but yesterday I grepped through compiler/ and saw the feature being enabled for one of the alternate backends (cranelift or gcc, I don't remember) and I assume rustfmt enables it too for rustc_{parse,ast}?

I'm just nitpicking but I don't wanna block this PR any longer.

pub struct Foo;

// @has foo/struct.Bar.html
// @has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
// @!has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
pub use issue_76736_2::Bar;
19 changes: 19 additions & 0 deletions tests/rustdoc/inline_cross/issue-76736-4.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//@ aux-build:issue-76736-1.rs
//@ aux-build:issue-76736-2.rs

// https://github.com/rust-lang/rust/issues/124635

#![crate_name = "foo"]
#![feature(rustc_private, staged_api)]
#![unstable(feature = "rustc_private", issue = "none")]

extern crate issue_76736_1;
extern crate issue_76736_2;

// @has foo/struct.Foo.html
// @has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
pub struct Foo;

// @has foo/struct.Bar.html
// @has - '//*[@class="impl"]//h3[@class="code-header"]' 'MaybeResult'
pub use issue_76736_2::Bar;
Loading