cc @Zalathar @Zoxc @zetanumbers

Is it correct to radically change the shard locking behaviour like this?

Looks like this effectively reverts #148777, which was a bugfix specifically relying on calling lock_shards instead of try_lock_shards.
cc @ywxt to confirm

Having the same thing in both the Ok and Err cases was strange.

I disagree, the first thing is a good map, the second thing is an erroneous map for diagnostics.
Result here works as usual and doesn't allow forgetting about the error checking, unlike the boolean.
(Replacing Option<()> with bools seems ok.)

Thanks for the info! Looks like making this a draft PR was the right decision :)

I still don't like the code structure. Even with the comments it was strange enough that I misunderstood what it was doing. I'll try to find another approach to improve that while preserving the try_lock/lock distinction.

I also wonder why a test wasn't added in #148777. My understanding of parallel front-end testing is poor, this might force me to look into it.

The testing isn't even merged yet, the relevant PR is #143953.
Zulip thread: #t-compiler/parallel-rustc > Add the parallel front-end test suite

The require_complete boolean plus the use/non-use of expect/unwrap on the result means there are four possible scenarios for this function.

// - unwrap: need the full map
// - true => lock_shards: willing to wait for locks, i.e. there might be
//   contention(?)
// - used from:
//   - depth_limit_error
collect_active_jobs_from_all_queries(tcx, true).unwrap()

// - unwrap: need the full map
// - false => try_lock_shards: not willing to wait for locks, i.e. we assume
//   there is no contention(?)
// - used from:
//   - the deadlock handler (in `rustc_in_thread_pool_with_globals`)
//   - cycle_error
collect_active_jobs_from_all_queries(tcx, false).unwrap()

// - unwrap_or_else: partial map is acceptable
// - true => lock_shards: willing to wait for locks, i.e. there might be
//   contention
// - used from:
//   - (nowhere)
collect_active_jobs_from_all_queries(tcx, true).unwrap_or_else(|m| m)

// - unwrap_or_else: partial map is acceptable
// - false => try_lock_shards: not willing to wait for locks, i.e. we assume
//   there is no contention(?)
// - used from:
//   - print_query_stack
collect_active_jobs_from_all_queries(tcx, true).unwrap_or_else(|m| m)

Only three of them are used in practice. Maybe require_complete should be replaced by a three-value enum like this:

enum CollectKind {
    Full,
    FullNoContention,
    PartialAllowed,
}

and collect_active_jobs_from_all_queries could just return the (full or partial) QueryJobMap instead of a Result.

I think the bottom two cases are confused. The print_query_stack function passes false.

Also IIRC, when true is passed, the function will always return Ok, so the true/unwrap-or-else combination is unused because it's inherently useless.

I think the bottom two cases are confused. The print_query_stack function passes false.

Fixed, thanks.

Also IIRC, when true is passed, the function will always return Ok, so the true/unwrap-or-else combination is unused because it's inherently useless.

Good point. All the more reason for the enum.

require_complete=true only is used in depth_limit_error, which ensures us can get the depth and root of queries from a complete job map.

Remove the require_complete parameter. We now always use try_lock_shards and let the caller abort on incompleteness

I don't think it's acceptable that the compiler panics during reporting the depth limit.

enum CollectKind {
    Full,
    FullNoContention,
    PartialAllowed,
}

that's better than a bool + Result 👍

I have updated the code to use the three-value enum. I'm not 100% sure about the names, happy to hear alternative suggestions there.

I also wonder why a test wasn't added in #148777. My understanding of parallel front-end testing is poor, this might force me to look into it.

Tests for A-parallel-compiler bugs are nearly impossible to make consistent. They shouldn't be relied upon unless you can relatively consistently reproduce a test failure on your branch's base commit and then can no longer reproduce the test failure on the branch itself. I even have fixed bugs without being able to reproduce A-parallel-compiler issue to begin with, just by diagnosing the source problem.