Stream: t-compiler/rust-analyzer

Topic: #4873

Zac Pullar-Strecker (Jul 30 2020 at 23:14, on Zulip):

I was wondering if anyone had time to give a light review of #4873. It's a little bit behind master now, but I'd like to get some feedback before I rebase it if possible.

matklad (Jul 30 2020 at 23:23, on Zulip):

I’ll try to give a look tomorrow. * deps and map lit create create a „this is a wp“ impression for me, so I didn’t react immediately and then it just sitting in the queue. I should have reacted earlier...

Zac Pullar-Strecker (Jul 30 2020 at 23:34, on Zulip):

Thanks, it's definitely a WIP, but I'd love to get some feedback :)

Zac Pullar-Strecker (Aug 12 2020 at 01:52, on Zulip):

About code organisation you suggested creating a resolve_doc_path method on types in code model. This would work but the code works with Definition which is defined in ra_ide_db. In order to avoid implementing the same method once for every variant of Definition I'd need to expose something like the Resolver in methods on code model types. However, you also said that this shouldn't be exposed.

Could you give me a bit more advice on how I should organise this? Thanks.

Paul Faria (Aug 12 2020 at 02:29, on Zulip):

Just a heads up, but I think matklad is on vacation this week. I'm not sure about next.

matklad (Aug 12 2020 at 16:14, on Zulip):

I think this should be a method on

matklad (Aug 12 2020 at 16:15, on Zulip):

Implementing this for every variant of Definition seems appropriate -- rust-analyzer is pretty boilerplate-heavy, and so far it seems like the strategy of writing a bit of boring code to get clearer boundaries between layers pays out

Zac Pullar-Strecker (Aug 16 2020 at 05:42, on Zulip):

The saga of me not knowing where to put code continues...

I've moved quite a bit of the work into ra_hir which is I think a reasonable place to put most of it. One function I'm struggling to place is get_doc_url. It uses ra_tt to parse the html_doc_url attribute. I was thinking of moving it to a free function somewhere in ra_hir_def, does that sound right?

matklad (Aug 18 2020 at 12:01, on Zulip):

I think it should be hir::Crate::html_root_urlfor starters.

matklad (Aug 18 2020 at 12:02, on Zulip):

In the future, I think we'll need some kind of way to flag "inert" attributes, so that these kind of features are implemenable outside of the base hir, but for now I think it's better to be simple and handle each attribute on the case-by-case basis.

matklad (Aug 18 2020 at 12:06, on Zulip):

Or, to be more precise, this function should retrieve the value of the attribute. Interpreting as an URL and selecting fallback should be done in the IDE layer I think

Zac Pullar-Strecker (Aug 23 2020 at 07:41, on Zulip):

The problem is being a method on Crate means it needs to be in ra_hir, which doesn't import ra_tt. So I think the actual implementation needs to be lower down, but again I have no idea where to put it. I contemplated ra_hir_def::attr::AttrQuery but all of that code seems a lot lower level, and it seems wrong considering it'll only ever be defined on a root module.

I'm working on an old version of the codebase (6b7cb8b5a), so things may have moved around a bit since.

matklad (Aug 23 2020 at 13:02, on Zulip):

ra_hir can import ra_tt, it can't re-export it

Zac Pullar-Strecker (Aug 24 2020 at 10:15, on Zulip):

Got it. I think I've resolved most of the issues on the PR now so should be ok for a second review when you have time :smile: .

Last update: Jul 24 2021 at 19:45UTC