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 ignores traits implemented inside of a function #52545

Closed
sgrif opened this issue Jul 19, 2018 · 13 comments · Fixed by #53162
Closed

Rustdoc ignores traits implemented inside of a function #52545

sgrif opened this issue Jul 19, 2018 · 13 comments · Fixed by #53162
Assignees
Labels
P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@sgrif
Copy link
Contributor

sgrif commented Jul 19, 2018

After #51952 landed, I attempted to make Diesel compatible with recent nightlies by following the advice of wrapping the output in a function rather than a module. However, we are now unable to build our documentation. The compiler complains that every derived trait within Diesel is not implemented. This only occurs when running cargo doc, the crate otherwise compiles fine. I have tested with 1.26.1, stable, and the most recent nightly.

@Mark-Simulacrum Mark-Simulacrum added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2018
@Mark-Simulacrum Mark-Simulacrum added this to the 1.29 milestone Jul 19, 2018
@GuillaumeGomez
Copy link
Member

To fix it, I suppose we could "just" remove the "loop" feature of rustdoc but we'd have some huge performance loss...

sgrif added a commit to diesel-rs/diesel that referenced this issue Jul 19, 2018
Our documentation is currently unable to build due to
rust-lang/rust#52545. Until that is resolved
we need to disable updates to our hosted documentation. Hopefully this
issue is resolved upstream soon, but there's nothing we can do until
then.

I'm not entirely sure why broken doc builds got uploaded, but I will be
looking into that separately.
@pnkfelix pnkfelix self-assigned this Jul 26, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jul 26, 2018

@GuillaumeGomez can't you do the same trick you did for dead_code and unused_import? Or is this an error?

@QuietMisdreavus
Copy link
Member

@Mark-Simulacrum Not sure the reasoning behind tagging things as regressions - this bug has been here for all time, it's just that the hygiene change has brought this to light. Rustdoc has never looked inside functions to document things, and the introduction of everybody-loops made that impossible anyway. To fix this, two things need to happen:

  1. everybody-loops needs to be revamped to somehow exclude trait impls from being removed. Alternately, we can stop doing everybody-loops in rustdoc, but I don't think it's just there for performance. IIRC it was introduced to rustdoc when #[doc(cfg)] was, to prevent trying to resolve things on the wrong platform.
  2. Rustdoc needs to start scanning inside functions for impl blocks. I fear this can start recursing arbitrarily deep, though - imagine a function with an impl, that contains a function with another impl, etc. However, i imagine in practice it won't be a problem.

At some point either rustdoc or everybody-loops needs to filter out types that are only in-scope for that function - rustdoc may already have an easier time at this, though, since it can just treat items declared inside functions to be the equivalent of #[doc(hidden)]. (Meaning that you'd need to use #[doc(no_hidden)] to reveal trait impls inside functions? Seems legit to me.)

@Mark-Simulacrum
Copy link
Member

I tag things as a regression mostly so that we can keep an eye on them; if we believe this isn't one we can of course untag. However, my reasoning in this case was that @sgrif indicated that Diesel changed in behavior, which feels like a regression to me, though I may have misunderstood.

@ollie27
Copy link
Member

ollie27 commented Jul 26, 2018

This looks like a duplicate of #36922 / #41480.

@QuietMisdreavus
Copy link
Member

From the issue description, it sounds like #51952 caused some new warnings to appear (i think? i haven't read too far into it), which @sgrif moved to fix. This caused an interaction with a long-standing behavior in rustdoc (always activating everybody-loops), which makes the crate fail to document. There's also this:

I have tested with 1.26.1, stable, and the most recent nightly.

Which makes me feel like if anything, it's a stable-to-stable regression.

@pietroalbini pietroalbini added regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jul 31, 2018
@pietroalbini pietroalbini removed this from the 1.29 milestone Jul 31, 2018
@pietroalbini
Copy link
Member

Marked as a stable-to-stable regression since "I have tested with 1.26.1, stable, and the most recent nightly.".

@RReverser
Copy link
Contributor

This is a duplicate of my earlier report at #41480 I believe.

@QuietMisdreavus
Copy link
Member

@sgrif Is the version of diesel that failed to document published somewhere? Is it on crates.io or pushed to GitHub? I'm working on a fix that will at least allow it to create docs (even if the traits don't show up) but i want to make sure the original problem gets fixed.

@sgrif
Copy link
Contributor Author

sgrif commented Aug 1, 2018

The master branch. It is not published to crates.io. The traits not showing up is a pretty sub-optimal case for us. It means that any impl we derive does not get documented, which is a pretty significant portion of our implemented traits

@QuietMisdreavus
Copy link
Member

If i can get the fixed version to compile in the first place, the fix to make the traits appear is next in line. For now, i want to make sure i can properly generate files for a crate that properly depends on the impls appearing in those positions before it can compile.

@sgrif
Copy link
Contributor Author

sgrif commented Aug 2, 2018

Just to make clear the importance of this from my point of view -- The change which forced us to move all our trait impls into functions is now in beta. There is currently no way for us to work on 1.29 without losing the ability to build our documentation, which isn't a tradeoff we're willing to make. I'm hoping the milestone means this will be fully resolved before then. (Thank you @QuietMisdreavus for working on this)

@pnkfelix pnkfelix assigned QuietMisdreavus and unassigned pnkfelix Aug 3, 2018
cramertj added a commit to cramertj/rust that referenced this issue Aug 3, 2018
…some-loops, r=pnkfelix

