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

Use same FxHashMap in rustdoc-json-types and librustdoc. #110051

Closed
wants to merge 1 commit into from

Conversation

aDotInTheVoid
Copy link
Member

The original motivation was me trying to remove the #![allow(rustc::default_hash_types)], as after #108626, we should be using FxHashMap here. I then realized I should also be able to remove the .into_iter().collect(), as we no longer need to convert from FxHashMap to std::HashMap.

However, this didn't work, and I got the following error

error[E0308]: mismatched types
   --> src/librustdoc/json/mod.rs:235:13
    |
235 |             index,
    |             ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher`
    |
    = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types
note: `FxHasher` is defined in crate `rustc_hash`
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1
note: `rustc_hash::FxHasher` is defined in crate `rustc_hash`
   --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1
    |
60  | pub struct FxHasher {
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `rustc_hash` are being used?

This is because librustdoc got it's FxHashMap via the sysroot rustc-data-strucures, whereas rustdoc-json-types got it via rustc-hash on crates.io.

To avoid this, rustdoc-json-types now uses
#![feature(rustc_private)] to load the same version as librustdoc.

However, this needs to be placed behind a feature, as rustdoc-json-types is also dependency of src/tools/jsondocck, which means need needs to be buildable without nightly features.

r? @jyn514

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2023

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@jyn514
Copy link
Member

jyn514 commented Apr 7, 2023

However, this needs to be placed behind a feature, as rustdoc-json-types is also dependency of src/tools/jsondocck, which means need needs to be buildable without nightly features.

Hmm, I thought the motivation was that you want to be able to auto publish rustdoc-json-types to crates.io so people can use it? Is jsondocck built with the beta compiler or something like that?

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

👍 this looks good, thanks! r=me when CI passes

@aDotInTheVoid
Copy link
Member Author

Is jsondocck built with the beta compiler or something like that?

Yeah, tools arn't allowed to use nightly features (Not sure if build with beta, but it's enforced with allowed-features). Trying to use rustc_private when build for jsondoclint looks like

Building tool jsondoclint (stage0)
   Compiling libc v0.2.140
   Compiling io-lifetimes v1.0.3
   Compiling rustix v0.36.5
   Compiling bitflags v1.3.2
   Compiling linux-raw-sys v0.1.4
   Compiling utf8parse v0.2.1
   Compiling anstyle v0.3.5
   Compiling concolor-query v0.3.3
   Compiling concolor-override v1.0.0
   Compiling heck v0.4.0
   Compiling syn v2.0.8
   Compiling serde_json v1.0.85
   Compiling anyhow v1.0.65
   Compiling anstyle-parse v0.1.1
   Compiling strsim v0.10.0
   Compiling clap_lex v0.4.1
   Compiling rustdoc-json-types v0.1.0 (/home/alona/dev/rust/rust/src/rustdoc-json-types)
error[E0725]: the feature `rustc_private` is not in the list of allowed features
 --> src/rustdoc-json-types/lib.rs:6:12
  |
6 | #![feature(rustc_private)]
  |            ^^^^^^^^^^^^^

error[E0463]: can't find crate for `rustc_hash`
 --> src/rustdoc-json-types/lib.rs:7:1
  |
7 | extern crate rustc_hash;
  | ^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate
  |
  = help: maybe you need to install the missing components with: `rustup component add rust-src rustc-dev llvm-tools-preview`

   Compiling rustc-hash v1.1.0

Hmm, I thought the motivation was that you want to be able to auto publish rustdoc-json-types to crates.io so people can use it?

I already do this (but not automaticly, and not in an official capacity) with aDotInTheVoid/rustdoc-types. This happens to make that slightly simpler, but wasn't the motivation (rust-lang/rustdoc-types#22)

@jyn514
Copy link
Member

jyn514 commented Apr 7, 2023

Building tool jsondoclint (stage0)

This bit means it's built with beta :)

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 7, 2023

📌 Commit 12ad0d6 has been approved by jyn514

It is now in the queue for this repository.

@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 Apr 7, 2023
notriddle added a commit to notriddle/rust that referenced this pull request Apr 8, 2023
Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`.

The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`.

However, this didn't work, and I got the following error

```
error[E0308]: mismatched types
   --> src/librustdoc/json/mod.rs:235:13
    |
235 |             index,
    |             ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher`
    |
    = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types
note: `FxHasher` is defined in crate `rustc_hash`
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1
note: `rustc_hash::FxHasher` is defined in crate `rustc_hash`
   --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1
    |
60  | pub struct FxHasher {
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `rustc_hash` are being used?
```

This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io.

To avoid this, `rustdoc-json-types` now uses
`#![feature(rustc_private)]` to load the same version as `librustdoc`.

However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features.

r? `@jyn514`
@Noratrieb
Copy link
Member

#110073 (comment)

@bors r-

@bors bors added 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 8, 2023
@aDotInTheVoid
Copy link
Member Author

Running ./x dist with profile = "compiler", I can't reproduce the issue locally after rebasing.

@bors try

@bors
Copy link
Contributor

bors commented Apr 14, 2023

⌛ Trying commit 1410337 with merge 45fa4ef132fa25eaff3a9b6d127746c25f1c7e4d...

@bors
Copy link
Contributor

bors commented Apr 14, 2023

