Stream: t-compiler/rust-analyzer

Topic: Option


Jonas Schievink [he/him] (Dec 17 2020 at 12:49, on Zulip):

I noticed that almost all types in code_model.rs have a pub fn krate(self, db: &dyn HirDatabase) -> Option<Crate> that always returns Some. Is there a reason for that or can I change it to return just Crate?

matklad (Dec 17 2020 at 12:52, on Zulip):

I think primitives are not associated with any crate

matklad (Dec 17 2020 at 12:52, on Zulip):

but yeah, they probably should refer to a specific crate

matklad (Dec 17 2020 at 12:53, on Zulip):

OTOH, I am not sure it's such a good idea to have every type implement fn module, fn crate

matklad (Dec 17 2020 at 12:53, on Zulip):

This feels like a missing abstraction somewhere

Jonas Schievink [he/him] (Dec 17 2020 at 12:53, on Zulip):

Hmm, interestingly a lot of types have a fn module(self, db: &dyn HirDatabase) -> Module, which then does allow you to get the defining crate anyways

Jonas Schievink [he/him] (Dec 17 2020 at 12:53, on Zulip):

The hir_def crate has HasModule

Jonas Schievink [he/him] (Dec 17 2020 at 12:54, on Zulip):

Which could have a provided method that returns the crate

matklad (Dec 17 2020 at 13:01, on Zulip):

Ok, I think I've recalled historical context here

matklad (Dec 17 2020 at 13:01, on Zulip):

Originally, defs were very tightly bounded to AST

matklad (Dec 17 2020 at 13:01, on Zulip):

Basically, there was an fn to_def(ast: ast::Struct) -> hir::Struct.

matklad (Dec 17 2020 at 13:01, on Zulip):

Not how it was infailable.

matklad (Dec 17 2020 at 13:02, on Zulip):

The idea was that you can create hir even for files which are not otherwise linked with the build process

matklad (Dec 17 2020 at 13:02, on Zulip):

Since we've made ast->hir conversion failable, now we should assume that the crate always exists

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

so, @Jonas Schievink , I think we should make everything return a non-None crate, modulo potential primite bug

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

But also, maybe we should only have fn module and use two calls for crate?

Jonas Schievink [he/him] (Dec 17 2020 at 13:03, on Zulip):

Interesting, thanks for the recap!

Jonas Schievink [he/him] (Dec 17 2020 at 13:04, on Zulip):

I kinda want a trait for this, since that would help with making HasAttrs work with cfg_attr

Jonas Schievink [he/him] (Dec 17 2020 at 13:04, on Zulip):

(the crate needs to be known to access attributes once we handle them)

Jonas Schievink [he/him] (Dec 17 2020 at 13:06, on Zulip):

Hmm, we have 2 HasSource traits already, so I guess it wouldn't be the end of the world to define another HasModule trait in hir

matklad (Dec 17 2020 at 13:09, on Zulip):

Yeah, we def should do something here, good catch about uselss option

matklad (Dec 17 2020 at 13:10, on Zulip):

I am not so sure about a trait though. I think we never actually use this in generic context

matklad (Dec 17 2020 at 13:10, on Zulip):

So, having inherent method would work better.

matklad (Dec 17 2020 at 13:10, on Zulip):

I actually want to provide inherent impls for all of the ast traits as well, so that you don't have to import them

matklad (Dec 17 2020 at 13:10, on Zulip):

(they trait would still be there for cases where you do want to handle stuff generically)

matklad (Dec 17 2020 at 13:11, on Zulip):

https://matklad.github.io/2020/08/15/concrete-abstraction.html

Jonas Schievink [he/him] (Dec 17 2020 at 13:13, on Zulip):

Problem is that HasAttrs is used generically, and to access the attributes once we handle cfg_attr, you need the CrateId (not just Crate)... so, not yet sure how to handle this

matklad (Dec 17 2020 at 13:18, on Zulip):

Hm, you'll need CrateId inside HasAttrs impl, right?

matklad (Dec 17 2020 at 13:18, on Zulip):

not outside?

