Stream: t-compiler/wg-rls-2.0

Topic: dyn Database


matklad (Nov 12 2019 at 07:18, on Zulip):

I've tried to re-rail name resolution onto a dyn Databse: https://github.com/matklad/rust-analyzer/commit/e4dabdb013c21ef13aa6e0555f5eb1c654315a0f

It works, macro-benchmark results are the same, I haven't measured compile time impact.

However, large-scale usage seems problemantic, because of the dyn upcasting problem. I had to copy a bit of code from another crate due to the lack of upcasts. This seems like it won't scale to a huge codebase, so I am sticking with impl DB for the time being.

cc @nikomatsakis, seems like we need to do something in salsa to make such cases easier.

matklad (Nov 12 2019 at 07:20, on Zulip):

Specifically, the problem is if you have crate a with A salsa databse, which is used as dyn A, and you have crate b, which has B: A, it's inconvenient to use dyn B in b: you are able to call A methods directly, but you can't pass that to helper functions that expect dyn A

matklad (Nov 12 2019 at 07:31, on Zulip):

One practical way to work around this is to have a per-subsystem mediating crate:

trait NameresCtx {
    fn crate_graph(&self) -> Arc<CrateGraph>;
}

impl<T: DefDatabase> NameresCtx for T {
    fn crate_graph(&self) -> Arc<CrateGraph> {
        <T as DefDatabase /* or AstDatabase, which gives upcasting*/>::crate_graph(self)
    }
}

fn do_name_res(ctx: &dyn NameresCtx) { ... }

That is, one can write a bunch of boilerplate to explicitly dynamize every helper function. I don't want to go into this direction, as it adds boilerplate and indirection (which makes it harder to read the code). Although the benefit of this approach is that you do get a pure trait

pachi (Nov 12 2019 at 08:52, on Zulip):

@matklad , I think @Alexander Regueiro and @nikomatsakis got upcasts working now, though I don't know how far away from being available they are.

matklad (Nov 12 2019 at 08:54, on Zulip):

By design, rust-anlayzer requires stable, so I would prefer to find a solution which is available in 1.39 :)

Alexander Regueiro (Nov 12 2019 at 15:38, on Zulip):

@pachi Yep. The idea is to merge that PR this week.

Last update: Dec 12 2019 at 01:10UTC