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 pollute docs/suggestions with libstd deps #73771

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

alexcrichton
Copy link
Member

Currently dependency crates of the standard library can sometimes leak
into error messages such as when traits to import are suggested.
Additionally they can leak into documentation such as in the list of
"all traits implemented by u32". The dependencies of the standard
library, however, are intended to be private.

The dependencies of the standard library can't actually be stabl-y
imported nor is the documentation that relevant since you can't import
them on stable either. This commit updates both the compiler and rustdoc
to ignore unstable traits in these two scenarios.

Specifically the suggestion for traits to import ignore unstable traits,
and similarly the list of traits implemented by a type excludes unstable
traits.

This commit is extracted from #73441 where the addition of some new
dependencies to the standard library was showed to leak into various
error messages and documentation. The intention here is to go ahead and
land these changes ahead of that since it will likely take some time to
land.

Currently dependency crates of the standard library can sometimes leak
into error messages such as when traits to import are suggested.
Additionally they can leak into documentation such as in the list of
"all traits implemented by `u32`". The dependencies of the standard
library, however, are intended to be private.

The dependencies of the standard library can't actually be stabl-y
imported nor is the documentation that relevant since you can't import
them on stable either. This commit updates both the compiler and rustdoc
to ignore unstable traits in these two scenarios.

Specifically the suggestion for traits to import ignore unstable traits,
and similarly the list of traits implemented by a type excludes unstable
traits.

This commit is extracted from rust-lang#73441 where the addition of some new
dependencies to the standard library was showed to leak into various
error messages and documentation. The intention here is to go ahead and
land these changes ahead of that since it will likely take some time to
land.
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 26, 2020
@tesuji
Copy link
Contributor

tesuji commented Jul 4, 2020

r? @estebank

