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

[WIP] Remove allow(rustc::potential_query_instability) in rustc_session #99065

Closed

Conversation

NiklasJonsson
Copy link
Contributor

@NiklasJonsson NiklasJonsson commented Jul 8, 2022

Replace a use of FxHashMap with FxIndexMap

Relates #84447

Haven't contributed before so not 100 % sure of who to ask for a review but

r? @cjgillot

seems reasonable as you commented last on the issue 😄

Replace a use of FxHashMap with FxIndexMap
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 8, 2022
@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 @cjgillot (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 Jul 8, 2022
let mut inner = self.spans.borrow_mut();
for (gate, mut gate_spans) in inner.drain() {
for (gate, mut gate_spans) in inner.drain(..) {
Copy link
Contributor Author

@NiklasJonsson NiklasJonsson Jul 8, 2022

Choose a reason for hiding this comment

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

As this just puts the contents of the drained hashmap into another one, I don't see a way that the iteration order can affect the resulting map, so we could also add a local allow here but replacing the map seemed preferable. Though, I'm not sure if this has any performance implications.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking rustc_session v0.0.0 (/checkout/compiler/rustc_session)
error[E0433]: failed to resolve: use of undeclared type `FxHashMap`
   --> compiler/rustc_session/src/parse.rs:201:51
    |
201 |             ambiguous_block_expr_parse: Lock::new(FxHashMap::default()),
    |
   ::: /cargo/registry/src/github.com-1ecc6299db9ec823/rustc-hash-1.1.0/src/lib.rs:47:1
    |
    |
47  | pub type FxHashSet<V> = HashSet<V, BuildHasherDefault<FxHasher>>;
    | ----------------------------------------------------------------- similarly named type alias `FxHashSet` defined here
help: a type alias with a similar name exists
    |
    |
201 |             ambiguous_block_expr_parse: Lock::new(FxHashSet::default()),
help: consider importing this type alias
    |
4   | use rustc_data_structures::stable_map::FxHashMap;
    |

@NiklasJonsson
Copy link
Contributor Author

NiklasJonsson commented Jul 8, 2022

This wasn't as ready as I thought, must've done some mistake with git. No, just a bit harder than I thought.

@rustbot label -S-waiting-on-review

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2022
@NiklasJonsson NiklasJonsson changed the title Remove allow(rustc::potential_query_instability) in rustc_session [WIP] Remove allow(rustc::potential_query_instability) in rustc_session Jul 8, 2022
@NiklasJonsson
Copy link
Contributor Author

compiler/rustc_session/src/config.rs has this code that fails when enabling the lint:

impl<T> CheckCfg<T> {
    fn map_data<O: Eq + Hash>(&self, f: impl Fn(&T) -> O) -> CheckCfg<O> {
        CheckCfg {
            names_valid: self
                .names_valid // FxHashSet
                .as_ref()
                .map(|names_valid| names_valid.iter().map(|a| f(a)).collect()),
            values_valid: self
                .values_valid // FxHashMap<T, FxHashSet>
                .iter()
                .map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect()))
                .collect(),
            well_known_values: self.well_known_values,
        }
    }
}

which seems fine enough as it just creates a new hashmap in-place with the mapped values and keys, assuming f doesn't have side-effects, which I don't think it can as it is Fn. But we want to replace this, which should be straight-forward but the file also has this code:

            // Get all values map at once otherwise it would be costly.
            // (8 values * 220 targets ~= 1760 times, at the time of writing this comment).
            let [
                values_target_os,
                values_target_family,
                values_target_arch,
                values_target_endian,
                values_target_env,
                values_target_abi,
                values_target_vendor,
                values_target_pointer_width,
            ] = self
                .values_valid
                .get_many_mut(VALUES)
                .expect("unable to get all the check-cfg values buckets");

which makes use of get_many_mut which is only implemented on HashMap and not the IndexMap, which means we can't replace it to avoid the lint. I'll close this for now and see if I can get together something with a smaller scope.

@jyn514
Copy link
Member

jyn514 commented Jul 8, 2022

which makes use of get_many_mut which is only implemented on HashMap and not the IndexMap, which means we can't replace it to avoid the lint

FWIW I think it would be fine to add that API to IndexMap

@NiklasJonsson
Copy link
Contributor Author

FWIW I think it would be fine to add that API to IndexMap

Yeah I figured but I didn't want to leave this PR in limbo here until that is done (possibly by me).

@NiklasJonsson
Copy link
Contributor Author

get_many_mut for IndexMap is in a PR here but blocked in the standard hashmap api getting stabilized.

@jyn514
Copy link
Member

jyn514 commented Jul 16, 2022

Can IndexMap add it under a nightly or unstable opt-in cargo feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants