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: Remove Crate.primitives #90447

Closed
wants to merge 1 commit into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Oct 31, 2021

This involves no longer filtering out impls on primitive types that
aren't defined with #[doc(primitive)] somewhere in the dependency
graph. But, I don't think that filtering is necessary or really has any
effect.

@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 31, 2021
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2021
@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the rm-crate-primitives branch from 64e0ea7 to ee2ada4 Compare October 31, 2021 18:31
@rust-log-analyzer

This comment has been minimized.

This involves no longer filtering out impls on primitive types that
aren't defined with `#[doc(primitive)]` somewhere in the dependency
graph. But, I don't think that filtering is necessary or really has any
effect.
@camelid camelid force-pushed the rm-crate-primitives branch from ee2ada4 to d663b8a Compare October 31, 2021 18:44
@jyn514
Copy link
Member

jyn514 commented Oct 31, 2021

This involves no longer filtering out impls on primitive types that
aren't defined with #[doc(primitive)] somewhere in the dependency
graph

Wait, what does this mean? doc(primitive) can only go on modules, I don't know what it has to do with impls ...

if let Some(target_prim) = target.primitive_type() {
cleaner.prims.insert(target_prim);
} else if let Some(target_did) = target.def_id_no_primitives() {
if let Some(target_did) = target.def_id_no_primitives() {
// `impl Deref<Target = S> for S`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in particular makes me worried we'll no longer show Deref methods if an item derefs to a primitive type. Can you add a test for that case?

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
failures:

---- [rustdoc] rustdoc/auto-impl-primitive.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/auto-impl-primitive" "/checkout/src/test/rustdoc/auto-impl-primitive.rs"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
4: @has check failed
 `XPATH PATTERN` did not match
 // @has 'foo/primitive.i16.html' '//h2[@id="synthetic-implementations"]' 'Auto Trait Implementation'
Encountered 1 errors

------------------------------------------

---
test result: FAILED. 475 passed; 1 failed; 4 ignored; 0 measured; 0 filtered out; finished in 45.88s



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--rustdoc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" "--src-base" "/checkout/src/test/rustdoc" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "rustdoc" "--mode" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-12/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "12.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:16:24

@camelid
Copy link
Member Author

camelid commented Oct 31, 2021

This involves no longer filtering out impls on primitive types that
aren't defined with #[doc(primitive)] somewhere in the dependency
graph

Wait, what does this mean? doc(primitive) can only go on modules, I don't know what it has to do with impls ...

Sorry, perhaps I wasn't clear. Let me explain my understanding of the old and new bheavior with an example. Take this impl:

impl SomeTrait for u8 { ... }

Before this PR, that impl would only be kept if #[doc(primitive = "u8")] mod some_mod {} existed somewhere in the dependency graph.

After this PR, that impl would be unconditionally kept (although the current code might not work correctly; I'll add some tests).

@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 31, 2021
@camelid
Copy link
Member Author

camelid commented Oct 31, 2021

Come to think of it, I wonder if BadImplStripper even needs to exist. Probably, but I'm checking just to be sure.

@camelid
Copy link
Member Author

camelid commented Oct 31, 2021

Come to think of it, I wonder if BadImplStripper even needs to exist. Probably, but I'm checking just to be sure.

It turns out that removing it fixes some latent bugs in rustdoc 😆

Specifically, the Notable traits tooltip appears in more places, e.g. the definition (not the std re-export) of Box::pin_in:

Before change After change
alloc
std

There might be regressions mixed into the results too, but this is promising!

@camelid
Copy link
Member Author

camelid commented Oct 31, 2021

There might be regressions mixed into the results too, but this is promising!

So far, the only changes I've found make the alloc docs consistent with the std re-exported docs!

@camelid
Copy link
Member Author

camelid commented Oct 31, 2021

@bors rollup=never

(This may affect perf.)

@GuillaumeGomez
Copy link
Member

Come to think of it, I wonder if BadImplStripper even needs to exist. Probably, but I'm checking just to be sure.

Running rustdoc on proc_macro panicked for me when I removed it. Also, it allows to have less implementations to keep around. I wonder if perf won't be impacted (negatively) by such a change...

@camelid
Copy link
Member Author

camelid commented Nov 1, 2021

Come to think of it, I wonder if BadImplStripper even needs to exist. Probably, but I'm checking just to be sure.

Running rustdoc on proc_macro panicked for me when I removed it. Also, it allows to have less implementations to keep around. I wonder if perf won't be impacted (negatively) by such a change...

I'll try it on proc_macro, that's unfortunate. Re perf: it's better for rustdoc to be consistent than to erroneously strip out impls. Also, perf may be somewhat neutral because there will be more impls, but rustdoc won't be spending time filtering them in the hot collect_trait_impls loop.

@GuillaumeGomez
Copy link
Member

In theory, we don't care about most of the foreign impls. The BadImplStripper is there to remove what we don't need.

@camelid
Copy link
Member Author

camelid commented Nov 1, 2021

In theory, we don't care about most of the foreign impls. The BadImplStripper is there to remove what we don't need.

But in practice, the BadImplStripper only seems to make std and alloc's docs inconsistent, like by excluding traits from the "Notable traits" dialog that should actually be there.

@GuillaumeGomez
Copy link
Member

Well, good luck in removing it then. 😉

Im really interested in the perf change if any.

@camelid
Copy link
Member Author

camelid commented Nov 1, 2021

Well, good luck in removing it then. 😉

Im really interested in the perf change if any.

The hard part of removing it is making sure I don't break anything, but if anything, this will be including more information in the docs, so it shouldn't be possible to have a serious bug (modulo panics).

@jyn514 jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 9, 2021

Marking this as blocked on #83761 due to the panic.

(@camelid mentioned to me it may be possible to fix this specific panic by fixing the MCVE in #90489, but I'm not confident that's true.)

@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2021
@bors
Copy link
Contributor

bors commented Nov 13, 2021

☔ The latest upstream changes (presumably #90385) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@camelid
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 27, 2022
…ives, r=notriddle

Remove Crate::primitives field

It is a new approach to rust-lang#90447. Instead of removing primitives from everywhere (ie from `BadImplStripper`), I just removed them from the `Crate` type, allowing to reduce its size.

cc `@camelid`
r? `@notriddle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants