Use closures more consistently in dep_graph.rs.

[nnethercote]

This file has several methods that take a FnOnce() -> R closure:

• DepGraph::with_ignore
• DepGraph::with_query_deserialization
• DepGraph::with_anon_task
• DepGraphData::with_anon_task_inner

It also has two methods that take a faux closure via an A argument and a fn(TyCtxt<'tcx>, A) -> R argument:

• DepGraph::with_task
• DepGraphData::with_task

The rationale is that the faux closure exercises tight control over what state they have access to. This seems silly when (a) they are passed a TyCtxt, and (b) when similar nearby functions take real closures. And they are more awkward to use, e.g. requiring multiple arguments to be gathered into a tuple. This commit changes the faux closures to real closures.

The commit also changes the types and names of the closures from the awkward op: OP to the more standard f: F.

r? @Zalathar

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[nnethercote]

@bors try @rust-timer queue

[Zalathar]

Based on comments, I think there was historically a desire to make sure that dep-graph tasks didn't accidentally pull in other untracked state via closure capture.

But given the existing inconsistency, and the fact that passing tcx gives access to the kitchen sink anyway, there probably isn't any harm in just using normal closures at this point.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 283c322 (283c322d1c3e0fd24ba3187d32027674e634f7e5, parent: b711f95f86b6489b91fdc55c876ed5f95a8d4560)

[rust-timer]

Finished benchmarking commit (283c322): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.5%, -1.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 481.409s -> 481.18s (-0.05%)
Artifact size: 394.91 MiB -> 396.71 MiB (0.45%)

[Zalathar]

In terms of aesthetics/readability, I'm personally not fond of using tiny identifiers like f for this sort of callback, because they get drowned out by the rest of the code and make it harder to see where the actual work happens.

I would have gone with something like task_fn: impl FnOnce() -> R here.

[nnethercote]

I grepped for \<f: impl Fn in compiler/ and got 210 hits, so it's a well-established idiom.

[Zalathar]

I feel like I’m missing a step here, because I don’t understand how the reply relates to my comment.

[nnethercote]

You're saying you don't like short identifiers like f and I'm saying there are many identifiers like that already in the code base.

[nnethercote]

More generally, I prefer f to something like task_fn, and existing style shows that f is common.

[petrochenkov]

I also prefer the longer style personally, both for callbacks and generic parameter names, despite the single-letter names being more common.
(Unless the callback or generic parameter truly has no additional attached semantics, like the callback in Iterator::map or arbitrary return type R in the example above.)

[Zalathar]

I don’t understand what benefit we’d be getting in exchange for hiding the callback point.

[nnethercote]

I disagree that a name like f is hiding anything.

There are four with_* methods in this file: with_task, with_anon_task, with_anon_task_inner, with_query_deserialization, and with_ignore. What names do you suggest for the parameters? task_fn for all of them? Something else?

(Unless the callback or generic parameter truly has no additional attached semantics, like the callback in Iterator::map or arbitrary return type R in the example above.)

I think these examples do fall into this category. And I count at least 48 other with_* methods in compiler/ that use f as the closure parameter, across 16 crates.

[Zalathar]

f is tiny.

It's physically hard to see, and physically hard to click on or navigate to, even when the editor or view is actively highlighting it.

Calling or forwarding the task function is one of the most important points of control flow in these complex higher-order functions, and such an important point should not be made difficult to find.

[rust-bors]

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