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 emits spurious build errors due to unsatisfied trait bounds on code that compiles fine #117796

Open
AlexTMjugador opened this issue Nov 10, 2023 · 5 comments
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@AlexTMjugador
Copy link

AlexTMjugador commented Nov 10, 2023

I tried this code: zopfli-rs/zopfli#36

I expected to see this happen: the CI build workflow completes fine.

Instead, this happened: the CI build workflow run failed to build the crate documentation. However, the crate compiles without any problems when using cargo test and cargo build with the same features:

Error screenshot

I suspect there's a new issue with the latest nightly rustdoc, leading to unexpected trait bound errors. This suspicion stems from the fact that the same command works well under rustc 1.75.0-nightly (df871fbf0 2023-10-24). Additionally, there have been recent changes made to the code that rustdoc accepts: #117622, #117450

Meta

rustc --version --verbose:

rustc 1.75.0-nightly (0f44eb32f 2023-11-09)
binary: rustc
commit-hash: 0f44eb32f1123ac93ab404d74c295263ce468343
commit-date: 2023-11-09
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.4
@AlexTMjugador AlexTMjugador added the C-bug Category: This is a bug. label Nov 10, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 10, 2023
@saethlin
Copy link
Member

You'll get the same errors if you run cargo rustc --lib -- --cfg=doc, because the code is invalid under cfg(doc). I think this is just a mistake in one of the cfg(doc) expressions in src/lib.rs.

Whether the rustdoc change should have happened without warning is a separate discussion, but I think that should unblock you at least?

@AlexTMjugador
Copy link
Author

Thank you, that is helpful for unblocking this!

In the meantime I also ran bisect-rustc, which confirmed that this behavior change was caused by the recent PR for more strict checking in rustdoc:

searched nightlies: from nightly-2023-10-24 to nightly-2023-11-09
regressed nightly: nightly-2023-11-01
searched commit range: 31bc7e2...9d83ac2
regressed commit: 50be229

bisected with cargo-bisect-rustc v0.6.7

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2023-10-24 --end=2023-11-09 -- doc 

@AlexTMjugador
Copy link
Author

AlexTMjugador commented Nov 11, 2023

Sadly however, your pointer is not immediately helpful for getting Rustdoc to generate the documentation I want (it can be checked out at docs.rs) because my code relied on Rustdoc's leniency to accept documenting feature-gated items, in combination with feature(doc_auto_cfg). I can work around the problem by skipping the documentation for some non-std alternative types, but that's not what I want. Also, I'm not sure at the moment on how I would refactor my code to achieve the same end result without causing this problem.

@Nemo157
Copy link
Member

Nemo157 commented Nov 11, 2023

It's a fair bit of duplication, but you could

#[cfg(doc)]
impl<W: std::io::Write> std::io::Write for DeflateEncoder<W>

so that it's implementing both Write traits, that would allow the std-using APIs to obey their trait bounds while still showing your custom subset Write trait at the crate root.

EDIT: Or maybe a blanket #[cg(doc)] impl<W: crate::Write> std::io::Write for W would work.

@saethlin saethlin added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 11, 2023
AlexTMjugador added a commit to zopfli-rs/zopfli that referenced this issue Nov 11, 2023
These changes work around a recent change in rustdoc's behavior. The
patch was suggested by @Nemo157.
@AlexTMjugador
Copy link
Author

Thank you, that's better! The manual std::io::Write implementation for each compression encoder did work! 🎉

The more concise blanket implementation did not work, however:

Blanket impl error

For what it's worth, I believe the recent changes in Rustdoc's behavior are not very intuitive. In my experience, Rustdoc has not been prone to ICEs to the extent that would justify rejecting slightly invalid views of the code for documentation purposes. Considering how many software projects lack good documentation, it does not seem like a good idea to discourage people from documenting their code by requiring them to think harder on technical correctness when considering the interaction of Rustdoc's code view with conditional compilation. Additionally, are those Rustdoc ICEs costly to fix when they happen? Documentation is an artifact for human consumption, so it's fair for Rustdoc to accept imprecise constructions as long as what to do with them is clear.

I consider my specific problem solved for now, but I'm not convinced this is the direction Rustdoc should take moving forward.

AlexTMjugador added a commit to zopfli-rs/zopfli that referenced this issue Nov 11, 2023
* chore(deps): update rust crate proptest to 1.4.0

* chore: work around rust-lang/rust#117796

These changes work around a recent change in rustdoc's behavior. The
patch was suggested by @Nemo157.

* chore: fix test compilation under non-std

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Alejandro González <me@alegon.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants