rustc_queries simplifications

[nnethercote]

Queries have two ways of specifying code snippets, in desc and cache_on_disk_if blocks. An example:

query check_liveness(key: LocalDefId) -> &'tcx rustc_index::bit_set::DenseBitSet<abi::FieldIdx> {
    arena_cache
    desc { |tcx| "checking liveness of variables in `{}`", tcx.def_path_str(key.to_def_id()) }
    cache_on_disk_if(tcx) { tcx.is_typeck_child(key.to_def_id()) }
}

If you need to use tcx in the snippet, you can use an explicit binding. But there are multiple problems with this.

• The syntax used is different in the two snippets: |tcx| within the block vs. (tcx) outside the block. (!!)
• Bug 1: In desc snippets you can leave out the |tcx| and still use tcx. Several existing queries do this.
• Bug 2: In desc snippets you can always use key in the snippet to refer to the key, even if that's not the identifier used in the query head.
• Finally, you can bind tcx and not use it, without a warning. Several existing queries do this.

I think explicit tcx binding is silly. Many queries need tcx and this macro is already super-magical, so just making tcx implicitly available seems fine, rather than making the query writer jump through a little syntactic hoop. This makes both the query definitions and the proc macro simpler.

r? @petrochenkov

[nnethercote]

If you want to understand the two bugs in the desc implementation, consider these following examples and the code generated for them.

// no key, no tcx: ok
query mir_keys(_: ()) -> &'tcx rustc_data_structures::fx::FxIndexSet<LocalDefId> {
    arena_cache
    desc { "getting a list of all mir_keys" }
}
#[allow(unused_variables)]
pub fn mir_keys<'tcx>(tcx: TyCtxt<'tcx>, key: ()) -> String {
    let (_, _) = (tcx, key);
    format! ("getting a list of all mir_keys")
}

// key, no tcx: ok
query symbol_name(key: ty::Instance<'tcx>) -> ty::SymbolName<'tcx> {
    desc { "computing the symbol for `{}`", key }
    cache_on_disk_if { true }
}
#[allow(unused_variables)]
pub fn symbol_name<'tcx>(tcx: TyCtxt<'tcx>, key: ty::Instance<'tcx>) -> String { 
    let (_, key) = (tcx, key);
    format!("computing the symbol for `{}`", key)
}

// key (with name other than `key`), tcx: good
query defaultness(def_id: DefId) -> hir::Defaultness {
    desc { |tcx| "looking up whether `{}` has `default`", tcx.def_path_str(def_id) }
    separate_provide_extern
    feedable
}
#[allow(unused_variables)]
pub fn defaultness<'tcx>(tcx: TyCtxt<'tcx>, key: DefId) -> String {
    let (tcx, def_id) = (tcx, key);
    format!("looking up whether `{}` has `default`", tcx.def_path_str(def_id))
}

// no key, no tcx: doubly bad, but works anyway        
query representability(_: LocalDefId) -> rustc_middle::ty::Representability {
    desc { "checking if `{}` is representable", tcx.def_path_str(key) }
    cycle_delay_bug                                    
    anon                                               
}                                                      
#[allow(unused_variables)]                             
pub fn representability<'tcx>(tcx: TyCtxt<'tcx>, key: LocalDefId) -> String {
    let (_, _) = (tcx, key);                           
    format!("checking if `{}` is representable", tcx.def_path_str(key))
}                                                      

The let line is part of the problem. It seems the macro is trying to ensure the function arguments are used, presumably to avoid warnings. But tcx and key end up always being available. And the allow(unused_variables) means that all this is pointless anyway.

A simple fix is to remove the let line, always use the given key identifier for the key (instead of hardwiring key), and make tcx implicitly available. That fixes the problem and makes the macro simpler, and it's what this PR does.

[nnethercote]

Best reviewed one commit at a time.

cc @Zalathar @oli-obk @cjgillot @Zoxc

[oli-obk]

r? @oli-obk

@bors r+

[rust-bors]

📌 Commit ecb778f has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

• In desc snippets you can leave out the |tcx| and still use tcx. Several existing queries do this.

Heh I noticed this before but didn't investigate