☀️ Try build successful - checks-actions
Build commit: 45fa4ef132fa25eaff3a9b6d127746c25f1c7e4d (45fa4ef132fa25eaff3a9b6d127746c25f1c7e4d)

@aDotInTheVoid
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Apr 14, 2023

📌 Commit 1410337 has been approved by jyn514

It is now in the queue for this repository.

@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 Apr 14, 2023
@bors
Copy link
Contributor

bors commented Apr 15, 2023

⌛ Testing commit 1410337 with merge e56d2f3fdfca65541fab42024effb381d393ebf2...

@Dylan-DPC Dylan-DPC added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2024
@aDotInTheVoid
Copy link
Member Author

@rustbot author

@rustbot rustbot added 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 Jul 6, 2024
@rust-log-analyzer

This comment has been minimized.

@aDotInTheVoid
Copy link
Member Author

@bors try

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@aDotInTheVoid
Copy link
Member Author

@bors ping

2 similar comments
@aDotInTheVoid
Copy link
Member Author

@bors ping

@aDotInTheVoid
Copy link
Member Author

@bors ping

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 7, 2024
Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`.

Reopening of rust-lang#110051, because bors has seemingly abandoned us there :'(

`@bors` try

r? `@ghost`
@pietroalbini pietroalbini reopened this Jul 7, 2024
@pietroalbini
Copy link
Member

@bors ping

@bors
Copy link
Contributor

bors commented Jul 7, 2024

😪 I'm awake I'm awake

@Noratrieb
Copy link
Member

Noratrieb commented Jul 7, 2024

uh, this is marked as pending, blocking CI from making progress.

@Noratrieb Noratrieb closed this Jul 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`.

Reopening of rust-lang#110051, because bors has seemingly abandoned us there :'(

try-job: dist-arm-linux

try-job: dist-armhf-linux

try-job: armhf-gnu

try-job: test-various

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
rustdoc-json: Use FxHashMap from rustdoc_json_types

Alternative to rust-lang#110051 and rust-lang#127456.

This lets us avoid rehashing the json index after building it by using the same hashmap type in construction and in rustdoc_json_types. The above PR's tried to do this by having rustdoc_json_types get the same hashmap that rustdoc has via rustc_data_structures. However, this can be made simpler if we change rustdoc instead.

For the rustdoc-type republish on crates.io, I'll filter out the `pub use`, and not change the API at all. That code [already replaces the hashmap](https://github.com/aDotInTheVoid/rustdoc-types/blob/8d6528669ec64c2af43d1c79a228b7711cefdad7/update.sh#L11) to use the one in `std::collections` (instead of `FxHashMap`)

try-job: dist-arm-linux

r? `@GuillaumeGomez`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2024
…llaumeGomez

rustdoc-json: Use FxHashMap from rustdoc_json_types

Alternative to rust-lang#110051 and rust-lang#127456.

This lets us avoid rehashing the json index after building it by using the same hashmap type in construction and in rustdoc_json_types. The above PR's tried to do this by having rustdoc_json_types get the same hashmap that rustdoc has via rustc_data_structures. However, this can be made simpler if we change rustdoc instead.

For the rustdoc-type republish on crates.io, It will filter out the `pub use`, and not change source at all. rust-lang/rustdoc-types#30. That code [already replaces the hashmap](https://github.com/aDotInTheVoid/rustdoc-types/blob/8d6528669ec64c2af43d1c79a228b7711cefdad7/update.sh#L11) to use the one in `std::collections` (instead of `FxHashMap`)

try-job: dist-arm-linux

r? `@GuillaumeGomez`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2024
…llaumeGomez

rustdoc-json: Use FxHashMap from rustdoc_json_types

Alternative to rust-lang#110051 and rust-lang#127456.

This lets us avoid rehashing the json index after building it by using the same hashmap type in construction and in rustdoc_json_types. The above PR's tried to do this by having rustdoc_json_types get the same hashmap that rustdoc has via rustc_data_structures. However, this can be made simpler if we change rustdoc instead.

For the rustdoc-type republish on crates.io, It will filter out the `pub use`, and not change source at all. rust-lang/rustdoc-types#30. That code [already replaces the hashmap](https://github.com/aDotInTheVoid/rustdoc-types/blob/8d6528669ec64c2af43d1c79a228b7711cefdad7/update.sh#L11) to use the one in `std::collections` (instead of `FxHashMap`)

try-job: dist-arm-linux

r? ``@GuillaumeGomez``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
Rollup merge of rust-lang#129124 - aDotInTheVoid:rdj-hashmap-3, r=GuillaumeGomez

rustdoc-json: Use FxHashMap from rustdoc_json_types

Alternative to rust-lang#110051 and rust-lang#127456.

This lets us avoid rehashing the json index after building it by using the same hashmap type in construction and in rustdoc_json_types. The above PR's tried to do this by having rustdoc_json_types get the same hashmap that rustdoc has via rustc_data_structures. However, this can be made simpler if we change rustdoc instead.

For the rustdoc-type republish on crates.io, It will filter out the `pub use`, and not change source at all. rust-lang/rustdoc-types#30. That code [already replaces the hashmap](https://github.com/aDotInTheVoid/rustdoc-types/blob/8d6528669ec64c2af43d1c79a228b7711cefdad7/update.sh#L11) to use the one in `std::collections` (instead of `FxHashMap`)

try-job: dist-arm-linux

r? ``@GuillaumeGomez``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.