Stream: t-compiler/rust-analyzer

Topic: BlockDefMap


Jonas Schievink [he/him] (Jan 28 2021 at 18:21, on Zulip):

After ItemTree, I am happy to announce another hugely wasteful salsa query:

   223mb ItemTreeQuery
   167mb BlockDefMapQuery
matklad (Jan 28 2021 at 18:37, on Zulip):

What are their relative counts? RA_COUNT=1

Jonas Schievink [he/him] (Jan 28 2021 at 18:38, on Zulip):
hir_def::item_tree::ItemTree            46_713       46_713       46_713
hir_def::nameres::DefMap                22_539       22_539       22_539
Jonas Schievink [he/him] (Jan 28 2021 at 18:40, on Zulip):

and this only does the groundwork for inner items, nothing user-facing yet

Jonas Schievink [he/him] (Jan 28 2021 at 18:41, on Zulip):

(this uses block_def_map during body lowering)

matklad (Jan 28 2021 at 18:44, on Zulip):

ah, we don't get different counts for block vs non-block defmaps

But, there are about 600 def maps

matklad (Jan 28 2021 at 18:44, on Zulip):

Why do we get so many block maps?

Jonas Schievink [he/him] (Jan 28 2021 at 18:45, on Zulip):

because we compute one for every single block during body lowering now :D

matklad (Jan 28 2021 at 18:45, on Zulip):

Like there are two oodles of magnitude...

matklad (Jan 28 2021 at 18:45, on Zulip):

aaah

matklad (Jan 28 2021 at 18:45, on Zulip):

yeah, that explains things )

matklad (Jan 28 2021 at 18:46, on Zulip):

we really should create this only for interesting blocks

Jonas Schievink [he/him] (Jan 28 2021 at 18:47, on Zulip):

yeah

Jonas Schievink [he/him] (Jan 28 2021 at 18:47, on Zulip):

last time I tried that it broke macro resolution in weird ways

matklad (Jan 28 2021 at 18:47, on Zulip):

I am somewhat surprised than this ends up smaller than item tree

Jonas Schievink [he/him] (Feb 01 2021 at 12:38, on Zulip):

okay, after only computing the DefMap for blocks that actually contain inner items, this brings the size down to 39 MB, which still seems way too high, but much more reasonable

Jonas Schievink [he/him] (Feb 01 2021 at 12:38, on Zulip):

I think we treat every macro in expression position as a potential item, which is wrong

Jonas Schievink [he/him] (Feb 01 2021 at 12:39, on Zulip):

but we do have to treat every macro in statement position as a potential item, so that might explain the remaining memory

Jonas Schievink [he/him] (Feb 01 2021 at 12:43, on Zulip):

I think we could check if the DefMap after nameres is empty and dispose of it

bjorn3 (Feb 01 2021 at 12:44, on Zulip):

A macro in expression position can expand to a block containing an item.

Jonas Schievink [he/him] (Feb 01 2021 at 12:45, on Zulip):

yeah, that's fine

Jonas Schievink [he/him] (Feb 01 2021 at 12:45, on Zulip):

it's contained in a different DefMap

matklad (Feb 01 2021 at 12:52, on Zulip):

but we do have to treat every macro in statement position as a potential item, so that might explain the remaining memory

uh, this is less than ideal.... Makes sense to at least access how costly would it be to make this more lazy

Jonas Schievink [he/him] (Feb 01 2021 at 12:52, on Zulip):

Jonas Schievink said:

I think we could check if the DefMap after nameres is empty and dispose of it

this doesn't seem to change memory usage

Jonas Schievink [he/him] (Feb 01 2021 at 12:53, on Zulip):

this is already as lazy as body lowering fwiw

matklad (Feb 01 2021 at 12:54, on Zulip):

Ah, right, that's for analysis stats, which looks into every body

Jonas Schievink [he/him] (Feb 01 2021 at 12:54, on Zulip):

yeah

matklad (Feb 01 2021 at 12:54, on Zulip):

so, its +39 in the inference phase, not the item map phaes. Then its ok I think

Jonas Schievink [he/him] (Feb 01 2021 at 12:55, on Zulip):