make `everybody_loops` preserve item declarations

First half of rust-lang#52545.

`everybody_loops` is used by rustdoc to ensure we don't contain erroneous references to platform APIs if one of its uses is pulled in by `#[doc(cfg)]`. However, you can also implement traits for public types inside of functions. This is used by Diesel (probably others, but they were the example that was reported) to get around a recent macro hygiene fix, which has caused their crate to fail to document. While this won't make the traits show up in documentation (that step comes later), it will at least allow files to be generated.
bors added a commit that referenced this issue Aug 6, 2018
… r=pnkfelix

make `everybody_loops` preserve item declarations

First half of #52545.

`everybody_loops` is used by rustdoc to ensure we don't contain erroneous references to platform APIs if one of its uses is pulled in by `#[doc(cfg)]`. However, you can also implement traits for public types inside of functions. This is used by Diesel (probably others, but they were the example that was reported) to get around a recent macro hygiene fix, which has caused their crate to fail to document. While this won't make the traits show up in documentation (that step comes later), it will at least allow files to be generated.
@QuietMisdreavus
Copy link
Member

I've opened #53162 to make those trait impls show up in documentation.

@nikomatsakis nikomatsakis added P-medium Medium priority and removed I-nominated labels Aug 9, 2018
bors added a commit that referenced this issue Sep 7, 2018
…=GuillaumeGomez

rustdoc: collect trait impls as an early pass

Fixes #52545, fixes #41480, fixes #36922

Right now, rustdoc pulls all its impl information by scanning a crate's HIR for any items it finds. However, it doesn't recurse into anything other than modules, preventing it from seeing trait impls that may be inside things like functions or consts. Thanks to #53002, now these items actually *exist* for rustdoc to see, but they still weren't getting collected for display.

But there was a secret. Whenever we pull in an item from another crate, we don't have any of its impls in the local HIR, so instead we ask the compiler for *everything* and filter out after the fact. This process is only triggered if there's a cross-crate re-export in the crate being documented, which can sometimes leave this info out of the docs. This PR instead moves this collection into an early pass, which occurs immediately after crate cleaning, so that that collection occurs regardless. In addition, by including the HIR's own `trait_impls` in addition to the existing `all_trait_implementations` calls, we can collect all these tricky trait impls without having to scan for them!
kennytm added a commit to kennytm/rust that referenced this issue Sep 7, 2018
…en-trait, r=GuillaumeGomez

rustdoc: collect trait impls as an early pass

Fixes rust-lang#52545, fixes rust-lang#41480, fixes rust-lang#36922

Right now, rustdoc pulls all its impl information by scanning a crate's HIR for any items it finds. However, it doesn't recurse into anything other than modules, preventing it from seeing trait impls that may be inside things like functions or consts. Thanks to rust-lang#53002, now these items actually *exist* for rustdoc to see, but they still weren't getting collected for display.

But there was a secret. Whenever we pull in an item from another crate, we don't have any of its impls in the local HIR, so instead we ask the compiler for *everything* and filter out after the fact. This process is only triggered if there's a cross-crate re-export in the crate being documented, which can sometimes leave this info out of the docs. This PR instead moves this collection into an early pass, which occurs immediately after crate cleaning, so that that collection occurs regardless. In addition, by including the HIR's own `trait_impls` in addition to the existing `all_trait_implementations` calls, we can collect all these tricky trait impls without having to scan for them!
kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018
…en-trait, r=GuillaumeGomez

rustdoc: collect trait impls as an early pass

Fixes rust-lang#52545, fixes rust-lang#41480, fixes rust-lang#36922

Right now, rustdoc pulls all its impl information by scanning a crate's HIR for any items it finds. However, it doesn't recurse into anything other than modules, preventing it from seeing trait impls that may be inside things like functions or consts. Thanks to rust-lang#53002, now these items actually *exist* for rustdoc to see, but they still weren't getting collected for display.

But there was a secret. Whenever we pull in an item from another crate, we don't have any of its impls in the local HIR, so instead we ask the compiler for *everything* and filter out after the fact. This process is only triggered if there's a cross-crate re-export in the crate being documented, which can sometimes leave this info out of the docs. This PR instead moves this collection into an early pass, which occurs immediately after crate cleaning, so that that collection occurs regardless. In addition, by including the HIR's own `trait_impls` in addition to the existing `all_trait_implementations` calls, we can collect all these tricky trait impls without having to scan for them!
bors added a commit that referenced this issue Sep 20, 2018
…=GuillaumeGomez

rustdoc: collect trait impls as an early pass

Fixes #52545, fixes #41480, fixes #36922

Right now, rustdoc pulls all its impl information by scanning a crate's HIR for any items it finds. However, it doesn't recurse into anything other than modules, preventing it from seeing trait impls that may be inside things like functions or consts. Thanks to #53002, now these items actually *exist* for rustdoc to see, but they still weren't getting collected for display.

But there was a secret. Whenever we pull in an item from another crate, we don't have any of its impls in the local HIR, so instead we ask the compiler for *everything* and filter out after the fact. This process is only triggered if there's a cross-crate re-export in the crate being documented, which can sometimes leave this info out of the docs. This PR instead moves this collection into an early pass, which occurs immediately after crate cleaning, so that that collection occurs regardless. In addition, by including the HIR's own `trait_impls` in addition to the existing `all_trait_implementations` calls, we can collect all these tricky trait impls without having to scan for them!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants