add missing potential_query_instability for keys and values in hashmap #120485

chenyukang · 2024-01-30T04:45:23Z

From #120435 (comment),

These API are also returning iterator, so we need add potential_query_instability for them?

rustbot · 2024-01-30T04:45:30Z

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

chenyukang · 2024-01-30T04:49:46Z

link #84447 #89558

michaelwoerister

Thanks for the PR, @chenyukang!

That's a great catch about the missing query instability attributes on the HashMap methods. That was definitely a bug.

I've left a few comments below where it should not be too hard to fix the problem in a cleaner way.

michaelwoerister · 2024-01-30T08:58:09Z

compiler/rustc_codegen_ssa/src/assert_module_sources.rs

@@ -267,6 +267,7 @@ impl CguReuseTracker {

    fn check_expected_reuse(&self, sess: &Session) {
        if let Some(ref data) = self.data {
+            #[allow(rustc::potential_query_instability)]
            let mut keys = data.expected_reuse.keys().collect::<Vec<_>>();


We can fix this properly by using UnordMap instead of FxHashMap for the actual_reuse and expected_reuse fields in TrackerData. That has the to_sorted_stable_ord method that does exactly what we want.

michaelwoerister · 2024-01-30T09:04:31Z

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -403,6 +403,7 @@ fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet {
    let mut result = items.clone();

    for cgu in cgus {
+        #[allow(rustc::potential_query_instability)]


Can you try turning the items field in CodegenUnit into an FxIndexMap. That should make iteration stable (if the collection is built in a deterministic order). An UnordMap would be even better, but I suspect that that would be more work.

michaelwoerister · 2024-01-30T09:13:03Z

compiler/rustc_codegen_ssa/src/back/link.rs

@@ -682,6 +682,7 @@ fn link_dwarf_object<'a>(
        }

        // Input rlibs contain .o/.dwo files from dependencies.
+        #[allow(rustc::potential_query_instability)]
        let input_rlibs = cg_results
            .crate_info
            .used_crate_source


Can you turn the used_crate_source field into an UnordMap? Then you should be able to do this:

// Input rlibs contain .o/.dwo files from dependencies. let input_rlibs = cg_results .crate_info .used_crate_source .items() .filter_map(|(_, csource)| csource.rlib.as_ref()) .map(|(path, _)| path) .into_sorted_stable_ord();

michaelwoerister · 2024-01-30T09:15:54Z

compiler/rustc_hir_typeck/src/expr.rs

@@ -1950,6 +1950,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
    ) {
        let len = remaining_fields.len();

+        #[allow(rustc::potential_query_instability)]


It should be straightforward to make remaining_fields and UnordMap.

chenyukang · 2024-01-30T09:39:19Z

Thanks for the PR, @chenyukang!

That's a great catch about the missing query instability attributes on the HashMap methods. That was definitely a bug.

I've left a few comments below where it should not be too hard to fix the problem in a cleaner way.

There was an issue to track these kinds of cleanups:
#84447

I may create a separate PR to clean up those replacements?

michaelwoerister · 2024-01-30T11:18:51Z

This is also closely related: rust-lang/compiler-team#533

It is where the unord collections were proposed. They've been implemented meanwhile and are quite mature (although it seems that UnordMap misses fn keys(&self) -> UnordItems<&K, impl Iterator<Item = &K>> for example).

I may create a separate PR to clean up those replacements?

Sure, you can r? @michaelwoerister on that. Let me know if you plan to do any further changes to this PR, otherwise I'm happy to approve it.

chenyukang · 2024-01-30T13:22:14Z

Ok, please approve this one, I may work on cleanups in the weekend.

michaelwoerister · 2024-01-30T13:48:44Z

@bors r+ rollup

bors · 2024-01-30T13:48:46Z

📌 Commit ad526d8 has been approved by michaelwoerister

It is now in the queue for this repository.

klensy · 2024-01-30T13:55:01Z

compiler/rustc_lint/src/context.rs

-        debug!(
-            "is_lint_group(lint_name={:?}, lint_groups={:?})",
-            lint_name,
-            self.lint_groups.keys().collect::<Vec<_>>()
-        );
+        #[allow(rustc::potential_query_instability)]
+        let lint_groups = self.lint_groups.keys().collect::<Vec<_>>();
+        debug!("is_lint_group(lint_name={:?}, lint_groups={:?})", lint_name, lint_groups);


Will collect things on release now, instead of debug only, next one too. cfg(debug_assertions)?

you are right, I will clean this up when I replace HashMap.

…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#117906 (Improve display of crate name when hovered) - rust-lang#118533 (Suppress unhelpful diagnostics for unresolved top level attributes) - rust-lang#120293 (Deduplicate more sized errors on call exprs) - rust-lang#120295 (Remove `raw_os_nonzero` feature.) - rust-lang#120310 (adapt test for v0 symbol mangling) - rust-lang#120342 (Remove various `has_errors` or `err_count` uses) - rust-lang#120434 (Revert outdated version of "Add the wasm32-wasi-preview2 target") - rust-lang#120445 (Fix some `Arc` allocator leaks) - rust-lang#120475 (Improve error message when `cargo build` is used to build the compiler) - rust-lang#120476 (Remove some unnecessary check logic for lang items in HIR typeck) - rust-lang#120485 (add missing potential_query_instability for keys and values in hashmap) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120485 - chenyukang:yukang-add-query-instability-check, r=michaelwoerister add missing potential_query_instability for keys and values in hashmap From rust-lang#120435 (comment), These API are also returning iterator, so we need add `potential_query_instability` for them?

…=<try> Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang#120485 (comment) r? `@michaelwoerister`

…=michaelwoerister Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang#120485 (comment) r? `@michaelwoerister`

…oerister Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang/rust#120485 (comment) r? `@michaelwoerister`

add missing potential_query_instability for keys and values in hashmap

ad526d8

rustbot assigned michaelwoerister Jan 30, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 30, 2024

michaelwoerister reviewed Jan 30, 2024

View reviewed changes

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 Jan 30, 2024

klensy reviewed Jan 30, 2024

View reviewed changes

GuillaumeGomez mentioned this pull request Jan 30, 2024

Rollup of 11 pull requests #120496

Merged

bors merged commit 27bc496 into rust-lang:master Jan 30, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 30, 2024

chenyukang mentioned this pull request Feb 11, 2024

Clean up potential_query_instability with FxIndexMap and UnordMap #120931

Merged

chenyukang mentioned this pull request May 7, 2024

Audit and handle remaining cases of rustc::potential_query_instability lint (iterating HashMaps) #84447

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add missing potential_query_instability for keys and values in hashmap #120485

add missing potential_query_instability for keys and values in hashmap #120485

chenyukang commented Jan 30, 2024

rustbot commented Jan 30, 2024

chenyukang commented Jan 30, 2024

michaelwoerister left a comment

michaelwoerister Jan 30, 2024

michaelwoerister Jan 30, 2024

michaelwoerister Jan 30, 2024

michaelwoerister Jan 30, 2024

chenyukang commented Jan 30, 2024 •

edited

Loading

michaelwoerister commented Jan 30, 2024

chenyukang commented Jan 30, 2024

michaelwoerister commented Jan 30, 2024

bors commented Jan 30, 2024

klensy Jan 30, 2024 •

edited

Loading

chenyukang Jan 31, 2024

add missing potential_query_instability for keys and values in hashmap #120485

add missing potential_query_instability for keys and values in hashmap #120485

Conversation

chenyukang commented Jan 30, 2024

rustbot commented Jan 30, 2024

chenyukang commented Jan 30, 2024

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Jan 30, 2024

Choose a reason for hiding this comment

michaelwoerister Jan 30, 2024

Choose a reason for hiding this comment

michaelwoerister Jan 30, 2024

Choose a reason for hiding this comment

michaelwoerister Jan 30, 2024

Choose a reason for hiding this comment

chenyukang commented Jan 30, 2024 • edited Loading

michaelwoerister commented Jan 30, 2024

chenyukang commented Jan 30, 2024

michaelwoerister commented Jan 30, 2024

bors commented Jan 30, 2024

klensy Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

chenyukang Jan 31, 2024

Choose a reason for hiding this comment

chenyukang commented Jan 30, 2024 •

edited

Loading

klensy Jan 30, 2024 •

edited

Loading