Uh oh!
There was an error while loading. Please reload this page.
[intra-doc links] Don't check feature gates of items re-exported across crates - #82295
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b4326e5 to
58cc89eCompareMark-Simulacrum
commented
Feb 19, 2021
I would expect us to check the features at definition time; if we're re-exporting from a crate then we can bypass the feature check IMO, rather than re-verifying. |
jyn514
commented
Feb 19, 2021
The problem is that this feature is checked in rustdoc, and rustdoc may not be run on the original crate. Consider a proc-macro that's re-exported into a facade crate, where only the facade is documented. |
jyn514
commented
Feb 19, 2021
Checking this in rustc instead of rustdoc would require moving intra-doc link parsing into rustc proper, which has other problems: #79542 |
jyn514
commented
Feb 19, 2021
Well, this isn't a very good example because rustdoc has to document the proc-macro, it won't be excluded by |
Mark-Simulacrum
commented
Feb 19, 2021
It seems like ultimately this doesn't help much then, because the feature gate can get omitted in the upstream crate (and not noticed as being actually required), and that causes downstream breakage during documentation building. Can we just leave the "loophole" open perhaps, and not check not-local definitions for using enabled features? |
jyn514
commented
Feb 19, 2021
The difference is that there's something the upstream crate can do to fix the breakage (enable the feature gate). Right now the upstream crate can't do anything and each individual crate re-exporting has to enable the feature gate themselves. Concretely, this means since |
This comment has been minimized.
This comment has been minimized.
…ss crates It should be never break another crate to re-export a public item. Note that this doesn't check the feature gate at *all* for other crates: - Feature-gates aren't currently serialized, so the only way to check the gate is with ad-hoc attribute checking. - Checking the feature gate twice (once when documenting the original crate and one when documenting the current crate) seems not great. This should still catch using the feature most of the time though, since people tend to document their own crates.
jyn514
commented
Feb 19, 2021
I reverted this, now it only checks items in the local crate. |
camelid
commented
Feb 19, 2021
I believe Manish is on break to work on Vaccinate CA, so you probably want to re-assign this PR. |
jyn514
commented
Feb 19, 2021
@camelid do you have time to review? |
Probably not soon, and also this bug seems too tricky for me to feel comfortable reviewing a fix for. |
Manishearth
commented
Feb 20, 2021
I'm not "on break" i'm just slower to do reviews, and prefer to not take on large tasks. This is probably okay |
Manishearth
commented
Feb 20, 2021
@bors r+ |
bors
commented
Feb 20, 2021
📌 Commit fdb32e9 has been approved by |
bors
commented
Feb 21, 2021
⌛ Testing commit fdb32e9 with merge c9905c59729e4ca0b67db0917344fea83c368cff... |
bors
commented
Feb 21, 2021
💔 Test failed - checks-actions |
rust-log-analyzer
commented
Feb 21, 2021
jyn514
commented
Feb 21, 2021
😕 @rust-lang/infra do you know why this keeps failing? It's happened twice now. @bors retry |
bors
commented
Feb 22, 2021
Dylan-DPC-zz
commented
Feb 22, 2021
@bors rollup=never |
bors
commented
Feb 22, 2021
☀️ Test successful - checks-actions |
jyn514
commented
Feb 22, 2021
jyn514
commented
Feb 22, 2021
Well, but I guess this is bad in general in case people use it in their own libraries. @rustbot label: +beta-nominated |
Backport approval checklist:
|
camelid
commented
Feb 22, 2021
cc @rust-lang/rustdoc for backport decision |
[beta] backports This backports some beta-accepted PRs and one additional LLVM fix for s390x. - rustdoc: treat edition 2021 as unstable rust-lang#82207 - Fix popping singleton paths in when generating E0433 rust-lang#82259 - libtest: Fix unwrap panic on duplicate TestDesc rust-lang#82274 - [intra-doc links] Don't check feature gates of items re-exported across crates rust-lang#82295 - rustdoc: Remove duplicate "List of all items" rust-lang#82484 - Substitute erased lifetimes on bad placeholder type rust-lang#82494 - Revert LLVM D81803 because it broke Windows 7 rust-lang#82605 - [SystemZ] Assign the full space for promoted and split outgoing args. rust-lang/llvm-project#95 r? `@Mark-Simulacrum`
It should be never break another crate to re-export a public item.
Note that this doesn't check the feature gate at
all for other crates:
the gate is with ad-hoc attribute checking.
crate and one when documenting the current crate) seems not great.
This should still catch using the feature most of the time though, since
people tend to document their own crates.
Closes#82284.
r? @Manishearth