Jonas Schievink [he/him] (Dec 17 2020 at 13:18, on Zulip):

I think I could make it work that way

matklad (Dec 17 2020 at 13:19, on Zulip):

I think you need fn crate(def: AttrDefId) -> CrateId

matklad (Dec 17 2020 at 13:19, on Zulip):

And that can be implemented in hir_def

Jonas Schievink [he/him] (Dec 17 2020 at 13:19, on Zulip):

What I have now is a method on Attrs: fn query(&self, db: &dyn DefDatabase, krate: CrateId) -> QueryableAttrs<'_>

Since this bororws self, there wouldn't be any way to return the QueryableAttrs to the caller.

matklad (Dec 17 2020 at 13:19, on Zulip):

(but you obv have more context here than I do )

matklad (Dec 17 2020 at 13:19, on Zulip):

Ahhh

Jonas Schievink [he/him] (Dec 17 2020 at 13:20, on Zulip):

If I make QueryableAttrs owned, this could be made to work

matklad (Dec 17 2020 at 13:20, on Zulip):

So you want to cfg-attr attributes lazily?

matklad (Dec 17 2020 at 13:20, on Zulip):

At the point where we query them, not at the point of definition?

Jonas Schievink [he/him] (Dec 17 2020 at 13:20, on Zulip):

Which just means cloning the Attrs, which shouldn't be too bad

Jonas Schievink [he/him] (Dec 17 2020 at 13:20, on Zulip):

Yeah that was my plan

matklad (Dec 17 2020 at 13:20, on Zulip):

:thinking:

Jonas Schievink [he/him] (Dec 17 2020 at 13:20, on Zulip):

I briefly tried going the other way, but this seemed easier

matklad (Dec 17 2020 at 13:20, on Zulip):

I would think that an easier model is that in which you only ever have active attributes

Jonas Schievink [he/him] (Dec 17 2020 at 13:21, on Zulip):

though maybe that's not true

matklad (Dec 17 2020 at 13:21, on Zulip):

I mean, an ieasier model to use

matklad (Dec 17 2020 at 13:21, on Zulip):

not sure about implementation-side

Jonas Schievink [he/him] (Dec 17 2020 at 13:21, on Zulip):

yeah

matklad (Dec 17 2020 at 13:21, on Zulip):

but, as an assist author, I would prefer to be in a world where all "macros" are expanded, and where conditional compilation doesn't exist

matklad (Dec 17 2020 at 13:22, on Zulip):

