Give a better error message when a query isn't supported for a local / external crate

[jyn514]

@richkadel made a great start on this in #83367, but it would be nice to explain the local/external distinction in the panic message; "unsupported by crate" is unhelpful. This is an ICE targeted at compiler developers so IMO it can be pretty much as long as we want.

Currently the error looks like this:

error: internal compiler error: compiler\rustc_middle\src\ty\query.rs:335:1: tcx.module_children(DefId(0:4 ~ similar_paths_reexported[71d7]::module::S)) unsupported by its crate; perhaps the module_children query was never assigned a provider function

I wrote up this description while helping someone debug on discord, which may be a good start:

There are two types of queries. Queries local to the crate, and queries for external crates. This error means that you try to use it for the one that's not supported - usually that's the local crate
But you can tell by looking at CrateId of the DefId

If possible, we should say "tcx.module_children unsupported by local crate" (or external as appropriate, selected at runtime), but if not, we should at least explain what's going wrong better. The link below has a lot of details about the code structure that may be helpful.

Originally posted by @richkadel in #83122 (comment)

[hanar3]

Claiming this. If @jyn514 someone could bring some color on whether key is always a valid rustc_query_impl::keys::Key in this context as per the discussion on #83122 (comment) would be very helpful as this is the first issue I'm taking on.

If key is guaranteed to be a valid key, we could call key.query_crate_is_local as per the docs to figure if it's a local or external crate?

[hanar3]

@rustbot claim

[jyn514]

@hanar3 yes, all keys must implement Key or they can't be used as the argument to a query in the first place. query_key_is_local is already used in that macro :) https://github.com/rust-lang/rust/blob/8d113ae658eec2feb3a5be0a782eabbfacfb0f2b/compiler/rustc_query_impl/src/plumbing.rs#L250

[hanar3]

Awesome, thanks for the reply! This is what I have in place so far.

        impl Default for Providers {
            fn default() -> Self {
                Providers {
                    $($name: |_, key| bug!(
                        "`tcx.{}({:?})` unsupported for {} crate; \
                         perhaps the `{}` query was never assigned a provider function. Queries can be either made to the local crate, or the external crate. This error means you tried to use it for one that's not supported.", 
                        stringify!($name),
                        key,
                        if key.query_crate_is_local() { "local" } else { "external" } ,
                        stringify!($name),
                    ),)*
                }
            }
        }

The compiler, however, is not happy with me calling key.query_crate_is_local()

175  | /  macro_rules! define_callbacks {
176  | |      (
177  | |       $($(#[$attr:meta])*
178  | |          [$($modifiers:tt)*] fn $name:ident($($K:tt)*) -> $V:ty,)*) => {
...    |
282  | |                          if key.query_crate_is_local() { "local" } else { "external" } ,
     | |                                 ^^^^^^^^^^^^^^^^^^^^ method not found in `(ty::Ty<'_>, Option<Binder<'_, ExistentialTraitRef<'_>>>)`
...    |
321  | |      };
322  | |  }
     | |__- in this expansion of `define_callbacks!` (#2)

This is the macro definition, I noticed it usestt for every argument of fn.

rust/compiler/rustc_middle/src/ty/query.rs

Line 178 in c97922d

[$($modifiers:tt)*] fn $name:ident($($K:tt)*) -> $V:ty,)*) => {

Is it right to assume that the method not found in (ty::Ty<'_>, Option<Binder<'_, ExistentialTraitRef<'_>>>) is due to the macro using tt to define the function arguments rather than :expr as per your example? If so, could we use use/define a macro similar to the one you referenced on your comment above? Something like:

[$($modifiers:tt)*] fn $name:ident($($K:expr)*) -> $V:ty,)*)

EDIT: If key is always a valid key at runtime, this wouldn't cause harm I believe?

Thanks in advance!

[jyn514]

@hanar3 hmm, I think I was wrong before - the problem is that the key only needs to implement the Key trait if the macro has the separate_provide_extern modifier. Maybe you can add a new get_default_provider macro; that will give a better error anyway since it can distinguish "you added a local provider but not an extern provider" from "you never added a provider at all".

tt vs expr isn't relevant, they expand to the same thing at expansion time.

[hanar3]

Hi @jyn514, if I understood correctly, the key is not guaranteed to implement the Key trait on the code snippet I provided, so we can't reliably call query_crate_is_local() due to that, which is fine.

To your point on the new get_default_provider macro, I tried a few takes, since I'm not very knowledgeable on the macros, and haven't found many docs around them, I tried a few takes basing myself off of the get_provider macro you referenced above, and seperate_provide_extern_default to come up with an implementation for the new get_default_provider macro. Since both macros I used as example have the separate_provide_extern, I thought this would help me figure out the new macro.

I got to a point where I tried a few solutions (I can share them here, if you want to reference), but still end up with very similar compiler issues for all of them.

key.query_crate_is_local(),
     | |                   ^^^^^^^^^^^^^^^^^^^^ method not found in `InstanceDef<'_>`

and

186  | |               key.query_crate_is_local(),
     | |                   ^^^^^^^^^^^^^^^^^^^^ method not found in `(rustc_span::def_id::CrateNum, SimplifiedTypeGen<rustc_span::def_id::DefId>)`

This one made me think I might be going to the right direction...I see a DefId here, and we know we can tell the crate by looking at the DefId...

This was my last take on get_default_provider. Again, I'm not very knowledgeable on this, so my solutions were heavily based off of macros that were already present and doing similar things, so I'm afraid this might not be the way to solve it.

macro_rules! get_default_provider {
    ([][$name:ident]) => {
        ()
    };

    ([(separate_provide_extern) $($rest:tt)*][$name:ident]) => {
        |_, key| bug!(
            "`tcx.{}({:?})` unsupported by {} crate;", // Removed a chunk of the message here just to make this more readable.
            stringify!($name),
            if key.query_crate_is_local() { "local" } else { "extern" },
            stringify!($name),
        )
    };
    ([$other:tt $($modifiers:tt)*][$($args:tt)*]) => {
        get_default_provider!([$($modifiers)*][$($args)*])
    };
}

If you could point me to the right direction here as it relates to this new macro, that would be super helpful!!

Again, thanks for the great support on this one. Really having fun while trying to push my first contribution! :)

[hanar3]

Hi @jyn514,
After some digging, I couldn't find a way to reliably add local / external to the message based on the key - so I ended up just improving the message without it specifying which crate the error is referring to.

[SarthakSingh31]

Hello @jyn514, I looked into this a little bit and to me it seems that all the keys do implement rustc_query_impl::key::Key. The main problem with calling Key::query_crate_is_local from rustc_middle::ty::query::Providers::default is that rustc_middle cannot depend on rustc_query_impl and therefore cannot use the Key trait.

I am new to contributing to rust. I don't know how feasible this is but maybe we can move compiler/rustc_query_impl/src/keys.rs to rustc_middle? Every dependency used in keys.rs is also available in rustc_middle.

[oMisaki]

@rustbot claim

[oMisaki]

@rustbot release-assignment