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.

Sign up for GitHub

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

Jump to bottom

rustdoc: use Type::def_id() instead of Type::def_id_no_primitives() #90385

Merged
merged 2 commits into from
Nov 13, 2021
Merged

rustdoc: use Type::def_id() instead of Type::def_id_no_primitives() #90385

merged 2 commits into from
Nov 13, 2021

Conversation

mfrw
Copy link
Contributor

@mfrw mfrw commented Oct 28, 2021 •
edited
Loading

For: #90187

r? @jyn514

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 28, 2021

Thanks for working on this! Do you mind also removing the def_id_no_primitives method at the same time to avoid dead code?

@jyn514 jyn514 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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 30, 2021

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member

camelid commented Nov 1, 2021

Hmm, somehow this makes generics show up in search results (that's why the test is failing AFAICT). I don't think that's supposed to happen; otherwise searching for T would return a lot of results. cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Indeed, for the search index, we don't want to keep generics as is. It can be fixed by turning the cache argument into an Option and passing None when using it for the search index.

@camelid
Copy link
Member

camelid commented Nov 1, 2021 •
edited
Loading

Indeed, for the search index, we don't want to keep generics as is. It can be fixed by turning the cache argument into an Option and passing None when using it for the search index.

But why does switching to def_id from def_id_no_primitives affect whether generics show up in the search results? They're not primitives...

Also, using Option doesn't seem like a robust way to solve the problem.

@GuillaumeGomez
Copy link
Member

I'll test locally tomorrow to see what's going on because from what I can see, the code is perfectly valid. You have allowed to uncover a bug in rustdoc. (yeay! 🥳 🎉 )

@mfrw
Copy link
Contributor Author

mfrw commented Nov 2, 2021

I'll test locally tomorrow to see what's going on because from what I can see, the code is perfectly valid. You have allowed to uncover a bug in rustdoc. (yeay! 🥳 🎉 )

Yayay .. I am noob and I only could stare at the code and now was reading more about rust if i was missing something trivial.
ALso while we are here, what is the guidance on removing this when we are implementing a trait like this. It seems it does not have access to the Cache directly.

@camelid
Copy link
Member

camelid commented Nov 2, 2021

ALso while we are here, what is the guidance on removing this when we are implementing a trait like this. It seems it does not have access to the Cache directly.

You should be able to add a cache: &'a Cache field to the struct that the trait is being implemented on. I'm in the process of adding Cache fields to all those places already, so you could also just skip that usage for now.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Sorry, I was focused on other things and completely forgot about this. Scheduling it for tomorrow.

@jyn514
Copy link
Member

jyn514 commented Nov 8, 2021

r? @GuillaumeGomez

@mfrw
Copy link
Contributor Author

mfrw commented Nov 9, 2021

You should be able to add a cache: &'a Cache field to the struct that the trait is being implemented on. I'm in the process of adding Cache fields to all those places already, so you could also just skip that usage for now.

Since you adding Cache fields, should I wait and then update the PR. @camelid

@camelid
Copy link
Member

camelid commented Nov 9, 2021

You should be able to add a cache: &'a Cache field to the struct that the trait is being implemented on. I'm in the process of adding Cache fields to all those places already, so you could also just skip that usage for now.

Since you adding Cache fields, should I wait and then update the PR. @camelid

That work is blocked for now, so feel free to add the fields yourself if you can.

This was referenced Nov 9, 2021
Closed
Merged
@GuillaumeGomez
Copy link
Member

I found where the issue was coming from: #90726. I'm just very surprised we didn't find it out sooner. Well, I'm quite happy you uncovered it because thanks to you, it allowed to fix this issue and helped me uncover another one: #90727. So thanks! :)

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 13, 2021

📌 Commit 5cfc7ce has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 13, 2021
@matthiaskrgr
Rollup merge of rust-lang#90385 - mfrw:mfrw/librustdoc, r=GuillaumeGomez
6831829 
rustdoc: use Type::def_id() instead of Type::def_id_no_primitives()

For: rust-lang#90187

r? `@jyn514`
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 13, 2021
Closed
@bors
Copy link
Contributor

bors commented Nov 13, 2021

⌛ Testing commit 5cfc7ce with merge d246517fab83720aea209818ae254f2a3e309ba8...

@bors
Copy link
Contributor

bors commented Nov 13, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 13, 2021
@GuillaumeGomez
Copy link
Member

Spurious error.

@bors: retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2021
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot) 
   Compiling toml v0.5.7