// example, from getting documented as "traits `u32` implements" which
// isn't really too helpful.
if let Some(stab) = cx.tcx.lookup_stability(did) {
if stab.level.is_unstable() {
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to filter them out only on non-nightly builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so because that still creates the same issues seen on #73441 where it causes broken links to appear in libstd's nightly documentation because libstd's internal dependencies aren't documented.

@Mark-Simulacrum
Copy link
Member

@bors try

I would like to make sure that this doesn't affect the published rustc docs -- all of those are currently marked unstable, but we do indeed want to display e.g. the HashStable impl on the Symbol page: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/symbol/struct.Symbol.html#impl-HashStable%3CCTX%3E

I think the current state of this PR will not do so; it might be preferable to limit the exclusion to apply to just std + core + alloc perhaps. I'm not sure if that's the best approach, though. Maybe we should be (for example) annotating these dependencies with #[doc(hidden)] and some rustc attribute (#[rustc_never_suggest]) and making those do the right thing for this case.

@bors
Copy link
Contributor

bors commented Jul 8, 2020

⌛ Trying commit fde8d11 with merge 9ffb7801363cff45dcb56fa2146a48aa0ae433db...

@bors
Copy link
Contributor

bors commented Jul 9, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9ffb7801363cff45dcb56fa2146a48aa0ae433db (9ffb7801363cff45dcb56fa2146a48aa0ae433db)

@alexcrichton
Copy link
Member Author

The intention here, to clarify, is that there are crates whose source code we do not control (e.g. gimli) which are going to become dependencies of the standard library. The problem is that rustdoc, under "traits i32 implements", listed impl GimliPrivateTrait for i32. Additionally when you did something like 0i32.gimli_private_method() rustc would say "oh did you mean to import gimli::GimliPrivateTrait"? (the latter didn't practically happen, but it happens when gimli_private_method is actually named something like read)

The intention here is that libstd would get all its documentation, although rustc is a good point I forgot to consider. Again to clarify though the heuristic isn't to avoid documenting things, it's just to skip listing foreign unstable traits under "traits implemented" in rustdoc (otherwise broken links are generated). Checking the rustc docs there's lots of impls and such for HashStable, including Symbol.

I do suspect that docs are lacking in a way I'm missing though. One example I've found is that Option<T> and Result<T> no longer list that they implement the Try trait when rendered into the libstd docs. When you look at the libcore docs, however, then you can see the Try implementation.

If this is crucial to keep implemented I could try and add some sort of check where only std, core, alloc, and rustc_* can have unstable trait impls imported from them. Or I guess we could try to rename the unstable feature for libstd deps and have a different unstable feature for rustc? I don't really have a great solution.

@Mark-Simulacrum
Copy link
Member

Yeah, I think keeping Try in docs is probably pretty important -- I'm not too worried about rustc docs, I think, but std docs missing public traits is pretty unfortunate.

I don't have any strong feelings on this though, and I do think the backtrace work is probably more important than figuring this out, I'd personally be fine landing this as-is (even with the slight regressions) -- realistically, unstable traits aren't that important.

We can also "fix" the rustc case by avoiding passing -Zforce-unstable-if-unmarked or w/e the flag is when documenting rustc, I think.

@ollie27
Copy link
Member

ollie27 commented Jul 9, 2020

rustdoc has a #[doc(masked)] attribute for this issue. Adding #[doc(masked)] extern crate gimli; lines to std for all of the dependencies should fix the docs part of this.

@alexcrichton
Copy link
Member Author

@ollie27 this is a problem not only for direct dependencies but also transitive dependencies of the standard library. For example libstd does not actually depend directly on gimli, it's transitively through other crates gimli is pulled in.

@estebank
Copy link
Contributor

r=me on the diagnostic affecting changes, but feel free to continue the conversation around rustdoc

@ollie27
Copy link
Member

ollie27 commented Jul 10, 2020

@ollie27 this is a problem not only for direct dependencies but also transitive dependencies of the standard library. For example libstd does not actually depend directly on gimli, it's transitively through other crates gimli is pulled in.

Is there any reason gimli can't be added a dependency to std in order to add #[doc(masked)]? It's not pretty but it's the current mechanism to handle this.

@alexcrichton
Copy link
Member Author

At this point it's already hard enough to add dependencies to the standard library and this is a hidden gotcha that seems very difficult to remember. I would much rather have a systematic solution that fixes the problem for all dependencies instead of just one at a time as it just so happens to be an issue. The list of crates that this needs to apply to are:

  • cfg-if
  • compiler-builtins
  • gimli
  • rustc-std-workspace-*
  • object
  • miniz_oxide
  • adler
  • hashbrown
  • libc
  • addr2line
  • unwind
  • panic_unwind
  • panic_abort

This isn't just a trivial list of dependencies, and this list is going to be difficult to manage over time. I'm not sure why we'd want to put ourselves through those paces rather than just fixing this once.

@Mark-Simulacrum
Copy link
Member

I am actually fairly surprised that rustdoc is listing trait impls for dependencies of std. Maybe it would be sufficient to just not document gimli etc (instead only a specific list, probably core/std/alloc only) in bootstrap?

@alexcrichton
Copy link
Member Author

The gimli crate is not documented, the problem is that rustdoc is listing ReaderOffset for u32 under "traits implemented for u32 on the u32 primitive page. The trait itself is not linked (it's just text), but one of the methods on that trait has a "Read more" link which is a dead link. This is what fails the link checker.

Showing impl ReaderOffset for u32 on the page isn't really needed anyway (although it's drowned out by hundreds of other impls).

All that basically to say that gimli is already not documented. This'd probably actually be fixed if it were since the link would be generated correctly.

@Mark-Simulacrum
Copy link
Member

Huh, so rustdoc is finding out about the trait impl some other way. I am personally inclined to land the PR in its current form -- we do lose Try impls and (presumably) some others like SliceConcatExt and so forth, but that seems largely fine. I also don't think we have a good handle on an "excellent" fix beyond the one this PR takes on.

In the long term, I suppose that rustdoc would receive information from Cargo about public/private dependencies and based on that show (or not show) trait impls on types.

@ollie27
Copy link
Member

ollie27 commented Jul 10, 2020

One option would be to invert #[doc(masked)], i.e. mask every crate that doesn't have a special attribute applied. That way std would only need to add an attribute for core and alloc.

Another option would be to tell rustdoc to hide anything from crates it can't generate local relative links to when documenting std. That would have the same effect.

Long term I wonder if #44663 could be used to resolve this.

I guess we can merge this temporarily if it helps unblock things though.

The trait itself is not linked (it's just text), but one of the methods on that trait has a "Read more" link which is a dead link.

That's very much a rustdoc bug, I've filed #74222.

@alexcrichton
Copy link
Member Author

FWIW these all do indeed sound like other alternatives to this, although they do all seem like more work than this local fix. This'll be required for #73441 but that's still waiting on review so it's not necessarily super urgent to fix this now but I suspect #73441 will be ready in the near-ish future.

@alexcrichton
Copy link
Member Author

So to clarify, it this still waiting for someone to take a look? Is it expected that I'm to take action and implement a different strategy other than this PR?

@Mark-Simulacrum
Copy link
Member

I believe either @GuillaumeGomez or @ollie27 need to decide whether they're okay with landing this patch as-is (@estebank has approved the diagnostics changes). I myself am fine with it as is, if not super happy, but I think it's good enough. I don't feel comfortable approving if there's outstanding objections on their ends though.

@GuillaumeGomez
Copy link
Member

I guess this is an acceptable fix for the meantime but please open an issue so we can write a full one later on. So unless @ollie27 has objections, r=me.

@alexcrichton
Copy link
Member Author

@ollie27 could you weigh in on the latest comments?

@ollie27
Copy link
Member

ollie27 commented Jul 16, 2020

If this is holding up other work and people are okay with the docs regression especially with the compiler docs then I'm not going to block this.

@bors r=estebank,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Jul 16, 2020

📌 Commit fde8d11 has been approved by estebank,GuillaumeGomez

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 16, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…ebank,GuillaumeGomez

Don't pollute docs/suggestions with libstd deps

Currently dependency crates of the standard library can sometimes leak
into error messages such as when traits to import are suggested.
Additionally they can leak into documentation such as in the list of
"all traits implemented by `u32`". The dependencies of the standard
library, however, are intended to be private.

The dependencies of the standard library can't actually be stabl-y
imported nor is the documentation that relevant since you can't import
them on stable either. This commit updates both the compiler and rustdoc
to ignore unstable traits in these two scenarios.

Specifically the suggestion for traits to import ignore unstable traits,
and similarly the list of traits implemented by a type excludes unstable
traits.

This commit is extracted from rust-lang#73441 where the addition of some new
dependencies to the standard library was showed to leak into various
error messages and documentation. The intention here is to go ahead and
land these changes ahead of that since it will likely take some time to
land.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…ebank,GuillaumeGomez

Don't pollute docs/suggestions with libstd deps

Currently dependency crates of the standard library can sometimes leak
into error messages such as when traits to import are suggested.
Additionally they can leak into documentation such as in the list of
"all traits implemented by `u32`". The dependencies of the standard
library, however, are intended to be private.

The dependencies of the standard library can't actually be stabl-y
imported nor is the documentation that relevant since you can't import
them on stable either. This commit updates both the compiler and rustdoc
to ignore unstable traits in these two scenarios.

Specifically the suggestion for traits to import ignore unstable traits,
and similarly the list of traits implemented by a type excludes unstable
traits.

This commit is extracted from rust-lang#73441 where the addition of some new
dependencies to the standard library was showed to leak into various
error messages and documentation. The intention here is to go ahead and
land these changes ahead of that since it will likely take some time to
land.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…ebank,GuillaumeGomez

Don't pollute docs/suggestions with libstd deps

Currently dependency crates of the standard library can sometimes leak
into error messages such as when traits to import are suggested.
Additionally they can leak into documentation such as in the list of
"all traits implemented by `u32`". The dependencies of the standard
library, however, are intended to be private.

The dependencies of the standard library can't actually be stabl-y
imported nor is the documentation that relevant since you can't import
them on stable either. This commit updates both the compiler and rustdoc
to ignore unstable traits in these two scenarios.

Specifically the suggestion for traits to import ignore unstable traits,
and similarly the list of traits implemented by a type excludes unstable
traits.

This commit is extracted from rust-lang#73441 where the addition of some new
dependencies to the standard library was showed to leak into various
error messages and documentation. The intention here is to go ahead and
land these changes ahead of that since it will likely take some time to
land.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2020
…arth

Rollup of 21 pull requests

Successful merges:

 - rust-lang#73566 (Don't run `everybody_loops` for rustdoc; instead ignore resolution errors)
 - rust-lang#73771 (Don't pollute docs/suggestions with libstd deps)
 - rust-lang#73794 (Small cleanup for E0705 explanation)
 - rust-lang#73807 (rustdoc: glue tokens before highlighting)
 - rust-lang#73835 (Clean up E0710 explanation)
 - rust-lang#73926 (Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64)
 - rust-lang#73981 (Remove some `ignore-stage1` annotations.)
 - rust-lang#73998 (add regression test for rust-lang#61216)
 - rust-lang#74140 (Make hir ProjectionKind more precise)
 - rust-lang#74148 (Move #[doc(alias)] check in rustc)
 - rust-lang#74159 (forbid generic params in the type of const params)
 - rust-lang#74171 (Fix 44056 test with debug on macos.)
 - rust-lang#74221 (Don't panic if the lhs of a div by zero is not statically known)
 - rust-lang#74325 (Focus on the current file in the source file sidebar)
 - rust-lang#74359 (rustdoc: Rename internal API fns to `into_string`)
 - rust-lang#74370 (Reintroduce spotlight / "important traits" feature)
 - rust-lang#74390 (Fix typo in std::mem::transmute documentation)
 - rust-lang#74391 (BtreeMap: superficially refactor root access)
 - rust-lang#74392 (const generics triage)
 - rust-lang#74397 (Fix typo in the latest release note)
 - rust-lang#74406 (Set shell for github actions CI)

Failed merges:

r? @ghost
@bors bors merged commit a8bb245 into rust-lang:master Jul 16, 2020
@Manishearth
Copy link
Member

Note: this caused a regression, see #74531

@alexcrichton alexcrichton deleted the ignore-unstable branch July 23, 2020 21:20
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 9, 2020
tesuji added a commit to tesuji/rustc that referenced this pull request Sep 9, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.