Stream: t-compiler/wg-rls-2.0

Topic: Visibility for modules


Kirill Bulatov (Dec 29 2019 at 20:11, on Zulip):

I could use the new visibility infra for the auto-import functionality https://github.com/rust-analyzer/rust-analyzer/issues/2180
But for that, I need to have the visibility data for modules at least (functions would be great to have either, but not that necessary for an initial auto import implementation).

It looks like I need to implement the HasVisibility trait for the ra_hir::code_model::Module struct.
If I understand it correctly, currently only the ra_hir_def::nameres::raw::ModuleData contains the data I need, but I have no idea how to get it with the ra_hir::code_model::Module data.
There's also ra_hir_def::nameres::raw::ModuleData struct which I can retrieve with the ra_hir::code_model::Module data, but there's no visibility data in 2 out of 3 places where this object is constructed and I have no clue on how to get it.

So, any hints on how can I get that working?

Florian Diebold (Dec 29 2019 at 20:19, on Zulip):

I think adding HasVisibility to Module would be the wrong approach for this. The visibility you're interested in should be that of a certain path, not the module itself. E.g.

mod foo {
  pub(crate) mod bar;
}
pub use foo::bar;

so in general, items can appear in many modules (everywhere they're used) and have a different visibility in each of those places. I think to find a path to import a certain item, we need to consider each of those places and whether it's visible from where we're importing.

Florian Diebold (Dec 29 2019 at 20:21, on Zulip):

Btw, I was planning on working on this (finding paths to an item) to use it to qualify paths in assists, but I have to admit I haven't gotten to it yet

Kirill Bulatov (Dec 29 2019 at 20:25, on Zulip):

That is what we want in the end, right.

The issue with this approach currently is that we don't have any reexport infra at all (https://github.com/rust-analyzer/rust-analyzer/issues/2180#issuecomment-569218100) and the only information we have currently is the ra_hir::code_model::Module and an information on the parent modules that contain it.

So now we can implement a very limited version of the auto imports, for cases where all modules are publicly available hence my initial question.
Or do you think we should go with the reexports instead?

Florian Diebold (Dec 29 2019 at 20:26, on Zulip):

the resolve_module_path method of Resolver returns a PerNs which contains the correct visibility for the end of the path. the publically visible methods don't return that yet though. Also the visibility of the intermediate segments isn't considered yet (I'm not sure whether it should)

Florian Diebold (Dec 29 2019 at 20:27, on Zulip):

I would suggest you keep it as simple as possible at first ;)

Florian Diebold (Dec 29 2019 at 20:27, on Zulip):

even if it means that we'll import some wrong paths at first

Florian Diebold (Dec 29 2019 at 20:28, on Zulip):

ah wait, resolve_module_path_in_items is public and also returns the PerNs

Kirill Bulatov (Dec 29 2019 at 20:33, on Zulip):

Yes, the easiest way would be great to have.
The methods you've suggested seem cool, but a bit more complex compared to what I've expected indeed :)

I can get the ra_syntax::ast::generated::Visibility easily from the ra_hir::code_model::Module with the following snippet:

let visibility: Option<Visibility> = module
                        .declaration_source(db)
                        .and_then(|module_declaration| module_declaration.value.visibility());

That's why I hoped that the proper Visibility object is easy to get.

Florian Diebold (Dec 29 2019 at 20:34, on Zulip):

what exactly do you want to do currently? build a path to import and then check whether it's visible? in that case I would suggest resolving it and checking the returned visibility

Florian Diebold (Dec 29 2019 at 20:35, on Zulip):

I'm not sure we should add a 'get visibility' API to Module at all, because it'd often be the wrong thing to use

Kirill Bulatov (Dec 29 2019 at 20:36, on Zulip):

Yes, exactly, to check that every module in the use foo::bar::baz is visible from the place we add the import to.
I'll try resolving it, thank you.

Kirill Bulatov (Dec 29 2019 at 20:38, on Zulip):

is visible from the place we add the import to

Just to clarify: under "visible" I mean the visibility from the module declaration, since we don't have the reexport support.

Florian Diebold (Dec 29 2019 at 20:40, on Zulip):

I personally think for a MVP of auto-import we wouldn't even need to do that -- not showing the assist if the path would not be visible vs. showing it and importing a 'wrong' (not visible) path is not that much of an improvement IMO, and in the future we will want to handle this differently (by generating a different path) anyway

Kirill Bulatov (Dec 29 2019 at 20:42, on Zulip):

Makes sense, I guess.
In that case I've got a hacky implementation that can add those imports already :tada: and all that's left is to refactor this mess.

Kirill Bulatov (Dec 29 2019 at 21:13, on Zulip):

cc @matklad , I'll go with the laziest approach to start with

Last update: Sep 18 2020 at 21:00UTC