Fix doc_cfg not working as expected on trait impls - #153964
Conversation
This comment has been minimized.
This comment has been minimized.
doc_cfg not working as expected on trait impls
This comment has been minimized.
This comment has been minimized.
|
Not great that |
|
And CI passed. \o/ |
| impl_: &hir::Impl<'tcx>, | ||
| def_id: LocalDefId, | ||
| cx: &mut DocContext<'tcx>, | ||
| // If `renamed` is some, then this is an inlined impl and it will be handled later on in the |
There was a problem hiding this comment.
nit: ambiguous pronoun, this would be less ambiguous if "this" was replaced with an actual name.
|
|
||
| clean::ImplItem(..) => {} | ||
|
|
||
| // They have no use anymore, so always remove them. |
There was a problem hiding this comment.
nit: great comments explain why code does something.
| @@ -0,0 +1,59 @@ | |||
| // This test ensures that `doc_cfg` feature is working as expected on trait impls. | |||
There was a problem hiding this comment.
just to be sure: we do have a test for non-trait impls, right? we definitely don't want to regress that.
also, do we have a test for non-auto doc(cfg(...)) on trait items?
There was a problem hiding this comment.
Doesn't hurt to add one in the same test I guess. Gonna add a test for non-auto doc(cfg(...)) too.
ce71ac8 to
2dd7677
Compare
This comment has been minimized.
This comment has been minimized.
|
Added comments and greatly extended the tests. |
This comment has been minimized.
This comment has been minimized.
|
Seems like a flaky failure. Restarted CI. |
There was a problem hiding this comment.
I took a closer look at the actual logic this time and it seems solid and straightforwards, I can't come up with any edge cases that it would cause problems in.
My only issue of immediate relevance is rustc_span::symbol::kw::Impl and renamed being used in a somewhat unintuitive way, not sure what the best way to address that is but it definitely has an impact on code readability. I also found some missing test coverage but that's mostly unrelated so that can be handled in a followup PR if you want.
2dd7677 to
3f0f7d7
Compare
|
I added code comments to explain that the symbol is used as a sentinel value. |
| if impl_.of_trait.is_none() { | ||
| None | ||
| } else { | ||
| Some(rustc_span::symbol::kw::Impl) | ||
| }, |
There was a problem hiding this comment.
Would replacing parameter type Option<Symbol> with Option<BikeshedTy> be possible / much of a hassle where BikeshedTy is a new type that could be defined like enum BikeshedTy { BikeshedName(Symbol), BikeshedAnon }? I haven't read the rest of the diff, lol
There was a problem hiding this comment.
Context: this argument is what an inlined item is renamed into. So no, I don't want to rename it. 😝
…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat
…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat
…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat
This comment has been minimized.
This comment has been minimized.
3f0f7d7 to
2752dc5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Applied all suggestions! |
|
Thanks! @bors r+ |
…cfg, r=lolbinarycat Fix `doc_cfg` not working as expected on trait impls Fixes rust-lang#153655. I spent waaaaay too much time on this fix. So the current issue is that rustdoc gets its items in two passes: 1. All items 2. Trait/blanket/auto impls Because of that, the trait impls are not stored "correctly" in the rustdoc AST, meaning that the `propagate_doc_cfg` pass doesn't work correctly on them. So initially, I tried to "clean" the impls at the same time as the other items. However, it created a monstruous amount of bugs and issues and after two days, I decided to give up on this approach (might be worth fixing that in the future!). You can see what I tried [here](https://github.com/rust-lang/rust/compare/main...GuillaumeGomez:trait-impls-doc_cfg?expand=1). So instead, since the impls are stored at the end, I create placeholders for impls and in `propagate_doc_cfg`, I store the `cfg` "context" (more clear when reading the code 😛) and re-use it later on when the "real" impl comes up. r? @lolbinarycat
Rollup of 10 pull requests Successful merges: - #153964 (Fix `doc_cfg` not working as expected on trait impls) - #153979 (Rename various query cycle things.) - #154132 (Add missing num_internals feature gate to coretests/benches) - #154153 (core: Implement `unchecked_funnel_{shl,shr}`) - #154236 (Clean up query-forcing functions) - #154252 (Don't store current-session side effects in `OnDiskCache`) - #154017 ( Fix invalid add of duplicated call locations for the rustdoc scraped examples feature) - #154163 (enzyme submodule update) - #154264 (Update books) - #154282 (rustc-dev-guide subtree update)
…, r=eggyal,tgross35 [libcore] Disable `doc(auto_cfg)` for integers trait impls Fixes rust-lang#153655. Thanks to rust-lang#153964, `doc(auto_cfg)` finally works as expected on impls. So now this fix works: <img width="1000" height="806" alt="image" src="https://github.com/user-attachments/assets/f37da375-c2eb-4a7b-abf2-1fdd3a73e2bb" /> cc @eggyal
…, r=eggyal,tgross35 [libcore] Disable `doc(auto_cfg)` for integers trait impls Fixes rust-lang#153655. Thanks to rust-lang#153964, `doc(auto_cfg)` finally works as expected on impls. So now this fix works: <img width="1000" height="806" alt="image" src="https://github.com/user-attachments/assets/f37da375-c2eb-4a7b-abf2-1fdd3a73e2bb" /> cc @eggyal
Rollup of 10 pull requests Successful merges: - rust-lang/rust#153964 (Fix `doc_cfg` not working as expected on trait impls) - rust-lang/rust#153979 (Rename various query cycle things.) - rust-lang/rust#154132 (Add missing num_internals feature gate to coretests/benches) - rust-lang/rust#154153 (core: Implement `unchecked_funnel_{shl,shr}`) - rust-lang/rust#154236 (Clean up query-forcing functions) - rust-lang/rust#154252 (Don't store current-session side effects in `OnDiskCache`) - rust-lang/rust#154017 ( Fix invalid add of duplicated call locations for the rustdoc scraped examples feature) - rust-lang/rust#154163 (enzyme submodule update) - rust-lang/rust#154264 (Update books) - rust-lang/rust#154282 (rustc-dev-guide subtree update)
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang#153964, rust-lang#154311, and rust-lang#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
Rollup merge of #159819 - Vastargazing:fix/poison-error-auto-cfg-note, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in #153964, #154311, and #156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue #149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as #156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
…, r=GuillaumeGomez std::sync::poison: disable auto_cfg on PoisonError::new `PoisonError::new` is defined twice, once for `#[cfg(panic = "unwind")]` and once for its negation. While exactly one definition survives expansion in any given build, the method itself is actually present in every configuration. Currently, rustdoc's `auto_cfg` only looks at whichever `cfg` survived expansion and tags the method accordingly, generating a misleading "Available on panic=unwind only" portability badge in the docs. This is the same `auto_cfg` limitation previously addressed in rust-lang/rust#153964, rust-lang/rust#154311, and rust-lang/rust#156426: when an item is split into mutually-exclusive `cfg` variants that collectively cover all builds, rustdoc incorrectly marks it as conditionally available. Issue rust-lang/rust#149786 pointed out `PoisonError::new` as another case of this pattern, but it hadn't been touched yet. This PR applies the same fix as rust-lang/rust#156426 (adding `#[doc(auto_cfg = false)]` to both variants) and adds a regression test to cover this pattern going forward. r? @GuillaumeGomez
Fixes #153655.
I spent waaaaay too much time on this fix. So the current issue is that rustdoc gets its items in two passes:
Because of that, the trait impls are not stored "correctly" in the rustdoc AST, meaning that the
propagate_doc_cfgpass doesn't work correctly on them. So initially, I tried to "clean" the impls at the same time as the other items. However, it created a monstruous amount of bugs and issues and after two days, I decided to give up on this approach (might be worth fixing that in the future!). You can see what I tried here.So instead, since the impls are stored at the end, I create placeholders for impls and in
propagate_doc_cfg, I store thecfg"context" (more clear when reading the code 😛) and re-use it later on when the "real" impl comes up.r? @lolbinarycat