(unrelated, but I've realized that early handing of conditional compilation is a thing whcih makes prortability lints and unified std not implementable in a "reliable" way)

Jonas Schievink [he/him] (Dec 17 2020 at 13:22, on Zulip):

mhmm, that's reasonable

Jonas Schievink [he/him] (Dec 17 2020 at 13:35, on Zulip):

seems like there's a lot of calls to Attrs::from_attrs_owner inside the IDE crates that would somehow need the CrateId

Jonas Schievink [he/him] (Dec 17 2020 at 13:35, on Zulip):

maybe this is a larger-scale refactoring than I thought, and should be done in phases

matklad (Dec 17 2020 at 13:40, on Zulip):

wait

matklad (Dec 17 2020 at 13:40, on Zulip):

how

matklad (Dec 17 2020 at 13:40, on Zulip):

that's wrong, IDE shouldn't call from_attrs_owner

matklad (Dec 17 2020 at 13:41, on Zulip):

Ook, I seee

matklad (Dec 17 2020 at 13:41, on Zulip):

all thouse usages are wrong :D

Jonas Schievink [he/him] (Dec 17 2020 at 13:43, on Zulip):

oh no :D

matklad (Dec 17 2020 at 13:44, on Zulip):

(as a meta note: in hir_(def|expand|ty) anything goes -- it's useful to keep abstraction thin and do whatever is the easiest to get the cake eaten. In hir, however, care must be taken to not expose impl detail. The latter is a futile battle and a whack-a-mole game, but we still need to fight it :-) )

Jonas Schievink [he/him] (Dec 17 2020 at 13:44, on Zulip):

yeah, it's a bit odd that Attrs is just used all over the stack here

matklad (Dec 17 2020 at 13:45, on Zulip):

I think there are only two usages though?

matklad (Dec 17 2020 at 13:45, on Zulip):

One inside nav_target, to get docs from symbols from symbol index

matklad (Dec 17 2020 at 13:45, on Zulip):

The second one inside runnables

matklad (Dec 17 2020 at 13:45, on Zulip):

I am dealing with runnables due to an unrealted issue, I might be able to paper over that today

matklad (Dec 17 2020 at 13:46, on Zulip):

THe one inside symbol index is interesting...

matklad (Dec 17 2020 at 13:47, on Zulip):

Not sure what's the best solution there. Maybe just don't try to render docs for symbols from index? Not sure where they'd be visible in the UI

Jonas Schievink [he/him] (Dec 17 2020 at 14:01, on Zulip):

Maybe this shows up on hover when the accurate resolution fails? Similar to the go to def fallback

Jonas Schievink [he/him] (Dec 17 2020 at 14:03, on Zulip):

hmm, no, doesn't look like it; only the source preview works, not rendered documentation

Jonas Schievink [he/him] (Dec 17 2020 at 14:48, on Zulip):

This PR makes from_attrs_owner private: https://github.com/rust-analyzer/rust-analyzer/pull/6916

Jonas Schievink [he/him] (Dec 17 2020 at 14:57, on Zulip):

Since the ItemTree still stores attributes, I think we'll have to split them up into RawAttrs, which are crate-independent and do not handle cfg_attr, and Attrs, which were filtered according to the cfgs active for the crate in question.

Jonas Schievink [he/him] (Dec 17 2020 at 14:57, on Zulip):

Since almost no attributes include cfg_attr, they can share the same data most of the time

Jonas Schievink [he/him] (Dec 17 2020 at 14:58, on Zulip):

(sorry, just thinking out loud)

matklad (Dec 17 2020 at 14:58, on Zulip):

Make sense

matklad (Dec 17 2020 at 15:31, on Zulip):

@Jonas Schievink regarding doubts about overall module/crate API.

I am looking at the ModuleDef::canonical_path API right now, and it doesn't handle associated items propertly

matklad (Dec 17 2020 at 15:31, on Zulip):

More generally, the problem with .module api is that the parent might actually be a block, and you might actually want to look at that, rather than at the module

matklad (Dec 17 2020 at 15:32, on Zulip):

but with .module so easily accessible, it might not be obvious

Jonas Schievink [he/him] (Dec 17 2020 at 15:32, on Zulip):

can associated items be ModuleDefs?

Jonas Schievink [he/him] (Dec 17 2020 at 15:32, on Zulip):

I thought they had a separate type

matklad (Dec 17 2020 at 15:33, on Zulip):

Sadly, the answer is "who knows" :(

matklad (Dec 17 2020 at 15:33, on Zulip):

We do have ModuleDef and AssocItemDef

matklad (Dec 17 2020 at 15:34, on Zulip):

But I don't thing anything actually checks whether a thing is in module or an assoc item

matklad (Dec 17 2020 at 15:34, on Zulip):

We might need to install such checks

Florian Diebold (Dec 17 2020 at 15:36, on Zulip):

I think I wrote a comment about that somewhere :sweat_smile:

Jonas Schievink [he/him] (Dec 17 2020 at 15:38, on Zulip):

Also, calling ModuleDef::module on a module returns the module's parent

Jonas Schievink [he/him] (Dec 17 2020 at 15:39, on Zulip):

So perhaps it should be containing_module or something

matklad (Dec 17 2020 at 15:40, on Zulip):

Maybe we need Def::container API which you can't get wrong

matklad (Dec 17 2020 at 15:40, on Zulip):

And Module::for_def(def: Def), Crate::for_def(def: Def) as a more verbose error-prone version?

matklad (Dec 17 2020 at 15:41, on Zulip):

:black_large_square: Design :black_large_square:

Last update: Jul 29 2021 at 21:30UTC