Stream: t-compiler/wg-rls-2.0

Topic: Local decls


ice1000 (Dec 03 2019 at 16:37, on Zulip):

Hi @matklad , do you have any suggestions on the next step to take? I think I've did the def collection, but I'm not sure if the approach works. What sort of test should I add?

ice1000 (Dec 03 2019 at 16:45, on Zulip):

There are like, a lot of questions

matklad (Dec 03 2019 at 16:45, on Zulip):

The first step would be to send a PR which justs makes a Block an ItemOwner

matklad (Dec 03 2019 at 16:45, on Zulip):

And you are very right that there are many questions :)

matklad (Dec 03 2019 at 16:46, on Zulip):

I think we probably should start with something relatively simple: not trying to properly link ExprScopes and CrateDefMap, but just treating functions as a separate scope for items

ice1000 (Dec 03 2019 at 16:47, on Zulip):

The Module's data contains AstId<Module>, while I'm using the blocks' Ids in ModuleData::Block

ice1000 (Dec 03 2019 at 16:48, on Zulip):

I'm having stacktraces when running ra on my computer

ice1000 (Dec 03 2019 at 16:48, on Zulip):

Where should I send them

ice1000 (Dec 03 2019 at 16:48, on Zulip):

Cus ra is paralyzed now, I can't even navigate

matklad (Dec 03 2019 at 16:48, on Zulip):

I think this can work like this:

ice1000 (Dec 03 2019 at 16:49, on Zulip):

Is it even possible to restart ra?

matklad (Dec 03 2019 at 16:49, on Zulip):

@ice1000 there's "reload window" action in VS Code

matklad (Dec 03 2019 at 16:50, on Zulip):

And also Rust Analyzer: restart server

Florian Diebold (Dec 03 2019 at 16:50, on Zulip):

I'm having stacktraces when running ra on my computer

https://github.com/rust-analyzer/rust-analyzer/issues/2467 ? maybe check that you're on newest master

ice1000 (Dec 03 2019 at 16:50, on Zulip):

I am

ice1000 (Dec 03 2019 at 16:51, on Zulip):

I've cargo xtask install --server 10 minutes ago after my PR gets merged

matklad (Dec 03 2019 at 16:51, on Zulip):

Another option is to properly design this merging of crate def map and bodies, and do the right thing from the start... But I personally don't have a design blueprint in my hand for this

ice1000 (Dec 03 2019 at 16:51, on Zulip):

Oh my god it's my own change that causes the panicking

ice1000 (Dec 03 2019 at 16:51, on Zulip):

:sweat_smile:

ice1000 (Dec 03 2019 at 16:52, on Zulip):

It says thread '<unnamed>' panicked at 'Can't find BLOCK@[1031; 1053) in AstIdMap, which probably because I was adding things like blocks' Ids to the database but it's somehow not collected

Florian Diebold (Dec 03 2019 at 16:53, on Zulip):

Another option is to properly design this merging of crate def map and bodies, and do the right thing from the start... But I personally don't have a design blueprint in my hand for this

yeah, I feel like we need to start experimenting before we'll come up with a proper design :)

ice1000 (Dec 03 2019 at 16:54, on Zulip):

I think that's what I'm doing

ice1000 (Dec 03 2019 at 16:54, on Zulip):

I mean, "merging of crate def map and bodies"

matklad (Dec 03 2019 at 16:54, on Zulip):

Starting experimentation is not a problem. Finishing it properly is the hard bit :)

ice1000 (Dec 03 2019 at 16:55, on Zulip):

Do I PR for unfinished features?

ice1000 (Dec 03 2019 at 16:55, on Zulip):

Like, is it preferred to send small PRs about the nonbreaking things added before the big thing launches

matklad (Dec 03 2019 at 16:57, on Zulip):

@ice1000 it depends. If it's a self-contained thing which will be needed regardless of the final direction the feature takes, than it's best to send it in a separate PR, however small it is. For example, adding ModuleItemsOwner trait to Block is an example of such change.

On the other hand, for something bigger, which represent a part of possible, but not unique path to the solution, it's better to send something which has user-visible effects and can be tested

ice1000 (Dec 03 2019 at 16:58, on Zulip):

Ok that sounds cool, I'll PR right after rust analyzer compiles

matklad (Dec 03 2019 at 16:59, on Zulip):

