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 doc_auto_cfg incorrectly picks up cfg's #90497

Open
Mark-Simulacrum opened this issue Nov 2, 2021 · 20 comments
Open

rustdoc doc_auto_cfg incorrectly picks up cfg's #90497

Mark-Simulacrum opened this issue Nov 2, 2021 · 20 comments
Labels
C-bug Category: This is a bug. F-doc_auto_cfg `#![feature(doc_auto_cfg)]` F-doc_cfg `#![feature(doc_cfg)]` P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 2, 2021

NOTE(camelid): This bug is now feature-gated behind the doc_auto_cfg flag and, as such, is no longer a regression. The original issue text follows.


For example, https://doc.rust-lang.org/nightly/std/primitive.usize.html#method.trailing_zeros has a tag indicating it is only supported on 64-bit, but in practice the method is defined on 32-bit as well; it's just defined separately from that definition.

This seems like it'll probably hurt particularly new users who may be annoyed at the lack of cross-target compatibility in such a method -- if we can't design a fix, I'd prefer that we disable the autosetting of cfgs for now, since it seems like this is likely to be widespread.

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. labels Nov 2, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.57.0 milestone Nov 2, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 2, 2021
@Mark-Simulacrum Mark-Simulacrum added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 2, 2021
@Mark-Simulacrum
Copy link
Member Author

cc @rust-lang/rustdoc

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2021

@Mark-Simulacrum this doesn't need rustdoc changes, you can just remove feature(doc_cfg) from the standard library (and replace it with the manual cfgs from before). I don't think we should disable it for the rest of the ecosystem just because the standard library has some weird edge cases.

@GuillaumeGomez
Copy link
Member

It's not a regression from stable to beta but from nightly to nightly.

@jyn514 It's actually because of rustdoc here:

#[cfg(target_pointer_width = "32")]
#[lang = "usize"]
impl usize {
    widening_impl! { usize, u64, 32 }
    uint_impl! { usize, u32, isize, 32, 4294967295, 8, "0x10000b3", "0xb301", "0x12345678",
    "0x78563412", "0x1e6a2c48", "[0x78, 0x56, 0x34, 0x12]", "[0x12, 0x34, 0x56, 0x78]",
    usize_isize_to_xe_bytes_doc!(), usize_isize_from_xe_bytes_doc!() }
}

#[cfg(target_pointer_width = "64")]
#[lang = "usize"]
impl usize {
    widening_impl! { usize, u128, 64 }
    uint_impl! { usize, u64, isize, 64, 18446744073709551615, 12, "0xaa00000000006e1", "0x6e10aa",
    "0x1234567890123456", "0x5634129078563412", "0x6a2c48091e6a2c48",
    "[0x56, 0x34, 0x12, 0x90, 0x78, 0x56, 0x34, 0x12]",
     "[0x12, 0x34, 0x56, 0x78, 0x90, 0x12, 0x34, 0x56]",
    usize_isize_to_xe_bytes_doc!(), usize_isize_from_xe_bytes_doc!() }
}

It picks automatically the #[cfg(target_pointer_width = "...")] since we merged #89596. The solution here would be to use cfg_hide on them.

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2021

@GuillaumeGomez cfg(hide) is global for a crate. I don't think it makes sense to hide it for all 64-bit specific items in libstd.

@GuillaumeGomez
Copy link
Member

Ah my bad, I thought we could use it both globally and locally. In such a case, it would make sense actually.

@Nemo157
Copy link
Member

Nemo157 commented Nov 2, 2021

You can apply a manual #[doc(cfg())] on these items to override the #[cfg].

@GuillaumeGomez
Copy link
Member

How would that help? You can only add cfg, not remove them, no?

@Nemo157
Copy link
Member

Nemo157 commented Nov 2, 2021

No, a doc(cfg) on an item replaces the cfg on that item for documentation purposes.

@GuillaumeGomez
Copy link
Member

Weird, but I guess it would solve the issue. Are you ok with this solution @Mark-Simulacrum ?

@Mark-Simulacrum
Copy link
Member Author

I don't think we should disable it for the rest of the ecosystem just because the standard library has some weird edge cases.