mhmm, BodyQuery itself already uses 73 MB there, and inference data is another 49 MB

Jonas Schievink [he/him] (Feb 01 2021 at 15:54, on Zulip):

welp, I broke macros :)

Jonas Schievink [he/him] (Feb 01 2021 at 16:08, on Zulip):

hmm, I don't understand what's going on here. This code fails to resolve the macro:

macro_rules! m {
    () => {};
}

fn f() {
    {
        m!(a);
    }
}

...but when I make this a body lowering test, the only diagnostic it emits is "leftover tokens", which is correct

Jonas Schievink [he/him] (Feb 01 2021 at 16:09, on Zulip):

maybe it's a nameres problem

Edwin Cheng (Feb 01 2021 at 16:22, on Zulip):

Maybe https://github.com/rust-analyzer/rust-analyzer/blob/2d9bb69990b866bad0b4300972f1706d38329ad3/crates/hir_expand/src/db.rs#L380 ?

Jonas Schievink [he/him] (Feb 01 2021 at 16:22, on Zulip):

hmm, what do you mean?

Edwin Cheng (Feb 01 2021 at 16:23, on Zulip):

I am not sure, but maybe it is used a wrong fragment kind to resolve the macro ?

Jonas Schievink [he/him] (Feb 01 2021 at 16:24, on Zulip):

could that affect resolution of the macro?

Jonas Schievink [he/him] (Feb 01 2021 at 16:24, on Zulip):

this started happening because of the changes for inner items

Edwin Cheng (Feb 01 2021 at 16:25, on Zulip):

this started happening because of the changes for inner items

Oh... nvm :)

Jonas Schievink [he/him] (Feb 01 2021 at 16:58, on Zulip):

we seem to resolve an invocation like name![x] as self::name

Jonas Schievink [he/him] (Feb 01 2021 at 16:59, on Zulip):

this confused note I wrote down last week might be relevant:

PathKind::Super(0) resolves to the containing module, but for block modules, the containing module is also relevant

Jonas Schievink [he/him] (Feb 01 2021 at 16:59, on Zulip):

I think what I meant was that Super(0) resolves to the current module, but the containing one is relevant

Jonas Schievink [he/him] (Feb 01 2021 at 16:59, on Zulip):

this does not explain why I can't manage to repro in a test though...

Jonas Schievink [he/him] (Feb 02 2021 at 11:26, on Zulip):

This test takes really long in analysis-stats, presumably because of the macros: https://github.com/rust-lang/rust/blob/b814b639836aa76b5c6deaa585367150bb2debf4/library/std/src/net/ip/tests.rs#L464

Laurențiu (Feb 02 2021 at 11:29, on Zulip):

I'm not sure why. I extracted that function to a new project and it's fast (more or less) there

Jonas Schievink [he/him] (Feb 02 2021 at 11:30, on Zulip):

Ah, it's not because of block_def_map, it was always slow

Laurențiu (Feb 02 2021 at 11:31, on Zulip):

I thought it regressed because I remember testing when it was added to CI, but even on that commit it still seems slow and also crashes in chalk

Laurențiu (Feb 02 2021 at 11:32, on Zulip):

https://bpa.st/FJ7LK if you want to give that test a try

Laurențiu (Feb 02 2021 at 11:32, on Zulip):

And I think type inference is slow, not parsing

Jonas Schievink [he/him] (Feb 02 2021 at 11:33, on Zulip):

yeah, that could also be it

Jonas Schievink [he/him] (Feb 02 2021 at 18:13, on Zulip):

Jonas Schievink said:

hmm, I don't understand what's going on here. This code fails to resolve the macro:

macro_rules! m {
    () => {};
}

fn f() {
    {
        m!(a);
    }
}

...but when I make this a body lowering test, the only diagnostic it emits is "leftover tokens", which is correct

Apparently this does not happen inside lib.rs, which would explain this behavior. Wow.

Jonas Schievink [he/him] (Feb 02 2021 at 18:15, on Zulip):

This reproduces it inside a test:

//- /lib.rs
mod module;

//- /module.rs
macro_rules! m {
    () => {
        struct Def {}
    };
}