error: could not compile `bootstrap`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustc --crate-name llvm_config_wrapper --edition=2021 src/bootstrap/bin/llvm-config-wrapper.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=1 -C metadata=fc81b2ae968cfb1d -C extra-filename=-fc81b2ae968cfb1d --out-dir /checkout/obj/build/bootstrap/debug/deps -C incremental=/checkout/obj/build/bootstrap/debug/incremental -L dependency=/checkout/obj/build/bootstrap/debug/deps --extern bootstrap=/checkout/obj/build/bootstrap/debug/deps/libbootstrap-b49b759706b6c45d.rlib --extern build_helper=/checkout/obj/build/bootstrap/debug/deps/libbuild_helper-dded4ef7a66d7fd2.rlib --extern cc=/checkout/obj/build/bootstrap/debug/deps/libcc-9022b5d7a536baba.rlib --extern cmake=/checkout/obj/build/bootstrap/debug/deps/libcmake-a1cd7916c7902e9e.rlib --extern filetime=/checkout/obj/build/bootstrap/debug/deps/libfiletime-21f7d6d27914ba34.rlib --extern getopts=/checkout/obj/build/bootstrap/debug/deps/libgetopts-3bc5fa5912184ba0.rlib --extern ignore=/checkout/obj/build/bootstrap/debug/deps/libignore-6eab7523daa887b2.rlib --extern lazy_static=/checkout/obj/build/bootstrap/debug/deps/liblazy_static-1f5d3923e02f6398.rlib --extern libc=/checkout/obj/build/bootstrap/debug/deps/liblibc-214e4f81fa8cd553.rlib --extern merge=/checkout/obj/build/bootstrap/debug/deps/libmerge-ba43cbd72628adcc.rlib --extern num_cpus=/checkout/obj/build/bootstrap/debug/deps/libnum_cpus-18e3b0eb22fae47b.rlib --extern once_cell=/checkout/obj/build/bootstrap/debug/deps/libonce_cell-5bf50caf48c031d7.rlib --extern opener=/checkout/obj/build/bootstrap/debug/deps/libopener-204d18544f7127ea.rlib --extern serde=/checkout/obj/build/bootstrap/debug/deps/libserde-e7ed636813468773.rlib --extern serde_json=/checkout/obj/build/bootstrap/debug/deps/libserde_json-b69a92aa1055d696.rlib --extern time=/checkout/obj/build/bootstrap/debug/deps/libtime-1b8611857d71d3f6.rlib --extern toml=/checkout/obj/build/bootstrap/debug/deps/libtoml-477f7006fd660090.rlib -Wrust_2018_idioms -Wunused_lifetimes -Wsemicolon_in_expressions_from_macros -Dwarnings` (signal: 11, SIGSEGV: invalid memory reference)
error: build failed
failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
Build completed unsuccessfully in 0:01:17
make: *** [prepare] Error 1
---
DirectMap4k:      247744 kB
DirectMap2M:     4995072 kB
DirectMap1G:    55574528 kB
    Finished dev [unoptimized] target(s) in 0.21s
*** Error in `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo': free(): invalid pointer: 0x000055de8ebf5560 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777f5)[0x7f1c1fb067f5]
/lib/x86_64-linux-gnu/libc.so.6(+0x8038a)[0x7f1c1fb0f38a]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f1c1fb1358c]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x9c5a6f)[0x55de8e421a6f]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x9b898a)[0x55de8e41498a]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x9c5ad5)[0x55de8e421ad5]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x9b138b)[0x55de8e40d38b]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x9b6683)[0x55de8e412683]
/lib/x86_64-linux-gnu/libc.so.6(+0x3a008)[0x7f1c1fac9008]
/lib/x86_64-linux-gnu/libc.so.6(+0x3a055)[0x7f1c1fac9055]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf7)[0x7f1c1faaf847]
/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo(+0x176f41)[0x55de8dbd2f41]
======= Memory map: ========
55de8da5c000-55de8e85b000 r-xp 00000000 08:11 13950351                   /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo
55de8ea5a000-55de8eb00000 r--p 00dfe000 08:11 13950351                   /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo
55de8eb00000-55de8eb08000 rw-p 00ea4000 08:11 13950351                   /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo
55de8eb08000-55de8eb0d000 rw-p 00000000 00:00 0 
55de8ebf5000-55de9013c000 rw-p 00000000 00:00 0                          [heap]
7f1c14000000-7f1c14021000 rw-p 00000000 00:00 0 
7f1c14021000-7f1c18000000 ---p 00000000 00:00 0 
7f1c1a013000-7f1c1a014000 ---p 00000000 00:00 0 
7f1c1a014000-7f1c1a214000 rw-p 00000000 00:00 0 
7f1c1fa8f000-7f1c1fc4f000 r-xp 00000000 00:35 4128909                    /lib/x86_64-linux-gnu/libc-2.23.so
7f1c1fc4f000-7f1c1fe4f000 ---p 001c0000 00:35 4128909                    /lib/x86_64-linux-gnu/libc-2.23.so
7f1c1fe4f000-7f1c1fe53000 r--p 001c0000 00:35 4128909                    /lib/x86_64-linux-gnu/libc-2.23.so
7f1c1fe53000-7f1c1fe55000 rw-p 001c4000 00:35 4128909                    /lib/x86_64-linux-gnu/libc-2.23.so
7f1c1fe55000-7f1c1fe59000 rw-p 00000000 00:00 0 
7f1c1fe59000-7f1c1fe5c000 r-xp 00000000 00:35 4128922                    /lib/x86_64-linux-gnu/libdl-2.23.so
7f1c1fe5c000-7f1c2005b000 ---p 00003000 00:35 4128922                    /lib/x86_64-linux-gnu/libdl-2.23.so
7f1c2005b000-7f1c2005c000 r--p 00002000 00:35 4128922                    /lib/x86_64-linux-gnu/libdl-2.23.so
7f1c2005c000-7f1c2005d000 rw-p 00003000 00:35 4128922                    /lib/x86_64-linux-gnu/libdl-2.23.so
7f1c2005d000-7f1c20165000 r-xp 00000000 00:35 4128941                    /lib/x86_64-linux-gnu/libm-2.23.so
7f1c20165000-7f1c20364000 ---p 00108000 00:35 4128941                    /lib/x86_64-linux-gnu/libm-2.23.so
7f1c20364000-7f1c20365000 r--p 00107000 00:35 4128941                    /lib/x86_64-linux-gnu/libm-2.23.so
7f1c20365000-7f1c20366000 rw-p 00108000 00:35 4128941                    /lib/x86_64-linux-gnu/libm-2.23.so
7f1c20366000-7f1c2037e000 r-xp 00000000 00:35 4128977                    /lib/x86_64-linux-gnu/libpthread-2.23.so
7f1c2037e000-7f1c2057d000 ---p 00018000 00:35 4128977                    /lib/x86_64-linux-gnu/libpthread-2.23.so
7f1c2057d000-7f1c2057e000 r--p 00017000 00:35 4128977                    /lib/x86_64-linux-gnu/libpthread-2.23.so
7f1c2057e000-7f1c2057f000 rw-p 00018000 00:35 4128977                    /lib/x86_64-linux-gnu/libpthread-2.23.so
7f1c2057f000-7f1c20583000 rw-p 00000000 00:00 0 
7f1c20583000-7f1c2058a000 r-xp 00000000 00:35 4128983                    /lib/x86_64-linux-gnu/librt-2.23.so
7f1c2058a000-7f1c20789000 ---p 00007000 00:35 4128983                    /lib/x86_64-linux-gnu/librt-2.23.so
7f1c20789000-7f1c2078a000 r--p 00006000 00:35 4128983                    /lib/x86_64-linux-gnu/librt-2.23.so
7f1c2078a000-7f1c2078b000 rw-p 00007000 00:35 4128983                    /lib/x86_64-linux-gnu/librt-2.23.so
7f1c2078b000-7f1c207a1000 r-xp 00000000 00:35 4128930                    /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1c207a1000-7f1c209a0000 ---p 00016000 00:35 4128930                    /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1c209a0000-7f1c209a1000 rw-p 00015000 00:35 4128930                    /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1c209a1000-7f1c209c7000 r-xp 00000000 00:35 4128889                    /lib/x86_64-linux-gnu/ld-2.23.so
7f1c20bbb000-7f1c20bc1000 rw-p 00000000 00:00 0 
7f1c20bc5000-7f1c20bc6000 rw-p 00000000 00:00 0 
7f1c20bc6000-7f1c20bc7000 r--p 00025000 00:35 4128889                    /lib/x86_64-linux-gnu/ld-2.23.so
7f1c20bc7000-7f1c20bc8000 rw-p 00026000 00:35 4128889                    /lib/x86_64-linux-gnu/ld-2.23.so
7f1c20bc8000-7f1c20bc9000 rw-p 00000000 00:00 0 
7fff09a6e000-7fff09a8f000 rw-p 00000000 00:00 0                          [stack]
7fff09af8000-7fff09afb000 r--p 00000000 00:00 0                          [vvar]
7fff09afb000-7fff09afc000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Build completed unsuccessfully in 0:00:00

@mfrw
Copy link
Contributor Author

mfrw commented Nov 13, 2021

@bors: retry

@bors
Copy link
Contributor

bors commented Nov 13, 2021

@mfrw: 🔑 Insufficient privileges: not in try users

@GuillaumeGomez
Copy link
Member

I think CI is broken. cc @rust-lang/infra

@bors
Copy link
Contributor

bors commented Nov 13, 2021

⌛ Testing commit 5cfc7ce with merge b416e38...

@bors
Copy link
Contributor

bors commented Nov 13, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing b416e38 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 13, 2021
@bors bors merged commit b416e38 into rust-lang:master Nov 13, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 13, 2021
@bors bors mentioned this pull request Nov 13, 2021
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b416e38): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@mfrw mfrw deleted the mfrw/librustdoc branch November 16, 2021 09:24
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.
9 participants
@mfrw @rust-highfive @jyn514 @rust-log-analyzer @bors @camelid @GuillaumeGomez @rust-timer @rustbot