Handle rustc_query_system cases of rustc::potential_query_instability lint

[ismailarilik]

This PR removes #![allow(rustc::potential_query_instability)] line from compiler/rustc_query_system/src/lib.rs and converts FxHash{Map,Set} types into FxIndex{Map,Set} to suppress lint errors.

A somewhat tracking issue: #84447

r? @compiler-errors

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[ismailarilik]

@rustbot author

[ismailarilik]

@rustbot review

[cjgillot]

Could you point to the place where each of these maps is iterated over?

[ismailarilik]

error: using `iter` can result in unstable query results
    --> compiler/rustc_query_system/src/dep_graph/graph.rs:1392:44
     |
1392 |             if let Some((node, _)) = shard.iter().find(|(_, index)| **index == dep_node_index) {
     |                                            ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale
     = note: `-D rustc::potential-query-instability` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(rustc::potential_query_instability)]`

error: using `values` can result in unstable query results
   --> compiler/rustc_query_system/src/dep_graph/serialized.rs:654:50
    |
654 |             let mut stats: Vec<_> = record_stats.values().collect();
    |                                                  ^^^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
  --> compiler/rustc_query_system/src/query/plumbing.rs:80:34
   |
80 |             for (k, v) in shard?.iter() {
   |                                  ^^^^
   |
   = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `keys` can result in unstable query results
   --> compiler/rustc_query_system/src/query/job.rs:517:47
    |
517 |     let mut jobs: Vec<QueryJobId> = query_map.keys().cloned().collect();
    |                                               ^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
  --> compiler/rustc_query_system/src/query/caches.rs:62:33
   |
62 |             for (k, v) in shard.iter() {
   |                                 ^^^^
   |
   = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

[cjgillot]

@ismailarilik that's not very helpful.
My question is: for each map whose type you are changing, where in code is it iterated over?
Just the iteration sites does not help me to connect to maps' creations.

The point is chosing for each map, what is the best course of action, between changing the type, and allowing the lint because the non-determinism does not show.

For instance, at graph.rs:1391, iterating is ok, as there can be at most one dep_node with the given dep_node_index. So allowing the lint is ok.

[ismailarilik]

For instance, at graph.rs:1391, iterating is ok, as there can be at most one dep_node with the given dep_node_index. So allowing the lint is ok.

Addressed here: 870d7c2

[ismailarilik]

My question is: for each map whose type you are changing, where in code is it iterated over?

In src/query/caches.rs:

• Line 26 (cache: Sharded<FxIndexMap<K, (V, DepNodeIndex)>>,) causes this lint to be triggered in line 62:

  for (k, v) in shard.iter() {
      f(k, &v.0, v.1);
  }

In src/query/job.rs:

• Line 33 (pub type QueryMap = FxIndexMap<QueryJobId, QueryJobInfo>;) causes this lint to be triggered in line 517:

  let mut jobs: Vec<QueryJobId> = query_map.keys().cloned().collect();

In src/query/plumbing.rs:

• Line 36 (active: Sharded<FxHashMap<K, QueryResult>>,) causes this lint to be triggered in line 80:

  for (k, v) in shard?.iter() {
      if let QueryResult::Started(ref job) = *v {
          active.push((*k, job.clone()));
      }
  }

[bors]

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

[apiraino]

r? compiler

[fee1-dead]

I am not sure if this PR is completely justified. For example, as far as I checked, the DefaultCache using a HashMap to store query inputs and outputs should be mostly fine. Everything that seems to iterate over this cache either insert it back into a map anyways or otherwise don't care about iteration order.

To move this forward I'd recommend opening a Zulip thread in t-compiler to get a wider attention for this.

Since I have limited experience in the query system,

r? compiler

[nnethercote]

Passing the review on to somebody who knows more about these data structures than me.

r? @Mark-Simulacrum

[Mark-Simulacrum]

Current state (I think largely matching what was reported before):

warning: using `iter` can result in unstable query results
    --> compiler/rustc_query_system/src/dep_graph/graph.rs:1432:47
     |
1432 |         if let Some((node, _)) = nodes.lock().iter().find(|&(_, index)| *index == dep_node_index) {
     |                                               ^^^^

Per #131200 (comment) this is OK, there's only ever one node result possible so this is deterministic. This is in code that's going to panic immediately afterwards anyway so it doesn't really matter what we do here.

warning: using `values` can result in unstable query results
   --> compiler/rustc_query_system/src/dep_graph/serialized.rs:665:50
    |
665 |             let mut stats: Vec<_> = record_stats.values().collect();
    |                                                  ^^^^^^

This is just stat-dumping code that can't have any effect on query stability. So also fine.

warning: using `keys` can result in unstable query results
   --> compiler/rustc_query_system/src/query/job.rs:512:47
    |
512 |     let mut jobs: Vec<QueryJobId> = query_map.keys().cloned().collect();
    |                                               ^^^^

This I'm less sure about -- @Zoxc is maybe most familiar with our cycle-breaking code, I seem to recall some recent work on that for parallel compilation as well?

I think the next step here is to either open a new PR or rebase this one, but not doing any structure modifications, just allowing the lints for all three cases with a FIXME comment on the 3rd one unless we hear otherwise from someone with more knowledge.

[Zoxc]

Generally this lint doesn't apply to rustc_query_system as it doesn't itself have queries or query results. It doesn't hurt to have more fine grained reasoning to why it's fine though, but I'd avoid the code changes.

This I'm less sure about -- @Zoxc is maybe most familiar with our cycle-breaking code, I seem to recall some recent work on that for parallel compilation as well?

In theory it's possible for query code to inspect cycle errors via Value::from_cycle_error, but it will result in an error, so we'll discard the current incremental session anyway.

[ismailarilik]

@rustbot review

[ismailarilik]

I think this PR can close #84447 since other usages of allow(rustc::potential_query_instability) are either intentional or OK. Could you check as well; I might miss somethings.

[bors]

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

[bors]

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

[fee1-dead]

@bors r+ rollup

[bors]

📌 Commit 85efae7 has been approved by fee1-dead

It is now in the queue for this repository.