fn f() {
    {
        m!();
        $0
    }
}
Jonas Schievink [he/him] (Feb 02 2021 at 18:16, on Zulip):

oh I think I know why

Jonas Schievink [he/him] (Feb 09 2021 at 16:36, on Zulip):

It is done.

Peek-2021-02-09-17-30.gif

matklad (Feb 09 2021 at 17:03, on Zulip):

Exciting!

matklad (Feb 09 2021 at 17:03, on Zulip):

(don't forget to post this gif to the gifs issue, to have it ready for fhe next changelog)

Jonas Schievink [he/him] (Feb 09 2021 at 17:04, on Zulip):

oh, does it matter where I post it?

Laurențiu (Feb 09 2021 at 17:04, on Zulip):

I posted a couple of them to the PRs, to keep them together. Probably not. The GIFs issue seems especially useful when there's no PR to post in.

matklad (Feb 09 2021 at 17:13, on Zulip):

As long as it is somewhere where it's easy to find!

matklad (Feb 09 2021 at 17:14, on Zulip):

Also .... are local import the last big language feature we were missing? Are we Rust-complete now (modulo bugs)?

matklad (Feb 09 2021 at 17:14, on Zulip):

Ah, I guess we have attr-like proc macros left, and const-generics (

Jonas Schievink [he/him] (Feb 09 2021 at 17:16, on Zulip):

macros in type position are still unsupported too

Jonas Schievink [he/him] (Feb 09 2021 at 17:17, on Zulip):

and maybe in pattern position? not sure

Lukas Wirth (Feb 09 2021 at 17:19, on Zulip):

Yes, pattern position macros aren't handled yet either

Florian Diebold (Feb 09 2021 at 18:43, on Zulip):

hmm does this already work with name resolution in inference? if so, I'm a bit confused how

Florian Diebold (Feb 09 2021 at 18:43, on Zulip):

or maybe it only works for the top-level block in a function

Florian Diebold (Feb 09 2021 at 18:43, on Zulip):

probably that?

Florian Diebold (Feb 09 2021 at 18:44, on Zulip):

(since inference currently just uses one resolver everywhere)

Laurențiu (Feb 09 2021 at 18:45, on Zulip):

Not always: https://github.com/rust-analyzer/rust-analyzer/pull/7614#issuecomment-776105995

Jonas Schievink [he/him] (Feb 09 2021 at 18:53, on Zulip):

This call is also not yet resolved correctly, and there's a few other known bugs

Jonas Schievink [he/him] (Feb 09 2021 at 18:54, on Zulip):

maybe that's what "name resolution in inference" would fix here

Jonas Schievink [he/him] (Feb 09 2021 at 18:56, on Zulip):

This seems to use the right resolver though

Florian Diebold (Feb 09 2021 at 18:58, on Zulip):

oh, right.
probably something like

fn foo() {
    struct S { field: u32 }
    let s: S;
    let f: s.field;
}

will not work correctly (infer the type of f) yet though, I expect?

Florian Diebold (Feb 09 2021 at 19:00, on Zulip):

also getting traits in scope like Laurențiu's case. we'll have to review all usages of the resolver in InferenceContext, basically

Jonas Schievink [he/him] (Feb 09 2021 at 19:02, on Zulip):

yeah, that doesn't infer the type yet

Jonas Schievink [he/him] (Feb 10 2021 at 13:42, on Zulip):

Florian Diebold said:

oh, right.
probably something like

fn foo() {
    struct S { field: u32 }
    let s: S;
    let f: s.field;
}

will not work correctly (infer the type of f) yet though, I expect?

turns out this one's trivial to fix :)

Jonas Schievink [he/him] (Feb 10 2021 at 13:50, on Zulip):

Opened https://github.com/rust-analyzer/rust-analyzer/pull/7627

Jonas Schievink [he/him] (Feb 10 2021 at 13:52, on Zulip):

Wow, even the PR from yesterday apparently halved the number of unknown types in ripgrep

Jonas Schievink [he/him] (Feb 10 2021 at 13:52, on Zulip):

(and doubled the type mismatches, but sssshhhhh)

detrumi (Feb 10 2021 at 13:53, on Zulip):

progress!

Last update: Jul 29 2021 at 09:45UTC