This is not a "weird edge case", afaict, it will affect any case where there are two separate items defined with cfgs on each (likely non-overlapping so that the items don't introduce errors in the build). That is very common, I expect. I think we should disable it for core/alloc/std at least for now. The alternative of adding doc(cfg) overrides a bunch of places, and maintaining them, seems like a nightmare (you need to think about it for every cfg you add, which is annoying and kinda worse than what we had before this new support landed).

It's not a regression from stable to beta but from nightly to nightly.

The stable documentation for std does not show this bug, but beta does, so I consider it a stable-to-beta regression.

It does seem like doc_cfg is not yet stabilized, so once the std docs are fixed this stops being a regression (just an unfortunate behavior/bug in the feature). Though I presume any external users building with rustdoc nightly (e.g., docs.rs) are also affected and have similar "bugs" in their docs that they didn't yet notice.

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2021

Though I presume any external users building with rustdoc nightly (e.g., docs.rs) are also affected and have similar "bugs" in their docs that they didn't yet notice.

Only if they explicitly opt in with feature(doc_cfg). It seems really strange to me to completely disable nightly features because they have bugs, that's kind of the point of a nightly channel.

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2021

That said, I think it would be fine to make the feature gate for "propagate doc_cfg automatically" different than the one for "allow writing doc(cfg) at all"

@GuillaumeGomez
Copy link
Member

Ah indeed, I forgot we had a release in between... We also used doc(cfg()) to fix some edge cases. I think the best thing to do in the meantime is to do this until we write a proper fix.

That said, I think it would be fine to make the feature gate for "propagate doc_cfg automatically" different than the one for "allow writing doc(cfg) at all"

Indeed.

@Mark-Simulacrum
Copy link
Member Author

Yes, I don't think there's cause to disable the feature completely for all users -- I had (incorrectly) assumed that the effects would be visible for all users, regardless of the feature gate, but that is not the case. I think splitting out this particular sub-part of the feature would be a good idea (and then we can avoid enabling it in-tree for now).

@GuillaumeGomez
Copy link
Member

Going to send a PR then.

@mejrs
Copy link
Contributor

mejrs commented Nov 2, 2021

Though I presume any external users building with rustdoc nightly (e.g., docs.rs) are also affected and have similar "bugs" in their docs that they didn't yet notice.

I did have this bug, and I noticed it (see PyO3/pyo3#1928).

For cases like this, where the item is always available but the implementation is different, I fixed this by having cfgs inside of the implementation, rather than on the outside.

i.e., instead of using

#[cfg(target_pointer_width = "32")]
fn foo(){
    // 32 bit impl
}

#[cfg(target_pointer_width = "64")]
fn foo(){
    // 64 bit impl
}

I would use

fn foo(){
    #[cfg(target_pointer_width = "32")]
    {
        // 32 bit impl
    }

    #[cfg(target_pointer_width = "64")]
    {
        // 64 bit impl
    }
}

That said, I'm not sure how easy that is for the int macros, they are pretty big.

@GuillaumeGomez
Copy link
Member

Would be better for rustdoc to be able to find out on its own but aggregating items with the same name. Just seems really tricky to do...

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2021

@GuillaumeGomez that's the same as solving #1998, I don't think we should make any plans that require solving that.

@GuillaumeGomez
Copy link
Member

At least not before we have a lot of free time on ours hands and solved all other rustdoc issues. :)

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 2, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 2, 2021
…e, r=jyn514

Split doc_cfg and doc_auto_cfg features

Part of rust-lang#90497.

With this feature, `doc_cfg` won't pick up items automatically anymore.

cc `@Mark-Simulacrum`
r? `@jyn514`
@camelid camelid removed this from the 1.57.0 milestone Dec 3, 2021
@camelid camelid removed the P-high High priority label Dec 3, 2021
@camelid camelid added F-doc_cfg `#![feature(doc_cfg)]` P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 3, 2021
@camelid camelid changed the title rustdoc incorrectly picks up cfg's rustdoc doc_auto_cfg incorrectly picks up cfg's Dec 3, 2021
@camelid camelid added the F-doc_auto_cfg `#![feature(doc_auto_cfg)]` label Dec 3, 2021
@camelid
Copy link
Member

camelid commented Dec 3, 2021

Updated issue to reflect that this is no longer a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-doc_auto_cfg `#![feature(doc_auto_cfg)]` F-doc_cfg `#![feature(doc_cfg)]` P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants