rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
99.14k stars 12.8k forks source link

Investigate replacing query Providers structs with trait objects. #66352

Closed eddyb closed 5 years ago

eddyb commented 5 years ago

Part of the motivation here is that for non-local crates, we right now have to go through dyn Any to actually get access to per-crate data, whereas if Providers were replaced with a trait object, the implementer would have direct access to any private data they may happen to need.


However, Providers is really handy in that we can construct it at runtime, piece by piece, but there is nothing equivalent to that when it comes to trait methods, short of this hack:

Instead of:

let mut providers = rustc::query::Providers::default();
foo::provide(&mut providers);
bar::provide(&mut providers);
baz::provide(&mut providers);
providers

we'd likely need:

// You wouldn't actually be able to have a match like this,
// instead the rest of the code would be in a generic function.
let providers = match krate_source {
    Local(hir) => rustc::hir::Provide { hir },
    NonLocal(data) => rustc_metadata::rmeta::Provide { data },
};
let providers = foo::Provide(providers);
let providers = bar::Provide(providers);
let providers = baz::Provide(providers);
Box::new(providers) as Box<dyn rustc::query::Provider>

Each of {foo,bar,baz}::Provide<T> would have to be a newtype (of T) with an impl that delegates all but a few methods, to T. (Also, we could use free functions fn provide(impl Provider) -> impl Provider, and have the entirety of the newtype as an implementation detail hidden inside the body, perhaps by using a macro?)

We can probably automate some of this this by having a marker trait DelegateTo<T>: Deref<Target = T> {} which enables a default impl of Provider delegating all the methods to T, and then the regular impl of Provider for each individual Provide<T> newtype would contain only the methods that are relevant to it.

This still feels horribly inefficient but at least we don't have to generate the delegation using macros so maybe it's not that bad?

cc @nikomatsakis @michaelwoerister @Zoxc

Zoxc commented 5 years ago

What do you mean by private data / per-crate data? I'm not sure what this would be used for.

eddyb commented 5 years ago

For the local crate, it's pretty much all of the stuff that's right now injected into TyCtxt at creation time (arguably a lot of this will end up behind queries eventually).

For other crates, it'd replace this use of dyn Any to get access to a rustc_metadata type (cstore::CrateMetadata) from a TyCtxt: https://github.com/rust-lang/rust/blob/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L53-L55

Zoxc commented 5 years ago

For the local crate, it's pretty much all of the stuff that's right now injected into TyCtxt at creation time (arguably a lot of this will end up behind queries eventually).

My PRs which moves stuff into queries probably do get rid of these.

For other crates, it'd replace this use of dyn Any to get access to a rustc_metadata type (cstore::CrateMetadata) from a TyCtxt:

rustc_metadata should get replaced by some mechanism which caches / exports query result from other crates, so I wouldn't do anything because of that.

eddyb commented 5 years ago

rustc_metadata should get replaced by some mechanism which caches / exports query result from other crates, so I wouldn't do anything because of that.

If we move Table from rustc_metadata::rmeta to somewhere in rustc, I think we can start doing this today for some queries (I guess we'll need some of Lazy, too).

Overall I think I agree with you and this is just another thing we should refactor into irrelevance.