The metric here is that we want to minimize the size of PR (there's no such thing as too small a PR), but we want to make sure that the merged code actually works: it's to easy to start building something. only to realise later that it can't possible work

ice1000 (Dec 03 2019 at 17:04, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/2471

matklad (Dec 03 2019 at 17:08, on Zulip):

looks liek the correct trait name is ModuleItemOwner

ice1000 (Dec 03 2019 at 17:08, on Zulip):

Just fixed

ice1000 (Dec 03 2019 at 17:08, on Zulip):

Cool, rust analyzer starts working again

ice1000 (Dec 03 2019 at 17:09, on Zulip):

I have seen some tabs mixed along with the spaces, and I changed them all to spaces

ice1000 (Dec 03 2019 at 17:12, on Zulip):

pasted image This resolution is really weird

ice1000 (Dec 03 2019 at 17:13, on Zulip):

Is it a known bug?

matklad (Dec 03 2019 at 17:13, on Zulip):

I think that's and index-based fallback: if we can't resolve the name properly, we just pick any declaration with a matching naming, in hope that it would be corrct

matklad (Dec 03 2019 at 17:14, on Zulip):

so, it's a "feature"

ice1000 (Dec 03 2019 at 17:16, on Zulip):

It's a bug because the really correct result is not resolved

ice1000 (Dec 03 2019 at 17:16, on Zulip):

The wrong resolution result is a feature

Edwin Cheng (Dec 03 2019 at 18:02, on Zulip):

Seem like we still not implement std::ops::Index handling yet.

ice1000 (Dec 03 2019 at 18:08, on Zulip):

I'm not sure about some compilation failures in https://github.com/ice1000/rust-analyzer/tree/114514, can anyone take a look? I'm trying to change ModuleData.declaration & definition pair into a three-state enum, ModuleOrigin (with one more state, as my ongoing PR will support block level modules), but that requires me to implement Default for it, while I don't know how.

ice1000 (Dec 03 2019 at 18:11, on Zulip):

Something like trait `std::convert::From<&ra_db::input::FileId>` is not implemented for `ra_hir_expand::HirFileId

ice1000 (Dec 03 2019 at 18:11, on Zulip):

self.def_map.modules[module_id].definition = Some(file_id)

Edwin Cheng (Dec 03 2019 at 18:15, on Zulip):

@ice1000 Add an enum variant in ModuleOrigin to indicate it is empty and make it default ?

Edwin Cheng (Dec 03 2019 at 18:23, on Zulip):

Or change to :

pub origin: Option<ModuleOrigin>,
ice1000 (Dec 03 2019 at 18:54, on Zulip):

I wonder if the Default trait is really used

ice1000 (Dec 03 2019 at 18:56, on Zulip):

It's used only twice

ice1000 (Dec 03 2019 at 19:03, on Zulip):

I got it. It's the root module that is initialized after (or during? Not sure) collecting def

ice1000 (Dec 03 2019 at 20:00, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/2473

ice1000 (Dec 03 2019 at 20:50, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/2474

ice1000 (Dec 03 2019 at 21:13, on Zulip):

If you admins prefer taking 2474 directly, then feel free to close 2473

ice1000 (Dec 03 2019 at 21:19, on Zulip):

I've closed 2473 myself

ice1000 (Dec 04 2019 at 02:23, on Zulip):

@matklad could you please take a look at this code https://github.com/ice1000/rust-analyzer/commit/541eb7e12e948058e8efa563f303828b3bebb713#diff-f26837190e672b2e01be4ca7511e50f8L535? I essentially added block_id to FunctionLoc, and try to find the (block-level anonymous) module corresponds to the block_id. This is making tests to fail, and I have no idea what's going on.

ice1000 (Dec 04 2019 at 02:30, on Zulip):

I'm like dying, debugging & refactoring rust analyzer is just so hard

ice1000 (Dec 04 2019 at 05:54, on Zulip):

I have made a "reviewable webpage" aka an in-fork pull request (https://github.com/ice1000/rust-analyzer/pull/1/files) to show the change after the rust-analyzer pull request.

ice1000 (Dec 05 2019 at 13:39, on Zulip):

Ok, finally got 2474 done

ice1000 (Dec 18 2019 at 02:27, on Zulip):

Anyone wanna take this on

Last update: Jan 21 2020 at 09:35UTC