Fix doc cfg on reexports #101006
Fix doc cfg on reexports #101006
Conversation
GuillaumeGomez
added
the
T-rustdoc
|
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| let mut attrs = Vec::new(); | ||
| for (parent_hir_id, _) in hir.parent_iter(hir_id) { | ||
| let def_id = hir.local_def_id(parent_hir_id).to_def_id(); | ||
| attrs.extend_from_slice(load_attrs(self.cx, def_id)); |
There was a problem hiding this comment.
It looks like this is going to wind up copying real cfg, and not just doc(cfg). Do we want that? The way libc uses re-exports, this seems like it would make their lives harder, not easier.
There was a problem hiding this comment.
We need to keep both until we call merge_attrs because it internally calls AttributesExt::cfg which makes the "filtering" depending on the doc_cfg and the doc_auto_cfg features (which is why it returns a tuple of two and not just a ast::Attributes).
There was a problem hiding this comment.
But here's a screenshot of what the libc looks like, when it's built from nightly and #![feature(doc_auto_cfg)] is added:
And here's a screenshot of what libc looks like, when it's built from your branch and #![feature(doc_auto_cfg)] is added:
There was a problem hiding this comment.
Sorry I'm a bit lost: isn't it how it should be? The reexports should display the cfg features of the reexported item, no?
There was a problem hiding this comment.
The docs are claiming things that aren't true:
This thing claims that c_short is present on Unix and absent on Windows. But c_short is definitely present on Windows; it's just implemented separately.
Basically, if this PR is going to be merged, then doc_auto_cfg should probably be off by default. Otherwise, a lot of crates with platform-specific implementations of common interfaces (libc is just a very high-profile example) are going to have wrong documentation.
There was a problem hiding this comment.
I don't agree on that but it confirms that allowing to disable/enable doc_auto_cfg is a must have.
There was a problem hiding this comment.
As long as we agree that doc_auto_cfg needs to be optional, I’m fine with merging this.
There was a problem hiding this comment.
I think this PR just highlighted it. The discussion now will be either it's enabled by default or not and what the attribute to enable/disable should look like.
GuillaumeGomez
force-pushed
the
doc-cfg-reexport
branch
from
126f712
to
2ed9454
Compare
|
@bors: r=notriddle rollup |
bors
added
S-waiting-on-bors
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#95005 (BTree: evaluate static type-related check at compile time) - rust-lang#99742 (Add comments about stdout locking) - rust-lang#100128 (Document that `RawWakerVTable` functions must be thread-safe.) - rust-lang#100956 (Reduce right-side DOM size) - rust-lang#101006 (Fix doc cfg on reexports) - rust-lang#101012 (rustdoc: remove unused CSS for `.variants_table`) - rust-lang#101023 (rustdoc: remove `type="text/css"` from stylesheet links) - rust-lang#101031 (Remove unused build dependency) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…r=notriddle [rustdoc] Remove Attrs type alias When working on rust-lang#101006, I was quite confused because of this type alias as I'm used to having rustdoc types into `clean/types.rs`. Anyway, considering how few uses of it we have, I simply removed it. r? `@notriddle`
…r=notriddle [rustdoc] Remove Attrs type alias When working on rust-lang#101006, I was quite confused because of this type alias as I'm used to having rustdoc types into `clean/types.rs`. Anyway, considering how few uses of it we have, I simply removed it. r? ``@notriddle``
…r=notriddle [rustdoc] Remove Attrs type alias When working on rust-lang#101006, I was quite confused because of this type alias as I'm used to having rustdoc types into `clean/types.rs`. Anyway, considering how few uses of it we have, I simply removed it. r? ```@notriddle```
…r=notriddle [rustdoc] Remove Attrs type alias When working on rust-lang#101006, I was quite confused because of this type alias as I'm used to having rustdoc types into `clean/types.rs`. Anyway, considering how few uses of it we have, I simply removed it. r? ````@notriddle````
…r=notriddle [rustdoc] Remove Attrs type alias When working on rust-lang#101006, I was quite confused because of this type alias as I'm used to having rustdoc types into `clean/types.rs`. Anyway, considering how few uses of it we have, I simply removed it. r? `````@notriddle`````
Fixes #83428.
The problem was that the newly inlined item cfg propagation was not working since its real parent is different than its current one.
For the implementation, I decided to put it directly into
CfgPropagationinstead of insideinline.rsbecause I thought it would be simpler to maintain and to not forget if new kind of items are added if it's all done in one place.r? @notriddle